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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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