Re: [PATCH] proc: fix use-after-free in proc_get_inode()

[Alexey Dobriyan <adobriyan@gmail.com>]

On Sat, Mar 01, 2025 at 07:46:13AM +0300, Alexey Dobriyan wrote:
> On Sat, Mar 01, 2025 at 11:40:24AM +0800, Ye Bin wrote:
> > There's a issue as follows:
> > BUG: unable to handle page fault for address: fffffbfff80a702b
> 
> > Above issue may happen as follows:
> >       rmmod                         lookup
> > sys_delete_module
> >                          proc_lookup_de
> >                            read_lock(&proc_subdir_lock);
> > 			   pde_get(de);
> > 			   read_unlock(&proc_subdir_lock);
> > 			   proc_get_inode(dir->i_sb, de);
> >   mod->exit()
> >     proc_remove
> >       remove_proc_subtree
> >        write_lock(&proc_subdir_lock);
> >        write_unlock(&proc_subdir_lock);
> >        proc_entry_rundown(de);
> >   free_module(mod);
> > 
> >                                if (S_ISREG(inode->i_mode))
> > 	                         if (de->proc_ops->proc_read_iter)
> >                            --> As module is already freed, will trigger UAF
> 
> Hey look, vintage 17.5 year old /proc bug.
> This just shows how long I didn't ran rmmod test. :-(
> 
> > To solve above issue there's need to get 'in_use' before use proc_dir_entry
> > in proc_get_inode().
> > 
> > Fixes: fd5a13f4893c ("proc: add a read_iter method to proc proc_ops")
> 
> OK, this is copy of the original sin below.
> 
> > Fixes: 778f3dd5a13c ("Fix procfs compat_ioctl regression")
> 
> This one is.
> 
> Let me think a little.
> 
> > --- a/fs/proc/inode.c
> > +++ b/fs/proc/inode.c
> > @@ -644,6 +644,11 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
> >  		return inode;
> >  	}
> >  
> > +	if (!pde_is_permanent(de) && !use_pde(de)) {
> > +		pde_put(de);
> > +		return NULL;
> > +	}
> > +
> >  	if (de->mode) {
> >  		inode->i_mode = de->mode;
> >  		inode->i_uid = de->uid;
> > @@ -677,5 +682,9 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
> >  	} else {
> >  		BUG();
> >  	}
> > +
> > +	if (!pde_is_permanent(de))
> > +		unuse_pde(de);
> > +

I can't reproduce. Can you test new patch -- it avoid 2 atomic ops on
common path.

If the bug is looking into pde->proc_ops, then don't do it.