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

[Dan Carpenter <dan.carpenter@linaro.org>]

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@linaro.org> 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@linaro.org>
> > ---
> > 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;

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;
	}
	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;