[PATCH v2 0/3] Videobuf2 corner case fixes

[PATCH v2 0/3] Videobuf2 corner case fixes

[Sakari Ailus]

Hi all,

Here's a second version of the set fixing a few videobuf2 corner cases.
Most drivers have limits for the size already but not necessarily all of
them.

since v1:

- Add a sanity check for alignment in vb2_dma_sg_alloc_compacted.

- Add a comment in __vb2_buf_mem_alloc noting that the size shall be page
  aligned.

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   | 5 +++++
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
 include/media/videobuf2-core.h                    | 3 ++-
 3 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH v2 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
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 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 0ca81d495bda..0234ddbfa4de 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 v2 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
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 015e737095cd..5fdb8d7051f6 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -59,7 +59,10 @@ 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;
+
+	if (WARN_ON(size & ~PAGE_MASK))
+		return -ENOMEM;
 
 	while (size > 0) {
 		struct page *pages;
-- 
2.11.0

[PATCH v2 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 in the only caller as well as ops
documentation.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/common/videobuf2/videobuf2-core.c | 1 +
 include/media/videobuf2-core.h                  | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 0234ddbfa4de..64ecb9a65a47 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -205,6 +205,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
 	 * NOTE: mmapped areas should be page aligned
 	 */
 	for (plane = 0; plane < vb->num_planes; ++plane) {
+		/* Memops alloc requires size to be page aligned. */
 		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
 
 		/* Did it wrap around? */
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index e86981d615ae..68b9fe660e4f 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 v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0

[Mauro Carvalho Chehab]

Em Tue,  8 Jan 2019 10:58:34 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> 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>
> ---
>  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 0ca81d495bda..0234ddbfa4de 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;
> +

Sorry, but I can't see how this could ever happen (except for a very serious
bug at the compiler or at the hardware).

See, the definition at PAGE_ALIGN is (from mm.h):

	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)

and the macro it uses come from kernel.h:

	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
	..
	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))

So, this:
	size = PAGE_ALIGN(length);

(assuming PAGE_SIZE= 0x1000)

becomes:

	size = (length + 0x0fff) & ~0xfff;

so, size will *always* be >= length.

Thanks,
Mauro

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

[Mauro Carvalho Chehab]

Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:

> Em Tue,  8 Jan 2019 10:58:34 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > 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>
> > ---
> >  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 0ca81d495bda..0234ddbfa4de 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;
> > +
> 
> Sorry, but I can't see how this could ever happen (except for a very serious
> bug at the compiler or at the hardware).
> 
> See, the definition at PAGE_ALIGN is (from mm.h):
> 
> 	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
> 
> and the macro it uses come from kernel.h:
> 
> 	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
> 	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> 	..
> 	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
> 
> So, this:
> 	size = PAGE_ALIGN(length);
> 
> (assuming PAGE_SIZE= 0x1000)
> 
> becomes:
> 
> 	size = (length + 0x0fff) & ~0xfff;
> 
> so, size will *always* be >= length.

Hmm... after looking at patch 2, now I understand what's your concern...

If someone indeed uses length = INT_MAX, size will indeed be zero.

Please adjust the description accordingly, as it doesn't reflect
that.

Btw, in this particular case, I would use a WARN_ON(), as this is
something that indicates not only a driver bug (as the driver is
letting someone to request a buffer a way too big), but probably
also an attempt from a hacker to try to crack the system.

Thanks,
Mauro

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

[Mauro Carvalho Chehab]

Em Tue, 8 Jan 2019 10:59:55 -0200
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:

> Em Tue, 8 Jan 2019 10:52:12 -0200
> Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:
> 
> > Em Tue,  8 Jan 2019 10:58:34 +0200
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > 
> > > 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>
> > > ---
> > >  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 0ca81d495bda..0234ddbfa4de 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;
> > > +
> > 
> > Sorry, but I can't see how this could ever happen (except for a very serious
> > bug at the compiler or at the hardware).
> > 
> > See, the definition at PAGE_ALIGN is (from mm.h):
> > 
> > 	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
> > 
> > and the macro it uses come from kernel.h:
> > 
> > 	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
> > 	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> > 	..
> > 	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
> > 
> > So, this:
> > 	size = PAGE_ALIGN(length);
> > 
> > (assuming PAGE_SIZE= 0x1000)
> > 
> > becomes:
> > 
> > 	size = (length + 0x0fff) & ~0xfff;
> > 
> > so, size will *always* be >= length.
> 
> Hmm... after looking at patch 2, now I understand what's your concern...
> 
> If someone indeed uses length = INT_MAX, size will indeed be zero.

