Re: [PATCH] HID: sony: clean up device list on probe failure

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing usb_kill_urb() in sony_remove() leads to a Use-After-Free and DMA to freed memory when the URB completes.
- [Critical] Multiple HID collections in a malicious device cause permanent IDA leak and list corruption.
- [High] Unconditional calls to hid_hw_close(hdev) cause ll_open_count underflow for devices without battery support.
--

--- Patch [1]: [PATCH] HID: sony: clean up device list on probe failure ---
commit 4f91112bdbfa06050765a7259fe71fafc5ed58f1
Author: Doruk Tan Ozturk <[email protected]>
Subject: HID: sony: clean up device list on probe failure

This commit attempts to fix a use-after-free by properly initializing the list
node and device ID, and making list removal idempotent. It introduces
sony_cleanup() to consolidate cleanup paths on probe failure and device
removal.

Link: https://lore.kernel.org/linux-input/[email protected]/

> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 253fff4066ebd..56471b7b052d1 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
[ ... ]
> @@ -2127,6 +2126,13 @@ static inline void sony_cancel_work_sync(struct sony_sc *sc)
>  	}
>  }
>  
> +static void sony_cleanup(struct sony_sc *sc)
> +{
> +	sony_cancel_work_sync(sc);
> +	sony_remove_dev_list(sc);
> +	sony_release_device_id(sc);
> +}
> +
>  static int sony_input_configured(struct hid_device *hdev,
>  					struct hid_input *hidinput)
>  {

[Severity: Critical]
This is a pre-existing issue, but what happens if a malicious USB device
exposes multiple HID Application Collections?

The HID core will invoke sony_input_configured() for each collection. Since
this function isn't reentrant for the same device, won't it allocate multiple
IDAs in sony_set_device_id(), permanently leaking the previous ones?

Furthermore, wouldn't a second invocation find the device in sony_device_list
and call list_add_tail() on an already-linked node in
sony_check_add_dev_list(), corrupting the list?

[ ... ]
> @@ -2313,9 +2319,7 @@ static int sony_input_configured(struct hid_device *hdev,
>  err_close:
>  	hid_hw_close(hdev);

[Severity: High]
This isn't a bug introduced by this patch, but if sony_init_ff() fails for a
device without the SONY_BATTERY_SUPPORT quirk, won't this unconditionally call
hid_hw_close() and underflow the hdev->ll_open_count?

>  err_stop:
> -	sony_cancel_work_sync(sc);
> -	sony_remove_dev_list(sc);
> -	sony_release_device_id(sc);
> +	sony_cleanup(sc);
>  	return ret;
>  }
[ ... ]
> @@ -2436,13 +2443,7 @@ static void sony_remove(struct hid_device *hdev)
>  	}

[Severity: Critical]
This is a pre-existing issue, but shouldn't we call usb_kill_urb() before
freeing the URB in this preceding block for GHL_GUITAR devices?

If the URB is still in flight, could the completion callback
ghl_magic_poke_cb() execute asynchronously after the sony_sc structure is
freed by devres, leading to a use-after-free and timer list corruption?

>  
>  	hid_hw_close(hdev);

[Severity: High]
This is a pre-existing issue, but doesn't this unconditionally call
hid_hw_close() even if hid_hw_open() was never called (e.g., if the device
lacks SONY_BATTERY_SUPPORT)?

Would this corrupt the reference count and permanently break the hardware
state tracking if the device is later rebound via sysfs?

> -
> -	sony_cancel_work_sync(sc);
> -
> -	sony_remove_dev_list(sc);
> -
> -	sony_release_device_id(sc);
> -
> +	sony_cleanup(sc);
>  	hid_hw_stop(hdev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1