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

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

[Dan Carpenter]

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.

Fixes: 9cb2173e6ea8 ("[media] media: Add stk1160 new driver (easycap replacement)")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
This patch is untested, I just spotted it in review.

When this bug is fixed, the two checks for negative values of "lencopy"
could be removed.  I wrote a version of this patch which removed the
checks, but in the end I decided to leave the checks.  They're harmless.

 drivers/media/usb/stk1160/stk1160-video.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c
index 366f0e4a5dc0..bfb97ea352e7 100644
--- a/drivers/media/usb/stk1160/stk1160-video.c
+++ b/drivers/media/usb/stk1160/stk1160-video.c
@@ -139,8 +139,8 @@ 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;
+	if (lencopy > buf->length - buf->bytesused) {
+		lencopy = buf->length - buf->bytesused;
 		remain = lencopy;
 	}
 
@@ -182,8 +182,8 @@ 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;
+		if (lencopy > buf->length - buf->bytesused) {
+			lencopy = buf->length - buf->bytesused;
 			remain = lencopy;
 		}
 
-- 
2.43.0

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

[Ricardo Ribalda]

Hi Dan

On Wed, 17 Apr 2024 at 19:51, 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.
>
> Fixes: 9cb2173e6ea8 ("[media] media: Add stk1160 new driver (easycap replacement)")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> This patch is untested, I just spotted it in review.
>
> When this bug is fixed, the two checks for negative values of "lencopy"
> could be removed.  I wrote a version of this patch which removed the
> checks, but in the end I decided to leave the checks.  They're harmless.
>
>  drivers/media/usb/stk1160/stk1160-video.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c
> index 366f0e4a5dc0..bfb97ea352e7 100644
> --- a/drivers/media/usb/stk1160/stk1160-video.c
> +++ b/drivers/media/usb/stk1160/stk1160-video.c
> @@ -139,8 +139,8 @@ 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;
> +       if (lencopy > buf->length - buf->bytesused) {
> +               lencopy = buf->length - buf->bytesused;
>                 remain = lencopy;
>         }

I think it is a bit more complicated than bytesused.

bytesused does not take into consideration the actual position where
it is going to write.

What we really want to check is if

offset = dst - buf->mem;
if (offset + lencopy > buf->length) {
  lencopy = buf->length - offset;
  remain = lencopy;
}

>
> @@ -182,8 +182,8 @@ 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;
> +               if (lencopy > buf->length - buf->bytesused) {
> +                       lencopy = buf->length - buf->bytesused;
>                         remain = lencopy;
>                 }
>
> --
> 2.43.0
>


-- 
Ricardo Ribalda

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

[Dan Carpenter]

On Wed, Apr 17, 2024 at 08:48:23PM +0200, Ricardo Ribalda wrote:
> Hi Dan
> 
> On Wed, 17 Apr 2024 at 19:51, 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.
> >
> > Fixes: 9cb2173e6ea8 ("[media] media: Add stk1160 new driver (easycap replacement)")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > This patch is untested, I just spotted it in review.
> >
> > When this bug is fixed, the two checks for negative values of "lencopy"
> > could be removed.  I wrote a version of this patch which removed the
> > checks, but in the end I decided to leave the checks.  They're harmless.
> >
> >  drivers/media/usb/stk1160/stk1160-video.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c
> > index 366f0e4a5dc0..bfb97ea352e7 100644
> > --- a/drivers/media/usb/stk1160/stk1160-video.c
> > +++ b/drivers/media/usb/stk1160/stk1160-video.c
> > @@ -139,8 +139,8 @@ 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;
> > +       if (lencopy > buf->length - buf->bytesused) {
> > +               lencopy = buf->length - buf->bytesused;
> >                 remain = lencopy;
> >         }
> 
> I think it is a bit more complicated than bytesused.
> 
> bytesused does not take into consideration the actual position where
> it is going to write.
> 
> What we really want to check is if
> 
> offset = dst - buf->mem;
> if (offset + lencopy > buf->length) {
>   lencopy = buf->length - offset;
>   remain = lencopy;
> }
> 