In time, I meant to say UINT_MAX.

> 
> Please adjust the description accordingly, as it doesn't reflect
> that.
> 
> Btw, in this particular case, I would use a WARN_ON(), as this is
> something that indicates not only a driver bug (as the driver is
> letting someone to request a buffer a way too big), but probably
> also an attempt from a hacker to try to crack the system.
> 
> Thanks,
> Mauro



Thanks,
Mauro

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

[Mauro Carvalho Chehab]

Em Tue,  8 Jan 2019 10:58:35 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> 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>
> ---
>  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index 015e737095cd..5fdb8d7051f6 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -59,7 +59,10 @@ 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;

OK.

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

Hmm... why do we need a warn on here? This is called by this code:

static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
{
	struct vb2_queue *q = vb->vb2_queue;
	void *mem_priv;
	int plane;
	int ret = -ENOMEM;

	/*
	 * Allocate memory for all planes in this buffer
	 * NOTE: mmapped areas should be page aligned
	 */
	for (plane = 0; plane < vb->num_planes; ++plane) {
		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);

		mem_priv = call_ptr_memop(vb, alloc,
				q->alloc_devs[plane] ? : q->dev,
				q->dma_attrs, size, q->dma_dir, q->gfp_flags);

With already insures that size is page aligned.

Thanks,
Mauro

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

[Sakari Ailus]

On Tue, Jan 08, 2019 at 11:09:42AM -0200, Mauro Carvalho Chehab wrote:
> Em Tue,  8 Jan 2019 10:58:35 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > 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>
> > ---
> >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > index 015e737095cd..5fdb8d7051f6 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > @@ -59,7 +59,10 @@ 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;
> 
> OK.
> 
> > +
> > +	if (WARN_ON(size & ~PAGE_MASK))
> > +		return -ENOMEM;
> 
> Hmm... why do we need a warn on here? This is called by this code:

This was suggested as a sanity check in review of v1 of the set.

Supposing that someone once removed that alignment, things would go rather
completely haywire. There would probably be lots of other troubles as well
but this one would probably corrupt system memory (at least).

> 
> static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> {
> 	struct vb2_queue *q = vb->vb2_queue;
> 	void *mem_priv;
> 	int plane;
> 	int ret = -ENOMEM;
> 
> 	/*
> 	 * Allocate memory for all planes in this buffer
> 	 * NOTE: mmapped areas should be page aligned
> 	 */
> 	for (plane = 0; plane < vb->num_planes; ++plane) {
> 		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
> 
> 		mem_priv = call_ptr_memop(vb, alloc,
> 				q->alloc_devs[plane] ? : q->dev,
> 				q->dma_attrs, size, q->dma_dir, q->gfp_flags);
> 
> With already insures that size is page aligned.
> 
> Thanks,
> Mauro

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

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

[Sakari Ailus]

Hi Mauro,

Thanks for the review.

On Tue, Jan 08, 2019 at 10:59:55AM -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 8 Jan 2019 10:52:12 -0200
> Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:
> 
> > Em Tue,  8 Jan 2019 10:58:34 +0200
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > 
> > > 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>
> > > ---
> > >  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 0ca81d495bda..0234ddbfa4de 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;
> > > +
> > 
> > Sorry, but I can't see how this could ever happen (except for a very serious
> > bug at the compiler or at the hardware).
> > 
> > See, the definition at PAGE_ALIGN is (from mm.h):
> > 
> > 	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
> > 
> > and the macro it uses come from kernel.h:
> > 
> > 	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
> > 	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> > 	..
> > 	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
> > 
> > So, this:
> > 	size = PAGE_ALIGN(length);
> > 
> > (assuming PAGE_SIZE= 0x1000)
> > 
> > becomes:
> > 
> > 	size = (length + 0x0fff) & ~0xfff;
> > 
> > so, size will *always* be >= length.
> 
> Hmm... after looking at patch 2, now I understand what's your concern...
> 
> If someone indeed uses length = INT_MAX, size will indeed be zero.
> 
> Please adjust the description accordingly, as it doesn't reflect
> that.
> 
> Btw, in this particular case, I would use a WARN_ON(), as this is
> something that indicates not only a driver bug (as the driver is
> letting someone to request a buffer a way too big), but probably

What's the maximum size a driver should allow? I guess this could be seen
be a failure from the driver's part to limit the size of the buffer, but
it's not trivial either to define that.

Hardware typically has maximum dimensions it can support, but the user may
want to add padding at the end of the lines. Perhaps a helper macro could
be used for this purpose: most likely there's no need to be more padding
than there's image data per line. If that turns out to be too restrictive,
the macro could be changed. That's probably unlikely, admittedly.

For some hardware these numbers could still be more than a 32-bit unsigned
integer can hold, so the check is still needed.

> also an attempt from a hacker to try to crack the system.

This could be also v4l2-compliance setting the length field to -1. A
warning is worth it only if there's good chance there's e.g. a kernel bug
involved.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

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

[Sakari Ailus]

On Tue, Jan 08, 2019 at 10:59:55AM -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 8 Jan 2019 10:52:12 -0200
> Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:
> 
> > Em Tue,  8 Jan 2019 10:58:34 +0200
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > 
> > > 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>
> > > ---
> > >  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 0ca81d495bda..0234ddbfa4de 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;
> > > +
> > 
> > Sorry, but I can't see how this could ever happen (except for a very serious
> > bug at the compiler or at the hardware).
> > 
> > See, the definition at PAGE_ALIGN is (from mm.h):
> > 
> > 	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
> > 
> > and the macro it uses come from kernel.h:
> > 
> > 	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
> > 	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> > 	..
> > 	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
> > 
> > So, this:
> > 	size = PAGE_ALIGN(length);
> > 
> > (assuming PAGE_SIZE= 0x1000)
> > 
> > becomes:
> > 
> > 	size = (length + 0x0fff) & ~0xfff;
> > 
> > so, size will *always* be >= length.
> 
> Hmm... after looking at patch 2, now I understand what's your concern...
> 
> If someone indeed uses length = INT_MAX, size will indeed be zero.
> 
> Please adjust the description accordingly, as it doesn't reflect
> that.

How about: 

PAGE_ALIGN() may wrap the buffer length around to 0 if the value to be
aligned is close to the top of the value range of the type. Prevent this by
checking that the aligned value is not smaller than the unaligned one.

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

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

[Mauro Carvalho Chehab]

Em Tue, 8 Jan 2019 15:29:26 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> On Tue, Jan 08, 2019 at 11:09:42AM -0200, Mauro Carvalho Chehab wrote:
> > Em Tue,  8 Jan 2019 10:58:35 +0200
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >   
> > > 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>
> > > ---
> > >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > index 015e737095cd..5fdb8d7051f6 100644
> > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > @@ -59,7 +59,10 @@ 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;  
> > 
> > OK.
> >   
> > > +
> > > +	if (WARN_ON(size & ~PAGE_MASK))
> > > +		return -ENOMEM;  
> > 
> > Hmm... why do we need a warn on here? This is called by this code:  
> 
> This was suggested as a sanity check in review of v1 of the set.
> 
> Supposing that someone once removed that alignment, things would go rather
> completely haywire. There would probably be lots of other troubles as well
> but this one would probably corrupt system memory (at least).

Well, patch 3 prevents that. See: this is not like something that driver
developers can mess with that, as the only place where the .alloc() ops
is called is by the VB2 core, and it already ensures page alignment.

If one would ever try to remove PAGE_ALIGN() from vb2 core, we'll nack it,
as we know that such change will break things.

Thanks,
Mauro

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

[Sakari Ailus]

On Tue, Jan 08, 2019 at 11:44:01AM -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 8 Jan 2019 15:29:26 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > On Tue, Jan 08, 2019 at 11:09:42AM -0200, Mauro Carvalho Chehab wrote:
> > > Em Tue,  8 Jan 2019 10:58:35 +0200
> > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > >   
> > > > 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>
> > > > ---
> > > >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > > index 015e737095cd..5fdb8d7051f6 100644
> > > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > > @@ -59,7 +59,10 @@ 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;  
> > > 
> > > OK.
> > >   
> > > > +
> > > > +	if (WARN_ON(size & ~PAGE_MASK))
> > > > +		return -ENOMEM;  
> > > 
> > > Hmm... why do we need a warn on here? This is called by this code:  
> > 
> > This was suggested as a sanity check in review of v1 of the set.
> > 
> > Supposing that someone once removed that alignment, things would go rather
> > completely haywire. There would probably be lots of other troubles as well
> > but this one would probably corrupt system memory (at least).
> 
> Well, patch 3 prevents that. See: this is not like something that driver
> developers can mess with that, as the only place where the .alloc() ops
> is called is by the VB2 core, and it already ensures page alignment.
> 
> If one would ever try to remove PAGE_ALIGN() from vb2 core, we'll nack it,
> as we know that such change will break things.

Indeed. I'm certainly fine with dropping the sanity check; I think there
are enough warnings elsewhere plus common sense to avoid making such a
change.

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

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

[Mauro Carvalho Chehab]

Em Tue, 8 Jan 2019 15:38:32 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> Thanks for the review.
> 
> On Tue, Jan 08, 2019 at 10:59:55AM -0200, Mauro Carvalho Chehab wrote:
> > Em Tue, 8 Jan 2019 10:52:12 -0200
> > Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:
> >   
> > > Em Tue,  8 Jan 2019 10:58:34 +0200
> > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > >   
> > > > 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>
> > > > ---
> > > >  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 0ca81d495bda..0234ddbfa4de 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;
> > > > +  
> > > 
> > > Sorry, but I can't see how this could ever happen (except for a very serious
> > > bug at the compiler or at the hardware).
> > > 
> > > See, the definition at PAGE_ALIGN is (from mm.h):
> > > 
> > > 	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
> > > 
> > > and the macro it uses come from kernel.h:
> > > 
> > > 	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
> > > 	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> > > 	..
> > > 	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
> > > 
> > > So, this:
> > > 	size = PAGE_ALIGN(length);
> > > 
> > > (assuming PAGE_SIZE= 0x1000)
> > > 
> > > becomes:
> > > 
> > > 	size = (length + 0x0fff) & ~0xfff;
> > > 
> > > so, size will *always* be >= length.  
> > 
> > Hmm... after looking at patch 2, now I understand what's your concern...
> > 
> > If someone indeed uses length = INT_MAX, size will indeed be zero.
> > 
> > Please adjust the description accordingly, as it doesn't reflect
> > that.
> > 
> > Btw, in this particular case, I would use a WARN_ON(), as this is
> > something that indicates not only a driver bug (as the driver is
> > letting someone to request a buffer a way too big), but probably  
> 
> What's the maximum size a driver should allow? I guess this could be seen
> be a failure from the driver's part to limit the size of the buffer, but
> it's not trivial either to define that.
> 
> Hardware typically has maximum dimensions it can support, but the user may
> want to add padding at the end of the lines. Perhaps a helper macro could
> be used for this purpose: most likely there's no need to be more padding
> than there's image data per line. If that turns out to be too restrictive,
> the macro could be changed. That's probably unlikely, admittedly.
> 
> For some hardware these numbers could still be more than a 32-bit unsigned
> integer can hold, so the check is still needed.

