Re: [PATCH] proc: fix wrong behavior of FMODE_LSEEK clearing for net related proc file

[Jiri Slaby <jirislaby@kernel.org>]

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,
-- 
js
suse labs