[PATCH 1/2] HID: u2fzero: explicitly check for errors

[PATCH 1/2] HID: u2fzero: explicitly check for errors

[Andrej Shadura]

The previous commit fixed handling of incomplete packets but broke error
handling: offsetof returns an unsigned value (size_t), but when compared
against the signed return value, the return value is interpreted as if
it were unsigned, so negative return values are never less than the
offset.

Fixes: 22d65765f211c("HID: u2fzero: ignore incomplete packets without data")
Fixes: 42337b9d4d958("HID: add driver for U2F Zero built-in LED and RNG")
Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
---
 drivers/hid/hid-u2fzero.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-u2fzero.c b/drivers/hid/hid-u2fzero.c
index d70cd3d7f583..5145d758bea0 100644
--- a/drivers/hid/hid-u2fzero.c
+++ b/drivers/hid/hid-u2fzero.c
@@ -200,7 +200,7 @@ static int u2fzero_rng_read(struct hwrng *rng, void *data,
 	ret = u2fzero_recv(dev, &req, &resp);
 
 	/* ignore errors or packets without data */
-	if (ret < offsetof(struct u2f_hid_msg, init.data))
+	if (ret < 0 || ret < offsetof(struct u2f_hid_msg, init.data))
 		return 0;
 
 	/* only take the minimum amount of data it is safe to take */
-- 
2.33.0

[PATCH 2/2] HID: u2fzero: properly handle timeouts in usb_submit_urb

[Andrej Shadura]

The wait_for_completion_timeout function returns 0 if timed out or a
positive value if completed. Hence, "less than zero" comparison always
misses timeouts and doesn't kill the URB as it should, leading to
re-sending it while it is active.

Fixes: 42337b9d4d958("HID: add driver for U2F Zero built-in LED and RNG")
Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
---
 drivers/hid/hid-u2fzero.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-u2fzero.c b/drivers/hid/hid-u2fzero.c
index 5145d758bea0..562da98cfb82 100644
--- a/drivers/hid/hid-u2fzero.c
+++ b/drivers/hid/hid-u2fzero.c
@@ -132,7 +132,7 @@ static int u2fzero_recv(struct u2fzero_device *dev,
 
 	ret = (wait_for_completion_timeout(
 		&ctx.done, msecs_to_jiffies(USB_CTRL_SET_TIMEOUT)));
-	if (ret < 0) {
+	if (ret == 0) {
 		usb_kill_urb(dev->urb);
 		hid_err(hdev, "urb submission timed out");
 	} else {
-- 
2.33.0

Re: [PATCH 1/2] HID: u2fzero: explicitly check for errors

[Greg KH]

On Mon, Oct 18, 2021 at 08:47:59AM +0200, Andrej Shadura wrote:
> The previous commit fixed handling of incomplete packets but broke error
> handling: offsetof returns an unsigned value (size_t), but when compared
> against the signed return value, the return value is interpreted as if
> it were unsigned, so negative return values are never less than the
> offset.
> 
> Fixes: 22d65765f211c("HID: u2fzero: ignore incomplete packets without data")
> Fixes: 42337b9d4d958("HID: add driver for U2F Zero built-in LED and RNG")

Nit, you need a ' ' before the '(' character here, and you only need 12
digits of the sha1.  It should look like:
	Fixes: 22d65765f211 ("HID: u2fzero: ignore incomplete packets without data")

Also, how about a cc: stable for these as well?

thanks,

greg k-h

Re: [PATCH 1/2] HID: u2fzero: explicitly check for errors

[Andrej Shadura]

On 18/10/2021 08:56, Greg KH wrote:
> On Mon, Oct 18, 2021 at 08:47:59AM +0200, Andrej Shadura wrote:
>> The previous commit fixed handling of incomplete packets but broke error
>> handling: offsetof returns an unsigned value (size_t), but when compared
>> against the signed return value, the return value is interpreted as if
>> it were unsigned, so negative return values are never less than the
>> offset.
>>
>> Fixes: 22d65765f211c("HID: u2fzero: ignore incomplete packets without data")
>> Fixes: 42337b9d4d958("HID: add driver for U2F Zero built-in LED and RNG")
> 
> Nit, you need a ' ' before the '(' character here, and you only need 12
> digits of the sha1.  It should look like:
> 	Fixes: 22d65765f211 ("HID: u2fzero: ignore incomplete packets without data")

Thanks, is there a script or something to help generate those? (I’m 
surprised --fixup cannot generate them.)

> Also, how about a cc: stable for these as well?

Sure, will do.

-- 
Cheers,
   Andrej

Re: [PATCH 1/2] HID: u2fzero: explicitly check for errors

[Andrej Shadura]

On 18/10/2021 14:06, Andrej Shadura wrote:
> On 18/10/2021 08:56, Greg KH wrote:
>> On Mon, Oct 18, 2021 at 08:47:59AM +0200, Andrej Shadura wrote:
>>> The previous commit fixed handling of incomplete packets but broke error
>>> handling: offsetof returns an unsigned value (size_t), but when compared
>>> against the signed return value, the return value is interpreted as if
>>> it were unsigned, so negative return values are never less than the
>>> offset.
>>>
>>> Fixes: 22d65765f211c("HID: u2fzero: ignore incomplete packets without 
>>> data")
>>> Fixes: 42337b9d4d958("HID: add driver for U2F Zero built-in LED and 
>>> RNG")
>>
>> Nit, you need a ' ' before the '(' character here, and you only need 12
>> digits of the sha1.  It should look like:
>>     Fixes: 22d65765f211 ("HID: u2fzero: ignore incomplete packets 
>> without data")
> 
> Thanks, is there a script or something to help generate those? (I’m 
> surprised --fixup cannot generate them.)

Replying to myself, apparently the answer to this is now in the 
"submitting patches" document.

-- 
Cheers,
   Andrej