I guess that, by changing from "int" to "unsigned long", we ensure that the 
number should be big enough to be able to represent the maximum allocation
size.

On Linux, sizeof(long) is usually assumed to be sizeof(void *). Such
assumption is used, for example, when we pass a structure pointer to
ioctl's, instead of passing a long integer.

I mean, on a 64 bits system, long has 64 bits. AFAIKT, even the latest
Xeon CPUs, the address space is lower than 64 bits. So, if one tries to
allocate a memory with sizeof(ULONG_MAX), this will fail with ENOMEM.

On any (true) 32 bits system, the physical address is to 32 bits.
So, if one tries to allocate a memory with ULONG_MAX, this should
also fail, as there won't be memory for anything else.

There are some special cases, like X86_PAE (and ARM_LPAE). There, the 
physical address space is 64 bits, but instruction set is the 32 bits one.
Yet, I'm almost sure that (at least on x86) a single memory block there 
can't be bigger than 32 bits.

What I'm trying to say is that I strongly suspect that we won't have 
any cases where someone using would need a buffer with more than 
32 bits size on a non-64 architecture.

> 
> > also an attempt from a hacker to try to crack the system.  
> 
> This could be also v4l2-compliance setting the length field to -1. A
> warning is worth it only if there's good chance there's e.g. a kernel bug
> involved.
> 



