linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] HID: u2fzero: clarify error check and length calculations
@ 2021-10-19 15:29 Andrej Shadura
  2021-10-19 15:29 ` [PATCH v3 2/2] HID: u2fzero: properly handle timeouts in usb_submit_urb Andrej Shadura
  2021-10-27  8:17 ` [PATCH v3 1/2] HID: u2fzero: clarify error check and length calculations Jiri Kosina
  0 siblings, 2 replies; 3+ messages in thread
From: Andrej Shadura @ 2021-10-19 15:29 UTC (permalink / raw)
  To: Jiří Kosina; +Cc: linux-input, linux-usb, stable, kernel, Alan Stern

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.

To make the code easier to read, calculate the minimal packet length
once and separately, and assign it to a signed int variable to eliminate
unsigned math and the need for type casts. It then becomes immediately
obvious how the actual data length is calculated and why the return
value cannot be less than the minimal length.

Fixes: 22d65765f211 ("HID: u2fzero: ignore incomplete packets without data")
Fixes: 42337b9d4d95 ("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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-u2fzero.c b/drivers/hid/hid-u2fzero.c
index d70cd3d7f583..94f78ffb76d0 100644
--- a/drivers/hid/hid-u2fzero.c
+++ b/drivers/hid/hid-u2fzero.c
@@ -191,6 +191,8 @@ static int u2fzero_rng_read(struct hwrng *rng, void *data,
 	struct u2f_hid_msg resp;
 	int ret;
 	size_t actual_length;
+	/* valid packets must have a correct header */
+	int min_length = offsetof(struct u2f_hid_msg, init.data);
 
 	if (!dev->present) {
 		hid_dbg(dev->hdev, "device not present");
@@ -200,12 +202,12 @@ 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 < min_length)
 		return 0;
 
 	/* only take the minimum amount of data it is safe to take */
-	actual_length = min3((size_t)ret - offsetof(struct u2f_hid_msg,
-		init.data), U2F_HID_MSG_LEN(resp), max);
+	actual_length = min3((size_t)ret - min_length,
+		U2F_HID_MSG_LEN(resp), max);
 
 	memcpy(data, resp.init.data, actual_length);
 
-- 
2.33.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH v3 2/2] HID: u2fzero: properly handle timeouts in usb_submit_urb
  2021-10-19 15:29 [PATCH v3 1/2] HID: u2fzero: clarify error check and length calculations Andrej Shadura
@ 2021-10-19 15:29 ` Andrej Shadura
  2021-10-27  8:17 ` [PATCH v3 1/2] HID: u2fzero: clarify error check and length calculations Jiri Kosina
  1 sibling, 0 replies; 3+ messages in thread
From: Andrej Shadura @ 2021-10-19 15:29 UTC (permalink / raw)
  To: Jiří Kosina; +Cc: linux-input, linux-usb, stable, kernel, Alan Stern

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: 42337b9d4d95 ("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 94f78ffb76d0..67ae2b18e33a 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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3 1/2] HID: u2fzero: clarify error check and length calculations
  2021-10-19 15:29 [PATCH v3 1/2] HID: u2fzero: clarify error check and length calculations Andrej Shadura
  2021-10-19 15:29 ` [PATCH v3 2/2] HID: u2fzero: properly handle timeouts in usb_submit_urb Andrej Shadura
@ 2021-10-27  8:17 ` Jiri Kosina
  1 sibling, 0 replies; 3+ messages in thread
From: Jiri Kosina @ 2021-10-27  8:17 UTC (permalink / raw)
  To: Andrej Shadura; +Cc: linux-input, linux-usb, stable, kernel, Alan Stern

On Tue, 19 Oct 2021, 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.
> 
> To make the code easier to read, calculate the minimal packet length
> once and separately, and assign it to a signed int variable to eliminate
> unsigned math and the need for type casts. It then becomes immediately
> obvious how the actual data length is calculated and why the return
> value cannot be less than the minimal length.
> 
> Fixes: 22d65765f211 ("HID: u2fzero: ignore incomplete packets without data")
> Fixes: 42337b9d4d95 ("HID: add driver for U2F Zero built-in LED and RNG")
> Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>

Both now applied, thanks.

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-10-27  8:17 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 15:29 [PATCH v3 1/2] HID: u2fzero: clarify error check and length calculations Andrej Shadura
2021-10-19 15:29 ` [PATCH v3 2/2] HID: u2fzero: properly handle timeouts in usb_submit_urb Andrej Shadura
2021-10-27  8:17 ` [PATCH v3 1/2] HID: u2fzero: clarify error check and length calculations Jiri Kosina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).