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