Thanks,
Mauro

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

[Mauro Carvalho Chehab]

Em Tue, 8 Jan 2019 15:40:47 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> On Tue, Jan 08, 2019 at 10:59:55AM -0200, Mauro Carvalho Chehab wrote:
> > Em Tue, 8 Jan 2019 10:52:12 -0200
> > Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:
> >   
> > > Em Tue,  8 Jan 2019 10:58:34 +0200
> > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > >   
> > > > 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>
> > > > ---
> > > >  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 0ca81d495bda..0234ddbfa4de 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;
> > > > +  
> > > 
> > > Sorry, but I can't see how this could ever happen (except for a very serious
> > > bug at the compiler or at the hardware).
> > > 
> > > See, the definition at PAGE_ALIGN is (from mm.h):
> > > 
> > > 	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
> > > 
> > > and the macro it uses come from kernel.h:
> > > 
> > > 	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
> > > 	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> > > 	..
> > > 	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
> > > 
> > > So, this:
> > > 	size = PAGE_ALIGN(length);
> > > 
> > > (assuming PAGE_SIZE= 0x1000)
> > > 
> > > becomes:
> > > 
> > > 	size = (length + 0x0fff) & ~0xfff;
> > > 
> > > so, size will *always* be >= length.  
> > 
> > Hmm... after looking at patch 2, now I understand what's your concern...
> > 
> > If someone indeed uses length = INT_MAX, size will indeed be zero.
> > 
> > Please adjust the description accordingly, as it doesn't reflect
> > that.  
> 
> How about: 
> 
> PAGE_ALIGN() may wrap the buffer length around to 0 if the value to be
> aligned is close to the top of the value range of the type. Prevent this by
> checking that the aligned value is not smaller than the unaligned one.

