HID: u2fzero: fix general protection fault in u2fzero_recv

[PATCH] HID: u2fzero: fix general protection fault in u2fzero_recv

Posted by l1za0.sec@gmail.com 1 month, 3 weeks ago

From: Haocheng Yu <l1za0.sec@gmail.com>

A general protection fault in u2fzero_recv is reported by a
modified Syzkaller-based kernel fuzzing tool we developed.

The cause is that u2fzero_probe() calls the u2fzero_fill_in_urb()
function but ignores its return value. When the urb setting fails,
dev->urb remains NULL, but u2fzero_probe() continues to run. When
`dev->urb->context = &ctx;` in u2fzero_recv() is executed, the
KASAN null pointer dereference crash will occur.

To fix this vulnerability, I added a check for the return value of
u2fzero_fill_in_urb() and aborted u2fzero_probe() on error. And I
added a NULL value check for dev->urb in u2fzero_recv() to further
ensure its integrity.

Signed-off-by: Haocheng Yu <l1za0.sec@gmail.com>
---
 drivers/hid/hid-u2fzero.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-u2fzero.c b/drivers/hid/hid-u2fzero.c
index 744a91e6e78c..c51f6dd80635 100644
--- a/drivers/hid/hid-u2fzero.c
+++ b/drivers/hid/hid-u2fzero.c
@@ -134,6 +134,12 @@ static int u2fzero_recv(struct u2fzero_device *dev,
 
 	memcpy(dev->buf_out, req, sizeof(struct u2f_hid_report));
 
+	if (!dev->urb) {
+		hid_err(hdev, "recv called without initialized URB");
+		ret = -ENODEV;
+		goto err;
+	}
+
 	dev->urb->context = &ctx;
 	init_completion(&ctx.done);
 
@@ -341,7 +347,11 @@ static int u2fzero_probe(struct hid_device *hdev,
 	if (ret)
 		return ret;
 
-	u2fzero_fill_in_urb(dev);
+	ret = u2fzero_fill_in_urb(dev);
+	if (ret) {
+		hid_hw_stop(hdev);
+		return ret;
+	}
 
 	dev->present = true;
 

base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
-- 
2.51.0

Re: [PATCH] HID: u2fzero: fix general protection fault in u2fzero_recv

Posted by Benjamin Tissoires 3 weeks, 5 days ago

Hi,

Thanks for the patch and sorry for the delay.

On Apr 21 2026, l1za0.sec@gmail.com wrote:
> From: Haocheng Yu <l1za0.sec@gmail.com>
> 
> A general protection fault in u2fzero_recv is reported by a
> modified Syzkaller-based kernel fuzzing tool we developed.
> 
> The cause is that u2fzero_probe() calls the u2fzero_fill_in_urb()
> function but ignores its return value. When the urb setting fails,
> dev->urb remains NULL, but u2fzero_probe() continues to run. When
> `dev->urb->context = &ctx;` in u2fzero_recv() is executed, the
> KASAN null pointer dereference crash will occur.
> 
> To fix this vulnerability, I added a check for the return value of
> u2fzero_fill_in_urb() and aborted u2fzero_probe() on error. And I
> added a NULL value check for dev->urb in u2fzero_recv() to further
> ensure its integrity.

Couple of things:
- sorry, but a couple of days later someone else send a patch to tackle
  the same problem at a better level IMO:
  https://lore.kernel.org/linux-input/20260424-u2fzero-probe-urb-unwind-v1-1-mhun512@gmail.com/
  So I'm going to take this one instead of yours. I would appreciate if
  you can confirm this fixes your current reproducer.

- Your commit description is not great. Please have a look at
  Documentation/process/submitting-patches.rst The thing that made me
  notice this is the non imperative form (quoting
  submitting-patches.rst):
  """
  Describe your changes in imperative mood,
  e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do
  frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders
  to the codebase to change its behaviour.
  """
  The paragraph above is also hard to follow but valuable for
  understanding why it needs to be fixed.

Again, in the end the problem is that is dev->urb is NULL, there are
little chances the system is healthy and the device will work correctly,
so it's best to bail out early in probe like the other patch I
mentioned.

Cheers,
Benjamin

> 
> Signed-off-by: Haocheng Yu <l1za0.sec@gmail.com>
> ---
>  drivers/hid/hid-u2fzero.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-u2fzero.c b/drivers/hid/hid-u2fzero.c
> index 744a91e6e78c..c51f6dd80635 100644
> --- a/drivers/hid/hid-u2fzero.c
> +++ b/drivers/hid/hid-u2fzero.c
> @@ -134,6 +134,12 @@ static int u2fzero_recv(struct u2fzero_device *dev,
>  
>  	memcpy(dev->buf_out, req, sizeof(struct u2f_hid_report));
>  
> +	if (!dev->urb) {
> +		hid_err(hdev, "recv called without initialized URB");
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
>  	dev->urb->context = &ctx;
>  	init_completion(&ctx.done);
>  
> @@ -341,7 +347,11 @@ static int u2fzero_probe(struct hid_device *hdev,
>  	if (ret)
>  		return ret;
>  
> -	u2fzero_fill_in_urb(dev);
> +	ret = u2fzero_fill_in_urb(dev);
> +	if (ret) {
> +		hid_hw_stop(hdev);
> +		return ret;
> +	}
>  
>  	dev->present = true;
>  
> 
> base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
> -- 
> 2.51.0
> 
>

Re: [PATCH] HID: u2fzero: fix general protection fault in u2fzero_recv

Posted by Li Zao 3 weeks, 4 days ago

> Couple of things:
> - sorry, but a couple of days later someone else send a patch to tackle
>   the same problem at a better level IMO:
>   https://lore.kernel.org/linux-input/20260424-u2fzero-probe-urb-unwind-v1-1-mhun512@gmail.com/
>   So I'm going to take this one instead of yours. I would appreciate if
>   you can confirm this fixes your current reproducer.
>
> - Your commit description is not great. Please have a look at
>   Documentation/process/submitting-patches.rst The thing that made me
>   notice this is the non imperative form (quoting
>   submitting-patches.rst):
>   """
>   Describe your changes in imperative mood,
>   e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do
>   frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders
>   to the codebase to change its behaviour.
>   """
>   The paragraph above is also hard to follow but valuable for
>   understanding why it needs to be fixed.
>
> Again, in the end the problem is that is dev->urb is NULL, there are
> little chances the system is healthy and the device will work correctly,
> so it's best to bail out early in probe like the other patch I
> mentioned.
>
> Cheers,
> Benjamin



Hi Benjamin,

I have verified the patch against our reproducer and can confirm that
it indeed fixes the issue. And thanks for the guidance on the commit
description. That's quite useful.

BTW, would it be possible to add a "Reported-by" tag to the patch? I'm
not sure about the exact convention for this situation, since the bug
was found by our private fuzzing tool and no public report is available
on the web.

Thanks again for your time and guidance!

Best regards,
Haocheng