LinuxTV Patchwork [v2,1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0

login
register
mail settings
Submitter Sakari Ailus
Date Jan. 8, 2019, 8:58 a.m.
Message ID <20190108085836.9376-2-sakari.ailus@linux.intel.com>
Download mbox | patch
Permalink /patch/53742/
State New
Headers show

Comments

Sakari Ailus - Jan. 8, 2019, 8:58 a.m.
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(+)
Mauro Carvalho Chehab - Jan. 8, 2019, 12:52 p.m.
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
Mauro Carvalho Chehab - Jan. 8, 2019, 12:59 p.m.
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
Mauro Carvalho Chehab - Jan. 8, 2019, 1:01 p.m.
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
Sakari Ailus - Jan. 8, 2019, 1:38 p.m.
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.
Sakari Ailus - Jan. 8, 2019, 1:40 p.m.
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.
Mauro Carvalho Chehab - Jan. 8, 2019, 2:23 p.m.
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
Mauro Carvalho Chehab - Jan. 8, 2019, 2:30 p.m.
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
Laurent Pinchart - Jan. 8, 2019, 4:05 p.m.
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);
Sakari Ailus - Jan. 9, 2019, 8:41 a.m.
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.
Mauro Carvalho Chehab - Jan. 9, 2019, 12:13 p.m.
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
Sakari Ailus - Jan. 9, 2019, 1:56 p.m.
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).

Patch

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

Privacy Policy