I would be a way more clear, as this is there to prevent a single
special case: length == ULEN_MAX. Something like:

	If one tried to allocate a buffer with sizeof(ULEN_MAX), this will cause
	an overflow at PAGE_ALIGN(), making it return zero as the size of the
	buffer, causing the code to fail.

I would even let it clearer at the code itself. So, instead of the
hunk you proposed, I would do:

	unsigned long size = vb->planes[plane].length;

	/* Prevent PAGE_ALIGN overflow */
	if (WARN_ON(size == ULONG_MAX))
		goto free;

	size = PAGE_ALIGN(vb->planes[plane].length);

Thanks,
Mauro

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

[Laurent Pinchart]

On Tuesday, 8 January 2019 16:30:22 EET Mauro Carvalho Chehab wrote:
> Em Tue, 8 Jan 2019 15:40:47 +0200
> 
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > On Tue, Jan 08, 2019 at 10:59:55AM -0200, Mauro Carvalho Chehab wrote:
> > > Em Tue, 8 Jan 2019 10:52:12 -0200
> > > 
> > > Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:
> > > > Em Tue,  8 Jan 2019 10:58:34 +0200
> > > > 
> > > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > > > > 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>
> > > > > ---
> > > > > 
> > > > >  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
> > > > > 0ca81d495bda..0234ddbfa4de 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;
> > > > > +
> > > > 
> > > > Sorry, but I can't see how this could ever happen (except for a very
> > > > serious bug at the compiler or at the hardware).
> > > > 
> > > > See, the definition at PAGE_ALIGN is (from mm.h):
> > > > 	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
> > > > 
> > > > and the macro it uses come from kernel.h:
> > > > 	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))
(a) -
> > > > 	1)
> > > > 	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> > > > 	..
> > > > 	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
> > > > 
> > > > So, this:
> > > > 	size = PAGE_ALIGN(length);
> > > > 
> > > > (assuming PAGE_SIZE= 0x1000)
> > > > 
> > > > becomes:
> > > > 	size = (length + 0x0fff) & ~0xfff;
> > > > 
> > > > so, size will *always* be >= length.
> > > 
> > > Hmm... after looking at patch 2, now I understand what's your concern...
> > > 
> > > If someone indeed uses length = INT_MAX, size will indeed be zero.
> > > 
> > > Please adjust the description accordingly, as it doesn't reflect
> > > that.
> > 
> > How about:
> > 
> > PAGE_ALIGN() may wrap the buffer length around to 0 if the value to be
> > aligned is close to the top of the value range of the type. Prevent this
> > by
> > checking that the aligned value is not smaller than the unaligned one.
> 
> I would be a way more clear, as this is there to prevent a single
> special case: length == ULEN_MAX. Something like:
> 
> 	If one tried to allocate a buffer with sizeof(ULEN_MAX), this will cause
> 	an overflow at PAGE_ALIGN(), making it return zero as the size of the
> 	buffer, causing the code to fail.
> 
> I would even let it clearer at the code itself. So, instead of the
> hunk you proposed, I would do:
> 
> 	unsigned long size = vb->planes[plane].length;
> 
> 	/* Prevent PAGE_ALIGN overflow */
> 	if (WARN_ON(size == ULONG_MAX))
> 		goto free;

