Re: [PATCH 1/2] ACPI: pfr_telemetry: 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 pfrt_log_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 mmap() 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 mmap() 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 mmap() 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: b0013e037a8b ("ACPI: Introduce Platform Firmware Runtime Telemetry driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Chad Aldreda <ca@xxxxxxxxxxxxxx>

There is Sashiko's feedback on this:

https://sashiko.dev/#/patchset/20261108233900.4043941-3-ca%40modelmiser.com

please address.

> ---
> The Coccinelle script reports a devm_k*() allocation when it, or an
> address inside it, is passed to misc_register() or a similar
> registration function in the same function, or when the allocated struct
> embeds such an object. It reports this driver for the misc_register() of
> &pfrt_log_dev->miscdev.
>
> Reproduced in QEMU 10.2.4 (q35, KASAN, booted with kasan_multi_shot). An
> SSDT adds a device with _HID INTC1081 and a _DSM that returns valid
> packages, and memmap= reserves memory for the log buffer. A process
> opens /dev/acpi_pfr_telemetry0, uses SET_INFO, GET_INFO, GET_DATA_INFO
> and mmap(), and unbinds the driver through sysfs with the file open.
> Repeating those calls gives, without this patch, 15 KASAN reports in
> pfrt_log_ioctl(), set_pfrt_log_level() and get_pfrt_log_data_info().
>
> With it there are none. All the calls succeed before the unbind and
> return -ENODEV after it, a mapping made before the unbind was still
> readable 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.
>
> 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. Mappings of
> the firmware log buffer outlive the unbind, as they did before.
>
> kzalloc() rather than kzalloc_obj(), which first appears in v7.0, for
> the stable trees.
>
>  drivers/acpi/pfr_telemetry.c | 55 +++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/pfr_telemetry.c b/drivers/acpi/pfr_telemetry.c
> index 2387376832a1..5ab8baabcc60 100644
> --- a/drivers/acpi/pfr_telemetry.c
> +++ b/drivers/acpi/pfr_telemetry.c
> @@ -15,10 +15,12 @@
>  #include <linux/errno.h>
>  #include <linux/file.h>
>  #include <linux/fs.h>
> +#include <linux/kref.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/mm.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/uaccess.h>
>  #include <linux/uio.h>
> @@ -62,6 +64,8 @@ struct pfrt_log_device {
>         struct pfrt_log_info info;
>         struct device *parent_dev;
>         struct miscdevice miscdev;
> +       struct kref kref;
> +       bool removed;
>  };
>
>  /* pfrt_guid is the parameter for _DSM method */
> @@ -247,6 +251,9 @@ static long pfrt_log_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>         void __user *p;
>         int ret = 0;
>
> +       if (READ_ONCE(pfrt_log_dev->removed))
> +               return -ENODEV;
> +
>         p = (void __user *)arg;
>
>         switch (cmd) {
> @@ -303,14 +310,17 @@ pfrt_log_mmap(struct file *file, struct vm_area_struct *vma)
>         phys_addr_t base_addr;
>         int ret;
>
> +       pfrt_log_dev = to_pfrt_log_dev(file);
> +
> +       if (READ_ONCE(pfrt_log_dev->removed))
> +               return -ENODEV;
> +
>         if (vma->vm_flags & VM_WRITE)
>                 return -EROFS;
>
>         /* changing from read to write with mprotect is not allowed */
>         vm_flags_clear(vma, VM_MAYWRITE);
>
> -       pfrt_log_dev = to_pfrt_log_dev(file);
> -
>         ret = get_pfrt_log_data_info(&info, pfrt_log_dev);
>         if (ret)
>                 return ret;
> @@ -337,8 +347,39 @@ pfrt_log_mmap(struct file *file, struct vm_area_struct *vma)
>         return 0;
>  }
>
> +static void pfrt_log_free(struct kref *kref)
> +{
> +       struct pfrt_log_device *pfrt_log_dev =
> +               container_of(kref, struct pfrt_log_device, kref);
> +
> +       put_device(pfrt_log_dev->parent_dev);
> +       kfree(pfrt_log_dev);
> +}
> +
> +static void pfrt_log_put(void *data)
> +{
> +       struct pfrt_log_device *pfrt_log_dev = data;
> +
> +       kref_put(&pfrt_log_dev->kref, pfrt_log_free);
> +}
> +
> +/* misc_open() holds misc_mtx here, and misc_deregister() takes it. */
> +static int pfrt_log_open(struct inode *inode, struct file *file)
> +{
> +       kref_get(&to_pfrt_log_dev(file)->kref);
> +       return 0;
> +}
> +
> +static int pfrt_log_release(struct inode *inode, struct file *file)
> +{
> +       pfrt_log_put(to_pfrt_log_dev(file));
> +       return 0;
> +}
> +
>  static const struct file_operations acpi_pfrt_log_fops = {
>         .owner          = THIS_MODULE,
> +       .open           = pfrt_log_open,
> +       .release        = pfrt_log_release,
>         .mmap           = pfrt_log_mmap,
>         .unlocked_ioctl = pfrt_log_ioctl,
>         .llseek         = noop_llseek,
> @@ -349,6 +390,7 @@ static void acpi_pfrt_log_remove(struct platform_device *pdev)
>         struct pfrt_log_device *pfrt_log_dev = platform_get_drvdata(pdev);
>
>         misc_deregister(&pfrt_log_dev->miscdev);
> +       WRITE_ONCE(pfrt_log_dev->removed, true);
>  }
>
>  static void pfrt_log_put_idx(void *data)
> @@ -373,10 +415,16 @@ static int acpi_pfrt_log_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> -       pfrt_log_dev = devm_kzalloc(&pdev->dev, sizeof(*pfrt_log_dev), GFP_KERNEL);
> +       pfrt_log_dev = kzalloc(sizeof(*pfrt_log_dev), GFP_KERNEL);
>         if (!pfrt_log_dev)
>                 return -ENOMEM;
>
> +       kref_init(&pfrt_log_dev->kref);
> +       pfrt_log_dev->parent_dev = get_device(&pdev->dev);
> +       ret = devm_add_action_or_reset(&pdev->dev, pfrt_log_put, pfrt_log_dev);
> +       if (ret)
> +               return ret;
> +
>         ret = ida_alloc(&pfrt_log_ida, GFP_KERNEL);
>         if (ret < 0)
>                 return ret;
> @@ -387,7 +435,6 @@ static int acpi_pfrt_log_probe(struct platform_device *pdev)
>                 return ret;
>
>         pfrt_log_dev->info.log_revid = PFRT_DEFAULT_REV_ID;
> -       pfrt_log_dev->parent_dev = &pdev->dev;
>
>         pfrt_log_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
>         pfrt_log_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> --
> 2.43.0
>
>