* Re: [PATCH v2] vduse: Validate vq_num in vduse_validate_config() [not found] <20221128083627.1199512-1-harshit.m.mogalapalli@oracle.com> @ 2022-11-28 10:53 ` Stefano Garzarella 2022-11-28 10:58 ` Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Stefano Garzarella @ 2022-11-28 10:53 UTC (permalink / raw) To: Harshit Mogalapalli Cc: error27, Michael S . Tsirkin, harshit.m.mogalapalli, linux-kernel, virtualization, Xie Yongji, Gautam Dawar, Eli Cohen 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 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] vduse: Validate vq_num in vduse_validate_config() 2022-11-28 10:53 ` [PATCH v2] vduse: Validate vq_num in vduse_validate_config() Stefano Garzarella @ 2022-11-28 10:58 ` Dan Carpenter 2022-11-28 11:13 ` Stefano Garzarella 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2022-11-28 10:58 UTC (permalink / raw) To: Stefano Garzarella Cc: Michael S . Tsirkin, harshit.m.mogalapalli, linux-kernel, virtualization, Xie Yongji, Harshit Mogalapalli, Gautam Dawar, Eli Cohen 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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 4+ 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 13:10 ` Michael S. Tsirkin 0 siblings, 1 reply; 4+ messages in thread From: Stefano Garzarella @ 2022-11-28 11:13 UTC (permalink / raw) To: Dan Carpenter, Harshit Mogalapalli Cc: Michael S . Tsirkin, harshit.m.mogalapalli, linux-kernel, virtualization, Xie Yongji, Gautam Dawar, Eli Cohen 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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] vduse: Validate vq_num in vduse_validate_config() 2022-11-28 11:13 ` Stefano Garzarella @ 2022-11-28 13:10 ` Michael S. Tsirkin 0 siblings, 0 replies; 4+ messages in thread From: Michael S. Tsirkin @ 2022-11-28 13:10 UTC (permalink / raw) To: Stefano Garzarella Cc: Dan Carpenter, harshit.m.mogalapalli, linux-kernel, virtualization, Xie Yongji, Harshit Mogalapalli, Gautam Dawar, Eli Cohen 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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-28 13:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20221128083627.1199512-1-harshit.m.mogalapalli@oracle.com>
2022-11-28 10:53 ` [PATCH v2] vduse: Validate vq_num in vduse_validate_config() Stefano Garzarella
2022-11-28 10:58 ` Dan Carpenter
2022-11-28 11:13 ` Stefano Garzarella
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).