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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > > Cc: [email protected] > > > > Reviewed-by: Hans Verkuil <[email protected]> > > > > --- > > > > 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 [email protected]