You're right...  There is a comment explaining why we multiply the
number of lines written by two, but it doesn't really clarify anything
for me:

	/* Multiply linesdone by two, to take account of the other field */

What's the "other field"?

I kind of suspect that the stk1160_buffer_done() might be wrong as well.

	vb2_set_plane_payload(&buf->vb.vb2_buf, 0, buf->bytesused);
                                                   ^^^^^^^^^^^^^^

We're calculating the space left based on ->pos which can be reset to
zero in stk1160_process_isoc().  But ->bytesused isn't reset, so
potentially we could end up in a situation where ->bytesused is greater
than the ->length of the buffer.  Should stk1160_process_isoc() set
->bytesused to zero as well?

regards,
dan carpenter

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

[Ricardo Ribalda]

Hi Dan

On Mon, 22 Apr 2024 at 17:30, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Wed, Apr 17, 2024 at 08:48:23PM +0200, Ricardo Ribalda wrote:
> > Hi Dan
> >
> > On Wed, 17 Apr 2024 at 19:51, 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.
> > >
> > > Fixes: 9cb2173e6ea8 ("[media] media: Add stk1160 new driver (easycap replacement)")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > ---
> > > This patch is untested, I just spotted it in review.
> > >
> > > When this bug is fixed, the two checks for negative values of "lencopy"
> > > could be removed.  I wrote a version of this patch which removed the
> > > checks, but in the end I decided to leave the checks.  They're harmless.
> > >
> > >  drivers/media/usb/stk1160/stk1160-video.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c
> > > index 366f0e4a5dc0..bfb97ea352e7 100644
> > > --- a/drivers/media/usb/stk1160/stk1160-video.c
> > > +++ b/drivers/media/usb/stk1160/stk1160-video.c
> > > @@ -139,8 +139,8 @@ 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;
> > > +       if (lencopy > buf->length - buf->bytesused) {
> > > +               lencopy = buf->length - buf->bytesused;
> > >                 remain = lencopy;
> > >         }
> >
> > I think it is a bit more complicated than bytesused.
> >
> > bytesused does not take into consideration the actual position where
> > it is going to write.
> >
> > What we really want to check is if
> >
> > offset = dst - buf->mem;
> > if (offset + lencopy > buf->length) {
> >   lencopy = buf->length - offset;
> >   remain = lencopy;
> > }
> >
>
> You're right...  There is a comment explaining why we multiply the
> number of lines written by two, but it doesn't really clarify anything
> for me:
>
>         /* Multiply linesdone by two, to take account of the other field */
>
> What's the "other field"?

I guess it p[0].


Looks like the device sends first the data for the even lines, and
then the data for the odd lines (or the other way around).
And we are descrambling the data in the kernel

The code uses p[0] to figure out if it is the beginning of a block of
odds/evens, and if it is odd or even

if (p[0] == 0xc0 || p[0] == 0x80) {
/* We set next packet parity and
* continue to get next one
*/
dev->isoc_ctl.buf->odd = *p & 0x40;
dev->isoc_ctl.buf->pos = 0;
continue;
}



>
> I kind of suspect that the stk1160_buffer_done() might be wrong as well.
>
>         vb2_set_plane_payload(&buf->vb.vb2_buf, 0, buf->bytesused);
>                                                    ^^^^^^^^^^^^^^
>
> We're calculating the space left based on ->pos which can be reset to
> zero in stk1160_process_isoc().  But ->bytesused isn't reset, so
> potentially we could end up in a situation where ->bytesused is greater
> than the ->length of the buffer.  Should stk1160_process_isoc() set
> ->bytesused to zero as well?

I do not think so. bytes->used is len(odd) + len(even). If you reset
bytesused then you are only returning 1/2 the size


>
> regards,
> dan carpenter
>


-- 
Ricardo Ribalda