linux-kernel - Re: [PATCH] HID: amd_sfh: free driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <d1c3ab42-107d-4ce0-9d47-9870084c9514@amd.com>
Date: Fri, 2 Aug 2024 16:25:33 +0530
From: Basavaraj Natikar <bnatikar@....com>
To: Jiri Kosina <jikos@...nel.org>, Olivier Sobrie <olivier@...rie.be>
Cc: Basavaraj Natikar <basavaraj.natikar@....com>,
 Benjamin Tissoires <bentiss@...nel.org>, linux-input@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: amd_sfh: free driver_data after destroying hid
 device



On 8/2/2024 4:18 PM, Jiri Kosina wrote:
> 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@...rie.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;
>>  		}

Looks good to me.

Acked-by: Basavaraj Natikar <Basavaraj.Natikar@....com>

Thanks,
--
Basavaraj

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