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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxx
