[PATCH v6 0/3] platform/chrome: Fix an UAF via replacing fops

[PATCH v6 0/3] platform/chrome: Fix an UAF via replacing fops

[Tzung-Bi Shih]

The series is separated from [1] to show the independency and compare
potential use cases easier.  This use case replaces filp->f_op to
revocable-aware warppers.  It relies on the revocable core part [2].

It tries to fix an UAF in the fops of cros_ec_chardev after the
underlying protocol device has gone by using revocable.

The warppers make sure file operations in drivers won't be called if the
resource has been revoked.

The 1st patch introduces revocable fops replacement.

The 2nd patch supports the fops replacement in miscdevice.

The 3rd patch uses the support from miscdevice to fix the UAF.

[1] https://lore.kernel.org/chrome-platform/20251016054204.1523139-1-tzungbi@kernel.org
[2] https://lore.kernel.org/chrome-platform/20251106152330.11733-1-tzungbi@kernel.org/

v6:
- New, separated from an existing series.

Tzung-Bi Shih (3):
  revocable: Add fops replacement
  char: misc: Leverage revocable fops replacement
  platform/chrome: cros_ec_chardev: Secure cros_ec_device via revocable

 drivers/char/misc.c                       |  18 ++-
 drivers/platform/chrome/cros_ec_chardev.c |   1 +
 fs/Makefile                               |   2 +-
 fs/fs_revocable.c                         | 156 ++++++++++++++++++++++
 include/linux/fs_revocable.h              |  14 ++
 include/linux/miscdevice.h                |   2 +
 6 files changed, 190 insertions(+), 3 deletions(-)
 create mode 100644 fs/fs_revocable.c
 create mode 100644 include/linux/fs_revocable.h

-- 
2.48.1

[PATCH v6 1/3] revocable: Add fops replacement

[Tzung-Bi Shih]

Introduce fs_revocable_replace() to simplify the use of the revocable
APIs with file_operations.

The function should only be used after filp->f_op->open().  It assumes
the filp->private_data would be set only once in filp->f_op->open() and
wouldn't update in subsequent file operations.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v6:
- Use filp->private_data for the replacement context.
- Prevent file operations from calling if the resource has been revoked.
- Support only 1 resource again.
- Rename REVOCABLE_TRY_ACCESS_WITH() -> REVOCABLE_TRY_ACCESS_SCOPED().
- Use new REVOCABLE_TRY_ACCESS_WITH() if applicable.

v5: https://lore.kernel.org/chrome-platform/20251016054204.1523139-6-tzungbi@kernel.org
- Rename to "fs_revocable".
- Move the replacement context to struct file.
- Support multiple revocable providers.

v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-6-tzungbi@kernel.org
- New in the series.

 fs/Makefile                  |   2 +-
 fs/fs_revocable.c            | 156 +++++++++++++++++++++++++++++++++++
 include/linux/fs_revocable.h |  14 ++++
 3 files changed, 171 insertions(+), 1 deletion(-)
 create mode 100644 fs/fs_revocable.c
 create mode 100644 include/linux/fs_revocable.h

diff --git a/fs/Makefile b/fs/Makefile
index a04274a3c854..f1e5d7b52781 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -16,7 +16,7 @@ obj-y :=	open.o read_write.o file_table.o super.o \
 		stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
 		fs_dirent.o fs_context.o fs_parser.o fsopen.o init.o \
 		kernel_read_file.o mnt_idmapping.o remap_range.o pidfs.o \
-		file_attr.o
+		file_attr.o fs_revocable.o
 
 obj-$(CONFIG_BUFFER_HEAD)	+= buffer.o mpage.o
 obj-$(CONFIG_PROC_FS)		+= proc_namespace.o
