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

[Mauro Carvalho Chehab <mchehab+samsung@kernel.org>]

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