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.
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
