Re: [PATCH] HID: fix a crash in hid_debug_events_release

[Rahul Rameshbabu <sergeantsagara@protonmail.com>]

Hi Charles,

On Mon, 23 Oct, 2023 17:35:00 +0800 "Charles Yi" <be286@163.com> 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@163.com>
> ---
>  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