Hi Mauro,
Thanks for the review.
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.
>
> Btw, in this particular case, I would use a WARN_ON(), as this is
> something that indicates not only a driver bug (as the driver is
> letting someone to request a buffer a way too big), but probably
What's the maximum size a driver should allow? I guess this could be seen
be a failure from the driver's part to limit the size of the buffer, but
it's not trivial either to define that.
Hardware typically has maximum dimensions it can support, but the user may
want to add padding at the end of the lines. Perhaps a helper macro could
be used for this purpose: most likely there's no need to be more padding
than there's image data per line. If that turns out to be too restrictive,
the macro could be changed. That's probably unlikely, admittedly.
For some hardware these numbers could still be more than a 32-bit unsigned
integer can hold, so the check is still needed.
> also an attempt from a hacker to try to crack the system.
This could be also v4l2-compliance setting the length field to -1. A
warning is worth it only if there's good chance there's e.g. a kernel bug
involved.
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
