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,