Received-SPF: pass (google.com: domain of linux-kernel+bounces-153127-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <e43980df-1ca5-459d-b037-788dd7d9085d@moroto.mountain>
In-Reply-To: <e43980df-1ca5-459d-b037-788dd7d9085d@moroto.mountain>
From: Ricardo Ribalda <ribalda@chromium.org>
Date: Mon, 22 Apr 2024 17:52:36 +0800
Message-ID: <CANiDSCtjEPqEstuo92QeVK_rWkW9icsjKWakPyN19ETM+MJuuQ@mail.gmail.com>
Subject: Re: [PATCH v2] media: stk1160: fix bounds checking in stk1160_copy_video()
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: =?UTF-8?Q?Ezequiel_Garc=C3=ADa?= <elezegarcia@gmail.com>, 
	Ghanshyam Agrawal <ghanshyam1898@gmail.com>, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>, 
	Mauro Carvalho Chehab <mchehab@kernel.org>, linux-media@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

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!
-- 
Ricardo Ribalda