diff --git a/fs/fs_revocable.c b/fs/fs_revocable.c
new file mode 100644
index 000000000000..9ffa71cb67ed
--- /dev/null
+++ b/fs/fs_revocable.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 Google LLC
+ *
+ * File operation replacement with Revocable
+ */
+
+#include <linux/cleanup.h>
+#include <linux/fs_revocable.h>
+#include <linux/poll.h>
+#include <linux/revocable.h>
+
+struct fops_replacement {
+	struct file *filp;
+	void *orig_private_data;
+	const struct file_operations *orig_fops;
+	struct file_operations fops;
+	struct revocable *rev;
+};
+
+/*
+ * Recover the private_data to its original one.
+ */
+static struct fops_replacement *_recover_private_data(struct file *filp)
+{
+	struct fops_replacement *fr = filp->private_data;
+
+	filp->private_data = fr->orig_private_data;
+	return fr;
+}
+
+/*
+ * Replace the private_data to fops_replacement.
+ */
+static void _replace_private_data(struct fops_replacement *fr)
+{
+	fr->filp->private_data = fr;
+}
+
+DEFINE_CLASS(fops_replacement, struct fops_replacement *,
+	     _replace_private_data(_T), _recover_private_data(filp),
+	     struct file *filp)
+
+static ssize_t fs_revocable_read(struct file *filp, char __user *buffer,
+				 size_t length, loff_t *offset)
+{
+	void *any;
+	CLASS(fops_replacement, fr)(filp);
+
+	REVOCABLE_TRY_ACCESS_WITH(fr->rev, any);
+	if (!any)
+		return -ENODEV;
+
+	return fr->orig_fops->read(filp, buffer, length, offset);
+}
+
+static __poll_t fs_revocable_poll(struct file *filp, poll_table *wait)
+{
+	void *any;
+	CLASS(fops_replacement, fr)(filp);
+
+	REVOCABLE_TRY_ACCESS_WITH(fr->rev, any);
+	if (!any)
+		return -ENODEV;
+
+	return fr->orig_fops->poll(filp, wait);
+}
+
+static long fs_revocable_unlocked_ioctl(struct file *filp, unsigned int cmd,
+					unsigned long arg)
+{
+	void *any;
+	CLASS(fops_replacement, fr)(filp);
+
+	REVOCABLE_TRY_ACCESS_WITH(fr->rev, any);
+	if (!any)
+		return -ENODEV;
+
+	return fr->orig_fops->unlocked_ioctl(filp, cmd, arg);
+}
+
+static int fs_revocable_release(struct inode *inode, struct file *filp)
+{
+	struct fops_replacement *fr = _recover_private_data(filp);
+	int ret = 0;
+	void *any;
+
+	filp->f_op = fr->orig_fops;
+
+	if (!fr->orig_fops->release)
+		goto leave;
+
+	REVOCABLE_TRY_ACCESS_SCOPED(fr->rev, any) {
+		if (!any) {
+			ret = -ENODEV;
+			goto leave;
+		}
+
+		ret = fr->orig_fops->release(inode, filp);
+	}
+
+leave:
+	kfree(fr);
+	return ret;
+}
+
+/**
+ * fs_revocable_replace() - Replace the file operations to be revocable-aware.
+ * @rp: The revocable resource provider.
+ * @filp: The opening file.
+ *
+ * This replaces @filp->f_op to a set of wrappers.  The wrappers return -ENODEV
+ * if the resource provided by @rp has been revoked.  Note that it doesn't
+ * concern how the file operations access the resource but only care about if
+ * the resource is still available.
+ *
+ * This should only be used after @filp->f_op->open().  It assumes the
+ * @filp->private_data would be set only once in @filp->f_op->open() and wouldn't
+ * update in subsequent file operations.
+ */
+int fs_revocable_replace(struct revocable_provider *rp, struct file *filp)
+{
+	struct fops_replacement *fr;
+
+	fr = kzalloc(sizeof(*fr), GFP_KERNEL);
+	if (!fr)
+		return -ENOMEM;
+
+	fr->rev = revocable_alloc(rp);
+	if (!fr->rev)
+		goto free_fr;
+
+	fr->filp = filp;
+	fr->orig_private_data = filp->private_data;
+	fr->orig_fops = filp->f_op;
+
+	memcpy(&fr->fops, filp->f_op, sizeof(fr->fops));
+	fr->fops.release = fs_revocable_release;
+
+	if (fr->fops.read)
+		fr->fops.read = fs_revocable_read;
+	if (fr->fops.poll)
+		fr->fops.poll = fs_revocable_poll;
+	if (fr->fops.unlocked_ioctl)
+		fr->fops.unlocked_ioctl = fs_revocable_unlocked_ioctl;
+
+	filp->f_op = &fr->fops;
+	filp->private_data = fr;
+	return 0;
+free_fr:
+	kfree(fr);
+	if (filp->f_op->release)
+		filp->f_op->release(filp->f_inode, filp);
+	return -ENOMEM;
+}
+EXPORT_SYMBOL_GPL(fs_revocable_replace);
diff --git a/include/linux/fs_revocable.h b/include/linux/fs_revocable.h
new file mode 100644
index 000000000000..498d035315e6
--- /dev/null
+++ b/include/linux/fs_revocable.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2025 Google LLC
+ */
+
+#ifndef __LINUX_FS_REVOCABLE_H
+#define __LINUX_FS_REVOCABLE_H
+
+#include <linux/fs.h>
+#include <linux/revocable.h>
+
+int fs_revocable_replace(struct revocable_provider *rp, struct file *filp);
+
+#endif /* __LINUX_FS_REVOCABLE_H */
-- 
2.48.1

