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