ULONG_MAX - PAGE_SIZE + 2 to ULONG_MAX would all cause the same issue.
> 
> 	size = PAGE_ALIGN(vb->planes[plane].length);

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

On Tue, Jan 08, 2019 at 12:23:49PM -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 8 Jan 2019 15:38:32 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > Hi Mauro,
> > 
> > Thanks for the review.
> > 
> > On Tue, Jan 08, 2019 at 10:59:55AM -0200, Mauro Carvalho Chehab wrote:
> > > Em Tue, 8 Jan 2019 10:52:12 -0200
> > > Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:
> > >   
> > > > Em Tue,  8 Jan 2019 10:58:34 +0200
> > > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > > >   
> > > > > 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>
> > > > > ---
> > > > >  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 0ca81d495bda..0234ddbfa4de 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;
> > > > > +  
> > > > 
> > > > Sorry, but I can't see how this could ever happen (except for a very serious
> > > > bug at the compiler or at the hardware).
> > > > 
> > > > See, the definition at PAGE_ALIGN is (from mm.h):
> > > > 
> > > > 	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
> > > > 
> > > > and the macro it uses come from kernel.h:
> > > > 
> > > > 	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
> > > > 	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> > > > 	..
> > > > 	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
> > > > 
> > > > So, this:
> > > > 	size = PAGE_ALIGN(length);
> > > > 
> > > > (assuming PAGE_SIZE= 0x1000)
> > > > 
> > > > becomes:
> > > > 
> > > > 	size = (length + 0x0fff) & ~0xfff;
> > > > 
> > > > so, size will *always* be >= length.  
> > > 
> > > Hmm... after looking at patch 2, now I understand what's your concern...
> > > 
> > > If someone indeed uses length = INT_MAX, size will indeed be zero.
> > > 
> > > Please adjust the description accordingly, as it doesn't reflect
> > > that.
> > > 
> > > Btw, in this particular case, I would use a WARN_ON(), as this is
> > > something that indicates not only a driver bug (as the driver is
> > > letting someone to request a buffer a way too big), but probably  
> > 
> > What's the maximum size a driver should allow? I guess this could be seen
> > be a failure from the driver's part to limit the size of the buffer, but
> > it's not trivial either to define that.
> > 
> > Hardware typically has maximum dimensions it can support, but the user may
> > want to add padding at the end of the lines. Perhaps a helper macro could
> > be used for this purpose: most likely there's no need to be more padding
> > than there's image data per line. If that turns out to be too restrictive,
> > the macro could be changed. That's probably unlikely, admittedly.
> > 
> > For some hardware these numbers could still be more than a 32-bit unsigned
> > integer can hold, so the check is still needed.
> 
> I guess that, by changing from "int" to "unsigned long", we ensure that the 
> number should be big enough to be able to represent the maximum allocation
> size.
> 
> On Linux, sizeof(long) is usually assumed to be sizeof(void *). Such
> assumption is used, for example, when we pass a structure pointer to
> ioctl's, instead of passing a long integer.
> 
> I mean, on a 64 bits system, long has 64 bits. AFAIKT, even the latest
> Xeon CPUs, the address space is lower than 64 bits. So, if one tries to
> allocate a memory with sizeof(ULONG_MAX), this will fail with ENOMEM.
> 
> On any (true) 32 bits system, the physical address is to 32 bits.
> So, if one tries to allocate a memory with ULONG_MAX, this should
> also fail, as there won't be memory for anything else.
> 
> There are some special cases, like X86_PAE (and ARM_LPAE). There, the 
> physical address space is 64 bits, but instruction set is the 32 bits one.
> Yet, I'm almost sure that (at least on x86) a single memory block there 
> can't be bigger than 32 bits.
> 
> What I'm trying to say is that I strongly suspect that we won't have 
> any cases where someone using would need a buffer with more than 
> 32 bits size on a non-64 architecture.

