For avoiding pde->proc_ops->... dereference(which may cause UAF in rmmod race scene), we call pde_set_flags() to save this kind of information in PDE itself before proc_register() and call pde_has_proc_XXX() to replace pde->proc_ops->... dereference. But there has omission of pde_set_flags() in net related proc file create, which cause the wroing behavior of FMODE_LSEEK clearing in proc_reg_open() for net related proc file after commit ff7ec8dc1b64("proc: use the same treatment to check proc_lseek as ones for proc_read_iter et.al"). Lars reported it in this link[1]. So call pde_set_flags() when create net related proc file to fix this bug.
[1]: https://lore.kernel.org/all/20250815195616.64497967@chagall.paradoxon.rec/
Fixes: ff7ec8dc1b64 ("proc: use the same treatment to check proc_lseek as ones for proc_read_iter et.al) Signed-off-by: wangzijie wangzijie1@honor.com --- fs/proc/generic.c | 2 +- fs/proc/internal.h | 1 + fs/proc/proc_net.c | 4 ++++ 3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 76e800e38..57ec5e385 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c @@ -561,7 +561,7 @@ struct proc_dir_entry *proc_create_reg(const char *name, umode_t mode, return p; }
-static void pde_set_flags(struct proc_dir_entry *pde) +void pde_set_flags(struct proc_dir_entry *pde) { if (pde->proc_ops->proc_flags & PROC_ENTRY_PERMANENT) pde->flags |= PROC_ENTRY_PERMANENT; diff --git a/fs/proc/internal.h b/fs/proc/internal.h index e737401d7..c4f7cbc7d 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -284,6 +284,7 @@ extern struct dentry *proc_lookup(struct inode *, struct dentry *, unsigned int) struct dentry *proc_lookup_de(struct inode *, struct dentry *, struct proc_dir_entry *); extern int proc_readdir(struct file *, struct dir_context *); int proc_readdir_de(struct file *, struct dir_context *, struct proc_dir_entry *); +void pde_set_flags(struct proc_dir_entry *);
static inline void pde_get(struct proc_dir_entry *pde) { diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c index 52f0b75cb..20bc7481b 100644 --- a/fs/proc/proc_net.c +++ b/fs/proc/proc_net.c @@ -124,6 +124,7 @@ struct proc_dir_entry *proc_create_net_data(const char *name, umode_t mode, p->proc_ops = &proc_net_seq_ops; p->seq_ops = ops; p->state_size = state_size; + pde_set_flags(p); return proc_register(parent, p); } EXPORT_SYMBOL_GPL(proc_create_net_data); @@ -170,6 +171,7 @@ struct proc_dir_entry *proc_create_net_data_write(const char *name, umode_t mode p->seq_ops = ops; p->state_size = state_size; p->write = write; + pde_set_flags(p); return proc_register(parent, p); } EXPORT_SYMBOL_GPL(proc_create_net_data_write); @@ -217,6 +219,7 @@ struct proc_dir_entry *proc_create_net_single(const char *name, umode_t mode, pde_force_lookup(p); p->proc_ops = &proc_net_single_ops; p->single_show = show; + pde_set_flags(p); return proc_register(parent, p); } EXPORT_SYMBOL_GPL(proc_create_net_single); @@ -261,6 +264,7 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo p->proc_ops = &proc_net_single_ops; p->single_show = show; p->write = write; + pde_set_flags(p); return proc_register(parent, p); } EXPORT_SYMBOL_GPL(proc_create_net_single_write);
Hi,
On 18. 08. 25, 6:05, wangzijie wrote:
For avoiding pde->proc_ops->... dereference(which may cause UAF in rmmod race scene), we call pde_set_flags() to save this kind of information in PDE itself before proc_register() and call pde_has_proc_XXX() to replace pde->proc_ops->... dereference. But there has omission of pde_set_flags() in net related proc file create, which cause the wroing behavior of FMODE_LSEEK clearing in proc_reg_open() for net related proc file after commit ff7ec8dc1b64("proc: use the same treatment to check proc_lseek as ones for proc_read_iter et.al"). Lars reported it in this link[1]. So call pde_set_flags() when create net related proc file to fix this bug.
I wonder, why is pde_set_flags() not a part of proc_register()?
Could you also use some LLM to reformat the message into something comprehensible?
thanks,
Hi,
On 18. 08. 25, 6:05, wangzijie wrote:
For avoiding pde->proc_ops->... dereference(which may cause UAF in rmmod race scene), we call pde_set_flags() to save this kind of information in PDE itself before proc_register() and call pde_has_proc_XXX() to replace pde->proc_ops->... dereference. But there has omission of pde_set_flags() in net related proc file create, which cause the wroing behavior of FMODE_LSEEK clearing in proc_reg_open() for net related proc file after commit ff7ec8dc1b64("proc: use the same treatment to check proc_lseek as ones for proc_read_iter et.al"). Lars reported it in this link[1]. So call pde_set_flags() when create net related proc file to fix this bug.
I wonder, why is pde_set_flags() not a part of proc_register()?
Not all proc_dir_entry have proc_ops, so you mean this will be a better modification?
diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 76e800e38..d52197c35 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c @@ -367,6 +367,20 @@ static const struct inode_operations proc_dir_inode_operations = { .setattr = proc_notify_change, };
+static void pde_set_flags(struct proc_dir_entry *pde) +{ + if (pde->proc_ops->proc_flags & PROC_ENTRY_PERMANENT) + pde->flags |= PROC_ENTRY_PERMANENT; + if (pde->proc_ops->proc_read_iter) + pde->flags |= PROC_ENTRY_proc_read_iter; +#ifdef CONFIG_COMPAT + if (pde->proc_ops->proc_compat_ioctl) + pde->flags |= PROC_ENTRY_proc_compat_ioctl; +#endif + if (pde->proc_ops->proc_lseek) + pde->flags |= PROC_ENTRY_proc_lseek; +} + /* returns the registered entry, or frees dp and returns NULL on failure */ struct proc_dir_entry *proc_register(struct proc_dir_entry *dir, struct proc_dir_entry *dp) @@ -374,6 +388,9 @@ struct proc_dir_entry *proc_register(struct proc_dir_entry *dir, if (proc_alloc_inum(&dp->low_ino)) goto out_free_entry;
+ if (dp->proc_ops) + pde_set_flags(dp); + write_lock(&proc_subdir_lock); dp->parent = dir; if (pde_subdir_insert(dir, dp) == false) {
Could you also use some LLM to reformat the message into something comprehensible?
English is not my native language, so using LLM to format message is a good suggestion for me, Thank you.
thanks,
js suse labs
On 18. 08. 25, 10:01, wangzijie wrote:
Hi,
On 18. 08. 25, 6:05, wangzijie wrote:
For avoiding pde->proc_ops->... dereference(which may cause UAF in rmmod race scene), we call pde_set_flags() to save this kind of information in PDE itself before proc_register() and call pde_has_proc_XXX() to replace pde->proc_ops->... dereference. But there has omission of pde_set_flags() in net related proc file create, which cause the wroing behavior of FMODE_LSEEK clearing in proc_reg_open() for net related proc file after commit ff7ec8dc1b64("proc: use the same treatment to check proc_lseek as ones for proc_read_iter et.al"). Lars reported it in this link[1]. So call pde_set_flags() when create net related proc file to fix this bug.
I wonder, why is pde_set_flags() not a part of proc_register()?
Not all proc_dir_entry have proc_ops, so you mean this will be a better modification?
diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 76e800e38..d52197c35 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c @@ -367,6 +367,20 @@ static const struct inode_operations proc_dir_inode_operations = { .setattr = proc_notify_change, };
+static void pde_set_flags(struct proc_dir_entry *pde) +{
if (pde->proc_ops->proc_flags & PROC_ENTRY_PERMANENT)pde->flags |= PROC_ENTRY_PERMANENT;if (pde->proc_ops->proc_read_iter)pde->flags |= PROC_ENTRY_proc_read_iter;+#ifdef CONFIG_COMPAT
if (pde->proc_ops->proc_compat_ioctl)pde->flags |= PROC_ENTRY_proc_compat_ioctl;+#endif
if (pde->proc_ops->proc_lseek)pde->flags |= PROC_ENTRY_proc_lseek;+}
- /* returns the registered entry, or frees dp and returns NULL on failure */ struct proc_dir_entry *proc_register(struct proc_dir_entry *dir, struct proc_dir_entry *dp)
@@ -374,6 +388,9 @@ struct proc_dir_entry *proc_register(struct proc_dir_entry *dir, if (proc_alloc_inum(&dp->low_ino)) goto out_free_entry;
if (dp->proc_ops)pde_set_flags(dp);
This occurs much saner and better to me. But I am no FS fellow. It's up to them to tell if it is the right thing to do.
(The above hunks are missing removal of pde_set_flags() from the original location and removal of now unnecessary calls of pde_set_flags(). But I assume you would do that in a real/submitted patch.)
thanks,
k.shutemov no longer @ intel -> use gmail.
On 18. 08. 25, 10:08, Jiri Slaby wrote:
On 18. 08. 25, 10:01, wangzijie wrote:
Hi,
On 18. 08. 25, 6:05, wangzijie wrote:
For avoiding pde->proc_ops->... dereference(which may cause UAF in rmmod race scene), we call pde_set_flags() to save this kind of information in PDE itself before proc_register() and call pde_has_proc_XXX() to replace pde-
proc_ops->... dereference.
But there has omission of pde_set_flags() in net related proc file create, which cause the wroing behavior of FMODE_LSEEK clearing in proc_reg_open() for net related proc file after commit ff7ec8dc1b64("proc: use the same treatment to check proc_lseek as ones for proc_read_iter et.al"). Lars reported it in this link[1]. So call pde_set_flags() when create net related proc file to fix this bug.
I wonder, why is pde_set_flags() not a part of proc_register()?
Not all proc_dir_entry have proc_ops, so you mean this will be a better modification?
diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 76e800e38..d52197c35 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c @@ -367,6 +367,20 @@ static const struct inode_operations proc_dir_inode_operations = { .setattr = proc_notify_change, };
+static void pde_set_flags(struct proc_dir_entry *pde) +{ + if (pde->proc_ops->proc_flags & PROC_ENTRY_PERMANENT) + pde->flags |= PROC_ENTRY_PERMANENT; + if (pde->proc_ops->proc_read_iter) + pde->flags |= PROC_ENTRY_proc_read_iter; +#ifdef CONFIG_COMPAT + if (pde->proc_ops->proc_compat_ioctl) + pde->flags |= PROC_ENTRY_proc_compat_ioctl; +#endif + if (pde->proc_ops->proc_lseek) + pde->flags |= PROC_ENTRY_proc_lseek; +}
/* returns the registered entry, or frees dp and returns NULL on failure */ struct proc_dir_entry *proc_register(struct proc_dir_entry *dir, struct proc_dir_entry *dp) @@ -374,6 +388,9 @@ struct proc_dir_entry *proc_register(struct proc_dir_entry *dir, if (proc_alloc_inum(&dp->low_ino)) goto out_free_entry;
+ if (dp->proc_ops)
And to be honest, I would possibly move the 'if' into pde_set_flags().
+ pde_set_flags(dp);
As here, it is not completely clear why you would want to test "dp->proc_ops" for something called "pde_set_flags".
k.shutemov no longer @ intel -> use gmail.
On 18. 08. 25, 10:08, Jiri Slaby wrote:
On 18. 08. 25, 10:01, wangzijie wrote:
Hi,
On 18. 08. 25, 6:05, wangzijie wrote:
For avoiding pde->proc_ops->... dereference(which may cause UAF in rmmod race scene), we call pde_set_flags() to save this kind of information in PDE itself before proc_register() and call pde_has_proc_XXX() to replace pde-
proc_ops->... dereference.
But there has omission of pde_set_flags() in net related proc file create, which cause the wroing behavior of FMODE_LSEEK clearing in proc_reg_open() for net related proc file after commit ff7ec8dc1b64("proc: use the same treatment to check proc_lseek as ones for proc_read_iter et.al"). Lars reported it in this link[1]. So call pde_set_flags() when create net related proc file to fix this bug.
I wonder, why is pde_set_flags() not a part of proc_register()?
Not all proc_dir_entry have proc_ops, so you mean this will be a better modification?
diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 76e800e38..d52197c35 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c @@ -367,6 +367,20 @@ static const struct inode_operations proc_dir_inode_operations = { .setattr = proc_notify_change, };
+static void pde_set_flags(struct proc_dir_entry *pde) +{ + if (pde->proc_ops->proc_flags & PROC_ENTRY_PERMANENT) + pde->flags |= PROC_ENTRY_PERMANENT; + if (pde->proc_ops->proc_read_iter) + pde->flags |= PROC_ENTRY_proc_read_iter; +#ifdef CONFIG_COMPAT + if (pde->proc_ops->proc_compat_ioctl) + pde->flags |= PROC_ENTRY_proc_compat_ioctl; +#endif + if (pde->proc_ops->proc_lseek) + pde->flags |= PROC_ENTRY_proc_lseek; +}
/* returns the registered entry, or frees dp and returns NULL on failure */ struct proc_dir_entry *proc_register(struct proc_dir_entry *dir, struct proc_dir_entry *dp) @@ -374,6 +388,9 @@ struct proc_dir_entry *proc_register(struct proc_dir_entry *dir, if (proc_alloc_inum(&dp->low_ino)) goto out_free_entry;
+ if (dp->proc_ops)
And to be honest, I would possibly move the 'if' into pde_set_flags().
+ pde_set_flags(dp);
As here, it is not completely clear why you would want to test "dp->proc_ops" for something called "pde_set_flags".
-- js suse labs
Thank you for all suggestions, it makes sense to me. Let me submit the next version patch.
linux-stable-mirror@lists.linaro.org
-
Jiri Slaby -
wangzijie