linux-kernel - Re: [PATCH] HID: hid-ntrig: fix the error handling code in ntrig

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:   Wed, 30 Mar 2022 17:25:40 +0200 (CEST)
From:   Jiri Kosina <jikos@...nel.org>
To:     Mingxuan Xiang <mx_xiang@...t.edu.cn>
cc:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Rafi Rubin <rafi@...s.upenn.edu>,
        Dongliang Mu <dzm91@...t.edu.cn>, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: hid-ntrig: fix the error handling code in
 ntrig_probe

On Sun, 13 Mar 2022, Mingxuan Xiang wrote:

> If sysfs_create_group in ntrig_probe fails, sysfs_remove_group in
> ntrig_remove would hit WARN().
> 
> Fix this by returning err in ntrig_probe rather than ignoring the
> failure of sysfs_create_group.
> 
> CC: Dongliang Mu <dzm91@...t.edu.cn>
> Fixes: eab32f5f6557 ("HID: ntrig: add sysfs access to filter parameters")
> Signed-off-by: Mingxuan Xiang <mx_xiang@...t.edu.cn>
> ---
>  drivers/hid/hid-ntrig.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> index b5d26f03fe6b..f49427815d98 100644
> --- a/drivers/hid/hid-ntrig.c
> +++ b/drivers/hid/hid-ntrig.c
> @@ -951,10 +951,14 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  
>  	ret = sysfs_create_group(&hdev->dev.kobj,
>  			&ntrig_attribute_group);
> -	if (ret)
> +	if (ret) {
>  		hid_err(hdev, "cannot create sysfs group\n");
> +		goto err_sysfs_create;
> +	}
>  
>  	return 0;
> +err_sysfs_create:
> +	hid_hw_stop(hdev);
>  err_free:
>  	kfree(nd);

Thanks for the fix.

However, I believe that the sole fact that sysfs_create_group() failed 
should not be a reason for bailing the probe completely (rendering the 
device completely dysfunctional), given the fact that the probe otherwise 
succeeded and this just means that the parameters can't be tuned in 
runtime.

So just taking a note whether we do actually have sysfs attributes created 
on probe and freeing them only conditionally on remove() sounds like a bit 
less drastic aproach to me.

Could you please adjust the fix and resubmit?

Thanks,

-- 
Jiri Kosina
SUSE Labs