[v1] HID: hid-ntrig: fix unable to handle page fault in ntrig_report_version()

[PATCH] HID: hid-ntrig: fix unable to handle page fault in ntrig_report_version()

Posted by Minjong Kim 2 months, 3 weeks ago

in ntrig_report_version(), hdev parameter passed from hid_probe().
sending descriptor to /dev/uhid can make hdev->dev.parent->parent to null
if hdev->dev.parent->parent is null, usb_dev has
invalid address(0xffffffffffffff58) that hid_to_usb_dev(hdev) returned
when usb_rcvctrlpipe() use usb_dev,it trigger
page fault error for address(0xffffffffffffff58)

add null check logic to ntrig_report_version()
before calling hid_to_usb_dev()

Signed-off-by: Minjong Kim <minbell.kim@samsung.com>
---
 drivers/hid/hid-ntrig.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 2738ce947434f904f32e9a1979b1681c66972ff9..96d3300655b5aa1621015b8e1fb511e6f616a713 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -139,6 +139,10 @@ static inline void ntrig_set_mode(struct hid_device *hdev, const int mode)
 
 static void ntrig_report_version(struct hid_device *hdev)
 {
+
+	if (!hdev->dev.parent->parent)
+		return;
+
 	int ret;
 	char buf[20];
 	struct usb_device *usb_dev = hid_to_usb_dev(hdev);

---
base-commit: 347e9f5043c89695b01e66b3ed111755afcf1911
change-id: 20250717-hid-ntrig-page-fault-fix-d13c49c86473

Best regards,
-- 
Minjong Kim <minbell.kim@samsung.com>

Re: [PATCH] HID: hid-ntrig: fix unable to handle page fault in ntrig_report_version()

Posted by Jiri Kosina 1 month, 3 weeks ago

On Thu, 17 Jul 2025, Minjong Kim wrote:

> in ntrig_report_version(), hdev parameter passed from hid_probe().
> sending descriptor to /dev/uhid can make hdev->dev.parent->parent to null
> if hdev->dev.parent->parent is null, usb_dev has
> invalid address(0xffffffffffffff58) that hid_to_usb_dev(hdev) returned
> when usb_rcvctrlpipe() use usb_dev,it trigger
> page fault error for address(0xffffffffffffff58)
> 
> add null check logic to ntrig_report_version()
> before calling hid_to_usb_dev()
> 
> Signed-off-by: Minjong Kim <minbell.kim@samsung.com>
> ---
>  drivers/hid/hid-ntrig.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> index 2738ce947434f904f32e9a1979b1681c66972ff9..96d3300655b5aa1621015b8e1fb511e6f616a713 100644
> --- a/drivers/hid/hid-ntrig.c
> +++ b/drivers/hid/hid-ntrig.c
> @@ -139,6 +139,10 @@ static inline void ntrig_set_mode(struct hid_device *hdev, const int mode)
>  
>  static void ntrig_report_version(struct hid_device *hdev)
>  {
> +
> +	if (!hdev->dev.parent->parent)
> +		return;
> +
>  	int ret;
>  	char buf[20];
>  	struct usb_device *usb_dev = hid_to_usb_dev(hdev);

I know that mixing declarations and code is fine these days, but we 
haven't been progressive enough to switch to that coding style in HID 
subsystem yet :) Would you be willing to move it below the declarations?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2] HID: hid-ntrig: fix unable to handle page fault in ntrig_report_version()

Posted by Minjong Kim 1 month, 3 weeks ago

On Tue, Aug 12, 2025 at 02:46:24PM +0200, Jiri Kosina wrote:
> 
> I know that mixing declarations and code is fine these days, but we 
> haven't been progressive enough to switch to that coding style in HID 
> subsystem yet :) Would you be willing to move it below the declarations?
>  

From 75e52defd4b2fd138285c5ad953942e2e6cf2fbb Mon Sep 17 00:00:00 2001
From: Minjong Kim <minbell.kim@samsung.com>
Date: Thu, 17 Jul 2025 14:37:47 +0900
Subject: [PATCH v2] HID: hid-ntrig: fix unable to handle page fault in
 ntrig_report_version()

in ntrig_report_version(), hdev parameter passed from hid_probe().
sending descriptor to /dev/uhid can make hdev->dev.parent->parent to null
if hdev->dev.parent->parent is null, usb_dev has
invalid address(0xffffffffffffff58) that hid_to_usb_dev(hdev) returned
when usb_rcvctrlpipe() use usb_dev,it trigger
page fault error for address(0xffffffffffffff58)

add null check logic to ntrig_report_version()
before calling hid_to_usb_dev()

Signed-off-by: Minjong Kim <minbell.kim@samsung.com>
---
 drivers/hid/hid-ntrig.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 2738ce947434..fa948d9e236c 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -144,6 +144,9 @@ static void ntrig_report_version(struct hid_device *hdev)
 	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
 	unsigned char *data = kmalloc(8, GFP_KERNEL);

+	if (!hdev->dev.parent->parent)
+		return;
+
 	if (!data)
 		goto err_free;

--
2.34.1

I move it below the declarations.

Best regards,

Re: [PATCH v2] HID: hid-ntrig: fix unable to handle page fault in ntrig_report_version()

Posted by Benjamin Tissoires 1 month, 3 weeks ago

On Aug 13 2025, Minjong Kim wrote:
> On Tue, Aug 12, 2025 at 02:46:24PM +0200, Jiri Kosina wrote:
> > 
> > I know that mixing declarations and code is fine these days, but we 
> > haven't been progressive enough to switch to that coding style in HID 
> > subsystem yet :) Would you be willing to move it below the declarations?
> >  
> 
> From 75e52defd4b2fd138285c5ad953942e2e6cf2fbb Mon Sep 17 00:00:00 2001
> From: Minjong Kim <minbell.kim@samsung.com>
> Date: Thu, 17 Jul 2025 14:37:47 +0900
> Subject: [PATCH v2] HID: hid-ntrig: fix unable to handle page fault in
>  ntrig_report_version()
> 
> in ntrig_report_version(), hdev parameter passed from hid_probe().
> sending descriptor to /dev/uhid can make hdev->dev.parent->parent to null
> if hdev->dev.parent->parent is null, usb_dev has
> invalid address(0xffffffffffffff58) that hid_to_usb_dev(hdev) returned
> when usb_rcvctrlpipe() use usb_dev,it trigger
> page fault error for address(0xffffffffffffff58)
> 
> add null check logic to ntrig_report_version()
> before calling hid_to_usb_dev()
> 
> Signed-off-by: Minjong Kim <minbell.kim@samsung.com>
> ---
>  drivers/hid/hid-ntrig.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> index 2738ce947434..fa948d9e236c 100644
> --- a/drivers/hid/hid-ntrig.c
> +++ b/drivers/hid/hid-ntrig.c
> @@ -144,6 +144,9 @@ static void ntrig_report_version(struct hid_device *hdev)
>  	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
>  	unsigned char *data = kmalloc(8, GFP_KERNEL);
> 
> +	if (!hdev->dev.parent->parent)

Why simply not use if(!hid_is_usb(hdev)) instead?

Cheers,
Benjamin

> +		return;
> +
>  	if (!data)
>  		goto err_free;
> 
> --
> 2.34.1
> 
> I move it below the declarations.
> 
> Best regards,
>

Re: [PATCH v3] HID: hid-ntrig: fix unable to handle page fault in ntrig_report_version()

Posted by Minjong Kim 1 month, 3 weeks ago

On Wed, Aug 13, 2025 at 10:39:37AM +0200, Benjamin Tissoires wrote:
> On Aug 13 2025, Minjong Kim wrote:
> > 
> > From 75e52defd4b2fd138285c5ad953942e2e6cf2fbb Mon Sep 17 00:00:00 2001
> > From: Minjong Kim <minbell.kim@samsung.com>
> > Date: Thu, 17 Jul 2025 14:37:47 +0900
> > Subject: [PATCH v2] HID: hid-ntrig: fix unable to handle page fault in
> >  ntrig_report_version()
> > 
> > in ntrig_report_version(), hdev parameter passed from hid_probe().
> > sending descriptor to /dev/uhid can make hdev->dev.parent->parent to null
> > if hdev->dev.parent->parent is null, usb_dev has
> > invalid address(0xffffffffffffff58) that hid_to_usb_dev(hdev) returned
> > when usb_rcvctrlpipe() use usb_dev,it trigger
> > page fault error for address(0xffffffffffffff58)
> > 
> > add null check logic to ntrig_report_version()
> > before calling hid_to_usb_dev()
> > 
> > Signed-off-by: Minjong Kim <minbell.kim@samsung.com>
> > ---
> >  drivers/hid/hid-ntrig.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> > index 2738ce947434..fa948d9e236c 100644
> > --- a/drivers/hid/hid-ntrig.c
> > +++ b/drivers/hid/hid-ntrig.c
> > @@ -144,6 +144,9 @@ static void ntrig_report_version(struct hid_device *hdev)
> >  	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
> >  	unsigned char *data = kmalloc(8, GFP_KERNEL);
> > 
> > +	if (!hdev->dev.parent->parent)
> 
> Why simply not use if(!hid_is_usb(hdev)) instead?
> 
> Cheers,
> Benjamin
>

