Re: [PATCH v3 02/12] KVM: x86: Introduce allow list for MSR emulation

From: Alexander Graf <graf@amazon.com>
To: Dan Carpenter <dan.carpenter@oracle.com>, <kbuild@lists.01.org>,
	"Aaron Lewis" <aaronlewis@google.com>, <jmattson@google.com>
Cc: <lkp@intel.com>, <kbuild-all@lists.01.org>, <pshier@google.com>,
	<oupton@google.com>, <kvm@vger.kernel.org>,
	KarimAllah Ahmed <karahmed@amazon.de>
Subject: Re: [PATCH v3 02/12] KVM: x86: Introduce allow list for MSR emulation
Date: Tue, 1 Sep 2020 21:13:10 +0200	[thread overview]
Message-ID: <79dd5f72-a332-a657-674d-f3a9c94146f1@amazon.com> (raw)
In-Reply-To: <20200831103933.GF8299@kadam>

On 31.08.20 12:39, Dan Carpenter wrote:
> 
> Hi Aaron,
> 
> url:    https://github.com/0day-ci/linux/commits/Aaron-Lewis/Allow-userspace-to-manage-MSRs/20200819-051903
> base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
> config: x86_64-randconfig-m001-20200827 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks a bunch for looking at this! I'd squash in the change with the 
actual patch as it's tiny, so I'm not sure how attribution would work in 
that case.

> 
> smatch warnings:
> arch/x86/kvm/x86.c:5248 kvm_vm_ioctl_add_msr_allowlist() error: 'bitmap' dereferencing possible ERR_PTR()
> 
> # https://github.com/0day-ci/linux/commit/107c87325cf461b7b1bd07bb6ddbaf808a8d8a2a
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review Aaron-Lewis/Allow-userspace-to-manage-MSRs/20200819-051903
> git checkout 107c87325cf461b7b1bd07bb6ddbaf808a8d8a2a
> vim +/bitmap +5248 arch/x86/kvm/x86.c
> 
> 107c87325cf461 Aaron Lewis 2020-08-18  5181  static int kvm_vm_ioctl_add_msr_allowlist(struct kvm *kvm, void __user *argp)
> 107c87325cf461 Aaron Lewis 2020-08-18  5182  {
> 107c87325cf461 Aaron Lewis 2020-08-18  5183     struct msr_bitmap_range *ranges = kvm->arch.msr_allowlist_ranges;
> 107c87325cf461 Aaron Lewis 2020-08-18  5184     struct kvm_msr_allowlist __user *user_msr_allowlist = argp;
> 107c87325cf461 Aaron Lewis 2020-08-18  5185     struct msr_bitmap_range range;
> 107c87325cf461 Aaron Lewis 2020-08-18  5186     struct kvm_msr_allowlist kernel_msr_allowlist;
> 107c87325cf461 Aaron Lewis 2020-08-18  5187     unsigned long *bitmap = NULL;
> 107c87325cf461 Aaron Lewis 2020-08-18  5188     size_t bitmap_size;
> 107c87325cf461 Aaron Lewis 2020-08-18  5189     int r = 0;
> 107c87325cf461 Aaron Lewis 2020-08-18  5190
> 107c87325cf461 Aaron Lewis 2020-08-18  5191     if (copy_from_user(&kernel_msr_allowlist, user_msr_allowlist,
> 107c87325cf461 Aaron Lewis 2020-08-18  5192                        sizeof(kernel_msr_allowlist))) {
> 107c87325cf461 Aaron Lewis 2020-08-18  5193             r = -EFAULT;
> 107c87325cf461 Aaron Lewis 2020-08-18  5194             goto out;
> 107c87325cf461 Aaron Lewis 2020-08-18  5195     }
> 107c87325cf461 Aaron Lewis 2020-08-18  5196
> 107c87325cf461 Aaron Lewis 2020-08-18  5197     bitmap_size = BITS_TO_LONGS(kernel_msr_allowlist.nmsrs) * sizeof(long);
>                                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^n
> On 32 bit systems the BITS_TO_LONGS() can integer overflow if
> kernel_msr_allowlist.nmsrs is larger than ULONG_MAX - bits_per_long.  In
> that case bitmap_size is zero.

Nice catch! It should be enough to ...

> 
> 107c87325cf461 Aaron Lewis 2020-08-18  5198     if (bitmap_size > KVM_MSR_ALLOWLIST_MAX_LEN) {

... add a check for !bitmap_size here as well then, right?

> 107c87325cf461 Aaron Lewis 2020-08-18  5199             r = -EINVAL;
> 107c87325cf461 Aaron Lewis 2020-08-18  5200             goto out;
> 107c87325cf461 Aaron Lewis 2020-08-18  5201     }
> 107c87325cf461 Aaron Lewis 2020-08-18  5202
> 107c87325cf461 Aaron Lewis 2020-08-18  5203     bitmap = memdup_user(user_msr_allowlist->bitmap, bitmap_size);
> 107c87325cf461 Aaron Lewis 2020-08-18  5204     if (IS_ERR(bitmap)) {
> 107c87325cf461 Aaron Lewis 2020-08-18  5205             r = PTR_ERR(bitmap);
> 107c87325cf461 Aaron Lewis 2020-08-18  5206             goto out;
>                                                          ^^^^^^^^
> "out" is always a vague label name.  It's better style to return
> directly instead of doing a complicated no-op.
> 
>          if (IS_ERR(bitmap))
>                  return PTR_ERR(bitmap);

I agree 100% :). In fact, I agree so much that I already did change it 
for v6 last week, just did not send it out yet.

> 
> 107c87325cf461 Aaron Lewis 2020-08-18  5207     }
> 107c87325cf461 Aaron Lewis 2020-08-18  5208
> 107c87325cf461 Aaron Lewis 2020-08-18  5209     range = (struct msr_bitmap_range) {
> 107c87325cf461 Aaron Lewis 2020-08-18  5210             .flags = kernel_msr_allowlist.flags,
> 107c87325cf461 Aaron Lewis 2020-08-18  5211             .base = kernel_msr_allowlist.base,
> 107c87325cf461 Aaron Lewis 2020-08-18  5212             .nmsrs = kernel_msr_allowlist.nmsrs,
> 107c87325cf461 Aaron Lewis 2020-08-18  5213             .bitmap = bitmap,
> 
> In case of overflow then "bitmap" is 0x16 and .nmsrs is a very high
> number.

The overflow case should disappear with the additional check above, right?

Alex

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