HSI: hsi_char: Fix use-after-free on device removal

[PATCH] HSI: hsi_char: Fix use-after-free on device removal

Posted by Shengzhuo Wei 1 month ago

hsc_open() stores a pointer to a channel embedded in the hsc_client_data
in file->private_data, but hsc_remove() frees the whole hsc_client_data
right after cdev_del(). If the HSI client device is removed while a
channel is open, the next access from the file descriptor (a read, an
ioctl or the final close) dereferences freed memory:

CPU0                                CPU1
hsc_remove                          hsc_read
  cdev_del(&cl_data->cdev);           channel->cl->rx_cfg ...
  kfree(cl_data);                     // use after free

Fix it by tracking the hsc_client_data with a kref: each open file
descriptor takes a reference, and hsc_remove() drops the initial one,
so the object is freed only after the last descriptor is closed.

Fixes: 4e69fc22753f ("HSI: hsi_char: Add HSI char device driver")
Cc: stable@vger.kernel.org
Signed-off-by: Shengzhuo Wei <me@cherr.cc>
---
 drivers/hsi/clients/hsi_char.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/hsi/clients/hsi_char.c b/drivers/hsi/clients/hsi_char.c
index a31cc1466dd3ddf73762ce3d0213c96d64a190fd..479e5d6e94c8c03489464c4b39d81d697d104ce0 100644
--- a/drivers/hsi/clients/hsi_char.c
+++ b/drivers/hsi/clients/hsi_char.c
@@ -96,6 +96,7 @@ struct hsc_channel {
  * @usecnt: Use count for claiming the HSI port (mutex protected)
  * @cl: Referece to the HSI client
  * @channels: Array of channels accessible by the client
+ * @kref: Reference count for the client data lifetime
  */
 struct hsc_client_data {
 	struct cdev		cdev;
@@ -104,6 +105,7 @@ struct hsc_client_data {
 	unsigned int		usecnt;
 	struct hsi_client	*cl;
 	struct hsc_channel	channels[HSC_DEVS];
+	struct kref		kref;
 };
 
 /* Stores the major number dynamically allocated for hsi_char */
@@ -576,6 +578,11 @@ static long hsc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return ret;
 }
 
+static void hsc_client_data_release(struct kref *kref)
+{
+	kfree(container_of(kref, struct hsc_client_data, kref));
+}
+
 static inline void __hsc_port_release(struct hsc_client_data *cl_data)
 {
 	BUG_ON(cl_data->usecnt == 0);
@@ -613,10 +620,12 @@ static int hsc_open(struct inode *inode, struct file *file)
 		hsi_setup(cl_data->cl);
 	}
 	cl_data->usecnt++;
+	kref_get(&cl_data->kref);
 
 	ret = hsc_msgs_alloc(channel);
 	if (ret < 0) {
 		__hsc_port_release(cl_data);
+		kref_put(&cl_data->kref, hsc_client_data_release);
 		goto out;
 	}
 
@@ -650,6 +659,8 @@ static int hsc_release(struct inode *inode __maybe_unused, struct file *file)
 	wake_up(&channel->tx_wait);
 	mutex_unlock(&cl_data->lock);
 
+	kref_put(&cl_data->kref, hsc_client_data_release);
+
 	return 0;
 }
 
@@ -703,6 +714,7 @@ static int hsc_probe(struct device *dev)
 		goto out1;
 	}
 	mutex_init(&cl_data->lock);
+	kref_init(&cl_data->kref);
 	hsi_client_set_drvdata(cl, cl_data);
 	cdev_init(&cl_data->cdev, &hsc_fops);
 	cl_data->cdev.owner = THIS_MODULE;
@@ -739,7 +751,7 @@ static int hsc_remove(struct device *dev)
 	cdev_del(&cl_data->cdev);
 	unregister_chrdev_region(hsc_dev, HSC_DEVS);
 	hsi_client_set_drvdata(cl, NULL);
