Re: [PATCH v2] ACPI: pfr_telemetry: keep the state until the last file is closed

[Rafael J. Wysocki (Intel)]

On Tue, Nov 10, 2026 at 3:44 PM Chad Aldreda <ca@xxxxxxxxxxxxxx> wrote:
>
> struct pfrt_log_device is allocated with devm_kzalloc_obj() 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_obj() 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.
>
> Also make remove() wait for calls already running. The firmware calls
> in ioctl() and mmap() run under a mutex, and remove() sets a flag under
> it that makes any later call that would reach the firmware fail with
> -ENODEV. Without that, a call that started before remove() can
> evaluate _DSM after remove() has returned, while device_del() unbinds
> the parent's ACPI companion. The mutex also serializes the updates to
> the log settings, which concurrent ioctl() calls could interleave.
> ioctl() copies to and from user memory outside the lock, because mmap()
> is called with mmap_lock held.
>
> The problem was found with a Coccinelle script that reports device
> nodes in devm memory [1]. The fix, the reproducers 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 and lockdep, using
> an SSDT whose _DSM stands in for the firmware. Hardware with this
> firmware interface was not tested.
>
> 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>
> ---
> Changes in v2:
> - remove() now waits for calls already running, under a new mutex, which
>   also serializes the log settings. Sashiko's review of v1, which Rafael
>   asked me to address, showed that a call that passed the removed check
>   can evaluate _DSM after remove() returns, while device_del() unbinds
>   the ACPI companion. The v1 notes said such a call finishes against
>   state that is still valid; that was wrong.
> - kzalloc_obj(), on top of the devm_kzalloc_obj() conversion in
>   linux-pm.
> v1: https://lore.kernel.org/r/20261108233900.4043941-3-ca@xxxxxxxxxxxxxx
>
> Tested in QEMU 10.2.4 (q35, KASAN with kasan_multi_shot, lockdep) with
> the SSDT from v1, which adds a device with _HID INTC1081 and a _DSM that
> returns valid packages. With a file held open across a sysfs unbind,
> SET_INFO, GET_INFO, GET_DATA_INFO and mmap() all work before the unbind
> and fail with -ENODEV after it, a mapping made before the unbind stays
> readable, the same holds after a bind, and there are no KASAN or lockdep
> reports.
>
> For the race, a test-only msleep(2000) was put before the SET_INFO
> firmware call and the driver unbound 0.5 s into the call. With v1 the
> unbind returned at once and the firmware call completed 1.5 s after
> remove() returned. With v2 the unbind returned 2.0 s in, after the call
> completed. The test used a sysfs unbind, which keeps the ACPI companion
> bound; removal of the device itself was not tested.
>
> A test variant that calls copy_to_user() inside the lock makes lockdep
> report a cycle between the new lock, taken in pfrt_log_mmap(), and
> mmap_lock, which is why the user copies stay outside it.
>
> Mainline and stable still allocate with devm_kzalloc(), so a backport
> needs kzalloc(sizeof(*pfrt_log_dev), GFP_KERNEL) in that hunk.
>
> The same race exists in pfr_update; a matching patch on top of the one
> you applied will follow once that is in your tree.
>
> The code, changelog and reproducers were written with an AI assistant
> (Claude Opus 5.5), and the assistant ran the tests above. I ran the
> final version myself and got the same results.
>
>  drivers/acpi/pfr_telemetry.c | 102 +++++++++++++++++++++++++++++------
>  1 file changed, 86 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/pfr_telemetry.c b/drivers/acpi/pfr_telemetry.c
> index 458cb99ae216..53d5e25497e9 100644
> --- a/drivers/acpi/pfr_telemetry.c
> +++ b/drivers/acpi/pfr_telemetry.c
> @@ -15,10 +15,13 @@
>  #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/mutex.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 +65,14 @@ struct pfrt_log_device {
>         struct pfrt_log_info info;
>         struct device *parent_dev;
>         struct miscdevice miscdev;
> +       struct kref kref;
> +       /*
> +        * Serializes the firmware calls and info of the file operations, and
> +        * remove(), which sets removed under it and so waits for calls
> +        * already running.
> +        */
> +       struct mutex lock;
> +       bool removed;
>  };
>
>  /* pfrt_guid is the parameter for _DSM method */
> @@ -254,33 +265,49 @@ static long pfrt_log_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>                 if (copy_from_user(&info, p, sizeof(info)))
>                         return -EFAULT;
>
> -               if (valid_log_revid(info.log_revid))
> -                       pfrt_log_dev->info.log_revid = info.log_revid;
> +               scoped_guard(mutex, &pfrt_log_dev->lock) {
> +                       if (pfrt_log_dev->removed)
> +                               return -ENODEV;
>
> -               if (valid_log_level(info.log_level)) {
> -                       ret = set_pfrt_log_level(info.log_level, pfrt_log_dev);
> -                       if (ret < 0)
> -                               return ret;
> +                       if (valid_log_revid(info.log_revid))
> +                               pfrt_log_dev->info.log_revid = info.log_revid;
>
> -                       pfrt_log_dev->info.log_level = info.log_level;
> -               }
> +                       if (valid_log_level(info.log_level)) {
> +                               ret = set_pfrt_log_level(info.log_level, pfrt_log_dev);
> +                               if (ret < 0)
> +                                       return ret;
> +
> +                               pfrt_log_dev->info.log_level = info.log_level;
> +                       }
>
> -               if (valid_log_type(info.log_type))
> -                       pfrt_log_dev->info.log_type = info.log_type;
> +                       if (valid_log_type(info.log_type))
> +                               pfrt_log_dev->info.log_type = info.log_type;
> +               }
>
>                 return 0;
>
>         case PFRT_LOG_IOC_GET_INFO:
> -               info.log_level = get_pfrt_log_level(pfrt_log_dev);
> -               info.log_type = pfrt_log_dev->info.log_type;
> -               info.log_revid = pfrt_log_dev->info.log_revid;
> +               scoped_guard(mutex, &pfrt_log_dev->lock) {
> +                       if (pfrt_log_dev->removed)
> +                               return -ENODEV;
> +
> +                       info.log_level = get_pfrt_log_level(pfrt_log_dev);
> +                       info.log_type = pfrt_log_dev->info.log_type;
> +                       info.log_revid = pfrt_log_dev->info.log_revid;
> +               }
> +
>                 if (copy_to_user(p, &info, sizeof(info)))
>                         return -EFAULT;
>
>                 return 0;
>
>         case PFRT_LOG_IOC_GET_DATA_INFO:
> -               ret = get_pfrt_log_data_info(&data_info, pfrt_log_dev);
> +               scoped_guard(mutex, &pfrt_log_dev->lock) {
> +                       if (pfrt_log_dev->removed)
> +                               return -ENODEV;
> +
> +                       ret = get_pfrt_log_data_info(&data_info, pfrt_log_dev);
> +               }
>                 if (ret)
>                         return ret;
>
> @@ -311,6 +338,10 @@ pfrt_log_mmap(struct file *file, struct vm_area_struct *vma)
>
>         pfrt_log_dev = to_pfrt_log_dev(file);
>
> +       guard(mutex)(&pfrt_log_dev->lock);
> +       if (pfrt_log_dev->removed)
> +               return -ENODEV;
> +
>         ret = get_pfrt_log_data_info(&info, pfrt_log_dev);
>         if (ret)
>                 return ret;
> @@ -337,8 +368,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 +411,8 @@ 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);
> +       scoped_guard(mutex, &pfrt_log_dev->lock)
> +               pfrt_log_dev->removed = true;
>  }
>
>  static void pfrt_log_put_idx(void *data)
> @@ -373,10 +437,17 @@ static int acpi_pfrt_log_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> -       pfrt_log_dev = devm_kzalloc_obj(&pdev->dev, *pfrt_log_dev);
> +       pfrt_log_dev = kzalloc_obj(*pfrt_log_dev);
>         if (!pfrt_log_dev)
>                 return -ENOMEM;
>
> +       kref_init(&pfrt_log_dev->kref);
> +       mutex_init(&pfrt_log_dev->lock);
> +       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 +458,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,
> --

Applied as 7.5 material, thanks!