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

[Greg KH <gregkh@linuxfoundation.org>]

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