[PATCH v6 2/3] char: misc: Leverage revocable fops replacement

[Tzung-Bi Shih]

Leverage revocable fops replacement if the driver requests.

The "resource" in the context means presence of the miscdevice.  It
prevents file operations (except .release()) in drivers to be called if
the miscdevice no longer exists.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v6:
- Call fs_revocable_replace() after f_op->open() and modify the API
  usage accordingly.
- Use presence of miscdevice as a virtual resource.

v5: https://lore.kernel.org/chrome-platform/20251016054204.1523139-7-tzungbi@kernel.org
- No primary changes but modify the API usage accordingly to support
  multiple revocable providers.

v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-7-tzungbi@kernel.org
- New in the series.

 drivers/char/misc.c        | 18 ++++++++++++++++--
 include/linux/miscdevice.h |  2 ++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 726516fb0a3b..e0106270c188 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -50,6 +50,8 @@
 #include <linux/tty.h>
 #include <linux/kmod.h>
 #include <linux/gfp.h>
+#include <linux/fs_revocable.h>
+#include <linux/revocable.h>
 
 /*
  * Head entry for the doubly linked miscdevice list
@@ -157,10 +159,16 @@ static int misc_open(struct inode *inode, struct file *file)
 	 */
 	file->private_data = c;
 
-	err = 0;
 	replace_fops(file, new_fops);
-	if (file->f_op->open)
+
+	if (file->f_op->open) {
 		err = file->f_op->open(inode, file);
+		if (err)
+			goto fail;
+	}
+
+	if (c->revocable)
+		err = fs_revocable_replace(c->rp, file);
 fail:
 	mutex_unlock(&misc_mtx);
 	return err;
@@ -218,6 +226,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);
@@ -290,6 +302,8 @@ void misc_deregister(struct miscdevice *misc)
 	if (misc->minor > MISC_DYNAMIC_MINOR)
 		misc->minor = MISC_DYNAMIC_MINOR;
 	mutex_unlock(&misc_mtx);
+
+	revocable_provider_revoke(misc->rp);
 }
 EXPORT_SYMBOL(misc_deregister);
 
diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index 7d0aa718499c..85fac108e485 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -92,6 +92,8 @@ struct miscdevice {
 	const struct attribute_group **groups;
 	const char *nodename;
 	umode_t mode;
+	bool revocable;
+	struct revocable_provider *rp;
 };
 
 extern int misc_register(struct miscdevice *misc);
-- 
2.48.1

[PATCH v6 3/3] platform/chrome: cros_ec_chardev: Secure cros_ec_device via revocable

[Tzung-Bi Shih]

Miscdevice now supports revocable fops replacement.  Use it to secure
the cros_ec_device is available.

Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
v6:
- Modify the API usage accordingly.

v5: https://lore.kernel.org/chrome-platform/20251016054204.1523139-8-tzungbi@kernel.org
- No primary changes but modify the API usage accordingly.

