[PATCH 0/3] Videobuf2 corner case fixes

[PATCH 0/3] Videobuf2 corner case fixes

[Sakari Ailus]

Hi folks,

Most drivers already have limits to size such that you don't hit these,
but if you do, then mayhem will follow. The two first are cc'd to stable.

Sakari Ailus (3):
  videobuf2-core: Prevent size alignment wrapping buffer size to 0
  videobuf2-dma-sg: Prevent size from overflowing
  videobuf2-core.h: Document the alloc memop size argument as page
    aligned

 drivers/media/common/videobuf2/videobuf2-core.c   | 4 ++++
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
 include/media/videobuf2-core.h                    | 3 ++-
 3 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0

[Sakari Ailus]

PAGE_ALIGN() may wrap the buffer size around to 0. Prevent this by
checking that the aligned value is not smaller than the unaligned one.

Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 0ca81d495bdaf..0234ddbfa4de2 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
 	for (plane = 0; plane < vb->num_planes; ++plane) {
 		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
 
+		/* Did it wrap around? */
+		if (size < vb->planes[plane].length)
+			goto free;
+
 		mem_priv = call_ptr_memop(vb, alloc,
 				q->alloc_devs[plane] ? : q->dev,
 				q->dma_attrs, size, q->dma_dir, q->gfp_flags);
-- 
2.11.0

[PATCH 2/3] videobuf2-dma-sg: Prevent size from overflowing

[Sakari Ailus]

buf->size is an unsigned long; casting that to int will lead to an
overflow if buf->size exceeds INT_MAX.

Fix this by changing the type to unsigned long instead. This is possible
as the buf->size is always aligned to PAGE_SIZE, and therefore the size
will never have values lesser than 0.

Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 015e737095cdd..e9bfea986cc47 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf,
 		gfp_t gfp_flags)
 {
 	unsigned int last_page = 0;
-	int size = buf->size;
+	unsigned long size = buf->size;
 
 	while (size > 0) {
 		struct page *pages;
-- 
2.11.0

[PATCH 3/3] videobuf2-core.h: Document the alloc memop size argument as page aligned

[Sakari Ailus]

The size argument of the alloc memop, which allocates buffer memory, is
page aligned. Document it as such, as code elsewhere has not taken this
into account.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/videobuf2-core.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index e86981d615ae4..68b9fe660e4f1 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -54,7 +54,8 @@ struct vb2_threadio_data;
  *		will then be passed as @buf_priv argument to other ops in this
  *		structure. Additional gfp_flags to use when allocating the
  *		are also passed to this operation. These flags are from the
- *		gfp_flags field of vb2_queue.
+ *		gfp_flags field of vb2_queue. The size argument to this function
+ *		shall be *page aligned*.
  * @put:	inform the allocator that the buffer will no longer be used;
  *		usually will result in the allocator freeing the buffer (if
  *		no other users of this buffer are present); the @buf_priv
-- 
2.11.0

Re: [PATCH 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0

[Hans Verkuil]

On 12/13/18 11:40 AM, Sakari Ailus wrote:
> PAGE_ALIGN() may wrap the buffer size around to 0. Prevent this by
> checking that the aligned value is not smaller than the unaligned one.
> 
> Note on backporting to stable: the file used to be under
> drivers/media/v4l2-core, it was moved to the current location after 4.14.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 0ca81d495bdaf..0234ddbfa4de2 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>  		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
>  
> +		/* Did it wrap around? */
> +		if (size < vb->planes[plane].length)
> +			goto free;
> +
>  		mem_priv = call_ptr_memop(vb, alloc,
>  				q->alloc_devs[plane] ? : q->dev,
>  				q->dma_attrs, size, q->dma_dir, q->gfp_flags);
> 

Re: [PATCH 2/3] videobuf2-dma-sg: Prevent size from overflowing

[Hans Verkuil]

On 12/13/18 11:40 AM, Sakari Ailus wrote:
> buf->size is an unsigned long; casting that to int will lead to an
> overflow if buf->size exceeds INT_MAX.
> 
> Fix this by changing the type to unsigned long instead. This is possible
> as the buf->size is always aligned to PAGE_SIZE, and therefore the size
> will never have values lesser than 0.
> 
> Note on backporting to stable: the file used to be under
> drivers/media/v4l2-core, it was moved to the current location after 4.14.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> ---
>  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index 015e737095cdd..e9bfea986cc47 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf,
>  		gfp_t gfp_flags)
>  {
>  	unsigned int last_page = 0;
> -	int size = buf->size;
> +	unsigned long size = buf->size;
>  
>  	while (size > 0) {
>  		struct page *pages;
> 

Re: [PATCH 3/3] videobuf2-core.h: Document the alloc memop size argument as page aligned

[Hans Verkuil]

On 12/13/18 11:40 AM, Sakari Ailus wrote:
> The size argument of the alloc memop, which allocates buffer memory, is
> page aligned. Document it as such, as code elsewhere has not taken this
> into account.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Regards,

	Hans

> ---
>  include/media/videobuf2-core.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index e86981d615ae4..68b9fe660e4f1 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -54,7 +54,8 @@ struct vb2_threadio_data;
>   *		will then be passed as @buf_priv argument to other ops in this
>   *		structure. Additional gfp_flags to use when allocating the
>   *		are also passed to this operation. These flags are from the
> - *		gfp_flags field of vb2_queue.
> + *		gfp_flags field of vb2_queue. The size argument to this function
> + *		shall be *page aligned*.
>   * @put:	inform the allocator that the buffer will no longer be used;
>   *		usually will result in the allocator freeing the buffer (if
>   *		no other users of this buffer are present); the @buf_priv
> 

Re: [PATCH 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thursday, 13 December 2018 12:40:04 EET Sakari Ailus wrote:
> PAGE_ALIGN() may wrap the buffer size around to 0. Prevent this by
> checking that the aligned value is not smaller than the unaligned one.
> 
> Note on backporting to stable: the file used to be under
> drivers/media/v4l2-core, it was moved to the current location after 4.14.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c index
> 0ca81d495bdaf..0234ddbfa4de2 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>  		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
> 
> +		/* Did it wrap around? */
> +		if (size < vb->planes[plane].length)
> +			goto free;
> +
>  		mem_priv = call_ptr_memop(vb, alloc,
>  				q->alloc_devs[plane] ? : q->dev,
>  				q->dma_attrs, size, q->dma_dir, q->gfp_flags);

Wouldn't it be better to reject length > INT_MAX (or some variations of that) 
a few steps before, for instance just before calling __vb2_queue_alloc() ? 
There's already a check in vb2_core_reqbufs():

        /* Check that driver has set sane values */
        if (WARN_ON(!num_planes))
                return -EINVAL;

        for (i = 0; i < num_planes; i++)
                if (WARN_ON(!plane_sizes[i]))
                        return -EINVAL;

It could be extended to validate the sizes against wrap-around, and moved to a 
separate function to be called in vb2_core_create_bufs() as well (as those 
checks are missing there). Alternatively, the checks could be moved to the 
beginning of __vb2_queue_alloc().

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] videobuf2-dma-sg: Prevent size from overflowing

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thursday, 13 December 2018 12:40:05 EET Sakari Ailus wrote:
> buf->size is an unsigned long; casting that to int will lead to an
> overflow if buf->size exceeds INT_MAX.
> 
> Fix this by changing the type to unsigned long instead. This is possible
> as the buf->size is always aligned to PAGE_SIZE, and therefore the size
> will never have values lesser than 0.

This feels a bit fragile to me. We at least need a big comment in the code to 
explain this. Another option would be a size -= min(..., size) just to make 
sure.

> Note on backporting to stable: the file used to be under
> drivers/media/v4l2-core, it was moved to the current location after 4.14.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index
> 015e737095cdd..e9bfea986cc47 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct
> vb2_dma_sg_buf *buf, gfp_t gfp_flags)
>  {
>  	unsigned int last_page = 0;
> -	int size = buf->size;
> +	unsigned long size = buf->size;
> 
>  	while (size > 0) {
>  		struct page *pages;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] videobuf2-core.h: Document the alloc memop size argument as page aligned

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thursday, 13 December 2018 12:40:06 EET Sakari Ailus wrote:
> The size argument of the alloc memop, which allocates buffer memory, is
> page aligned. Document it as such, as code elsewhere has not taken this
> into account.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/videobuf2-core.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index e86981d615ae4..68b9fe660e4f1 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -54,7 +54,8 @@ struct vb2_threadio_data;
>   *		will then be passed as @buf_priv argument to other ops in this
>   *		structure. Additional gfp_flags to use when allocating the
>   *		are also passed to this operation. These flags are from the
> - *		gfp_flags field of vb2_queue.
> + *		gfp_flags field of vb2_queue. The size argument to this function
> + *		shall be *page aligned*.
>   * @put:	inform the allocator that the buffer will no longer be used;
>   *		usually will result in the allocator freeing the buffer (if
>   *		no other users of this buffer are present); the @buf_priv

I wonder if a WARN_ON() to ensure this would make sense. In any case,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] videobuf2-dma-sg: Prevent size from overflowing

[Sakari Ailus]

Hi Laurent,

On Thu, Dec 13, 2018 at 02:57:46PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Thursday, 13 December 2018 12:40:05 EET Sakari Ailus wrote:
> > buf->size is an unsigned long; casting that to int will lead to an
> > overflow if buf->size exceeds INT_MAX.
> > 
> > Fix this by changing the type to unsigned long instead. This is possible
> > as the buf->size is always aligned to PAGE_SIZE, and therefore the size
> > will never have values lesser than 0.
> 
> This feels a bit fragile to me. We at least need a big comment in the code to 
> explain this. Another option would be a size -= min(..., size) just to make 
> sure.

I was thinking of something like:

	if (WARN_ON(size & ~PAGE_MASK))
		return -ENOMEM;

But I opted to writing the third patch as this is not the only place where
the page alignment could be relevant.

What do you think?

> 
> > Note on backporting to stable: the file used to be under
> > drivers/media/v4l2-core, it was moved to the current location after 4.14.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index
> > 015e737095cdd..e9bfea986cc47 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > @@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct
> > vb2_dma_sg_buf *buf, gfp_t gfp_flags)
> >  {
> >  	unsigned int last_page = 0;
> > -	int size = buf->size;
> > +	unsigned long size = buf->size;
> > 
> >  	while (size > 0) {
> >  		struct page *pages;
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 3/3] videobuf2-core.h: Document the alloc memop size argument as page aligned

[Sakari Ailus]

On Thu, Dec 13, 2018 at 02:59:50PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Thursday, 13 December 2018 12:40:06 EET Sakari Ailus wrote:
> > The size argument of the alloc memop, which allocates buffer memory, is
> > page aligned. Document it as such, as code elsewhere has not taken this
> > into account.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/media/videobuf2-core.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index e86981d615ae4..68b9fe660e4f1 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -54,7 +54,8 @@ struct vb2_threadio_data;
> >   *		will then be passed as @buf_priv argument to other ops in this
> >   *		structure. Additional gfp_flags to use when allocating the
> >   *		are also passed to this operation. These flags are from the
> > - *		gfp_flags field of vb2_queue.
> > + *		gfp_flags field of vb2_queue. The size argument to this function
> > + *		shall be *page aligned*.
> >   * @put:	inform the allocator that the buffer will no longer be used;
> >   *		usually will result in the allocator freeing the buffer (if
> >   *		no other users of this buffer are present); the @buf_priv
> 
> I wonder if a WARN_ON() to ensure this would make sense. In any case,

There's a single place where the alloc() op is called. I thought it'd be
silly to put a check after the line of code that performs the alignment.

Perhaps a comment right there?

I'm open to other ideas that don't seem silly. :-)

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 2/3] videobuf2-dma-sg: Prevent size from overflowing

[Laurent Pinchart]

On Thursday, 13 December 2018 15:00:23 EET Sakari Ailus wrote:
> Hi Laurent,
> 
> On Thu, Dec 13, 2018 at 02:57:46PM +0200, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> > 
> > On Thursday, 13 December 2018 12:40:05 EET Sakari Ailus wrote:
> > > buf->size is an unsigned long; casting that to int will lead to an
> > > overflow if buf->size exceeds INT_MAX.
> > > 
> > > Fix this by changing the type to unsigned long instead. This is possible
> > > as the buf->size is always aligned to PAGE_SIZE, and therefore the size
> > > will never have values lesser than 0.
> > 
> > This feels a bit fragile to me. We at least need a big comment in the code
> > to explain this. Another option would be a size -= min(..., size) just to
> > make sure.
> 
> I was thinking of something like:
> 
> 	if (WARN_ON(size & ~PAGE_MASK))
> 		return -ENOMEM;
> 
> But I opted to writing the third patch as this is not the only place where
> the page alignment could be relevant.
> 
> What do you think?

I'd do both :-)

> > > Note on backporting to stable: the file used to be under
> > > drivers/media/v4l2-core, it was moved to the current location after
> > > 4.14.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > 
> > >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index
> > > 015e737095cdd..e9bfea986cc47 100644
> > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > @@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct
> > > vb2_dma_sg_buf *buf, gfp_t gfp_flags)
> > > 
> > >  {
> > >  
> > >  	unsigned int last_page = 0;
> > > 
> > > -	int size = buf->size;
> > > +	unsigned long size = buf->size;
> > > 
> > >  	while (size > 0) {
> > >  	
> > >  		struct page *pages;


-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0

[Sakari Ailus]

On Thu, Dec 13, 2018 at 02:49:44PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Thursday, 13 December 2018 12:40:04 EET Sakari Ailus wrote:
> > PAGE_ALIGN() may wrap the buffer size around to 0. Prevent this by
> > checking that the aligned value is not smaller than the unaligned one.
> > 
> > Note on backporting to stable: the file used to be under
> > drivers/media/v4l2-core, it was moved to the current location after 4.14.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> > b/drivers/media/common/videobuf2/videobuf2-core.c index
> > 0ca81d495bdaf..0234ddbfa4de2 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> >  	for (plane = 0; plane < vb->num_planes; ++plane) {
> >  		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
> > 
> > +		/* Did it wrap around? */
> > +		if (size < vb->planes[plane].length)
> > +			goto free;
> > +
> >  		mem_priv = call_ptr_memop(vb, alloc,
> >  				q->alloc_devs[plane] ? : q->dev,
> >  				q->dma_attrs, size, q->dma_dir, q->gfp_flags);
> 
> Wouldn't it be better to reject length > INT_MAX (or some variations of that) 
> a few steps before, for instance just before calling __vb2_queue_alloc() ? 
> There's already a check in vb2_core_reqbufs():
> 
>         /* Check that driver has set sane values */
>         if (WARN_ON(!num_planes))
>                 return -EINVAL;
> 
>         for (i = 0; i < num_planes; i++)
>                 if (WARN_ON(!plane_sizes[i]))
>                         return -EINVAL;
> 
> It could be extended to validate the sizes against wrap-around, and moved to a 
> separate function to be called in vb2_core_create_bufs() as well (as those 
> checks are missing there). Alternatively, the checks could be moved to the 
> beginning of __vb2_queue_alloc().

I'd hate to distribute the aligning to PAGE_SIZE in the two functions. An
alternative would be to add a new function argument for the aligned size,
but I like that even less. I think it might be better as-is.

I don't have a strong opinion either way though, and you certainly have a
point as well. What do you think?

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 2/3] videobuf2-dma-sg: Prevent size from overflowing

[Sakari Ailus]

On Thu, Dec 13, 2018 at 03:03:47PM +0200, Laurent Pinchart wrote:
> On Thursday, 13 December 2018 15:00:23 EET Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Thu, Dec 13, 2018 at 02:57:46PM +0200, Laurent Pinchart wrote:
> > > Hi Sakari,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Thursday, 13 December 2018 12:40:05 EET Sakari Ailus wrote:
> > > > buf->size is an unsigned long; casting that to int will lead to an
> > > > overflow if buf->size exceeds INT_MAX.
> > > > 
> > > > Fix this by changing the type to unsigned long instead. This is possible
> > > > as the buf->size is always aligned to PAGE_SIZE, and therefore the size
> > > > will never have values lesser than 0.
> > > 
> > > This feels a bit fragile to me. We at least need a big comment in the code
> > > to explain this. Another option would be a size -= min(..., size) just to
> > > make sure.
> > 
> > I was thinking of something like:
> > 
> > 	if (WARN_ON(size & ~PAGE_MASK))
> > 		return -ENOMEM;
> > 
> > But I opted to writing the third patch as this is not the only place where
> > the page alignment could be relevant.
> > 
> > What do you think?
> 
> I'd do both :-)

Then my next question is: as this is relevant elsewhere, too, where else
should I add a similar check?

This is just the SG DMA implementation...

> 
> > > > Note on backporting to stable: the file used to be under
> > > > drivers/media/v4l2-core, it was moved to the current location after
> > > > 4.14.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > ---
> > > > 
> > > >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > > b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index
> > > > 015e737095cdd..e9bfea986cc47 100644
> > > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > > @@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct
> > > > vb2_dma_sg_buf *buf, gfp_t gfp_flags)
> > > > 
> > > >  {
> > > >  
> > > >  	unsigned int last_page = 0;
> > > > 
> > > > -	int size = buf->size;
> > > > +	unsigned long size = buf->size;
> > > > 
> > > >  	while (size > 0) {
> > > >  	
> > > >  		struct page *pages;
> 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 3/3] videobuf2-core.h: Document the alloc memop size argument as page aligned

[Laurent Pinchart]

Hi Sakari,

On Thursday, 13 December 2018 15:02:03 EET Sakari Ailus wrote:
> On Thu, Dec 13, 2018 at 02:59:50PM +0200, Laurent Pinchart wrote:
> > On Thursday, 13 December 2018 12:40:06 EET Sakari Ailus wrote:
> > > The size argument of the alloc memop, which allocates buffer memory, is
> > > page aligned. Document it as such, as code elsewhere has not taken this
> > > into account.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > 
> > >  include/media/videobuf2-core.h | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/media/videobuf2-core.h
> > > b/include/media/videobuf2-core.h index e86981d615ae4..68b9fe660e4f1
> > > 100644
> > > --- a/include/media/videobuf2-core.h
> > > +++ b/include/media/videobuf2-core.h
> > > @@ -54,7 +54,8 @@ struct vb2_threadio_data;
> > >   *		will then be passed as @buf_priv argument to other ops in this
> > >   *		structure. Additional gfp_flags to use when allocating the
> > >   *		are also passed to this operation. These flags are from the
> > > - *		gfp_flags field of vb2_queue.
> > > + *		gfp_flags field of vb2_queue. The size argument to this function
> > > + *		shall be *page aligned*.
> > >   * @put:	inform the allocator that the buffer will no longer be used;
> > >   *		usually will result in the allocator freeing the buffer (if
> > >   *		no other users of this buffer are present); the @buf_priv
> > 
> > I wonder if a WARN_ON() to ensure this would make sense. In any case,
> 
> There's a single place where the alloc() op is called. I thought it'd be
> silly to put a check after the line of code that performs the alignment.

I was thinking about WARN_ON in the memory allocators, but if it's called from 
a single place, I agree it may not be needed. A comment in the dma-sg 
allocator would be useful though.

> Perhaps a comment right there?
> 
> I'm open to other ideas that don't seem silly. :-)

Stop calling my ideas silly :-)

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart