linux-kernel - Re: [HID Patchsets v1 2/2] HID: Removed USB Validation check

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]

Message-ID: <np2hnhy7jcc66rsfwym42qwteqda6slszinrjqb6jg7ie4qt3e@2fllxsza544p>
Date:   Mon, 24 Jul 2023 12:10:15 +0200
From:   Benjamin Tissoires <bentiss@...nel.org>
To:     "sandeep.cs" <sandeep.cs@...sung.com>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        junwan.cho@...sung.com, jitender.s21@...sung.com,
        suhyun_.kim@...sung.com, ih0923.kim@...sung.com,
        gaudium.lee@...sung.com, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [HID Patchsets v1 2/2] HID: Removed USB Validation check

Hi Sandeep,

On Jul 24 2023, sandeep.cs wrote:
> Earlier Samsung driver only handles USB HID devices and returns an error if it encounters a Bluetooth type of HID device.
> By removing this USB validation check, we allow the driver to handle other types of HID devices including Bluetooth HID devices, which were previously excluded.

Please no, not with that patch at least.

hid_is_usb() protects the kernel from making an oops if the actual
transport layer is not USB, let's say an emulated uhid device. So by
removing that check you are just allowing anybody with root access to
access random memory in the kernel.

The correct fix is to move the check where it's needed, in
samsung_kbd_mouse_input_mapping().
I'll let you decide what need should be done if it's not a USB device
there: consider the interface to be 0 or just abort the function.

Cheers,
Benjamin

> 
> This change improves driver compatibility and extends its support for a wide range of devices.
> 
> Signed-off-by: Sandeep C S<sandeep.cs@...sung.com>
> Signed-off-by: Junwan Cho <junwan.cho@...sung.com>
> Signed-off-by: Jitender Sajwan <jitender.s21@...sung.com>
> ---
>  drivers/hid/hid-samsung.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
> index 33e963303d11..3cafbf4d9dc6 100644
> --- a/drivers/hid/hid-samsung.c
> +++ b/drivers/hid/hid-samsung.c
> @@ -517,9 +517,6 @@ static int samsung_probe(struct hid_device *hdev,
>  	int ret;
>  	unsigned int cmask = HID_CONNECT_DEFAULT;
>  
> -	if (!hid_is_usb(hdev))
> -		return -EINVAL;
> -
>  	ret = hid_parse(hdev);
>  	if (ret) {
>  		hid_err(hdev, "parse failed\n");
> -- 
> 2.25.1
>