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

[Laurent Pinchart]

Hi Mauro,

On Mon, Jun 21, 2021 at 04:34:08PM +0200, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Jun 2021 17:11:46 +0300 Laurent Pinchart escreveu:
> > 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@xxxxxxxxxxxxxxx	# Kernel 4.9 and upper
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxxxxxxxx>
> 
> 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.

Please merge it directly, it's less work for me :-)

-- 
Regards,

Laurent Pinchart