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

[Jiri Slaby <jirislaby@kernel.org>]

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