Message ID | 20210503122111.13775-1-sidcha@amazon.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: x86: Hoist input checks in kvm_add_msr_filter() | expand |
On Mon, May 03, 2021, Siddharth Chandrasekaran wrote: > In ioctl KVM_X86_SET_MSR_FILTER, input from user space is validated > after a memdup_user(). For invalid inputs we'd memdup and then call > kfree unnecessarily. Hoist input validation to avoid kfree altogether. > > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> > --- > arch/x86/kvm/x86.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index ee0dc58ac3a5..15c20b31cc91 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -5393,11 +5393,16 @@ static int kvm_add_msr_filter(struct kvm_x86_msr_filter *msr_filter, > struct msr_bitmap_range range; > unsigned long *bitmap = NULL; > size_t bitmap_size; > - int r; > > if (!user_range->nmsrs) > return 0; > > + if (user_range->flags & ~(KVM_MSR_FILTER_READ | KVM_MSR_FILTER_WRITE)) > + return -EINVAL; > + > + if (!user_range->flags) > + return -EINVAL; > + > bitmap_size = BITS_TO_LONGS(user_range->nmsrs) * sizeof(long); > if (!bitmap_size || bitmap_size > KVM_MSR_FILTER_MAX_BITMAP_SIZE) > return -EINVAL; > @@ -5413,24 +5418,10 @@ static int kvm_add_msr_filter(struct kvm_x86_msr_filter *msr_filter, > .bitmap = bitmap, > }; > > - if (range.flags & ~(KVM_MSR_FILTER_READ | KVM_MSR_FILTER_WRITE)) { > - r = -EINVAL; > - goto err; > - } > - > - if (!range.flags) { > - r = -EINVAL; > - goto err; > - } > - > - /* Everything ok, add this range identifier. */ > msr_filter->ranges[msr_filter->count] = range; Might be worth elminating the intermediate "range", too. Doesn't affect output, but it would make it a little more obvious that the new range is mostly coming straight from userspace input. E.g. msr_filter->ranges[msr_filter->count] = (struct msr_bitmap_range) { .flags = user_range->flags, .base = user_range->base, .nmsrs = user_range->nmsrs, .bitmap = bitmap, }; Either way: Reviewed-by: Sean Christopherson <seanjc@google.com> > msr_filter->count++; > > return 0; > -err: > - kfree(bitmap); > - return r; > } > > static int kvm_vm_ioctl_set_msr_filter(struct kvm *kvm, void __user *argp) > --
On Mon, May 03, 2021 at 08:53:09PM +0000, Sean Christopherson wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > On Mon, May 03, 2021, Siddharth Chandrasekaran wrote: > > In ioctl KVM_X86_SET_MSR_FILTER, input from user space is validated > > after a memdup_user(). For invalid inputs we'd memdup and then call > > kfree unnecessarily. Hoist input validation to avoid kfree altogether. > > > > Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> > > --- > > arch/x86/kvm/x86.c | 21 ++++++--------------- > > 1 file changed, 6 insertions(+), 15 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index ee0dc58ac3a5..15c20b31cc91 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -5393,11 +5393,16 @@ static int kvm_add_msr_filter(struct kvm_x86_msr_filter *msr_filter, > > struct msr_bitmap_range range; > > unsigned long *bitmap = NULL; > > size_t bitmap_size; > > - int r; > > > > if (!user_range->nmsrs) > > return 0; > > > > + if (user_range->flags & ~(KVM_MSR_FILTER_READ | KVM_MSR_FILTER_WRITE)) > > + return -EINVAL; > > + > > + if (!user_range->flags) > > + return -EINVAL; > > + > > bitmap_size = BITS_TO_LONGS(user_range->nmsrs) * sizeof(long); > > if (!bitmap_size || bitmap_size > KVM_MSR_FILTER_MAX_BITMAP_SIZE) > > return -EINVAL; > > @@ -5413,24 +5418,10 @@ static int kvm_add_msr_filter(struct kvm_x86_msr_filter *msr_filter, > > .bitmap = bitmap, > > }; > > > > - if (range.flags & ~(KVM_MSR_FILTER_READ | KVM_MSR_FILTER_WRITE)) { > > - r = -EINVAL; > > - goto err; > > - } > > - > > - if (!range.flags) { > > - r = -EINVAL; > > - goto err; > > - } > > - > > - /* Everything ok, add this range identifier. */ > > msr_filter->ranges[msr_filter->count] = range; > > Might be worth elminating the intermediate "range", too. Doesn't affect output, > but it would make it a little more obvious that the new range is mostly coming > straight from userspace input. E.g. > > msr_filter->ranges[msr_filter->count] = (struct msr_bitmap_range) { > .flags = user_range->flags, > .base = user_range->base, > .nmsrs = user_range->nmsrs, > .bitmap = bitmap, > }; > > Either way: > > Reviewed-by: Sean Christopherson <seanjc@google.com> Thanks for the review. Changed as suggested. ~ Sid. Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index ee0dc58ac3a5..15c20b31cc91 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -5393,11 +5393,16 @@ static int kvm_add_msr_filter(struct kvm_x86_msr_filter *msr_filter, struct msr_bitmap_range range; unsigned long *bitmap = NULL; size_t bitmap_size; - int r; if (!user_range->nmsrs) return 0; + if (user_range->flags & ~(KVM_MSR_FILTER_READ | KVM_MSR_FILTER_WRITE)) + return -EINVAL; + + if (!user_range->flags) + return -EINVAL; + bitmap_size = BITS_TO_LONGS(user_range->nmsrs) * sizeof(long); if (!bitmap_size || bitmap_size > KVM_MSR_FILTER_MAX_BITMAP_SIZE) return -EINVAL; @@ -5413,24 +5418,10 @@ static int kvm_add_msr_filter(struct kvm_x86_msr_filter *msr_filter, .bitmap = bitmap, }; - if (range.flags & ~(KVM_MSR_FILTER_READ | KVM_MSR_FILTER_WRITE)) { - r = -EINVAL; - goto err; - } - - if (!range.flags) { - r = -EINVAL; - goto err; - } - - /* Everything ok, add this range identifier. */ msr_filter->ranges[msr_filter->count] = range; msr_filter->count++; return 0; -err: - kfree(bitmap); - return r; } static int kvm_vm_ioctl_set_msr_filter(struct kvm *kvm, void __user *argp)
In ioctl KVM_X86_SET_MSR_FILTER, input from user space is validated after a memdup_user(). For invalid inputs we'd memdup and then call kfree unnecessarily. Hoist input validation to avoid kfree altogether. Signed-off-by: Siddharth Chandrasekaran <sidcha@amazon.de> --- arch/x86/kvm/x86.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-)