lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 27 Aug 2023 12:41:54 -0700
From:   Rahul Rameshbabu <rrameshbabu@...dia.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     jikos@...nel.org, benjamin.tissoires@...hat.com,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1/3] HID: nvidia-shield: Fix a missing
 led_classdev_unregister() in the probe error handling path

On Sat, 26 Aug, 2023 19:42:17 +0200 Christophe JAILLET <christophe.jaillet@...adoo.fr> wrote:
> The commit in Fixes updated the error handling path of
> thunderstrike_create() and the remove function but not the error handling
> path of shield_probe(), should an error occur after a successful
> thunderstrike_create() call.
>
> Add the missing call.

You are right that the led instance needs to be cleaned up here.
However, there is another bug that is introduced by this patch since
this is in the error path where hid_hw_start failed. The problem is that
led_classdev_unregister makes sure to set the led state to off in the
cleanup actions. The problem is that it is unsafe to do so in this
context since we cannot send hid_hw_raw_request at this point.

I discuss this in the following patch submission.

  https://lore.kernel.org/linux-input/20230807163620.16855-1-rrameshbabu@nvidia.com/

I think we should take the alternative approach I mentioned in the
mentioned patch where we set the LED_RETAIN_AT_SHUTDOWN led_classdev
flag. Then in the context of shield_remove, we can explicitly call
led_set_brightness(&ts->led_dev, LED_OFF).

You will want to base this suggested change on top of the for-6.6/nvidia
branch in the hid subsystem tree.

  https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.6/nvidia

>
> Fixes: f88af60e74a5 ("HID: nvidia-shield: Support LED functionality for Thunderstrike")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> ---
>  drivers/hid/hid-nvidia-shield.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
> index 9a3576dbf421..66a7478e2c9d 100644
> --- a/drivers/hid/hid-nvidia-shield.c
> +++ b/drivers/hid/hid-nvidia-shield.c
> @@ -1076,6 +1076,7 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  err_haptics:
>  	if (ts->haptics_dev)
>  		input_unregister_device(ts->haptics_dev);
> +	led_classdev_unregister(&ts->led_dev);
>  	return ret;
>  }

--
Thanks,

Rahul Rameshbabu

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