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

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

[Sahitya Tummala]

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@codeaurora.org>
---
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.

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

[Sahitya Tummala]

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@codeaurora.org>
> ---
> 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.
> 

-- 
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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@codeaurora.org>
>> ---
>> 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.
>> 

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

[Christoph Hellwig]

On Thu, May 16, 2019 at 06:27:53PM +0530, stummala@codeaurora.org wrote:
> Hi Christoph, Al,
>
> Can you please consider this patch for merging?

I've been sitting on this for a while, mostly because I can't convince
myself it is safe.  What protects other threads from using ->s_dentry
just when we clear it?  Also why would sd->s_dentry == dentry ever be
false?

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

[Sahitya Tummala]

On Fri, May 17, 2019 at 10:23:12AM +0200, Christoph Hellwig wrote:
> On Thu, May 16, 2019 at 06:27:53PM +0530, stummala@codeaurora.org wrote:
> > Hi Christoph, Al,
> >
> > Can you please consider this patch for merging?
> 
> I've been sitting on this for a while, mostly because I can't convince
> myself it is safe.  What protects other threads from using ->s_dentry
> just when we clear it?  Also why would sd->s_dentry == dentry ever be
> false?

Thanks Christoph for getting back on this.
I will try to find answers to your queries and get back on this.

Besides, Al Viro reviewed this patch [1] and commented that fix looks
good. Hence, I was following up to get this merged as I thought it
must be a miss to not pick it up :)

[1] - https://lkml.org/lkml/2019/1/3/47

Thanks,
Sahitya.

-- 
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[Christoph Hellwig]

Thanks,

applied to configfs-for-next.