| Message ID | e43980df-1ca5-459d-b037-788dd7d9085d@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: stk1160: fix bounds checking in stk1160_copy_video() | expand |
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. 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!
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;
Hi Dan Thanks for the patch On Mon, 22 Apr 2024 at 19:23, Dan Carpenter <dan.carpenter@linaro.org> 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@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; 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@chromium.org> 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; >
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; }
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(-)