Messages in this thread | | | Date | Fri, 17 Nov 2006 09:20:49 +0200 | From | Avi Kivity <> | Subject | Re: [PATCH 3/3] KVM: Expose MSRs to userspace |
| |
Andrew Morton wrote: > On Thu, 16 Nov 2006 18:04:22 -0000 > Avi Kivity <avi@qumranet.com> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |