Re: [PATCH v3] media: uvc: don't do DMA on stack

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Mon, 21 Jun 2021 17:11:46 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> On Mon, Jun 21, 2021 at 03:40:19PM +0200, Mauro Carvalho Chehab wrote:
> > As warned by smatch:
> > 	drivers/media/usb/uvc/uvc_v4l2.c:911 uvc_ioctl_g_input() error: doing dma on the stack (&i)
> > 	drivers/media/usb/uvc/uvc_v4l2.c:943 uvc_ioctl_s_input() error: doing dma on the stack (&i)
> > 
> > those two functions call uvc_query_ctrl passing a pointer to
> > a data at the DMA stack. those are used to send URBs via
> > usb_control_msg(). Using DMA stack is not supported and should
> > not work anymore on modern Linux versions.
> > 
> > So, use a kmalloc'ed buffer.
> > 
> > Cc: stable@vger.kernel.org	# Kernel 4.9 and upper
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  drivers/media/usb/uvc/uvc_v4l2.c | 30 ++++++++++++++++++++++--------
> >  1 file changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 252136cc885c..a95bf7318848 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -899,8 +899,8 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
> >  {
> >  	struct uvc_fh *handle = fh;
> >  	struct uvc_video_chain *chain = handle->chain;
> > +	u8 *buf;
> >  	int ret;
> > -	u8 i;
> >  
> >  	if (chain->selector == NULL ||
> >  	    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> > @@ -908,13 +908,20 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
> >  		return 0;
> >  	}
> >  
> > +	buf = kmalloc(1, GFP_KERNEL);
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> >  	ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id,
> >  			     chain->dev->intfnum,  UVC_SU_INPUT_SELECT_CONTROL,
> > -			     &i, 1);
> > +			     buf, 1);
> >  	if (ret < 0)
> >  		return ret;  
> 
> Memory leak :-)

Argh ;-)

Clearly, I'm needing more caffeine today, but it is too damn hot
here...

> 
> 	if (!ret)
> 		*input = *buf - 1;
> 
> 	kfree(buf);
> 
> 	return ret;
> 
> >  
> > -	*input = i - 1;
> > +	*input = *buf - 1;
> > +
> > +	kfree(buf);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -922,8 +929,8 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
> >  {
> >  	struct uvc_fh *handle = fh;
> >  	struct uvc_video_chain *chain = handle->chain;
> > +	char *buf;  
> 
> 	u8 *buf;
> 
> With these two changes,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

> Do I need to take the patch in my tree ?

It is up to you.

I suspect that it would be easier to just merge it at media_stage,
together with the other patches from the smatch series, but it is
up to you.

Just let me know if you prefer to merge it via your tree, and I'll drop
it from my queue, or otherwise I'll merge directly at media_stage,
after waiting for a while on feedbacks on the remaining patches.

Thanks,
Mauro