Re: [PATCH 2/2] ACPI: pfr_update: keep the state until the last file is closed

[Rafael J. Wysocki (Intel)]

On Mon, Nov 9, 2026 at 12:39 AM Chad Aldreda <ca@xxxxxxxxxxxxxx> wrote:
>
> struct pfru_device is allocated with devm_kzalloc() and embeds the
> miscdevice, so it is freed when the driver unbinds. A file that is open
> on the node outlives the unbind and nothing holds a reference for it:
> the next ioctl() or write() on that file uses freed memory. Unbinding
> the driver needs root.
>
> Allocate the structure with kzalloc() and reference count it: open()
> takes a reference, under misc_mtx, which misc_deregister() also takes,
> release() drops it, and a devm action drops the driver's own. The
> structure also holds a reference on the parent device it uses. A call to
> ioctl() or write() that starts after remove() returns fails with
> -ENODEV; one already running finishes against state that is still valid.
>
> The problem was found with a Coccinelle script that reports device nodes
> in devm memory [1]. The fix, the reproducer and this message were
> written with an AI assistant (Claude) and checked by hand. The bug was
> reproduced and the fix tested in QEMU with KASAN, using an SSDT whose
> _DSM stands in for the firmware: ioctl() and write() worked before an
> unbind and, on a file held open across it, failed with -ENODEV. A call
> racing with the unbind was not tested, nor was hardware with this
> firmware interface.
>
> Link: https://lore.kernel.org/r/20261106193517.3267013-2-ca@xxxxxxxxxxxxxx [1]
> Fixes: 0db89fa243e5 ("ACPI: Introduce Platform Firmware Runtime Update device driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Chad Aldreda <ca@xxxxxxxxxxxxxx>
> ---
> The same Coccinelle script reports this driver for the misc_register()
> of &pfru_dev->miscdev.
>
> Reproduced in QEMU 10.2.4 (q35, KASAN, booted with kasan_multi_shot). An
> SSDT adds a device with _HID INTC1080 and a _DSM that returns valid
> packages, and memmap= reserves the communication buffer. A process opens
> /dev/acpi_pfr_update0, uses QUERY_CAP, SET_REV, write() of a minimal
> capsule and STAGE, and unbinds the driver through sysfs with the file
> open. Repeating those calls gives, without this patch, 9 KASAN reports
> in pfru_ioctl(), query_capability(), pfru_write() and start_update().
>
> With it there are none. All the calls succeed before the unbind and
> return -ENODEV after it, and the same sequence behaves the same after a
> bind. Unbinding with the file already closed, or never opened, gives no
> reports either. ACTIVATE and STAGE_ACTIVATE were not run; they share
> start_update() with STAGE.
>
> No lock is added. A call that passed the removed check can run during
> the unbind, against state and a parent device that are still valid; the
> driver has never serialized calls from different files. An rwsem would
> make the unbind wait for a firmware update to finish.
>
> kzalloc() rather than kzalloc_obj(), which first appears in v7.0, for
> the stable trees.
>
>  drivers/acpi/pfr_update.c | 51 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/pfr_update.c b/drivers/acpi/pfr_update.c
> index 98ace679601b..b9e4c1065ad5 100644
> --- a/drivers/acpi/pfr_update.c
> +++ b/drivers/acpi/pfr_update.c
> @@ -16,9 +16,11 @@
>  #include <linux/file.h>
>  #include <linux/fs.h>
>  #include <linux/idr.h>
> +#include <linux/kref.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/uaccess.h>
>  #include <linux/uio.h>
> @@ -82,6 +84,8 @@ struct pfru_device {
>         u32 rev_id, index;
>         struct device *parent_dev;
>         struct miscdevice miscdev;
> +       struct kref kref;
> +       bool removed;
>  };
>
>  static DEFINE_IDA(pfru_ida);
> @@ -428,6 +432,9 @@ static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>         u32 rev;
>         int ret;
>
> +       if (READ_ONCE(pfru_dev->removed))
> +               return -ENODEV;
> +
>         switch (cmd) {
>         case PFRU_IOC_QUERY_CAP:
>                 ret = query_capability(&cap_hdr, pfru_dev);
> @@ -476,6 +483,9 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
>         char *buf_ptr;
>         int ret;
>
> +       if (READ_ONCE(pfru_dev->removed))
> +               return -ENODEV;
> +
>         ret = query_buffer(&buf_info, pfru_dev);
>         if (ret)
>                 return ret;
> @@ -518,8 +528,39 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
>         return ret ?: len;
>  }
>
> +static void pfru_free(struct kref *kref)
> +{
> +       struct pfru_device *pfru_dev =
> +               container_of(kref, struct pfru_device, kref);
> +
> +       put_device(pfru_dev->parent_dev);
> +       kfree(pfru_dev);
> +}
> +
> +static void pfru_put(void *data)
> +{
> +       struct pfru_device *pfru_dev = data;
> +
> +       kref_put(&pfru_dev->kref, pfru_free);
> +}
> +
> +/* misc_open() holds misc_mtx here, and misc_deregister() takes it. */
> +static int pfru_open(struct inode *inode, struct file *file)
> +{
> +       kref_get(&to_pfru_dev(file)->kref);
> +       return 0;
> +}
> +
> +static int pfru_release(struct inode *inode, struct file *file)
> +{
> +       pfru_put(to_pfru_dev(file));
> +       return 0;
> +}
> +
>  static const struct file_operations acpi_pfru_fops = {
>         .owner          = THIS_MODULE,
> +       .open           = pfru_open,
> +       .release        = pfru_release,
>         .write          = pfru_write,
>         .unlocked_ioctl = pfru_ioctl,
>         .llseek         = noop_llseek,
> @@ -530,6 +571,7 @@ static void acpi_pfru_remove(struct platform_device *pdev)
>         struct pfru_device *pfru_dev = platform_get_drvdata(pdev);
>
>         misc_deregister(&pfru_dev->miscdev);
> +       WRITE_ONCE(pfru_dev->removed, true);
>  }
>
>  static void pfru_put_idx(void *data)
> @@ -554,10 +596,16 @@ static int acpi_pfru_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> -       pfru_dev = devm_kzalloc(&pdev->dev, sizeof(*pfru_dev), GFP_KERNEL);
> +       pfru_dev = kzalloc(sizeof(*pfru_dev), GFP_KERNEL);
>         if (!pfru_dev)
>                 return -ENOMEM;
>
> +       kref_init(&pfru_dev->kref);
> +       pfru_dev->parent_dev = get_device(&pdev->dev);
> +       ret = devm_add_action_or_reset(&pdev->dev, pfru_put, pfru_dev);
> +       if (ret)
> +               return ret;
> +
>         ret = ida_alloc(&pfru_ida, GFP_KERNEL);
>         if (ret < 0)
>                 return ret;
> @@ -568,7 +616,6 @@ static int acpi_pfru_probe(struct platform_device *pdev)
>                 return ret;
>
>         pfru_dev->rev_id = PFRU_DEFAULT_REV_ID;
> -       pfru_dev->parent_dev = &pdev->dev;
>
>         pfru_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
>         pfru_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> --

Applied as 7.5 material, thanks!