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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx> escreveu:
> > > > > > Em Tue, 8 Jan 2019 10:58:34 +0200
> > > > > >
> > > > > > Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > > > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> > > > > > > ---
> > > > > > >
> > > > > > > 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).
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
