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]
Date:   Tue, 7 Nov 2023 15:59:43 +0800 (CST)
From:   be286 <be286@....com>
To:     "Rahul Rameshbabu" <sergeantsagara@...tonmail.com>
Cc:     jikos@...nel.org, benjamin.tissoires@...hat.com,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re:Re: [PATCH v2] HID: fix a crash in hid_debug_events_release

Hi Rahul,

Thank you for your reply. It has been very helpful to me and I accept it.

With regards,

Charles Yi




At 2023-11-06 12:51:38, "Rahul Rameshbabu" <sergeantsagara@...tonmail.com> wrote:
>Lets clean up the subject/commit message heading.
>
>  HID: fix HID device resource race between HID core and debugging support
>
>In the commit message body, we can expand on the details a bit more.
>
>On Tue, 31 Oct, 2023 12:32:39 +0800 "Charles Yi" <be286@....com> wrote:
>> hid_debug_events_release() access released memory by
>> hid_device_release(). This is fixed by the patch.
>>
>> When hid_debug_events_release() was being called, in most case,
>> hid_device_release() finish already, the memory of list->hdev
>> freed by hid_device_release(), if list->hdev memory
>> reallocate by others, and it's modified, zeroed, then
>> list->hdev->debug_list_lock occasioned crash come out.
>
>Lets clean up these paragraphs a bit.
>
>  hid_debug_events_release releases resources bound to the HID device
>  instance. hid_device_release releases the underlying HID device
>  instance potentially before hid_debug_events_release has completed
>  releasing debug resources bound to the same HID device instance.
>
>  Reference count to prevent the HID device instance from being torn
>  down preemptively when HID debugging support is used. When count
>  reaches zero, release core resources of HID device instance using
>  hiddev_free.
>
>Feel free to use the above if you think its nice or feel free to polish
>up the commit message body you originally had a bit more.
>
>>
>> The crash:
>>
>> [  120.728477][ T4396] kernel BUG at lib/list_debug.c:53!
>> [  120.728505][ T4396] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>> [  120.739806][ T4396] Modules linked in: bcmdhd dhd_static_buf 8822cu pcie_mhi r8168
>> [  120.747386][ T4396] CPU: 1 PID: 4396 Comm: hidt_bridge Not tainted 5.10.110 #257
>> [  120.754771][ T4396] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
>> [  120.761643][ T4396] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
>> [  120.768338][ T4396] pc : __list_del_entry_valid+0x98/0xac
>> [  120.773730][ T4396] lr : __list_del_entry_valid+0x98/0xac
>> [  120.779120][ T4396] sp : ffffffc01e62bb60
>> [  120.783126][ T4396] x29: ffffffc01e62bb60 x28: ffffff818ce3a200
>> [  120.789126][ T4396] x27: 0000000000000009 x26: 0000000000980000
>> [  120.795126][ T4396] x25: ffffffc012431000 x24: ffffff802c6d4e00
>> [  120.801125][ T4396] x23: ffffff8005c66f00 x22: ffffffc01183b5b8
>> [  120.807125][ T4396] x21: ffffff819df2f100 x20: 0000000000000000
>> [  120.813124][ T4396] x19: ffffff802c3f0700 x18: ffffffc01d2cd058
>> [  120.819124][ T4396] x17: 0000000000000000 x16: 0000000000000000
>> [  120.825124][ T4396] x15: 0000000000000004 x14: 0000000000003fff
>> [  120.831123][ T4396] x13: ffffffc012085588 x12: 0000000000000003
>> [  120.837123][ T4396] x11: 00000000ffffbfff x10: 0000000000000003
>> [  120.843123][ T4396] x9 : 455103d46b329300 x8 : 455103d46b329300
>> [  120.849124][ T4396] x7 : 74707572726f6320 x6 : ffffffc0124b8cb5
>> [  120.855124][ T4396] x5 : ffffffffffffffff x4 : 0000000000000000
>> [  120.861123][ T4396] x3 : ffffffc011cf4f90 x2 : ffffff81fee7b948
>> [  120.867122][ T4396] x1 : ffffffc011cf4f90 x0 : 0000000000000054
>> [  120.873122][ T4396] Call trace:
>> [  120.876259][ T4396]  __list_del_entry_valid+0x98/0xac
>> [  120.881304][ T4396]  hid_debug_events_release+0x48/0x12c
>> [  120.886617][ T4396]  full_proxy_release+0x50/0xbc
>> [  120.891323][ T4396]  __fput+0xdc/0x238
>> [  120.895075][ T4396]  ____fput+0x14/0x24
>> [  120.898911][ T4396]  task_work_run+0x90/0x148
>> [  120.903268][ T4396]  do_exit+0x1bc/0x8a4
>> [  120.907193][ T4396]  do_group_exit+0x8c/0xa4
>> [  120.911458][ T4396]  get_signal+0x468/0x744
>> [  120.915643][ T4396]  do_signal+0x84/0x280
>> [  120.919650][ T4396]  do_notify_resume+0xd0/0x218
>> [  120.924262][ T4396]  work_pending+0xc/0x3f0
>>
>> Fixes: <cd667ce24796> (HID: use debugfs for events/reports dumping)
>
>The formatting of the Fixes: tag would look like the following.
>
>  Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
>
>You can also eliminate the whitespace between your git trailers, so the
>end result looks like the following (minus the indentation).
>
>  Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
>  Signed-off-by: Charles Yi <be286@....com>
>
>>
>> Signed-off-by: Charles Yi <be286@....com>
>>
>> ---
>> Changes in V2:
>> - Add "Fixes:" tag and call trace to commit message.
>> ---
>>  drivers/hid/hid-core.c  | 12 ++++++++++--
>>  drivers/hid/hid-debug.c |  3 +++
>>  include/linux/hid.h     |  3 +++
>>  3 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 8992e3c1e769..e0181218ad85 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -702,15 +702,22 @@ static void hid_close_report(struct hid_device *device)
>>   * Free a device structure, all reports, and all fields.
>>   */
>>
>> -static void hid_device_release(struct device *dev)
>> +void hiddev_free(struct kref *ref)
>
>Lets call this hid_hiddev_free. Took a look through hid-core.c, and I
>think this would be better than calling it just hiddev_free.
>
>>  {
>> -	struct hid_device *hid = to_hid_device(dev);
>> +	struct hid_device *hid = container_of(ref, struct hid_device, ref);
>>
>>  	hid_close_report(hid);
>>  	kfree(hid->dev_rdesc);
>>  	kfree(hid);
>>  }
>>
>> +static void hid_device_release(struct device *dev)
>> +{
>> +	struct hid_device *hid = to_hid_device(dev);
>> +
>> +	kref_put(&hid->ref, hiddev_free);
>> +}
>> +
>>  /*
>>   * Fetch a report description item from the data stream. We support long
>>   * items, though they are not used yet.
>> @@ -2846,6 +2853,7 @@ struct hid_device *hid_allocate_device(void)
>>  	spin_lock_init(&hdev->debug_list_lock);
>>  	sema_init(&hdev->driver_input_lock, 1);
>>  	mutex_init(&hdev->ll_open_lock);
>> +	kref_init(&hdev->ref);
>>
>>  	hid_bpf_device_init(hdev);
>>
>> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
>> index e7ef1ea107c9..7dd83ec74f8a 100644
>> --- a/drivers/hid/hid-debug.c
>> +++ b/drivers/hid/hid-debug.c
>> @@ -1135,6 +1135,7 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
>>  		goto out;
>>  	}
>>  	list->hdev = (struct hid_device *) inode->i_private;
>> +	kref_get(&list->hdev->ref);
>>  	file->private_data = list;
>>  	mutex_init(&list->read_mutex);
>>
>> @@ -1227,6 +1228,8 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
>>  	list_del(&list->node);
>>  	spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
>>  	kfifo_free(&list->hid_debug_fifo);
>> +
>> +	kref_put(&list->hdev->ref, hiddev_free);
>>  	kfree(list);
>>
>>  	return 0;
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 964ca1f15e3f..3b08a2957229 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -679,6 +679,7 @@ struct hid_device {							/* device report descriptor */
>>  	struct list_head debug_list;
>>  	spinlock_t  debug_list_lock;
>>  	wait_queue_head_t debug_wait;
>> +	struct kref			ref;
>>
>>  	unsigned int id;						/* system unique id */
>>
>> @@ -687,6 +688,8 @@ struct hid_device {							/* device report descriptor */
>>  #endif /* CONFIG_BPF */
>>  };
>>
>> +void hiddev_free(struct kref *ref);
>> +
>>  #define to_hid_device(pdev) \
>>  	container_of(pdev, struct hid_device, dev)
>
>--
>Thanks for the patch,
>
>Rahul Rameshbabu

Powered by blists - more mailing lists