From 61818c85614ad40beab53cee421272814576836d Mon Sep 17 00:00:00 2001
From: Minjong Kim <minbell.kim@samsung.com>
Date: Thu, 17 Jul 2025 14:37:47 +0900
Subject: [PATCH v3] HID: hid-ntrig: fix unable to handle page fault in
 ntrig_report_version()

in ntrig_report_version(), hdev parameter passed from hid_probe().
sending descriptor to /dev/uhid can make hdev->dev.parent->parent to null
if hdev->dev.parent->parent is null, usb_dev has
invalid address(0xffffffffffffff58) that hid_to_usb_dev(hdev) returned
when usb_rcvctrlpipe() use usb_dev,it trigger
page fault error for address(0xffffffffffffff58)

add null check logic to ntrig_report_version()
before calling hid_to_usb_dev()

Signed-off-by: Minjong Kim <minbell.kim@samsung.com>
---
 drivers/hid/hid-ntrig.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 2738ce947434..0f76e241e0af 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -144,6 +144,9 @@ static void ntrig_report_version(struct hid_device *hdev)
 	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
 	unsigned char *data = kmalloc(8, GFP_KERNEL);
 
+	if (!hid_is_usb(hdev))
+		return;
+
 	if (!data)
 		goto err_free;
 
-- 
2.34.1


I checked that crashes didn't occuered this patch
then, I'm just wondering why it is effective?
could you explain me about this?

Best regards,

Re: [PATCH v3] HID: hid-ntrig: fix unable to handle page fault in ntrig_report_version()

Posted by Benjamin Tissoires 1 month, 3 weeks ago

On Aug 13 2025, Minjong Kim wrote:
> On Wed, Aug 13, 2025 at 10:39:37AM +0200, Benjamin Tissoires wrote:
> > On Aug 13 2025, Minjong Kim wrote:
> > > 
> > > From 75e52defd4b2fd138285c5ad953942e2e6cf2fbb Mon Sep 17 00:00:00 2001
> > > From: Minjong Kim <minbell.kim@samsung.com>
> > > Date: Thu, 17 Jul 2025 14:37:47 +0900
> > > Subject: [PATCH v2] HID: hid-ntrig: fix unable to handle page fault in
> > >  ntrig_report_version()
> > > 
> > > in ntrig_report_version(), hdev parameter passed from hid_probe().
> > > sending descriptor to /dev/uhid can make hdev->dev.parent->parent to null
> > > if hdev->dev.parent->parent is null, usb_dev has
> > > invalid address(0xffffffffffffff58) that hid_to_usb_dev(hdev) returned
> > > when usb_rcvctrlpipe() use usb_dev,it trigger
> > > page fault error for address(0xffffffffffffff58)
> > > 
> > > add null check logic to ntrig_report_version()
> > > before calling hid_to_usb_dev()
> > > 
> > > Signed-off-by: Minjong Kim <minbell.kim@samsung.com>
> > > ---
> > >  drivers/hid/hid-ntrig.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> > > index 2738ce947434..fa948d9e236c 100644
> > > --- a/drivers/hid/hid-ntrig.c
> > > +++ b/drivers/hid/hid-ntrig.c
> > > @@ -144,6 +144,9 @@ static void ntrig_report_version(struct hid_device *hdev)
> > >  	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
> > >  	unsigned char *data = kmalloc(8, GFP_KERNEL);
> > > 
> > > +	if (!hdev->dev.parent->parent)
> > 
> > Why simply not use if(!hid_is_usb(hdev)) instead?
> > 
> > Cheers,
> > Benjamin
> >
> 
> From 61818c85614ad40beab53cee421272814576836d Mon Sep 17 00:00:00 2001
> From: Minjong Kim <minbell.kim@samsung.com>
> Date: Thu, 17 Jul 2025 14:37:47 +0900
> Subject: [PATCH v3] HID: hid-ntrig: fix unable to handle page fault in
>  ntrig_report_version()
> 
> in ntrig_report_version(), hdev parameter passed from hid_probe().
> sending descriptor to /dev/uhid can make hdev->dev.parent->parent to null
> if hdev->dev.parent->parent is null, usb_dev has
> invalid address(0xffffffffffffff58) that hid_to_usb_dev(hdev) returned
> when usb_rcvctrlpipe() use usb_dev,it trigger
> page fault error for address(0xffffffffffffff58)
> 
> add null check logic to ntrig_report_version()
> before calling hid_to_usb_dev()
> 
> Signed-off-by: Minjong Kim <minbell.kim@samsung.com>
> ---
>  drivers/hid/hid-ntrig.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> index 2738ce947434..0f76e241e0af 100644
> --- a/drivers/hid/hid-ntrig.c
> +++ b/drivers/hid/hid-ntrig.c
> @@ -144,6 +144,9 @@ static void ntrig_report_version(struct hid_device *hdev)
>  	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
>  	unsigned char *data = kmalloc(8, GFP_KERNEL);
>  
> +	if (!hid_is_usb(hdev))
> +		return;
> +
>  	if (!data)
>  		goto err_free;
>  
> -- 
> 2.34.1
> 
> 
> I checked that crashes didn't occuered this patch
> then, I'm just wondering why it is effective?
> could you explain me about this?
> 
> Best regards,

Could you please properly resend the patch, without replying to the
thread? I can't seem to make b4 happy to apply the patch directly so
there is something wrong with your submission here.

Cheers,
Benjamin

Re: [PATCH v3] HID: hid-ntrig: fix unable to handle page fault in ntrig_report_version()

Posted by Benjamin Tissoires 1 month, 3 weeks ago

On Aug 13 2025, Minjong Kim wrote:
> On Wed, Aug 13, 2025 at 10:39:37AM +0200, Benjamin Tissoires wrote:
> > On Aug 13 2025, Minjong Kim wrote:
> > > 
> > > From 75e52defd4b2fd138285c5ad953942e2e6cf2fbb Mon Sep 17 00:00:00 2001
> > > From: Minjong Kim <minbell.kim@samsung.com>
> > > Date: Thu, 17 Jul 2025 14:37:47 +0900
> > > Subject: [PATCH v2] HID: hid-ntrig: fix unable to handle page fault in
> > >  ntrig_report_version()
> > > 
> > > in ntrig_report_version(), hdev parameter passed from hid_probe().
> > > sending descriptor to /dev/uhid can make hdev->dev.parent->parent to null
> > > if hdev->dev.parent->parent is null, usb_dev has
> > > invalid address(0xffffffffffffff58) that hid_to_usb_dev(hdev) returned
> > > when usb_rcvctrlpipe() use usb_dev,it trigger
> > > page fault error for address(0xffffffffffffff58)
> > > 
> > > add null check logic to ntrig_report_version()
> > > before calling hid_to_usb_dev()
> > > 
> > > Signed-off-by: Minjong Kim <minbell.kim@samsung.com>
> > > ---
> > >  drivers/hid/hid-ntrig.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> > > index 2738ce947434..fa948d9e236c 100644
> > > --- a/drivers/hid/hid-ntrig.c
> > > +++ b/drivers/hid/hid-ntrig.c
> > > @@ -144,6 +144,9 @@ static void ntrig_report_version(struct hid_device *hdev)
> > >  	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
> > >  	unsigned char *data = kmalloc(8, GFP_KERNEL);
> > > 
> > > +	if (!hdev->dev.parent->parent)
> > 
> > Why simply not use if(!hid_is_usb(hdev)) instead?
> > 
> > Cheers,
> > Benjamin
> >
> 
> From 61818c85614ad40beab53cee421272814576836d Mon Sep 17 00:00:00 2001
> From: Minjong Kim <minbell.kim@samsung.com>
> Date: Thu, 17 Jul 2025 14:37:47 +0900
> Subject: [PATCH v3] HID: hid-ntrig: fix unable to handle page fault in
>  ntrig_report_version()
> 
> in ntrig_report_version(), hdev parameter passed from hid_probe().
> sending descriptor to /dev/uhid can make hdev->dev.parent->parent to null
> if hdev->dev.parent->parent is null, usb_dev has
> invalid address(0xffffffffffffff58) that hid_to_usb_dev(hdev) returned
> when usb_rcvctrlpipe() use usb_dev,it trigger
> page fault error for address(0xffffffffffffff58)
> 
> add null check logic to ntrig_report_version()
> before calling hid_to_usb_dev()
> 
> Signed-off-by: Minjong Kim <minbell.kim@samsung.com>
> ---
>  drivers/hid/hid-ntrig.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> index 2738ce947434..0f76e241e0af 100644
> --- a/drivers/hid/hid-ntrig.c
> +++ b/drivers/hid/hid-ntrig.c
> @@ -144,6 +144,9 @@ static void ntrig_report_version(struct hid_device *hdev)
>  	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
>  	unsigned char *data = kmalloc(8, GFP_KERNEL);
>  
> +	if (!hid_is_usb(hdev))
> +		return;
> +
>  	if (!data)
>  		goto err_free;
>  
> -- 
> 2.34.1
> 
> 
> I checked that crashes didn't occuered this patch
> then, I'm just wondering why it is effective?
> could you explain me about this?

You are basically trying to detect if a device is connected through uhid
or usb. uhid doesn't set the hdev->dev.parent->parent field, which is
only available when connected over an actual USB port.

So instead of relying on struct internals, I just told you to use the
proper mechanism to ensure that the function which will call usb
specifics will actually work on usb connected devices only, not emulated
devices.

Cheers,
Benjamin

> 
> Best regards,