I agree; also the length field in struct v4l2_buffer is a __u32 so that
limits the value range for size as well.

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

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

[Mauro Carvalho Chehab]

Em Tue, 08 Jan 2019 18:05:57 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> On Tuesday, 8 January 2019 16:30:22 EET Mauro Carvalho Chehab wrote:
> > Em Tue, 8 Jan 2019 15:40:47 +0200
> > 
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:  
> > > On Tue, Jan 08, 2019 at 10:59:55AM -0200, Mauro Carvalho Chehab wrote:  
> > > > Em Tue, 8 Jan 2019 10:52:12 -0200
> > > > 
> > > > Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:  
> > > > > Em Tue,  8 Jan 2019 10:58:34 +0200
> > > > > 
> > > > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:  
> > > > > > 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>
> > > > > > ---
> > > > > > 
> > > > > >  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
> > > > > > 0ca81d495bda..0234ddbfa4de 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;
> > > > > > +  
> > > > > 
> > > > > Sorry, but I can't see how this could ever happen (except for a very
> > > > > serious bug at the compiler or at the hardware).
> > > > > 
> > > > > See, the definition at PAGE_ALIGN is (from mm.h):
> > > > > 	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
> > > > > 
> > > > > and the macro it uses come from kernel.h:
> > > > > 	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))  
> (a) -
> > > > > 	1)
> > > > > 	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> > > > > 	..
> > > > > 	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
> > > > > 
> > > > > So, this:
> > > > > 	size = PAGE_ALIGN(length);
> > > > > 
> > > > > (assuming PAGE_SIZE= 0x1000)
> > > > > 
> > > > > becomes:
> > > > > 	size = (length + 0x0fff) & ~0xfff;
> > > > > 
> > > > > so, size will *always* be >= length.  
> > > > 
> > > > Hmm... after looking at patch 2, now I understand what's your concern...
> > > > 
> > > > If someone indeed uses length = INT_MAX, size will indeed be zero.
> > > > 
> > > > Please adjust the description accordingly, as it doesn't reflect
> > > > that.  
> > > 
> > > How about:
> > > 
> > > PAGE_ALIGN() may wrap the buffer length around to 0 if the value to be
> > > aligned is close to the top of the value range of the type. Prevent this
> > > by
> > > checking that the aligned value is not smaller than the unaligned one.  
> > 
> > I would be a way more clear, as this is there to prevent a single
> > special case: length == ULEN_MAX. Something like:
> > 
> > 	If one tried to allocate a buffer with sizeof(ULEN_MAX), this will cause
> > 	an overflow at PAGE_ALIGN(), making it return zero as the size of the
> > 	buffer, causing the code to fail.
> > 
> > I would even let it clearer at the code itself. So, instead of the
> > hunk you proposed, I would do:
> > 
> > 	unsigned long size = vb->planes[plane].length;
> > 
> > 	/* Prevent PAGE_ALIGN overflow */
> > 	if (WARN_ON(size == ULONG_MAX))
> > 		goto free;  
> 
> ULONG_MAX - PAGE_SIZE + 2 to ULONG_MAX would all cause the same issue.

True. The actual check should be:

	if (WARN_ON(size > ULONG_MAX - PAGE_SIZE + 1))

Not that the original proposal of checking after the overflow is wrong, but, 
IMHO, something linking the size to ULONG_MAX makes clearer about what kind
of issue the code is meant to solve. A good comment before that would work
fine.

Thanks,
Mauro

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

