diff mbox series

[HID,Patchsets,v1,2/2] HID: Removed USB Validation check

Message ID 20230724084752.371245-3-sandeep.cs@samsung.com (mailing list archive)
State New
Delegated to: Jiri Kosina
Headers show
Series HID Patchsets for Samsung driver | expand

Commit Message

sandeep.cs July 24, 2023, 8:47 a.m. UTC 
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.

This change improves driver compatibility and extends its support for a wide range of devices.

Signed-off-by: Sandeep C S<sandeep.cs@samsung.com>
Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
Signed-off-by: Jitender Sajwan <jitender.s21@samsung.com>
---
 drivers/hid/hid-samsung.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Benjamin Tissoires July 24, 2023, 10:10 a.m. UTC | #1 
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@samsung.com>
> Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
> Signed-off-by: Jitender Sajwan <jitender.s21@samsung.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
>
sandeep.cs July 31, 2023, 11:13 a.m. UTC | #2 
Hi Benjamin,

Thanks for the quick review our changes

As suggested we will refactor as below and send you an update shortly.


1. USB check validation moving to appropriate function
(samsung_kbd_mouse_input_mapping())
2. fix the checkpatch complain
3+ Split the remaining changes one per device

Clarifying for the Point 1 , below is the pseudocode:

static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev,
  struct hid_input *hi, struct hid_field *field, struct hid_usage *usage,
  unsigned long **bit, int *max)
{
 + if (!hid_is_usb(hdev))
   + return 0;
    ...
}   


Thanks & Regards
Sandeep C S

-----Original Message-----
From: Benjamin Tissoires <bentiss@kernel.org> 
Sent: 24 July 2023 15:40
To: sandeep.cs <sandeep.cs@samsung.com>
Cc: Jiri Kosina <jikos@kernel.org>; Benjamin Tissoires
<benjamin.tissoires@redhat.com>; junwan.cho@samsung.com;
jitender.s21@samsung.com; suhyun_.kim@samsung.com; ih0923.kim@samsung.com;
gaudium.lee@samsung.com; linux-input@vger.kernel.org;
linux-kernel@vger.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@samsung.com>
> Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
> Signed-off-by: Jitender Sajwan <jitender.s21@samsung.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
>
sandeep.cs Aug. 16, 2023, 9:46 a.m. UTC | #3 
Hi Benjamin,

I hope this email finds you well. 
I wanted to remind you about our last email where we discussed about moving
USB validation check.
Your thoughts and input are important to us.
Please guide!

Thanks & regards
Sandeep C S

-----Original Message-----
From: sandeep.cs <sandeep.cs@samsung.com> 
Sent: 31 July 2023 16:44
To: 'Benjamin Tissoires' <bentiss@kernel.org>
Cc: 'Jiri Kosina' <jikos@kernel.org>; 'Benjamin Tissoires'
<benjamin.tissoires@redhat.com>; 'junwan.cho@samsung.com'
<junwan.cho@samsung.com>; 'jitender.s21@samsung.com'
<jitender.s21@samsung.com>; 'suhyun_.kim@samsung.com'
<suhyun_.kim@samsung.com>; 'ih0923.kim@samsung.com'
<ih0923.kim@samsung.com>; 'gaudium.lee@samsung.com'
<gaudium.lee@samsung.com>; 'linux-input@vger.kernel.org'
<linux-input@vger.kernel.org>; 'linux-kernel@vger.kernel.org'
<linux-kernel@vger.kernel.org>
Subject: RE: [HID Patchsets v1 2/2] HID: Removed USB Validation check

Hi Benjamin,

Thanks for the quick review our changes

As suggested we will refactor as below and send you an update shortly.


1. USB check validation moving to appropriate function
(samsung_kbd_mouse_input_mapping())
2. fix the checkpatch complain
3+ Split the remaining changes one per device

Clarifying for the Point 1 , below is the pseudocode:

static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev,
  struct hid_input *hi, struct hid_field *field, struct hid_usage *usage,
  unsigned long **bit, int *max)
{
 + if (!hid_is_usb(hdev))
   + return 0;
    ...
}   


Thanks & Regards
Sandeep C S

-----Original Message-----
From: Benjamin Tissoires <bentiss@kernel.org>
Sent: 24 July 2023 15:40
To: sandeep.cs <sandeep.cs@samsung.com>
Cc: Jiri Kosina <jikos@kernel.org>; Benjamin Tissoires
<benjamin.tissoires@redhat.com>; junwan.cho@samsung.com;
jitender.s21@samsung.com; suhyun_.kim@samsung.com; ih0923.kim@samsung.com;
gaudium.lee@samsung.com; linux-input@vger.kernel.org;
linux-kernel@vger.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@samsung.com>
> Signed-off-by: Junwan Cho <junwan.cho@samsung.com>
> Signed-off-by: Jitender Sajwan <jitender.s21@samsung.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
>
diff mbox series

Patch

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");