* [PATCH v2] vduse: Validate vq_num in vduse_validate_config() @ 2022-11-28 8:36 Harshit Mogalapalli 2022-11-28 10:53 ` Stefano Garzarella 0 siblings, 1 reply; 6+ messages in thread From: Harshit Mogalapalli @ 2022-11-28 8:36 UTC (permalink / raw) Cc: harshit.m.mogalapalli, error27, harshit.m.mogalapalli, Michael S . Tsirkin, Jason Wang, Xie Yongji, Gautam Dawar, Parav Pandit, Eli Cohen, virtualization, linux-kernel Add a limit to 'config->vq_num' which is user controlled data which comes from an vduse_ioctl to prevent large memory allocations. This is found using static analysis with smatch. Suggested-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> --- v1->v2: Change title of the commit and description, add a limit to vq_num. Note: I think here 0xffff is the max size of vring = no: of vqueues. Only compile and boot tested. --- drivers/vdpa/vdpa_user/vduse_dev.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c index 35dceee3ed56..31017ebc4d7c 100644 --- a/drivers/vdpa/vdpa_user/vduse_dev.c +++ b/drivers/vdpa/vdpa_user/vduse_dev.c @@ -1440,6 +1440,9 @@ static bool vduse_validate_config(struct vduse_dev_config *config) if (config->config_size > PAGE_SIZE) return false; + if (config->vq_num > 0xffff) + return false; + if (!device_is_allowed(config->device_id)) return false; -- 2.38.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] vduse: Validate vq_num in vduse_validate_config() 2022-11-28 8:36 [PATCH v2] vduse: Validate vq_num in vduse_validate_config() Harshit Mogalapalli @ 2022-11-28 10:53 ` Stefano Garzarella 2022-11-28 10:58 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Stefano Garzarella @ 2022-11-28 10:53 UTC (permalink / raw) To: Harshit Mogalapalli Cc: error27, harshit.m.mogalapalli, Michael S . Tsirkin, Jason Wang, Xie Yongji, Gautam Dawar, Parav Pandit, Eli Cohen, virtualization, linux-kernel On Mon, Nov 28, 2022 at 12:36:26AM -0800, Harshit Mogalapalli wrote: >Add a limit to 'config->vq_num' which is user controlled data which >comes from an vduse_ioctl to prevent large memory allocations. > >This is found using static analysis with smatch. > >Suggested-by: Michael S. Tsirkin <mst@redhat.com> >Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> >--- >v1->v2: Change title of the commit and description, add a limit to > vq_num. > >Note: I think here 0xffff is the max size of vring = no: of vqueues. >Only compile and boot tested. >--- > drivers/vdpa/vdpa_user/vduse_dev.c | 3 +++ > 1 file changed, 3 insertions(+) > >diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c >index 35dceee3ed56..31017ebc4d7c 100644 >--- a/drivers/vdpa/vdpa_user/vduse_dev.c >+++ b/drivers/vdpa/vdpa_user/vduse_dev.c >@@ -1440,6 +1440,9 @@ static bool vduse_validate_config(struct vduse_dev_config *config) > if (config->config_size > PAGE_SIZE) > return false; > >+ if (config->vq_num > 0xffff) What about using U16_MAX here? Thanks, Stefano >+ return false; >+ > if (!device_is_allowed(config->device_id)) > return false; > >-- >2.38.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] vduse: Validate vq_num in vduse_validate_config() 2022-11-28 10:53 ` Stefano Garzarella @ 2022-11-28 10:58 ` Dan Carpenter 2022-11-28 11:13 ` Stefano Garzarella 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2022-11-28 10:58 UTC (permalink / raw) To: Stefano Garzarella Cc: Harshit Mogalapalli, harshit.m.mogalapalli, Michael S . Tsirkin, Jason Wang, Xie Yongji, Gautam Dawar, Parav Pandit, Eli Cohen, virtualization, linux-kernel On Mon, Nov 28, 2022 at 11:53:12AM +0100, Stefano Garzarella wrote: > On Mon, Nov 28, 2022 at 12:36:26AM -0800, Harshit Mogalapalli wrote: > > Add a limit to 'config->vq_num' which is user controlled data which > > comes from an vduse_ioctl to prevent large memory allocations. > > > > This is found using static analysis with smatch. > > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> > > --- > > v1->v2: Change title of the commit and description, add a limit to > > vq_num. > > > > Note: I think here 0xffff is the max size of vring = no: of vqueues. > > Only compile and boot tested. > > --- > > drivers/vdpa/vdpa_user/vduse_dev.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c > > index 35dceee3ed56..31017ebc4d7c 100644 > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c > > @@ -1440,6 +1440,9 @@ static bool vduse_validate_config(struct vduse_dev_config *config) > > if (config->config_size > PAGE_SIZE) > > return false; > > > > + if (config->vq_num > 0xffff) > > What about using U16_MAX here? Where is the ->vq_num stored in a u16? I looked for this but didn't see it. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] vduse: Validate vq_num in vduse_validate_config() 2022-11-28 10:58 ` Dan Carpenter @ 2022-11-28 11:13 ` Stefano Garzarella 2022-11-28 12:35 ` Harshit Mogalapalli 2022-11-28 13:10 ` Michael S. Tsirkin 0 siblings, 2 replies; 6+ messages in thread From: Stefano Garzarella @ 2022-11-28 11:13 UTC (permalink / raw) To: Dan Carpenter, Harshit Mogalapalli Cc: harshit.m.mogalapalli, Michael S . Tsirkin, Jason Wang, Xie Yongji, Gautam Dawar, Parav Pandit, Eli Cohen, virtualization, linux-kernel On Mon, Nov 28, 2022 at 01:58:00PM +0300, Dan Carpenter wrote: >On Mon, Nov 28, 2022 at 11:53:12AM +0100, Stefano Garzarella wrote: >> On Mon, Nov 28, 2022 at 12:36:26AM -0800, Harshit Mogalapalli wrote: >> > Add a limit to 'config->vq_num' which is user controlled data which >> > comes from an vduse_ioctl to prevent large memory allocations. >> > >> > This is found using static analysis with smatch. >> > >> > Suggested-by: Michael S. Tsirkin <mst@redhat.com> >> > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> >> > --- >> > v1->v2: Change title of the commit and description, add a limit to >> > vq_num. >> > >> > Note: I think here 0xffff is the max size of vring = no: of vqueues. >> > Only compile and boot tested. >> > --- >> > drivers/vdpa/vdpa_user/vduse_dev.c | 3 +++ >> > 1 file changed, 3 insertions(+) >> > >> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c >> > index 35dceee3ed56..31017ebc4d7c 100644 >> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c >> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c >> > @@ -1440,6 +1440,9 @@ static bool vduse_validate_config(struct vduse_dev_config *config) >> > if (config->config_size > PAGE_SIZE) >> > return false; >> > >> > + if (config->vq_num > 0xffff) >> >> What about using U16_MAX here? > >Where is the ->vq_num stored in a u16? I looked for this but didn't >see it. I thought vq_num referred to the number of elements in the vq (like .get_vq_num_max), since this patch wants to limit to 0xffff. But it actually refers to the number of virtqueue, so @Harshit why do we limit it to 0xffff? Maybe we should explain it in the commit message or in a comment. Thanks, Stefano ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] vduse: Validate vq_num in vduse_validate_config() 2022-11-28 11:13 ` Stefano Garzarella @ 2022-11-28 12:35 ` Harshit Mogalapalli 2022-11-28 13:10 ` Michael S. Tsirkin 1 sibling, 0 replies; 6+ messages in thread From: Harshit Mogalapalli @ 2022-11-28 12:35 UTC (permalink / raw) To: Stefano Garzarella, Dan Carpenter Cc: harshit.m.mogalapalli, Michael S . Tsirkin, Jason Wang, Xie Yongji, Gautam Dawar, Parav Pandit, Eli Cohen, virtualization, linux-kernel On 28/11/22 4:43 pm, Stefano Garzarella wrote: > On Mon, Nov 28, 2022 at 01:58:00PM +0300, Dan Carpenter wrote: >> On Mon, Nov 28, 2022 at 11:53:12AM +0100, Stefano Garzarella wrote: >>> On Mon, Nov 28, 2022 at 12:36:26AM -0800, Harshit Mogalapalli wrote: >>> > Add a limit to 'config->vq_num' which is user controlled data which >>> > comes from an vduse_ioctl to prevent large memory allocations. >>> > >>> > This is found using static analysis with smatch. >>> > >>> > Suggested-by: Michael S. Tsirkin <mst@redhat.com> >>> > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> >>> > --- >>> > v1->v2: Change title of the commit and description, add a limit to >>> > vq_num. >>> > >>> > Note: I think here 0xffff is the max size of vring = no: of vqueues. >>> > Only compile and boot tested. >>> > --- >>> > drivers/vdpa/vdpa_user/vduse_dev.c | 3 +++ >>> > 1 file changed, 3 insertions(+) >>> > >>> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c >>> b/drivers/vdpa/vdpa_user/vduse_dev.c >>> > index 35dceee3ed56..31017ebc4d7c 100644 >>> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c >>> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c >>> > @@ -1440,6 +1440,9 @@ static bool vduse_validate_config(struct >>> vduse_dev_config *config) >>> > if (config->config_size > PAGE_SIZE) >>> > return false; >>> > >>> > + if (config->vq_num > 0xffff) >>> >>> What about using U16_MAX here? >> >> Where is the ->vq_num stored in a u16? I looked for this but didn't >> see it. > > I thought vq_num referred to the number of elements in the vq (like > .get_vq_num_max), since this patch wants to limit to 0xffff. > > But it actually refers to the number of virtqueue, so @Harshit why do we > limit it to 0xffff? > Hi Stefano, I may be incorrect about the details of this driver, my v1 was purely based on static analysis, Micheal suggested me to put a limit of 0xffff on vq_num. I really don't know about the driver, while I was searching other parts of code, I thought 0xffff is based vring size, I have asked the same question on v1 today. Ref to v1: https://lore.kernel.org/all/82e8ce27-0743-59bf-fbe8-a25093167451@oracle.com/ > Maybe we should explain it in the commit message or in a comment. > yeah, we should clarify the limit in commit message, once Micheal shares about the limit '0xffff), I will add those details and send a next version if that's okay. Thanks, Harshit > Thanks, > Stefano > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] vduse: Validate vq_num in vduse_validate_config() 2022-11-28 11:13 ` Stefano Garzarella 2022-11-28 12:35 ` Harshit Mogalapalli @ 2022-11-28 13:10 ` Michael S. Tsirkin 1 sibling, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2022-11-28 13:10 UTC (permalink / raw) To: Stefano Garzarella Cc: Dan Carpenter, Harshit Mogalapalli, harshit.m.mogalapalli, Jason Wang, Xie Yongji, Gautam Dawar, Parav Pandit, Eli Cohen, virtualization, linux-kernel On Mon, Nov 28, 2022 at 12:13:10PM +0100, Stefano Garzarella wrote: > On Mon, Nov 28, 2022 at 01:58:00PM +0300, Dan Carpenter wrote: > > On Mon, Nov 28, 2022 at 11:53:12AM +0100, Stefano Garzarella wrote: > > > On Mon, Nov 28, 2022 at 12:36:26AM -0800, Harshit Mogalapalli wrote: > > > > Add a limit to 'config->vq_num' which is user controlled data which > > > > comes from an vduse_ioctl to prevent large memory allocations. > > > > > > > > This is found using static analysis with smatch. > > > > > > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > > > > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> > > > > --- > > > > v1->v2: Change title of the commit and description, add a limit to > > > > vq_num. > > > > > > > > Note: I think here 0xffff is the max size of vring = no: of vqueues. > > > > Only compile and boot tested. > > > > --- > > > > drivers/vdpa/vdpa_user/vduse_dev.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c > > > > index 35dceee3ed56..31017ebc4d7c 100644 > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c > > > > @@ -1440,6 +1440,9 @@ static bool vduse_validate_config(struct vduse_dev_config *config) > > > > if (config->config_size > PAGE_SIZE) > > > > return false; > > > > > > > > + if (config->vq_num > 0xffff) > > > > > > What about using U16_MAX here? > > > > Where is the ->vq_num stored in a u16? I looked for this but didn't > > see it. > > I thought vq_num referred to the number of elements in the vq (like > .get_vq_num_max), since this patch wants to limit to 0xffff. > > But it actually refers to the number of virtqueue, so @Harshit why do we > limit it to 0xffff? > > Maybe we should explain it in the commit message or in a comment. > > Thanks, > Stefano This limit is somewhat arbitrary. However, currently virtio pci and ccw are limited to a 16 bit vq number. While MMIO isn't it is also isn't used with lots of VQs due to current lack of support for per-vq interrupts. Thus, the 0xffff limit on number of VQs corresponding to a 16-bit VQ number seems sufficient for now. Feel free to put the above in a code comment. -- MST ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-28 13:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-28 8:36 [PATCH v2] vduse: Validate vq_num in vduse_validate_config() Harshit Mogalapalli 2022-11-28 10:53 ` Stefano Garzarella 2022-11-28 10:58 ` Dan Carpenter 2022-11-28 11:13 ` Stefano Garzarella 2022-11-28 12:35 ` Harshit Mogalapalli 2022-11-28 13:10 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).