[Sakari Ailus]

Hi Mauro,

On Wed, Jan 09, 2019 at 10:13:42AM -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 08 Jan 2019 18:05:57 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> 
> > On Tuesday, 8 January 2019 16:30:22 EET Mauro Carvalho Chehab wrote:
> > > Em Tue, 8 Jan 2019 15:40:47 +0200
> > > 
> > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:  
> > > > On Tue, Jan 08, 2019 at 10:59:55AM -0200, Mauro Carvalho Chehab wrote:  
> > > > > Em Tue, 8 Jan 2019 10:52:12 -0200
> > > > > 
> > > > > Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:  
> > > > > > Em Tue,  8 Jan 2019 10:58:34 +0200
> > > > > > 
> > > > > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:  
> > > > > > > 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>
> > > > > > > ---
> > > > > > > 
> > > > > > >  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
> > > > > > > 0ca81d495bda..0234ddbfa4de 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;
> > > > > > > +  
> > > > > > 
> > > > > > Sorry, but I can't see how this could ever happen (except for a very
> > > > > > serious bug at the compiler or at the hardware).
> > > > > > 
> > > > > > See, the definition at PAGE_ALIGN is (from mm.h):
> > > > > > 	#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
> > > > > > 
> > > > > > and the macro it uses come from kernel.h:
> > > > > > 	#define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (typeof(x))  
> > (a) -
> > > > > > 	1)
> > > > > > 	#define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> > > > > > 	..
> > > > > > 	#define ALIGN(x, a)		__ALIGN_KERNEL((x), (a))
> > > > > > 
> > > > > > So, this:
> > > > > > 	size = PAGE_ALIGN(length);
> > > > > > 
> > > > > > (assuming PAGE_SIZE= 0x1000)
> > > > > > 
> > > > > > becomes:
> > > > > > 	size = (length + 0x0fff) & ~0xfff;
> > > > > > 
> > > > > > so, size will *always* be >= length.  
> > > > > 
> > > > > Hmm... after looking at patch 2, now I understand what's your concern...
> > > > > 
> > > > > If someone indeed uses length = INT_MAX, size will indeed be zero.
> > > > > 
> > > > > Please adjust the description accordingly, as it doesn't reflect
> > > > > that.  
> > > > 
> > > > How about:
> > > > 
> > > > PAGE_ALIGN() may wrap the buffer length around to 0 if the value to be
> > > > aligned is close to the top of the value range of the type. Prevent this
> > > > by
> > > > checking that the aligned value is not smaller than the unaligned one.  
> > > 
> > > I would be a way more clear, as this is there to prevent a single
> > > special case: length == ULEN_MAX. Something like:
> > > 
> > > 	If one tried to allocate a buffer with sizeof(ULEN_MAX), this will cause
> > > 	an overflow at PAGE_ALIGN(), making it return zero as the size of the
> > > 	buffer, causing the code to fail.
> > > 
> > > I would even let it clearer at the code itself. So, instead of the
> > > hunk you proposed, I would do:
> > > 
> > > 	unsigned long size = vb->planes[plane].length;
> > > 
> > > 	/* Prevent PAGE_ALIGN overflow */
> > > 	if (WARN_ON(size == ULONG_MAX))
> > > 		goto free;  
> > 
> > ULONG_MAX - PAGE_SIZE + 2 to ULONG_MAX would all cause the same issue.
> 
> True. The actual check should be:
> 
> 	if (WARN_ON(size > ULONG_MAX - PAGE_SIZE + 1))

That is also wrong: aligning ULONG_MAX - PAGE_SIZE + 1 to page is 0, too.

> 
> Not that the original proposal of checking after the overflow is wrong, but, 
> IMHO, something linking the size to ULONG_MAX makes clearer about what kind
> of issue the code is meant to solve. A good comment before that would work
> fine.

I find the original test more simple to grasp: it is independent of how the
alignment is done, to PAGE_SIZE or to something else. It was also right
from the start unlike the two other checks that have been proposed so far.
Therefore my preference to stick to that. The slight downside is that it is
not apparent from the problem is related to aligning to page, but that is
mitigated by the comment added by this patch.

This property of PAGE_ALIGN() is not usually relevant as typically it's not
used to align addresses on the last page (of the value range).

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com