Re: [PATCH 3/3] KVM: Expose MSRs to userspace

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Andrew Morton wrote:
On Thu, 16 Nov 2006 18:04:22 -0000
Avi Kivity <[email protected]> wrote:

+static int kvm_dev_ioctl_set_msrs(struct kvm *kvm, struct kvm_msrs *msrs)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_msr_entry *entry, *entries;
+	int rc;
+	u32 size, num_entries, i;
+
+	if (msrs->vcpu < 0 || msrs->vcpu >= KVM_MAX_VCPUS)
+		return -EINVAL;
+
+	num_entries = ARRAY_SIZE(msrs_to_save);
+	if (msrs->nmsrs < num_entries) {
+		msrs->nmsrs = num_entries; /* inform actual size */
+		return -EINVAL;
+	}
+
+	vcpu = vcpu_load(kvm, msrs->vcpu);
+	if (!vcpu)
+		return -ENOENT;
+
+	size = msrs->nmsrs * sizeof(struct kvm_msr_entry);
+	rc = -E2BIG;
+	if (size > 4096)
+		goto out_vcpu;

Classic mutiplicative overflow bug.

Right, will fix. The 4096 limit is arbitrary anyway, and can be replaced by an arbitrary limit on nmsrs.


Only msrs->nmsrs doesn't get used
again, so there is no bug here.  Yet.


But why isn't it used again? Looks like the kernel is forcing the user to send at least num_entries for no good reason, and ignoring any entries beyond num_entries.

+	rc = -ENOMEM;
+	entries = vmalloc(size);
+	if (entries == NULL)
+		goto out_vcpu;
+
+	rc = -EFAULT;
+	if (copy_from_user(entries, msrs->entries, size))
+		goto out_free;
+
+	rc = -EINVAL;
+	for (i=0; i<num_entries; i++) {
+		entry = &entries[i];
+		if (set_msr(vcpu, entry->index,  entry->data))
+			goto out_free;
+	}
+
+	rc = 0;
+out_free:
+	vfree(entries);
+
+out_vcpu:
+	vcpu_put(vcpu);
+
+	return rc;
+}

This function returns no indication of how many msrs it actually did set. Should it?

It can't hurt. Is returning the number of msrs set in the return code (ala short write) acceptable, or do I need to make this a read/write ioctl?

--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux