From: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: error27@gmail.com, harshit.m.mogalapalli@gmail.com,
Jason Wang <jasowang@redhat.com>,
Xie Yongji <xieyongji@bytedance.com>,
Gautam Dawar <gautam.dawar@xilinx.com>,
Maxime Coquelin <maxime.coquelin@redhat.com>,
Guanjun <guanjun@linux.alibaba.com>,
Parav Pandit <parav@nvidia.com>, Eli Cohen <elic@nvidia.com>,
virtualization@lists.linux-foundation.org,
linux-kernel@vger.kernel.org,
Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Subject: Re: [PATCH] vduse: Fix a possible warning in vduse_create_dev()
Date: Sun, 27 Nov 2022 08:16:24 +0530 [thread overview]
Message-ID: <8253ab3f-b4f2-ff60-7e87-6455c9c10dc8@oracle.com> (raw)
In-Reply-To: <20221126181822-mutt-send-email-mst@kernel.org>
Hi Micheal,
On 27/11/22 4:52 am, Michael S. Tsirkin wrote:
> On Fri, Nov 25, 2022 at 07:59:58PM -0800, Harshit Mogalapalli wrote:
>> As 'dev->vq_num' is user-controlled data, if user tries to allocate
>> memory larger than(>=) MAX_ORDER, then kcalloc() will fail, it
>> creates a stack trace and messes up dmesg with a warning.
>>
>> Call trace:
>> -> vduse_ioctl
>> --> vduse_create_dev
>> 'config->vq_num' is user data as it comes from ioctl, which is
>> assigned to 'dev->vq_num'.
>>
>> Add __GFP_NOWARN in order to avoid too large allocation warning.
>> This is detected by static analysis using smatch.
>>
>> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
>> ---
>> drivers/vdpa/vdpa_user/vduse_dev.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>> index 35dceee3ed56..5e9546b16165 100644
>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>> @@ -1512,7 +1512,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>> dev->config_size = config->config_size;
>> dev->vq_align = config->vq_align;
>> dev->vq_num = config->vq_num;
>> - dev->vqs = kcalloc(dev->vq_num, sizeof(*dev->vqs), GFP_KERNEL);
>> + dev->vqs = kcalloc(dev->vq_num, sizeof(*dev->vqs),
>> + GFP_KERNEL | __GFP_NOWARN);
>> if (!dev->vqs)
>> goto err_vqs;
>
Thanks for checking the patch.
> This is insufficient - the real source of the problem is that
> vq_num is not validated.
> The thing to do is to validate config and limit vq_num to 0xffff;
>
1557 static long vduse_ioctl(struct file *file, unsigned int cmd,
1558 unsigned long arg)
1559 {
1560 int ret;
1561 void __user *argp = (void __user *)arg;
1564 mutex_lock(&vduse_lock);
1565 switch (cmd) {
....
1584 case VDUSE_CREATE_DEV: {
1585 struct vduse_dev_config config;
1587 void *buf;
1588
1589 ret = -EFAULT;
1590 if (copy_from_user(&config, argp, size))
1591 break;
1592
1593 ret = -EINVAL;
1594 if (vduse_validate_config(&config) == false)
1595 break;
1596
1597 buf = vmemdup_user(argp + size, config.config_size);
1598 if (IS_ERR(buf)) {
1599 ret = PTR_ERR(buf);
1600 break;
1601 }
1602 config.name[VDUSE_NAME_MAX - 1] = '\0';
1603 ret = vduse_create_dev(&config, buf,
control->api_version);
1604 if (ret)
1605 kvfree(buf);
1606 break;
1607 }
we have vduse_validate_config() being called in vduse_ioctl() which is
the caller of vduse_create_dev(), so validate_config() is not necessary
in vduse_create_dev() ?
Thanks,
Harshit
>
>> --
>> 2.38.1
>
next prev parent reply other threads:[~2022-11-27 2:46 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-26 3:59 [PATCH] vduse: Fix a possible warning in vduse_create_dev() Harshit Mogalapalli
2022-11-26 23:22 ` Michael S. Tsirkin
2022-11-26 23:22 ` Michael S. Tsirkin
2022-11-27 2:46 ` Harshit Mogalapalli [this message]
2022-11-27 16:34 ` Michael S. Tsirkin
2022-11-27 16:34 ` Michael S. Tsirkin
2022-11-28 4:13 ` Dan Carpenter
2022-11-28 4:13 ` Dan Carpenter
2022-11-28 8:38 ` Harshit Mogalapalli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8253ab3f-b4f2-ff60-7e87-6455c9c10dc8@oracle.com \
--to=harshit.m.mogalapalli@oracle.com \
--cc=elic@nvidia.com \
--cc=error27@gmail.com \
--cc=gautam.dawar@xilinx.com \
--cc=guanjun@linux.alibaba.com \
--cc=harshit.m.mogalapalli@gmail.com \
--cc=jasowang@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=maxime.coquelin@redhat.com \
--cc=mst@redhat.com \
--cc=parav@nvidia.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=xieyongji@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.