linux-kernel - Re: [PATCH v3] HID: hid-ntrig: fix unable to handle page fault in ntrig_report

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53uert6bh3h5iieft6hokij5sq73zs3scpoqtmtpg3tuhfc6w4@vxf5m4gmkc57>
Date: Wed, 13 Aug 2025 11:41:38 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Minjong Kim <minbell.kim@...sung.com>
Cc: Jiri Kosina <jikos@...nel.org>, linux-input@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] HID: hid-ntrig: fix unable to handle page fault in
 ntrig_report_version()

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@...sung.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@...sung.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@...sung.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@...sung.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,