[PATCH v1] misc: amd-sbi: Fix use-after-free on driver unbind

[PATCH v1] misc: amd-sbi: Fix use-after-free on driver unbind

[Akshay Gupta]

From: Prathima L K <Prathima.Lk@amd.com>

sbrmi_data was allocated with devm_kzalloc() and freed at unbind while
an open /dev/sbrmi-* fd could still reference it, leading to a
use-after-free on ioctl after removal.

Refcount sbrmi_data with a kref: take a reference in open(), drop it in
release(), and free the structure only when the last fd closes. Mark the
device 'detached' under lock when the misc device is deregistered so all
ioctl/xfer paths return -ENODEV. Move misc deregistration to a devm
action and drop the now-unneeded i2c/i3c ".remove" callbacks.

Fixes: 35ac2034db72bbbc73609aab5f05ff6e0d38fdd0 ("misc: amd-sbi: Add support for AMD_SBI IOCTL")

Reviewed-by: Akshay Gupta <Akshay.Gupta@amd.com>
Signed-off-by: Akshay Gupta <Akshay.Gupta@amd.com>
Signed-off-by: Prathima L K <Prathima.Lk@amd.com>
---
 drivers/misc/amd-sbi/rmi-core.c | 54 +++++++++++++++++++++++++++++++++
 drivers/misc/amd-sbi/rmi-core.h |  6 ++++
 drivers/misc/amd-sbi/rmi-i2c.c  | 54 +++++++++++++++++++--------------
 3 files changed, 91 insertions(+), 23 deletions(-)

diff --git a/drivers/misc/amd-sbi/rmi-core.c b/drivers/misc/amd-sbi/rmi-core.c
index 95c9109101b5..e77f114c16c6 100644
--- a/drivers/misc/amd-sbi/rmi-core.c
+++ b/drivers/misc/amd-sbi/rmi-core.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/regmap.h>
+#include <linux/slab.h>
 #include "rmi-core.h"
 
 /* Mask for Status Register bit[1] */
