Re: [PATCH v2] configfs: Fix use-after-free when accessing sd->s_dentry

[stummala]

Hi Christoph, Al,

Can you please consider this patch for merging?

Thanks,
Sahitya.

On 2019-01-31 08:50, Sahitya Tummala wrote:
> Al,
>
> Can we merge this patch, if there are no further comments?
>
> Thanks,
> Sahitya.
>
> On Thu, Jan 03, 2019 at 04:48:15PM +0530, Sahitya Tummala wrote:
> > In the vfs_statx() context, during path lookup, the dentry gets
> > added to sd->s_dentry via configfs_attach_attr(). In the end,
> > vfs_statx() kills the dentry by calling path_put(), which invokes
> > configfs_d_iput(). Ideally, this dentry must be removed from
> > sd->s_dentry but it doesn't if the sd->s_count >= 3. As a result,
> > sd->s_dentry is holding reference to a stale dentry pointer whose
> > memory is already freed up. This results in use-after-free issue,
> > when this stale sd->s_dentry is accessed later in
> > configfs_readdir() path.
> >
> > This issue can be easily reproduced, by running the LTP test case -
> > sh fs_racer_file_list.sh /config
> > (https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/fs/racer/fs_racer_file_list.sh)
> >
> > Fixes: 76ae281f6307 ('configfs: fix race between dentry put and 
> > lookup')
> > Signed-off-by: Sahitya Tummala <stummala@xxxxxxxxxxxxxx>
> > ---
> > v2:
> > - update comments relevant to the code.
> >
> >  fs/configfs/dir.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> > index 39843fa..f113101 100644
> > --- a/fs/configfs/dir.c
> > +++ b/fs/configfs/dir.c
> > @@ -58,15 +58,14 @@ static void configfs_d_iput(struct dentry * 
> > dentry,
> >  	if (sd) {
> >  		/* Coordinate with configfs_readdir */
> >  		spin_lock(&configfs_dirent_lock);
> > -		/* Coordinate with configfs_attach_attr where will increase
> > -		 * sd->s_count and update sd->s_dentry to new allocated one.
> > -		 * Only set sd->dentry to null when this dentry is the only
> > -		 * sd owner.
> > +		/*
> > +		 * Set sd->s_dentry to null only when this dentry is the
> > +		 * one that is going to be killed.
> >  		 * If not do so, configfs_d_iput may run just after
> >  		 * configfs_attach_attr and set sd->s_dentry to null
> >  		 * even it's still in use.
> >  		 */
> > -		if (atomic_read(&sd->s_count) <= 2)
> > +		if (sd->s_dentry == dentry)
> >  			sd->s_dentry = NULL;
> >
> >  		spin_unlock(&configfs_dirent_lock);
> > --
> > Qualcomm India Private Limited, on behalf of Qualcomm Innovation 
> > Center, Inc.
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
> > Linux Foundation Collaborative Project.