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