@@ -193,6 +194,10 @@ static int rmi_cpuid_read(struct sbrmi_data *data,
 	u16 thread;
 
 	mutex_lock(&data->lock);
+	if (data->detached) {
+		ret = -ENODEV;
+		goto exit_unlock;
+	}
 	/* cache the rev value to identify if protocol is supported or not */
 	if (!data->rev) {
 		ret = sbrmi_get_rev(data);
@@ -307,6 +312,10 @@ static int rmi_mca_msr_read(struct sbrmi_data *data,
 	u16 thread;
 
 	mutex_lock(&data->lock);
+	if (data->detached) {
+		ret = -ENODEV;
+		goto exit_unlock;
+	}
 	/* cache the rev value to identify if protocol is supported or not */
 	if (!data->rev) {
 		ret = sbrmi_get_rev(data);
@@ -384,6 +393,10 @@ int rmi_mailbox_xfer(struct sbrmi_data *data,
 	u8 byte;
 
 	mutex_lock(&data->lock);
+	if (data->detached) {
+		ret = -ENODEV;
+		goto exit_unlock;
+	}
 
 	msg->fw_ret_code = 0;
 
@@ -475,6 +488,10 @@ static int apml_rmi_reg_xfer(struct sbrmi_data *data,
 		return -EFAULT;
 
 	mutex_lock(&data->lock);
+	if (data->detached) {
+		mutex_unlock(&data->lock);
+		return -ENODEV;
+	}
 	if (msg.rflag) {
 		ret = regmap_read(data->regmap, msg.reg_addr, &data_read);
 		if (!ret)
@@ -548,6 +565,41 @@ static int apml_mcamsr_xfer(struct sbrmi_data *data, struct apml_mcamsr_msg __us
 	return ret;
 }
 
+void sbrmi_data_release(struct kref *kref)
+{
+	struct sbrmi_data *data = container_of(kref, struct sbrmi_data, kref);
+
+	mutex_destroy(&data->lock);
+	kfree(data);
+}
+
+static int sbrmi_open(struct inode *inode, struct file *fp)
+{
+	struct sbrmi_data *data;
+
+	data = container_of(fp->private_data, struct sbrmi_data, sbrmi_misc_dev);
+
+	mutex_lock(&data->lock);
+	if (data->detached) {
+		mutex_unlock(&data->lock);
+		return -ENODEV;
+	}
+	mutex_unlock(&data->lock);
+
+	kref_get(&data->kref);
+
+	return 0;
+}
+
+static int sbrmi_release(struct inode *inode, struct file *fp)
+{
+	struct sbrmi_data *data;
+
+	data = container_of(fp->private_data, struct sbrmi_data, sbrmi_misc_dev);
+	kref_put(&data->kref, sbrmi_data_release);
+	return 0;
+}
+
 static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 {
 	void __user *argp = (void __user *)arg;
@@ -570,6 +622,8 @@ static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 
 static const struct file_operations sbrmi_fops = {
 	.owner		= THIS_MODULE,
+	.open		= sbrmi_open,
+	.release	= sbrmi_release,
 	.unlocked_ioctl	= sbrmi_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
 };
diff --git a/drivers/misc/amd-sbi/rmi-core.h b/drivers/misc/amd-sbi/rmi-core.h
index 975ae858e9fd..ae084beb6c03 100644
--- a/drivers/misc/amd-sbi/rmi-core.h
+++ b/drivers/misc/amd-sbi/rmi-core.h
@@ -9,6 +9,7 @@
 #include <linux/miscdevice.h>
 #include <linux/mutex.h>
 #include <linux/i2c.h>
+#include <linux/kref.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <uapi/misc/amd-apml.h>
@@ -56,9 +57,13 @@ struct sbrmi_data {
 	struct regmap *regmap;
 	/* Mutex locking */
 	struct mutex lock;
+	/* Reference count; keeps @sbrmi_data alive while misc fds are open */
+	struct kref kref;
 	u32 pwr_limit_max;
 	u8 dev_static_addr;
 	u8 rev;
+	/* Set on driver unbind; open/ioctl return -ENODEV afterward */
+	bool detached;
 };
 
 int rmi_mailbox_xfer(struct sbrmi_data *data, struct apml_mbox_msg *msg);
@@ -71,4 +76,5 @@ static inline int create_hwmon_sensor_device(struct device *dev, struct sbrmi_da
 }
 #endif
 int create_misc_rmi_device(struct sbrmi_data *data, struct device *dev);
+void sbrmi_data_release(struct kref *kref);
 #endif /*_SBRMI_CORE_H_*/
diff --git a/drivers/misc/amd-sbi/rmi-i2c.c b/drivers/misc/amd-sbi/rmi-i2c.c
index 463a9a757464..456f17806107 100644
--- a/drivers/misc/amd-sbi/rmi-i2c.c
+++ b/drivers/misc/amd-sbi/rmi-i2c.c
@@ -16,6 +16,7 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/regmap.h>
+#include <linux/slab.h>
 #include "rmi-core.h"
 
 #define REV_TWO_BYTE_ADDR	0x21
@@ -54,17 +55,40 @@ static int sbrmi_get_max_pwr_limit(struct sbrmi_data *data)
 	return ret;
 }
 
+static void sbrmi_misc_unregister(void *arg)
+{
+	struct sbrmi_data *data = arg;
+
+	misc_deregister(&data->sbrmi_misc_dev);
+
+	mutex_lock(&data->lock);
+	data->detached = true;
+	mutex_unlock(&data->lock);
+}
+
+static void sbrmi_driver_unref(void *arg)
+{
+	struct sbrmi_data *data = arg;
+
+	kref_put(&data->kref, sbrmi_data_release);
+}
+
 static int sbrmi_common_probe(struct device *dev, struct regmap *regmap, uint8_t address)
 {
 	struct sbrmi_data *data;
 	int ret;
 
-	data = devm_kzalloc(dev, sizeof(struct sbrmi_data), GFP_KERNEL);
+	data = kzalloc_obj(*data);
 	if (!data)
 		return -ENOMEM;
 
 	data->regmap = regmap;
 	mutex_init(&data->lock);
+	kref_init(&data->kref);
+
+	ret = devm_add_action_or_reset(dev, sbrmi_driver_unref, data);
+	if (ret)
+		return ret;
 
 	/* Enable alert for SB-RMI sequence */
 	ret = sbrmi_enable_alert(data);
@@ -83,7 +107,12 @@ static int sbrmi_common_probe(struct device *dev, struct regmap *regmap, uint8_t
 	ret = create_hwmon_sensor_device(dev, data);
 	if (ret < 0)
 		return ret;
-	return create_misc_rmi_device(data, dev);
+
+	ret = create_misc_rmi_device(data, dev);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, sbrmi_misc_unregister, data);
 }
 
 static struct regmap_config sbrmi_regmap_config = {
@@ -128,18 +157,6 @@ static int sbrmi_i2c_probe(struct i2c_client *client)
 	return sbrmi_common_probe(dev, regmap, client->addr);
 }
 
-static void sbrmi_i2c_remove(struct i2c_client *client)
-{
-	struct sbrmi_data *data = dev_get_drvdata(&client->dev);
-
-	misc_deregister(&data->sbrmi_misc_dev);
-	/* Assign fops and parent of misc dev to NULL */
-	data->sbrmi_misc_dev.fops = NULL;
-	data->sbrmi_misc_dev.parent = NULL;
-	dev_info(&client->dev, "Removed sbrmi-i2c driver\n");
-	return;
-}
-
 static const struct i2c_device_id sbrmi_id[] = {
 	{ .name = "sbrmi-i2c" },
 	{ }
@@ -160,7 +177,6 @@ static struct i2c_driver sbrmi_driver = {
 		.of_match_table = of_match_ptr(sbrmi_of_match),
 	},
 	.probe = sbrmi_i2c_probe,
-	.remove = sbrmi_i2c_remove,
 	.id_table = sbrmi_id,
 };
 
@@ -212,13 +228,6 @@ static int sbrmi_i3c_probe(struct i3c_device *i3cdev)
 	return sbrmi_common_probe(dev, regmap, i3cdev->desc->info.dyn_addr);
 }
 
-static void sbrmi_i3c_remove(struct i3c_device *i3cdev)
-{
-	struct sbrmi_data *data = dev_get_drvdata(&i3cdev->dev);
-
-	misc_deregister(&data->sbrmi_misc_dev);
-}
-
 static const struct i3c_device_id sbrmi_i3c_id[] = {
 	/* PID for AMD SBRMI device */
 	I3C_DEVICE_EXTRA_INFO(0x112, 0x0, 0x2, NULL),
@@ -235,7 +244,6 @@ static struct i3c_driver sbrmi_i3c_driver = {
 		.name = "sbrmi-i3c",
 	},
 	.probe = sbrmi_i3c_probe,
-	.remove = sbrmi_i3c_remove,
 	.id_table = sbrmi_i3c_id,
 };
 
-- 
2.34.1

Re: [PATCH v1] misc: amd-sbi: Fix use-after-free on driver unbind

[Greg KH]

On Mon, Oct 26, 2026 at 05:09:10PM +0530, Akshay Gupta wrote:
> From: Prathima L K <Prathima.Lk@amd.com>
> 
> sbrmi_data was allocated with devm_kzalloc() and freed at unbind while
> an open /dev/sbrmi-* fd could still reference it, leading to a
> use-after-free on ioctl after removal.
> 
> Refcount sbrmi_data with a kref: take a reference in open(), drop it in
> release(), and free the structure only when the last fd closes. Mark the
> device 'detached' under lock when the misc device is deregistered so all
> ioctl/xfer paths return -ENODEV. Move misc deregistration to a devm
> action and drop the now-unneeded i2c/i3c ".remove" callbacks.
> 
> Fixes: 35ac2034db72bbbc73609aab5f05ff6e0d38fdd0 ("misc: amd-sbi: Add support for AMD_SBI IOCTL")

The documentation shows how to properly format this line, please fix it
up, and don't put a blank line between it and these:

> Reviewed-by: Akshay Gupta <Akshay.Gupta@amd.com>
> Signed-off-by: Akshay Gupta <Akshay.Gupta@amd.com>

signed-off-by implies review.

> Signed-off-by: Prathima L K <Prathima.Lk@amd.com>

Wait, this is out of order, yours should be below this, right?



> ---
>  drivers/misc/amd-sbi/rmi-core.c | 54 +++++++++++++++++++++++++++++++++
>  drivers/misc/amd-sbi/rmi-core.h |  6 ++++
>  drivers/misc/amd-sbi/rmi-i2c.c  | 54 +++++++++++++++++++--------------
>  3 files changed, 91 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/misc/amd-sbi/rmi-core.c b/drivers/misc/amd-sbi/rmi-core.c
> index 95c9109101b5..e77f114c16c6 100644
> --- a/drivers/misc/amd-sbi/rmi-core.c
> +++ b/drivers/misc/amd-sbi/rmi-core.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/regmap.h>
> +#include <linux/slab.h>
>  #include "rmi-core.h"
>  
>  /* Mask for Status Register bit[1] */
> @@ -193,6 +194,10 @@ static int rmi_cpuid_read(struct sbrmi_data *data,
>  	u16 thread;
>  
>  	mutex_lock(&data->lock);
> +	if (data->detached) {
> +		ret = -ENODEV;
> +		goto exit_unlock;

Why isn't a guard being used here instead?  Then you can do a simple
"return -ENODEV", right?

> +static int sbrmi_open(struct inode *inode, struct file *fp)
> +{
> +	struct sbrmi_data *data;
> +
> +	data = container_of(fp->private_data, struct sbrmi_data, sbrmi_misc_dev);
> +
> +	mutex_lock(&data->lock);

Again, please use a guard.

> +	if (data->detached) {
> +		mutex_unlock(&data->lock);
> +		return -ENODEV;
> +	}
> +	mutex_unlock(&data->lock);
> +
> +	kref_get(&data->kref);

Are you attempting to use a kref without a lock?  That way lies madness
and is sure to be wrong.

Are you attempting to keep the data "alive" somehow?  Are you sure this
is correct?

> +
> +	return 0;
> +}
> +
> +static int sbrmi_release(struct inode *inode, struct file *fp)
> +{
> +	struct sbrmi_data *data;
> +
> +	data = container_of(fp->private_data, struct sbrmi_data, sbrmi_misc_dev);
> +	kref_put(&data->kref, sbrmi_data_release);

If open is the only thing that can get the reference to the kref, and
this is the thing that drops it, why is the kref needed at all?



> +	return 0;
> +}
> +
>  static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  {
>  	void __user *argp = (void __user *)arg;
> @@ -570,6 +622,8 @@ static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  
>  static const struct file_operations sbrmi_fops = {
>  	.owner		= THIS_MODULE,
> +	.open		= sbrmi_open,
> +	.release	= sbrmi_release,
>  	.unlocked_ioctl	= sbrmi_ioctl,
>  	.compat_ioctl	= compat_ptr_ioctl,
>  };
> diff --git a/drivers/misc/amd-sbi/rmi-core.h b/drivers/misc/amd-sbi/rmi-core.h
> index 975ae858e9fd..ae084beb6c03 100644
> --- a/drivers/misc/amd-sbi/rmi-core.h
> +++ b/drivers/misc/amd-sbi/rmi-core.h
> @@ -9,6 +9,7 @@
>  #include <linux/miscdevice.h>
>  #include <linux/mutex.h>
>  #include <linux/i2c.h>
> +#include <linux/kref.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <uapi/misc/amd-apml.h>
> @@ -56,9 +57,13 @@ struct sbrmi_data {
>  	struct regmap *regmap;
>  	/* Mutex locking */
>  	struct mutex lock;
> +	/* Reference count; keeps @sbrmi_data alive while misc fds are open */
> +	struct kref kref;
>  	u32 pwr_limit_max;
>  	u8 dev_static_addr;
>  	u8 rev;
> +	/* Set on driver unbind; open/ioctl return -ENODEV afterward */
> +	bool detached;

How can open/ioctl even run if the driver is unloaded?  You aren't using
the cdev api properly I think...

thanks,

greg k-h