-	kfree(cl_data);
+	kref_put(&cl_data->kref, hsc_client_data_release);
 
 	return 0;
 }

---
base-commit: 45c13f3f9e3bb15fd89ff2864c6f627a3b4b4229
change-id: 20260827-hsi-char-uaf-4013a742dcd9

Best regards,
-- 
Shengzhuo Wei <me@cherr.cc>

Re: [PATCH] HSI: hsi_char: Fix use-after-free on device removal

Posted by Greg KH 1 month ago

On Thu, Aug 27, 2026 at 04:43:08AM +0800, Shengzhuo Wei wrote:
> hsc_open() stores a pointer to a channel embedded in the hsc_client_data
> in file->private_data, but hsc_remove() frees the whole hsc_client_data
> right after cdev_del(). If the HSI client device is removed while a
> channel is open, the next access from the file descriptor (a read, an
> ioctl or the final close) dereferences freed memory:
> 
> CPU0                                CPU1
> hsc_remove                          hsc_read
>   cdev_del(&cl_data->cdev);           channel->cl->rx_cfg ...
>   kfree(cl_data);                     // use after free
> 
> Fix it by tracking the hsc_client_data with a kref: each open file
> descriptor takes a reference, and hsc_remove() drops the initial one,
> so the object is freed only after the last descriptor is closed.
> 
> Fixes: 4e69fc22753f ("HSI: hsi_char: Add HSI char device driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shengzhuo Wei <me@cherr.cc>
> ---
>  drivers/hsi/clients/hsi_char.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hsi/clients/hsi_char.c b/drivers/hsi/clients/hsi_char.c
> index a31cc1466dd3ddf73762ce3d0213c96d64a190fd..479e5d6e94c8c03489464c4b39d81d697d104ce0 100644
> --- a/drivers/hsi/clients/hsi_char.c
> +++ b/drivers/hsi/clients/hsi_char.c
> @@ -96,6 +96,7 @@ struct hsc_channel {
>   * @usecnt: Use count for claiming the HSI port (mutex protected)
>   * @cl: Referece to the HSI client
>   * @channels: Array of channels accessible by the client
> + * @kref: Reference count for the client data lifetime
>   */
>  struct hsc_client_data {
>  	struct cdev		cdev;
> @@ -104,6 +105,7 @@ struct hsc_client_data {
>  	unsigned int		usecnt;
>  	struct hsi_client	*cl;
>  	struct hsc_channel	channels[HSC_DEVS];
> +	struct kref		kref;

You now have 2 reference counts for the same structure, which is not how
to handle this at all :(

Please either make the cdev be a pointer, or use the correct cdev api
for handling this type of common problem.

thanks,

greg k-h

Re: [PATCH] HSI: hsi_char: Fix use-after-free on device removal

Posted by Shengzhuo Wei 1 month ago

On 2026-08-27 06:48, Greg KH wrote:

> You now have 2 reference counts for the same structure, which is not how
> to handle this at all :(
> 
> Please either make the cdev be a pointer, or use the correct cdev api
> for handling this type of common problem.

Right, adding the kref on top of the embedded cdev was the wrong call.
Thanks for catching it.

I'd like to go with the pointer option, because of how this driver is
structured: one hsc_client_data serves 16 minor numbers through a
single cdev_add(&cl_data->cdev, hsc_dev, HSC_DEVS), and cdev_device_add()
pairs one cdev with one struct device, so switching to it would mean
inventing 16 device objects for no other purpose.

With a dynamically allocated cdev (cdev_alloc() in probe, cdev_del() in
remove), the kobject reference that chrdev_open() already takes on the
cdev would keep the containing object alive until the last file
descriptor is closed, and the final release would go through the cdev's
kobject release callback instead of a hand-written kref — no second
reference count anywhere.

Does that sound like the right direction to you? If so I'll send a v2
along those lines.

Regards,
Shengzhuo

Re: [PATCH] HSI: hsi_char: Fix use-after-free on device removal

Posted by Greg KH 1 month ago

On Thu, Aug 27, 2026 at 01:09:31PM +0800, Shengzhuo Wei wrote:
> On 2026-08-27 06:48, Greg KH wrote:
> 
> > You now have 2 reference counts for the same structure, which is not how
> > to handle this at all :(
> > 
> > Please either make the cdev be a pointer, or use the correct cdev api
> > for handling this type of common problem.
> 
> Right, adding the kref on top of the embedded cdev was the wrong call.
> Thanks for catching it.
> 
> I'd like to go with the pointer option, because of how this driver is
> structured: one hsc_client_data serves 16 minor numbers through a
> single cdev_add(&cl_data->cdev, hsc_dev, HSC_DEVS), and cdev_device_add()
> pairs one cdev with one struct device, so switching to it would mean
> inventing 16 device objects for no other purpose.
> 
> With a dynamically allocated cdev (cdev_alloc() in probe, cdev_del() in
> remove), the kobject reference that chrdev_open() already takes on the
> cdev would keep the containing object alive until the last file
> descriptor is closed, and the final release would go through the cdev's
> kobject release callback instead of a hand-written kref — no second
> reference count anywhere.
> 
> Does that sound like the right direction to you? If so I'll send a v2
> along those lines.

I'll defer to the hsi maintainers as to what they wish to do here.

Also, how do you remove a hsi device from the system?  Is this on a
dynamic bus?  For some reason I didn't think that was possible.

thanks,

greg k-h

Re: [PATCH] HSI: hsi_char: Fix use-after-free on device removal

Posted by Shengzhuo Wei 1 month ago

On 2026-08-27 07:15, Greg KH wrote:

> I'll defer to the hsi maintainers as to what they wish to do here.
> 
> Also, how do you remove a hsi device from the system?  Is this on a
> dynamic bus?  For some reason I didn't think that was possible.

Yes, HSI is a regular driver-model bus (hsi_bus_type in
drivers/hsi/hsi_core.c): sysfs unbind and module unload reach
hsc_remove(), and omap_ssi's own remove() cascades into it through
hsi_port_unregister_clients(). I verified the unbind path in QEMU
while auditing the sibling cmt_speech driver, which has the same bug
and whose fix I'll post separately.

Thanks for the review, by the way — the second refcount was wrong and
I've withdrawn that approach. For the v2 I'm planning to follow mei's
pattern: an embedded struct device in hsc_client_data as the release
anchor, a cdev_alloc()'ed cdev attached with cdev_set_parent(), the
minor number resolving the container at open, and a device reference
taken in open and dropped in release. One reference count, the
device's; the 16 minors keep sharing one cdev, so userspace sees no
change.

I'll wait for Sebastian's decision on the preferred shape before
sending it.

Re: [PATCH] HSI: hsi_char: Fix use-after-free on device removal

Posted by Greg KH 1 month ago

On Thu, Aug 27, 2026 at 01:40:25PM +0800, Shengzhuo Wei wrote:
> On 2026-08-27 07:15, Greg KH wrote:
> 
> > I'll defer to the hsi maintainers as to what they wish to do here.
> > 
> > Also, how do you remove a hsi device from the system?  Is this on a
> > dynamic bus?  For some reason I didn't think that was possible.
> 
> Yes, HSI is a regular driver-model bus (hsi_bus_type in
> drivers/hsi/hsi_core.c): sysfs unbind and module unload reach
> hsc_remove(), and omap_ssi's own remove() cascades into it through
> hsi_port_unregister_clients(). I verified the unbind path in QEMU
> while auditing the sibling cmt_speech driver, which has the same bug
> and whose fix I'll post separately.

unbind is not a "normal" thing to do, so much so that hopefully the
kernel will be tainted in the future if you do it:
	https://lore.kernel.org/r/20260826-bind_taint-v1-0-52b05f4a965c@linuxfoundation.org

So don't go through lots of work to add code/logic for something that a
normal user can never ever hit.

thanks,

greg k-h