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

[Benjamin Tissoires <bentiss@kernel.org>]

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