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

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

[Mauro Carvalho Chehab]

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;
 
-	*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;
 	int ret;
-	u32 i;
 
 	ret = uvc_acquire_privileges(handle);
 	if (ret < 0)
@@ -939,10 +946,17 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
 	if (input >= chain->selector->bNrInPins)
 		return -EINVAL;
 
-	i = input + 1;
-	return uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id,
-			      chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
-			      &i, 1);
+	buf = kmalloc(1, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	*buf = input + 1;
+	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id,
+			     chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
+			     buf, 1);
+	kfree(buf);
+
+	return ret;
 }
 
 static int uvc_ioctl_queryctrl(struct file *file, void *fh,
-- 
2.31.1

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

[Laurent Pinchart]

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 :-)

	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>

Do I need to take the patch in my tree ?

>  	int ret;
> -	u32 i;
>  
>  	ret = uvc_acquire_privileges(handle);
>  	if (ret < 0)
> @@ -939,10 +946,17 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
>  	if (input >= chain->selector->bNrInPins)
>  		return -EINVAL;
>  
> -	i = input + 1;
> -	return uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id,
> -			      chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
> -			      &i, 1);
> +	buf = kmalloc(1, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	*buf = input + 1;
> +	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id,
> +			     chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
> +			     buf, 1);
> +	kfree(buf);
> +
> +	return ret;
>  }
>  
>  static int uvc_ioctl_queryctrl(struct file *file, void *fh,

-- 
Regards,

Laurent Pinchart

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

[Mauro Carvalho Chehab]

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

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

[David Laight]

From: Mauro Carvalho Chehab
> Sent: 21 June 2021 14:40
> 
> 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.
...
> +	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);

Thought...

Is kmalloc(1, GFP_KERNEL) guaranteed to return a pointer into
a cache line that will not be accessed by any other code?
(This is slightly weaker than requiring a cache-line aligned
pointer - but very similar.)

Without that guarantee you can't use the returned buffer for
read dma unless the memory accesses are coherent.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Greg KH]

On Tue, Jun 22, 2021 at 08:07:12AM +0000, David Laight wrote:
> From: Mauro Carvalho Chehab
> > Sent: 21 June 2021 14:40
> > 
> > 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.
> ...
> > +	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);
> 
> Thought...
> 
> Is kmalloc(1, GFP_KERNEL) guaranteed to return a pointer into
> a cache line that will not be accessed by any other code?
> 
> (This is slightly weaker than requiring a cache-line aligned
> pointer - but very similar.)
> 
> Without that guarantee you can't use the returned buffer for
> read dma unless the memory accesses are coherent.

For USB buffers, that should be fine, we have been doing this for
decades now...

thanks,

greg k-h

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@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.

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

-- 
Regards,

Laurent Pinchart

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

[Alan Stern]

On Tue, Jun 22, 2021 at 08:07:12AM +0000, David Laight wrote:
> From: Mauro Carvalho Chehab
> > Sent: 21 June 2021 14:40
> > 
> > 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.
> ...
> > +	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);
> 
> Thought...
> 
> Is kmalloc(1, GFP_KERNEL) guaranteed to return a pointer into
> a cache line that will not be accessed by any other code?
> (This is slightly weaker than requiring a cache-line aligned
> pointer - but very similar.)

As I understand it, on architectures that do not have cache-coherent 
I/O, kmalloc is guaranteed to return a buffer that is 
cacheline-aligned and whose length is a multiple of the cacheline 
size.

Now, whether that buffer ends up being accessed by any other code 
depends on what your driver does with the pointer it gets from 
kmalloc.  :-)

Alan Stern

> Without that guarantee you can't use the returned buffer for
> read dma unless the memory accesses are coherent.
> 
> 	David

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

[David Laight]

From: Alan Stern
> Sent: 22 June 2021 14:29
...
> > Thought...
> >
> > Is kmalloc(1, GFP_KERNEL) guaranteed to return a pointer into
> > a cache line that will not be accessed by any other code?
> > (This is slightly weaker than requiring a cache-line aligned
> > pointer - but very similar.)
> 
> As I understand it, on architectures that do not have cache-coherent
> I/O, kmalloc is guaranteed to return a buffer that is
> cacheline-aligned and whose length is a multiple of the cacheline
> size.
> 
> Now, whether that buffer ends up being accessed by any other code
> depends on what your driver does with the pointer it gets from
> kmalloc.  :-)

Thanks for the clarification.

Most of the small allocates in the usb stack are for transmits
where it is only necessary to ensure a cache write-back.

I know there has been some confusion because one of the
allocators can add a small header to every allocation.
This can lead to unexpectedly inadequately aligned pointers.
If it is updated when the preceding block is freed (as some
user-space mallocs do) then it would need to be in a
completely separate cache line.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

['Alan Stern']

On Tue, Jun 22, 2021 at 02:21:27PM +0000, David Laight wrote:
> From: Alan Stern
> > Sent: 22 June 2021 14:29
> ...
> > > Thought...
> > >
> > > Is kmalloc(1, GFP_KERNEL) guaranteed to return a pointer into
> > > a cache line that will not be accessed by any other code?
> > > (This is slightly weaker than requiring a cache-line aligned
> > > pointer - but very similar.)
> > 
> > As I understand it, on architectures that do not have cache-coherent
> > I/O, kmalloc is guaranteed to return a buffer that is
> > cacheline-aligned and whose length is a multiple of the cacheline
> > size.
> > 
> > Now, whether that buffer ends up being accessed by any other code
> > depends on what your driver does with the pointer it gets from
> > kmalloc.  :-)
> 
> Thanks for the clarification.
> 
> Most of the small allocates in the usb stack are for transmits
> where it is only necessary to ensure a cache write-back.
> 
> I know there has been some confusion because one of the
> allocators can add a small header to every allocation.
> This can lead to unexpectedly inadequately aligned pointers.
> If it is updated when the preceding block is freed (as some
> user-space mallocs do) then it would need to be in a
> completely separate cache line.

If you really want to find out what the true story is, you should ask 
on the linux-mm mailing list.  The rest of us are not experts on this 
stuff.

Alan Stern