Re: [PATCH] HID: amd_sfh: free driver_data after destroying hid device

[Jiri Kosina <jikos@kernel.org>]

On Tue, 23 Jul 2024, Olivier Sobrie wrote:

> HID driver callbacks aren't called anymore once hid_destroy_device() has
> been called. Hence, hid driver_data should be freed only after the
> hid_destroy_device() function returned as driver_data is used in several
> callbacks.
> 
> I observed a crash with kernel 6.10.0 on my T14s Gen 3, after enabling
> KASAN to debug memory allocation, I got this output:
[ ... snip ... ]
> KASAN reports a use-after-free of hid->driver_data in function
> amd_sfh_get_report(). The backtrace indicates that the function is called
> by amdtp_hid_request() which is one of the callbacks of hid device.
> The current make sure that driver_data is freed only once
> hid_destroy_device() returned.
> 
> Note that I observed the crash both on v6.9.9 and v6.10.0. The
> code seems to be as it was from the early days of the driver.
> 
> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> ---
>  drivers/hid/amd-sfh-hid/amd_sfh_hid.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> index 705b52337068..81f3024b7b1b 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.c
> @@ -171,11 +171,13 @@ int amdtp_hid_probe(u32 cur_hid_dev, struct amdtp_cl_data *cli_data)
>  void amdtp_hid_remove(struct amdtp_cl_data *cli_data)
>  {
>  	int i;
> +	struct amdtp_hid_data *hid_data;
>  
>  	for (i = 0; i < cli_data->num_hid_devices; ++i) {
>  		if (cli_data->hid_sensor_hubs[i]) {
> -			kfree(cli_data->hid_sensor_hubs[i]->driver_data);
> +			hid_data = cli_data->hid_sensor_hubs[i]->driver_data;
>  			hid_destroy_device(cli_data->hid_sensor_hubs[i]);
> +			kfree(hid_data);
>  			cli_data->hid_sensor_hubs[i] = NULL;
>  		}

This seems reasonable. Basavaraj, can you please provide your Ack for 
this? Thanks,

-- 
Jiri Kosina
SUSE Labs