From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 3/3] KVM: Expose MSRs to userspace Date: Fri, 17 Nov 2006 09:20:49 +0200 Message-ID: <455D62D1.6040203@qumranet.com> References: <455CA70C.9060307@qumranet.com> <20061116180422.0CC9325015E@cleopatra.q> <20061116170214.b7785bd0.akpm@osdl.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, uril-atKUWr5tajBWk0Htik3J/w@public.gmane.org Return-path: To: Andrew Morton In-Reply-To: <20061116170214.b7785bd0.akpm-3NddpPZAyC0@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Andrew Morton wrote: > On Thu, 16 Nov 2006 18:04:22 -0000 > Avi Kivity 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> + 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. ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV