Re: [PATCH] HID: cp2112: don't dereference hdev->hidraw unconditionally

[Benjamin Tissoires <bentiss@kernel.org>]

Hi Jinmo,

On Oct 14 2026, Jinmo Yang wrote:
> cp2112_probe() builds the I2C adapter name from the hidraw minor:
> 
> 	snprintf(dev->adap.name, sizeof(dev->adap.name),
> 		 "CP2112 SMBus Bridge on hidraw%d",
> 		 ((struct hidraw *)hdev->hidraw)->minor);
> 
> hdev->hidraw is set only when hidraw_connect() succeeds; on failure it
> stays NULL and reading ->minor faults.  hidraw_connect() fails when all
> HIDRAW_MAX_DEVICES minors are in use, or on allocation failure.
> 
> hid_hw_start() does not report that.  hid_connect() has
> 
> 	if ((connect_mask & HID_CONNECT_HIDRAW) && !hidraw_connect(hdev))
> 		hdev->claimed |= HID_CLAIMED_HIDRAW;
> 	...
> 	if (!hdev->claimed && !hdev->driver->raw_event) {
> 		hid_err(hdev, "device has no listeners, quitting\n");
> 		return -ENODEV;
> 	}
> 
> When hidraw_connect() fails, claimed stays 0 -- but that bail-out also
> requires the driver to have no ->raw_event.  cp2112 sets
> .raw_event = cp2112_raw_event, so the -ENODEV path is skipped,
> hid_connect() returns 0, cp2112_probe() passes its "if (ret)" check, and
> the dereference faults.
> 
> The minor is only used for the adapter name, so fall back to the device
> name instead of failing the probe.
> 
> Found by coverage-guided fuzzing of the HID subsystem through /dev/uhid
> (syzkaller with extended descriptions), arm64 with KASAN:
> 
> Unable to handle kernel paging request at virtual address dfff800000000000
> KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> pc : cp2112_probe+0x794/0xef0 drivers/hid/hid-cp2112.c:1333
> Call trace:
>  cp2112_probe
>  __hid_device_probe
>  hid_device_probe
>  really_probe
>  device_add
>  hid_add_device
>  uhid_device_add_worker
>  process_one_work
> 
> hid-led.c and hid-u2fzero.c read hdev->hidraw the same way, but neither
> sets ->raw_event, so for them hid_connect() does return -ENODEV and their
> error checks catch it.  cp2112 is the only one of the three where the
> bail-out is defeated.
> 
> Fixes: 42cb6b35b9e6 ("HID: cp2112: use proper hidraw name with minor number")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
> ---
> This is the minimal, backportable fix.  Dmitry's series
> 
>   [PATCH 0/5] HID: hidraw: Encapsulate struct hidraw and make hidraw.h private
>   https://lore.kernel.org/linux-input/20261014051326.3784498-1-dmitry.torokhov@gmail.com/
> 
> solves the same thing more thoroughly in mainline: 2/5 adds
> hidraw_instance(), which performs this check in one place, 3/5 converts
> cp2112 (and hid-led and hid-u2fzero) to it, and 5/5 makes the raw cast
> impossible by moving hidraw.h out of include/linux.  That is the better
> end state and I am not proposing an alternative to it.

Well, you are kind of.

> 
> I am sending this separately only because that series moves headers and
> touches include/linux/hid.h, so it is not a stable candidate, while the
> dereference itself has been reachable since v4.12.  If you would rather
> tag 2/5 and 3/5 for stable instead, please drop this patch -- that works
> equally well for me and I will not resend it.

Rule of thumbs: upstream shouldn't care about backports to LTS. And
especially in that case: this patch extracts one element from Dmitry's
series, and creates a half backed patch. What about the 3 other drivers
Dmitry fixed?

So yes, there is a bug, it dates back to 4.12 if you say so, but stable
is not my problem FWIW. Given the amount of autoselected patches stable
takes, that's their problem to backport the series, not mine.

As a contributor, you are free to send a [STABLE] series if you care,
but we won't take a lesser quality patch because it's easier to backport.

> 
> Either way the oops is real and unprivileged; the trace above is from
> /dev/uhid with no special capabilities.  I can test whichever way you go.

Well, unprivileged is wrong here: /dev/uhid should only be root readable
and writeable. If you have a distribution that does make it user
accessible, it's a security hole, and that's their issue. I know some
third party packages are changing the permissions behind the back of the
user, and that's their problem, not our.

Cheers,
Benjamin

> 
>  drivers/hid/hid-cp2112.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 04379db93571..8883023d432d 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -1330,9 +1330,13 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	dev->adap.algo		= &smbus_algorithm;
>  	dev->adap.algo_data	= dev;
>  	dev->adap.dev.parent	= &hdev->dev;
> -	snprintf(dev->adap.name, sizeof(dev->adap.name),
> -		 "CP2112 SMBus Bridge on hidraw%d",
> -		 ((struct hidraw *)hdev->hidraw)->minor);
> +	if (hdev->claimed & HID_CLAIMED_HIDRAW)
> +		snprintf(dev->adap.name, sizeof(dev->adap.name),
> +			 "CP2112 SMBus Bridge on hidraw%d",
> +			 ((struct hidraw *)hdev->hidraw)->minor);
> +	else
> +		snprintf(dev->adap.name, sizeof(dev->adap.name),
> +			 "CP2112 SMBus Bridge on %s", dev_name(&hdev->dev));
>  	dev->hwversion = buf[2];
>  	init_waitqueue_head(&dev->wait);
>  
> -- 
> 2.43.0
> 
>