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 <[email protected]>
---
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) {
+ dev_warn_ratelimited(dev->dev, "out of bounds offset\n");
+ return;
+ }
+ if (lencopy > buf->length - offset) {
+ lencopy = buf->length - offset;
remain = lencopy;
}
@@ -182,8 +187,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) {
+ dev_warn_ratelimited(dev->dev, "offset out of bounds\n");
+ return;
+ }
+ if (lencopy > buf->length - offset) {
+ lencopy = buf->length - offset;
remain = lencopy;
}
--
2.43.0
Hi Dan
On Mon, 22 Apr 2024 at 17:32, Dan Carpenter <[email protected]> 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 <[email protected]>
> ---
> 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.
And remember to add at the beginning of the function
if (!len)
return 0;
And I would have done:
len -= 4;
src += 4;
In the caller function
> + dev_warn_ratelimited(dev->dev, "out of bounds offset\n");
> + return;
> + }
> + if (lencopy > buf->length - offset) {
> + lencopy = buf->length - offset;
> remain = lencopy;
> }
>
> @@ -182,8 +187,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) {
ditto >=
> + dev_warn_ratelimited(dev->dev, "offset out of bounds\n");
> + return;
> + }
> + if (lencopy > buf->length - offset) {
> + lencopy = buf->length - offset;
> remain = lencopy;
> }
>
> --
> 2.43.0
Thanks!
--
Ricardo Ribalda
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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;
Hi Dan
Thanks for the patch
On Mon, 22 Apr 2024 at 19:23, Dan Carpenter <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
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