Hi Charles, On Mon, 23 Oct, 2023 17:35:00 +0800 "Charles Yi" <be286@xxxxxxx> wrote: > hid_debug_events_release() access released memory by > hid_device_release(). This is fixed by the patch. > A couple of things here. Can you add a Fixes: tag? https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes Before any v2 however, it would be nice to understand where this issue is coming from. I am wondering if it's really a core issue or rather an issue with a higher level device specific driver making use of the hid subsystem. I am having a hard time seeing how this issue occurs currently. A stack trace in a follow-up email to this one pertaining to the crash would be helpful. If you are resolving a syzbot report, a link to that report would suffice. This patch doesn't make a lot of sense to me as-is because hid_debug_events_release is about release resources related to hid debug events (at least from my current understanding). It should not be free-ing the underlying hid device/instance itself. > Signed-off-by: Charles Yi <be286@xxxxxxx> > --- > 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) > { > - 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) -- Thank you for the patch, Rahul Rameshbabu