v4: https://lore.kernel.org/chrome-platform/20250923075302.591026-8-tzungbi@kernel.org

 drivers/platform/chrome/cros_ec_chardev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
index c9d80ad5b57e..a7543e1bc07a 100644
--- a/drivers/platform/chrome/cros_ec_chardev.c
+++ b/drivers/platform/chrome/cros_ec_chardev.c
@@ -385,6 +385,7 @@ static int cros_ec_chardev_probe(struct platform_device *pdev)
 	misc->fops = &chardev_fops;
 	misc->name = ec_platform->ec_name;
 	misc->parent = pdev->dev.parent;
+	misc->revocable = true;
 
 	dev_set_drvdata(&pdev->dev, misc);
 
-- 
2.48.1

Re: [PATCH v6 1/3] revocable: Add fops replacement

[Jason Gunthorpe]

On Thu, Nov 06, 2025 at 11:27:10PM +0800, Tzung-Bi Shih wrote:
> +/*
> + * Recover the private_data to its original one.
> + */
> +static struct fops_replacement *_recover_private_data(struct file *filp)
> +{
> +	struct fops_replacement *fr = filp->private_data;
> +
> +	filp->private_data = fr->orig_private_data;
> +	return fr;
> +}
> +
> +/*
> + * Replace the private_data to fops_replacement.
> + */
> +static void _replace_private_data(struct fops_replacement *fr)
> +{
> +	fr->filp->private_data = fr;
> +}

This switching of private_data isn't reasonable, it breaks too much
stuff. I think I showed a better idea in my sketch.

I still think this is a bad use case of revocable, we don't need to
obfuscate very simple locks in *core* kernel code like this. I'd rather
see you propose this series without using it.

