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 <[email protected]>
---
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.
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
>> ---
>> 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.
>>
On Thu, May 16, 2019 at 06:27:53PM +0530, [email protected] 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?
On Fri, May 17, 2019 at 10:23:12AM +0200, Christoph Hellwig wrote:
> On Thu, May 16, 2019 at 06:27:53PM +0530, [email protected] 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.
Thanks,
applied to configfs-for-next.