Re: [PATCH v2] media: stk1160: fix bounds checking in stk1160_copy_video()

[Ricardo Ribalda]

Hi Dan

Thanks for the patch

On Mon, 22 Apr 2024 at 19:23, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Mon, Apr 22, 2024 at 05:52:36PM +0800, Ricardo Ribalda wrote:
> > Hi Dan
> >
> > On Mon, 22 Apr 2024 at 17:32, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > >
> > > The subtract in this condition is reversed.  The ->length is the length
> > > of the buffer.  The ->bytesused is how many bytes we have copied thus
> > > far.  When the condition is reversed that means the result of the
> > > subtraction is always negative but since it's unsigned then the result
> > > is a very high positive value.  That means the overflow check is never
> > > true.
> > >
> > > Additionally, the ->bytesused doesn't actually work for this purpose
> > > because we're not writing to "buf->mem + buf->bytesused".  Instead, the
> > > math to calculate the destination where we are writing is a bit
> > > involved.  You calculate the number of full lines already written,
> > > multiply by two, skip a line if necessary so that we start on an odd
> > > numbered line, and add the offset into the line.
> > >
> > > To fix this buffer overflow, just take the actual destination where we
> > > are writing, if the offset is already out of bounds print an error and
> > > return.  Otherwise, write up to buf->length bytes.
> > >
> > > Fixes: 9cb2173e6ea8 ("[media] media: Add stk1160 new driver (easycap replacement)")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > ---
> > > v2: My first patch just reversed the if statement but that wasn't the
> > > correct fix.
> > >
> > > Ghanshyam Agrawal sent a patch last year to ratelimit the output from
> > > this function because it was spamming dmesg.  This patch should
> > > hopefully fix the issue.  Let me know if there are still problems.
> > >
> > >  drivers/media/usb/stk1160/stk1160-video.c | 20 +++++++++++++++-----
> > >  1 file changed, 15 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c
> > > index 366f0e4a5dc0..e79c45db60ab 100644
> > > --- a/drivers/media/usb/stk1160/stk1160-video.c
> > > +++ b/drivers/media/usb/stk1160/stk1160-video.c
> > > @@ -99,7 +99,7 @@ void stk1160_buffer_done(struct stk1160 *dev)
> > >  static inline
> > >  void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len)
> > >  {
> > > -       int linesdone, lineoff, lencopy;
> > > +       int linesdone, lineoff, lencopy, offset;
> > >         int bytesperline = dev->width * 2;
> > >         struct stk1160_buffer *buf = dev->isoc_ctl.buf;
> > >         u8 *dst = buf->mem;
> > > @@ -139,8 +139,13 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len)
> > >          * Check if we have enough space left in the buffer.
> > >          * In that case, we force loop exit after copy.
> > >          */
> > > -       if (lencopy > buf->bytesused - buf->length) {
> > > -               lencopy = buf->bytesused - buf->length;
> > > +       offset = dst - (u8 *)buf->mem;
> > > +       if (offset > buf->length) {
> > Maybe you want offset >= buf->length.
> >
>
> The difference between > and >= is whether or not we print an error
> message.  In the original code, we didn't print an error message for
> this and I feel like that's the correct behavior
>
> > And remember to add at the beginning of the function
> >
> > if (!len)
> >  return 0;
> >
>
> That's checked in the caller so it's fine.
>
>    260                  /* Empty packet */
>    261                  if (len <= 4)
>    262                          continue;

It is also checked later on:

/* Check if the copy is done */
if (lencopy == 0 || remain == 0)
return;

I meant that we could move that check to the beginning of the funcion

But I agree, the scope of this patch is to fix the error not to
improve the code.

The stubborn part of me still thinks that it is better  offset >=
buf->length. :P
But even without that you can add my

Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>

I wish we could find someone to test it though.

>
> Generally we don't add duplicate checks.
>
> > And I would have done:
> > len -= 4;
> > src += 4;
> >
> > In the caller function
> >
>
> I don't really think it makes sense to move that into the caller and
> anyway, doing cleanups like this is outside the scope of this patch.
> Really, there is a lot that could be cleaned up here.  People knew there
> was a bug here but they didn't figure out what was causing it.  We could
> delete that code.  Looking at it now, I think that code would actually
> be enough to prevent a buffer overflow, although the correct behavior is
> to write up to the end of the buffer instead of returning early.
> Probably?
>
> To be honest, I'm still concerned there is a read overflow in
> stk1160_buffer_done().  I'd prefer to do
>
>         len = buf->bytesused;
>         if (len > buf->length) {
>                 dev_warn_ratelimited(dev->dev, "buf->bytesused invalid %u\n", len);
>                 len = buf->length;
>         }

After your patch I cannot see when this condition will be hitten...
but it is a cheap check, better safe than sorry.


>         vb2_set_plane_payload(&buf->vb.vb2_buf, 0, len);
>
> regards,
> dan carpenter
>
> diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c
> index ed261f0241da..f7977b07c066 100644
> --- a/drivers/media/usb/stk1160/stk1160-video.c
> +++ b/drivers/media/usb/stk1160/stk1160-video.c
> @@ -112,16 +112,6 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len)
>         u8 *dst = buf->mem;
>         int remain;
>
> -       /*
> -        * TODO: These stk1160_dbg are very spammy!
> -        * We should check why we are getting them.
> -        *
> -        * UPDATE: One of the reasons (the only one?) for getting these
> -        * is incorrect standard (mismatch between expected and configured).
> -        * So perhaps, we could add a counter for errors. When the counter
> -        * reaches some value, we simply stop streaming.
> -        */
> -
>         len -= 4;
>         src += 4;
>
> @@ -160,18 +150,6 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len)
>         if (lencopy == 0 || remain == 0)
>                 return;
>
> -       /* Let the bug hunt begin! sanity checks! */
> -       if (lencopy < 0) {
> -               printk_ratelimited(KERN_DEBUG "copy skipped: negative lencopy\n");
> -               return;
> -       }
> -
> -       if ((unsigned long)dst + lencopy >
> -               (unsigned long)buf->mem + buf->length) {
> -               printk_ratelimited(KERN_WARNING "stk1160: buffer overflow detected\n");
> -               return;
> -       }
> -
>         memcpy(dst, src, lencopy);
>
>         buf->bytesused += lencopy;
> @@ -208,17 +186,6 @@ void stk1160_copy_video(struct stk1160 *dev, u8 *src, int len)
>                 if (lencopy == 0 || remain == 0)
>                         return;
>
> -               if (lencopy < 0) {
> -                       printk_ratelimited(KERN_WARNING "stk1160: negative lencopy detected\n");
> -                       return;
> -               }
> -
> -               if ((unsigned long)dst + lencopy >
> -                       (unsigned long)buf->mem + buf->length) {
> -                       printk_ratelimited(KERN_WARNING "stk1160: buffer overflow detected\n");
> -                       return;
> -               }
> -
>                 memcpy(dst, src, lencopy);
>                 remain -= lencopy;
>


-- 
Ricardo Ribalda