> +static int fs_revocable_release(struct inode *inode, struct file *filp)
> +{
> +	struct fops_replacement *fr = _recover_private_data(filp);
> +	int ret = 0;
> +	void *any;
> +
> +	filp->f_op = fr->orig_fops;
> +
> +	if (!fr->orig_fops->release)
> +		goto leave;
> +
> +	REVOCABLE_TRY_ACCESS_SCOPED(fr->rev, any) {
> +		if (!any) {
> +			ret = -ENODEV;
> +			goto leave;
> +		}
> +
> +		ret = fr->orig_fops->release(inode, filp);
> +	}

This probably doesn't work out, is likely to make a memory leak.
It will be hard for the owning driver to free its per-file memory
without access to release.

Jason

Re: [PATCH v6 1/3] revocable: Add fops replacement

[kernel test robot]

Hi Tzung-Bi,

kernel test robot noticed the following build errors:

[auto build test ERROR on brauner-vfs/vfs.all]
[also build test ERROR on v6.18-rc4 next-20251106]
[cannot apply to char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus chrome-platform/for-next chrome-platform/for-firmware-next linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tzung-Bi-Shih/revocable-Add-fops-replacement/20251106-233108
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20251106152712.11850-2-tzungbi%40kernel.org
patch subject: [PATCH v6 1/3] revocable: Add fops replacement
config: openrisc-allnoconfig (https://download.01.org/0day-ci/archive/20251107/202511070033.7X18bWdJ-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251107/202511070033.7X18bWdJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511070033.7X18bWdJ-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from fs/fs_revocable.c:9:
>> include/linux/fs_revocable.h:10:10: fatal error: linux/revocable.h: No such file or directory
      10 | #include <linux/revocable.h>
         |          ^~~~~~~~~~~~~~~~~~~
   compilation terminated.


vim +10 include/linux/fs_revocable.h

     8	
     9	#include <linux/fs.h>
  > 10	#include <linux/revocable.h>
    11	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 1/3] revocable: Add fops replacement

[kernel test robot]

Hi Tzung-Bi,

kernel test robot noticed the following build errors:

[auto build test ERROR on brauner-vfs/vfs.all]
[also build test ERROR on v6.18-rc4 next-20251106]
[cannot apply to char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus chrome-platform/for-next chrome-platform/for-firmware-next linus/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tzung-Bi-Shih/revocable-Add-fops-replacement/20251106-233108
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20251106152712.11850-2-tzungbi%40kernel.org
patch subject: [PATCH v6 1/3] revocable: Add fops replacement
config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20251107/202511070909.JnNsfyvx-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251107/202511070909.JnNsfyvx-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511070909.JnNsfyvx-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from fs/fs_revocable.c:9:
>> include/linux/fs_revocable.h:10:10: fatal error: 'linux/revocable.h' file not found
      10 | #include <linux/revocable.h>
         |          ^~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +10 include/linux/fs_revocable.h

     8	
     9	#include <linux/fs.h>
  > 10	#include <linux/revocable.h>
    11	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 1/3] revocable: Add fops replacement

[Tzung-Bi Shih]

On Thu, Nov 06, 2025 at 11:47:15AM -0400, Jason Gunthorpe wrote:
> On Thu, Nov 06, 2025 at 11:27:10PM +0800, Tzung-Bi Shih wrote:
> > +/*
> > + * Recover the private_data to its original one.
> > + */
> > +static struct fops_replacement *_recover_private_data(struct file *filp)
> > +{
> > +	struct fops_replacement *fr = filp->private_data;
> > +
> > +	filp->private_data = fr->orig_private_data;
> > +	return fr;
> > +}
> > +
> > +/*
> > + * Replace the private_data to fops_replacement.
> > + */
> > +static void _replace_private_data(struct fops_replacement *fr)
> > +{
> > +	fr->filp->private_data = fr;
> > +}
> 
> This switching of private_data isn't reasonable, it breaks too much
> stuff. I think I showed a better idea in my sketch.

The approach assumes the filp->private_data should be set once by the
filp->f_op->open() if any.  Is it common that the filp->private_data
be updated in other file operations?

> I still think this is a bad use case of revocable, we don't need to
> obfuscate very simple locks in *core* kernel code like this. I'd rather
> see you propose this series without using it.
> 
> > +static int fs_revocable_release(struct inode *inode, struct file *filp)
> > +{
> > +	struct fops_replacement *fr = _recover_private_data(filp);
> > +	int ret = 0;
> > +	void *any;
> > +
> > +	filp->f_op = fr->orig_fops;
> > +
> > +	if (!fr->orig_fops->release)
> > +		goto leave;
> > +
> > +	REVOCABLE_TRY_ACCESS_SCOPED(fr->rev, any) {
> > +		if (!any) {
> > +			ret = -ENODEV;
> > +			goto leave;
> > +		}
> > +
> > +		ret = fr->orig_fops->release(inode, filp);
> > +	}
> 
> This probably doesn't work out, is likely to make a memory leak.
> It will be hard for the owning driver to free its per-file memory
> without access to release.

Ah, I think this reveals a drawback of the approach.
- Without calling ->release(), some memory may leak.
- With calling ->release(), some UAF may happen. 

Re: [PATCH v6 1/3] revocable: Add fops replacement

[Jason Gunthorpe]

On Fri, Nov 07, 2025 at 05:07:54AM +0000, Tzung-Bi Shih wrote:
> On Thu, Nov 06, 2025 at 11:47:15AM -0400, Jason Gunthorpe wrote:
> > On Thu, Nov 06, 2025 at 11:27:10PM +0800, Tzung-Bi Shih wrote:
> > > +/*
> > > + * Recover the private_data to its original one.
> > > + */
> > > +static struct fops_replacement *_recover_private_data(struct file *filp)
> > > +{
> > > +	struct fops_replacement *fr = filp->private_data;
> > > +
> > > +	filp->private_data = fr->orig_private_data;
> > > +	return fr;
> > > +}
> > > +
> > > +/*
> > > + * Replace the private_data to fops_replacement.
> > > + */
> > > +static void _replace_private_data(struct fops_replacement *fr)
> > > +{
> > > +	fr->filp->private_data = fr;
> > > +}
> > 
> > This switching of private_data isn't reasonable, it breaks too much
> > stuff. I think I showed a better idea in my sketch.
> 
> The approach assumes the filp->private_data should be set once by the
> filp->f_op->open() if any.  Is it common that the filp->private_data
> be updated in other file operations?

You can set it once during open, but you can't change it around every
fops callback. This stuff is all concurrent.

> > This probably doesn't work out, is likely to make a memory leak.
> > It will be hard for the owning driver to free its per-file memory
> > without access to release.
> 
> Ah, I think this reveals a drawback of the approach.
> - Without calling ->release(), some memory may leak.
> - With calling ->release(), some UAF may happen. 

It just means the user of this needs to understand there are
limitations on what release can do. Usually release just frees memory,
that is fine.

I think it would be strange for a release to touch revocable data,
that might suggest some larger problem.

Jason

Re: [PATCH v6 1/3] revocable: Add fops replacement

[Tzung-Bi Shih]

On Fri, Nov 07, 2025 at 10:15:09AM -0400, Jason Gunthorpe wrote:
> On Fri, Nov 07, 2025 at 05:07:54AM +0000, Tzung-Bi Shih wrote:
> > On Thu, Nov 06, 2025 at 11:47:15AM -0400, Jason Gunthorpe wrote:
> > > On Thu, Nov 06, 2025 at 11:27:10PM +0800, Tzung-Bi Shih wrote:
> > > > +/*
> > > > + * Recover the private_data to its original one.
> > > > + */
> > > > +static struct fops_replacement *_recover_private_data(struct file *filp)
> > > > +{
> > > > +	struct fops_replacement *fr = filp->private_data;
> > > > +
> > > > +	filp->private_data = fr->orig_private_data;
> > > > +	return fr;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Replace the private_data to fops_replacement.
> > > > + */
> > > > +static void _replace_private_data(struct fops_replacement *fr)
> > > > +{
> > > > +	fr->filp->private_data = fr;
> > > > +}
> > > 
> > > This switching of private_data isn't reasonable, it breaks too much
> > > stuff. I think I showed a better idea in my sketch.
> > 
> > The approach assumes the filp->private_data should be set once by the
> > filp->f_op->open() if any.  Is it common that the filp->private_data
> > be updated in other file operations?
> 
> You can set it once during open, but you can't change it around every
> fops callback. This stuff is all concurrent.

Ah, yes, I see.

> > > This probably doesn't work out, is likely to make a memory leak.
> > > It will be hard for the owning driver to free its per-file memory
> > > without access to release.
> > 
> > Ah, I think this reveals a drawback of the approach.
> > - Without calling ->release(), some memory may leak.
> > - With calling ->release(), some UAF may happen. 
> 
> It just means the user of this needs to understand there are
> limitations on what release can do. Usually release just frees memory,
> that is fine.
> 
> I think it would be strange for a release to touch revocable data,
> that might suggest some larger problem.

I think it'd be inevitable for accessing some devm memory in ->release(),
e.g. [1].

[1] https://elixir.bootlin.com/linux/v6.17/source/drivers/platform/chrome/cros_ec_chardev.c#L260

Re: [PATCH v6 1/3] revocable: Add fops replacement

[Jason Gunthorpe]

On Mon, Nov 10, 2025 at 06:28:17AM +0000, Tzung-Bi Shih wrote:
> > It just means the user of this needs to understand there are
> > limitations on what release can do. Usually release just frees memory,
> > that is fine.
> > 
> > I think it would be strange for a release to touch revocable data,
> > that might suggest some larger problem.
> 
> I think it'd be inevitable for accessing some devm memory in ->release(),
> e.g. [1].
> 
> [1] https://elixir.bootlin.com/linux/v6.17/source/drivers/platform/chrome/cros_ec_chardev.c#L260

Again, that's symptomatic of a "larger problem" :\

The blocking notifier registration to receive events to relay out to
the file descriptors should be part this drivers probe/remove
lifecycle so it has the proper lifetime. Trying to use a revocable
idea here will create a weirdly unbalanced/dangling blocking notifier
registration :(

Inside cros_ec_chardev it should have a simple list of open file
descriptors and the single blocking notifier callback can iterate over
the list and memdup the events.  Then your release is just a simple
locked list del on a global list which doesn't have a lifecycle
problem.

Jason