Message-ID: <aPT-7TTgW_Xop99j@tzungbi-laptop>
Date: Sun, 19 Oct 2025 23:08:29 +0800
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: Benson Leung <bleung@...omium.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>,
	Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	chrome-platform@...ts.linux.dev, linux-kselftest@...r.kernel.org,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>,
	Simona Vetter <simona.vetter@...ll.ch>,
	Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH v5 5/7] revocable: Add fops replacement

On Fri, Oct 17, 2025 at 01:21:16PM -0300, Jason Gunthorpe wrote:
> On Sat, Oct 18, 2025 at 12:07:58AM +0800, Tzung-Bi Shih wrote:
> > > This is already properly lifetime controlled!
> > > 
> > > It *HAS* to be, and even your patches are assuming it by blindly
> > > reaching into the parent's memory!
> > > 
> > > +	misc->rps[0] = ec->ec_dev->revocable_provider;
> > > 
> > > If the parent driver has been racily unbound at this point the
> > > ec->ec_dev is already a UAF!
> > 
> > Not really, it uses the fact that the caller is from probe().  I think the
> > driver can't be unbound when it is still in probe().
> 
> Right, but that's my point you are already relying on driver binding
> lifetime rules to make your access valid. You should continue to rely
> on that and fix the lack of synchronous remove to fix the bug.

I think what you're looking for is something similar to the following
patches.

- Instead of having a real resource to protect with revocable, use the
  subsystem device itself as a virtual resource.  Revoke the virtual
  resource when unregistering the device from the subsystem.

- Exit earlier if the virtual resource is NULL (i.e. the subsystem device
  has been unregistered) in the file operation wrappers.

By doing so, we don't need to provide a misc_deregister_sync() which could
probably maintain a list of opening files in miscdevice and handle with all
opening files when unregistering.  The device unbound is free to go and
doesn't need to wait for closing or interrupting all opening files.


diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 27078541489f..bc4d249c4d0f 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -173,8 +175,12 @@ static int misc_open(struct inode *inode, struct file *file)
         */
        file->private_data = c;
 
-       err = 0;
        replace_fops(file, new_fops);
+
+       err = fs_revocable_replace(c->rp, file);
+       if (err)
+               goto fail;
+
        if (file->f_op->open)
                err = file->f_op->open(inode, file);
 fail:
@@ -234,6 +240,10 @@ int misc_register(struct miscdevice *misc)
                return -EINVAL;
        }
 
+       misc->rp = revocable_provider_alloc(misc);
+       if (!misc->rp)
+               return -ENOMEM;
+
        INIT_LIST_HEAD(&misc->list);
 
        mutex_lock(&misc_mtx);
@@ -291,6 +291,8 @@ EXPORT_SYMBOL(misc_register);

 void misc_deregister(struct miscdevice *misc)
 {
+       revocable_provider_revoke(misc->rp);
+
        mutex_lock(&misc_mtx);
        list_del_init(&misc->list);
        device_destroy(&misc_class, MKDEV(MISC_MAJOR, misc->minor));

diff --git a/fs/fs_revocable.c b/fs/fs_revocable.c
new file mode 100644
...
+struct fs_revocable_replacement {
+       struct revocable *rev;
+       const struct file_operations *orig_fops;
+       struct file_operations fops;
+};
+
+static ssize_t fs_revocable_read(struct file *filp, char __user *buffer,
+                                size_t length, loff_t *offset)
+{
+       void *any;
+       struct fs_revocable_replacement *rr = filp->f_rr;
+
+       REVOCABLE_TRY_ACCESS_WITH(rr->rev, any) {
+               if (!any)
+                       return -ENODEV;
+               return rr->orig_fops->read(filp, buffer, length, offset);
+       }
+}
...
+int fs_revocable_replace(struct revocable_provider *rp, struct file *filp)
+{
+       struct fs_revocable_replacement *rr;
+
+       rr = kzalloc(sizeof(*rr), GFP_KERNEL);
+       if (!rr)
+               return -ENOMEM;
+
+       rr->rev = revocable_alloc(rp);
+       if (!rr->rev)
+               goto free_rr;
+
+       rr->orig_fops = filp->f_op;
+       memcpy(&rr->fops, filp->f_op, sizeof(rr->fops));
+       rr->fops.release = fs_revocable_release;
+
+       if (rr->fops.read)
+               rr->fops.read = fs_revocable_read;
+       if (rr->fops.poll)
+               rr->fops.poll = fs_revocable_poll;
+       if (rr->fops.unlocked_ioctl)
+               rr->fops.unlocked_ioctl = fs_revocable_unlocked_ioctl;
+
+       filp->f_rr = rr;
+       filp->f_op = &rr->fops;
+       return 0;
+free_rr:
+       kfree(rr);
+       return -ENOMEM;
+}
+EXPORT_SYMBOL_GPL(fs_revocable_replace);

diff --git a/include/linux/fs.h b/include/linux/fs.h
index a6de8d93838d..163496a5df6c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1066,6 +1066,7 @@ struct file {
                freeptr_t               f_freeptr;
        };
        /* --- cacheline 3 boundary (192 bytes) --- */
+       struct fs_revocable_replacement *f_rr;
 } __randomize_layout
   __attribute__((aligned(4))); /* lest something weird decides that 2 is OK */

diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index aca911687bd5..c98b97f84c07 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -94,6 +94,7 @@ struct miscdevice  {
        const struct attribute_group **groups;
        const char *nodename;
        umode_t mode;
+       struct revocable_provider *rp;
 };

Powered by blists - more mailing lists