[PATCH] usb: fotg210-hcd: delete an incorrect bounds test

[PATCH] usb: fotg210-hcd: delete an incorrect bounds test

[Dan Carpenter]

Here "temp" is the number of characters that we have written and "size"
is the size of the buffer.  The intent was clearly to say that if we have
written to the end of the buffer then stop.

However, for that to work the comparison should have been done on the
original "size" value instead of the "size -= temp" value.  Not only
will that not trigger when we want to, but there is a small chance that
it will trigger incorrectly before we want it to and we break from the
loop slightly earlier than intended.

This code was recently changed from using snprintf() to scnprintf().  With
snprintf() we likely would have continued looping and passed a negative
size parameter to snprintf().  This would have triggered an annoying
WARN().  Now that we have converted to scnprintf() "size" will never
drop below 1 and there is no real need for this test.  We could change
the condition to "if (temp <= 1) goto done;" but just deleting the test
is cleanest.

Fixes: 1dd33a9f1b95 ("usb: fotg210: Collect pieces of dual mode controller")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/usb/fotg210/fotg210-hcd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/fotg210/fotg210-hcd.c b/drivers/usb/fotg210/fotg210-hcd.c
index b2f8b53cc8ef..8c5aaf860635 100644
--- a/drivers/usb/fotg210/fotg210-hcd.c
+++ b/drivers/usb/fotg210/fotg210-hcd.c
@@ -426,8 +426,6 @@ static void qh_lines(struct fotg210_hcd *fotg210, struct fotg210_qh *qh,
 				td->urb);
 		size -= temp;
 		next += temp;
-		if (temp == size)
-			goto done;
 	}
 
 	temp = scnprintf(next, size, "\n");
@@ -435,7 +433,6 @@ static void qh_lines(struct fotg210_hcd *fotg210, struct fotg210_qh *qh,
 	size -= temp;
 	next += temp;
 
-done:
 	*sizep = size;
 	*nextp = next;
 }
-- 
2.42.0

Re: [PATCH] usb: fotg210-hcd: delete an incorrect bounds test

[Linus Walleij]

Hi Dan,

thanks for your patch!

On Fri, Dec 8, 2023 at 10:23 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:

> Here "temp" is the number of characters that we have written and "size"
> is the size of the buffer.  The intent was clearly to say that if we have
> written to the end of the buffer then stop.
>
> However, for that to work the comparison should have been done on the
> original "size" value instead of the "size -= temp" value.  Not only
> will that not trigger when we want to, but there is a small chance that
> it will trigger incorrectly before we want it to and we break from the
> loop slightly earlier than intended.
>
> This code was recently changed from using snprintf() to scnprintf().  With
> snprintf() we likely would have continued looping and passed a negative
> size parameter to snprintf().  This would have triggered an annoying
> WARN().  Now that we have converted to scnprintf() "size" will never
> drop below 1 and there is no real need for this test.  We could change
> the condition to "if (temp <= 1) goto done;" but just deleting the test
> is cleanest.

This is a great fix! Thanks for going the extra mile and fix this
when looking at the code.

> Fixes: 1dd33a9f1b95 ("usb: fotg210: Collect pieces of dual mode controller")

That's the wrong commit.
This commit just brings stuff together from old code...

I believe it should be:
Fixes: 7d50195f6c50 ("usb: host: Faraday fotg210-hcd driver")

It won't backport cleanly but it's the right commit.

> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

With the right Fixes:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] usb: fotg210-hcd: delete an incorrect bounds test

[Lee Jones]

On Fri, 08 Dec 2023, Dan Carpenter wrote:

> Here "temp" is the number of characters that we have written and "size"
> is the size of the buffer.  The intent was clearly to say that if we have
> written to the end of the buffer then stop.
> 
> However, for that to work the comparison should have been done on the
> original "size" value instead of the "size -= temp" value.  Not only
> will that not trigger when we want to, but there is a small chance that
> it will trigger incorrectly before we want it to and we break from the
> loop slightly earlier than intended.
> 
> This code was recently changed from using snprintf() to scnprintf().  With
> snprintf() we likely would have continued looping and passed a negative
> size parameter to snprintf().  This would have triggered an annoying
> WARN().  Now that we have converted to scnprintf() "size" will never
> drop below 1 and there is no real need for this test.  We could change
> the condition to "if (temp <= 1) goto done;" but just deleting the test
> is cleanest.
> 
> Fixes: 1dd33a9f1b95 ("usb: fotg210: Collect pieces of dual mode controller")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/usb/fotg210/fotg210-hcd.c | 3 ---
>  1 file changed, 3 deletions(-)

Super additional clean-up, thanks.

Reviewed-by: Lee Jones <lee@kernel.org>

> diff --git a/drivers/usb/fotg210/fotg210-hcd.c b/drivers/usb/fotg210/fotg210-hcd.c
> index b2f8b53cc8ef..8c5aaf860635 100644
> --- a/drivers/usb/fotg210/fotg210-hcd.c
> +++ b/drivers/usb/fotg210/fotg210-hcd.c
> @@ -426,8 +426,6 @@ static void qh_lines(struct fotg210_hcd *fotg210, struct fotg210_qh *qh,
>  				td->urb);
>  		size -= temp;
>  		next += temp;
> -		if (temp == size)
> -			goto done;
>  	}
>  
>  	temp = scnprintf(next, size, "\n");
> @@ -435,7 +433,6 @@ static void qh_lines(struct fotg210_hcd *fotg210, struct fotg210_qh *qh,
>  	size -= temp;
>  	next += temp;
>  
> -done:
>  	*sizep = size;
>  	*nextp = next;
>  }
> -- 
> 2.42.0
> 

-- 
Lee Jones [李琼斯]