[v2] ocfs2: fix slab-use-after-free due to dangling pointer dqi_priv

Commit Message

Dennis Lam Dec. 18, 2024, 2:39 a.m. UTC

When mounting ocfs2 and then remounting it as read-only, a
slab-use-after-free occurs after the user uses a syscall to
quota_getnextquota. Specifically, sb_dqinfo(sb, type)->dqi_priv is the
dangling pointer.

During the remounting process, the pointer dqi_priv is freed but is
never set as null leaving it to to be accessed. Additionally, the
read-only option for remounting sets the DQUOT_SUSPENDED flag instead of
setting the DQUOT_USAGE_ENABLED flags. Moreover, later in the process of
getting the next quota, the function ocfs2_get_next_id is called and
only checks the quota usage flags and not the quota suspended flags.

To fix this, I set dqi_priv to null when it is freed after remounting
with read-only and put a check for DQUOT_SUSPENDED in ocfs2_get_next_id.

Signed-off-by: Dennis Lam <[e-mail]>
Reported-by: [e-mail]
Tested-by: [e-mail]
Closes: https://lore.kernel.org/all/[e-mail]/T/
---
Changes in v2:
- replaced dquot suspended check with !sb_has_quota_active instead
- link to v1: https://lore.kernel.org/lkml/[e-mail]/

 fs/ocfs2/quota_global.c | 2 +-
 fs/ocfs2/quota_local.c  | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Joseph Qi Dec. 19, 2024, 2:24 a.m. UTC | #1

On 2024/12/18 10:39, Dennis Lam wrote:
> When mounting ocfs2 and then remounting it as read-only, a
> slab-use-after-free occurs after the user uses a syscall to
> quota_getnextquota. Specifically, sb_dqinfo(sb, type)->dqi_priv is the
> dangling pointer.
> 
> During the remounting process, the pointer dqi_priv is freed but is
> never set as null leaving it to to be accessed. Additionally, the
> read-only option for remounting sets the DQUOT_SUSPENDED flag instead of
> setting the DQUOT_USAGE_ENABLED flags. Moreover, later in the process of
> getting the next quota, the function ocfs2_get_next_id is called and
> only checks the quota usage flags and not the quota suspended flags.
> 
> To fix this, I set dqi_priv to null when it is freed after remounting
> with read-only and put a check for DQUOT_SUSPENDED in ocfs2_get_next_id.
> 
> Signed-off-by: Dennis Lam <[e-mail]>
> Reported-by: [e-mail]
> Tested-by: [e-mail]
> Closes: https://lore.kernel.org/all/[e-mail]/T/

Fixes: 8f9e8f5fcc05 ("ocfs2: Fix Q_GETNEXTQUOTA for filesystem without quotas")
Cc: <[e-mail]>
Reviewed-by: Joseph Qi <[e-mail]>

> ---
> Changes in v2:
> - replaced dquot suspended check with !sb_has_quota_active instead
> - link to v1: https://lore.kernel.org/lkml/[e-mail]/
> 
>  fs/ocfs2/quota_global.c | 2 +-
>  fs/ocfs2/quota_local.c  | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
> index 2b0daced98eb..096b799d60a0 100644
> --- a/fs/ocfs2/quota_global.c
> +++ b/fs/ocfs2/quota_global.c
> @@ -893,7 +893,7 @@ static int ocfs2_get_next_id(struct super_block *sb, struct kqid *qid)
>  	int status = 0;
>  
>  	trace_ocfs2_get_next_id(from_kqid(&init_user_ns, *qid), type);
> -	if (!sb_has_quota_loaded(sb, type)) {
> +	if (!sb_has_quota_active(sb, type)){
>  		status = -ESRCH;
>  		goto out;
>  	}
> diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c
> index 73d3367c533b..2956d888c131 100644
> --- a/fs/ocfs2/quota_local.c
> +++ b/fs/ocfs2/quota_local.c
> @@ -867,6 +867,7 @@ static int ocfs2_local_free_info(struct super_block *sb, int type)
>  	brelse(oinfo->dqi_libh);
>  	brelse(oinfo->dqi_lqi_bh);
>  	kfree(oinfo);
> +	info->dqi_priv = NULL;
>  	return status;
>  }
>

Joseph Qi Dec. 19, 2024, 3:47 a.m. UTC | #2

On 2024/12/19 10:24, Joseph Qi wrote:
> 
> 
> On 2024/12/18 10:39, Dennis Lam wrote:
>> When mounting ocfs2 and then remounting it as read-only, a
>> slab-use-after-free occurs after the user uses a syscall to
>> quota_getnextquota. Specifically, sb_dqinfo(sb, type)->dqi_priv is the
>> dangling pointer.
>>
>> During the remounting process, the pointer dqi_priv is freed but is
>> never set as null leaving it to to be accessed. Additionally, the
>> read-only option for remounting sets the DQUOT_SUSPENDED flag instead of
>> setting the DQUOT_USAGE_ENABLED flags. Moreover, later in the process of
>> getting the next quota, the function ocfs2_get_next_id is called and
>> only checks the quota usage flags and not the quota suspended flags.
>>
>> To fix this, I set dqi_priv to null when it is freed after remounting
>> with read-only and put a check for DQUOT_SUSPENDED in ocfs2_get_next_id.
>>
>> Signed-off-by: Dennis Lam <[e-mail]>
>> Reported-by: [e-mail]
>> Tested-by: [e-mail]
>> Closes: https://lore.kernel.org/all/[e-mail]/T/
> 
> Fixes: 8f9e8f5fcc05 ("ocfs2: Fix Q_GETNEXTQUOTA for filesystem without quotas")
> Cc: <[e-mail]>
> Reviewed-by: Joseph Qi <[e-mail]>
> 
>> ---
>> Changes in v2:
>> - replaced dquot suspended check with !sb_has_quota_active instead
>> - link to v1: https://lore.kernel.org/lkml/[e-mail]/
>>
>>  fs/ocfs2/quota_global.c | 2 +-
>>  fs/ocfs2/quota_local.c  | 1 +
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
>> index 2b0daced98eb..096b799d60a0 100644
>> --- a/fs/ocfs2/quota_global.c
>> +++ b/fs/ocfs2/quota_global.c
>> @@ -893,7 +893,7 @@ static int ocfs2_get_next_id(struct super_block *sb, struct kqid *qid)
>>  	int status = 0;
>>  
>>  	trace_ocfs2_get_next_id(from_kqid(&init_user_ns, *qid), type);
>> -	if (!sb_has_quota_loaded(sb, type)) {
>> +	if (!sb_has_quota_active(sb, type)){

Moreover, better to leave a space before '{'.
Sorry for not pointing out this in my first reply.

Thanks,
Joseph

>>  		status = -ESRCH;
>>  		goto out;
>>  	}
>> diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c
>> index 73d3367c533b..2956d888c131 100644
>> --- a/fs/ocfs2/quota_local.c
>> +++ b/fs/ocfs2/quota_local.c
>> @@ -867,6 +867,7 @@ static int ocfs2_local_free_info(struct super_block *sb, int type)
>>  	brelse(oinfo->dqi_libh);
>>  	brelse(oinfo->dqi_lqi_bh);
>>  	kfree(oinfo);
>> +	info->dqi_priv = NULL;
>>  	return status;
>>  }
>>  
>

Dennis Lam Dec. 19, 2024, 5:46 a.m. UTC | #3

> Moreover, better to leave a space before '{'.
> Sorry for not pointing out this in my first reply.
>
> Thanks,
> Joseph
>

No worries, I think it's actually my fault for forgetting to use the
checkpatch script before sending. I'll send another patch with the
checkpatch warnings and errors fixed, and I'll try to be more mindful
the next time I send a patch.

Regards,
Dennis

Joseph Qi Dec. 19, 2024, 5:51 a.m. UTC | #4

On 2024/12/19 13:46, Dennis Lam wrote:
>> Moreover, better to leave a space before '{'.
>> Sorry for not pointing out this in my first reply.
>>
>> Thanks,
>> Joseph
>>
> 
> No worries, I think it's actually my fault for forgetting to use the
> checkpatch script before sending. I'll send another patch with the
> checkpatch warnings and errors fixed, and I'll try to be more mindful
> the next time I send a patch.
> 

Andrew has already done the favor.

Thanks,
Joseph

Dennis Lam Dec. 19, 2024, 6:07 a.m. UTC | #5

On Thu, Dec 19, 2024 at 12:51 AM Joseph Qi <[e-mail]> wrote:
>
> Andrew has already done the favor.
>
> Thanks,
> Joseph
>
>

Thanks for notifying me. I will not be sending another updated patch then.

Regards,
Dennis

Patch

diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
index 2b0daced98eb..096b799d60a0 100644
--- a/fs/ocfs2/quota_global.c
+++ b/fs/ocfs2/quota_global.c
@@ -893,7 +893,7 @@  static int ocfs2_get_next_id(struct super_block *sb, struct kqid *qid)
 	int status = 0;
 
 	trace_ocfs2_get_next_id(from_kqid(&init_user_ns, *qid), type);
-	if (!sb_has_quota_loaded(sb, type)) {
+	if (!sb_has_quota_active(sb, type)){
 		status = -ESRCH;
 		goto out;
 	}
diff --git a/fs/ocfs2/quota_local.c b/fs/ocfs2/quota_local.c
index 73d3367c533b..2956d888c131 100644
--- a/fs/ocfs2/quota_local.c
+++ b/fs/ocfs2/quota_local.c
@@ -867,6 +867,7 @@  static int ocfs2_local_free_info(struct super_block *sb, int type)
 	brelse(oinfo->dqi_libh);
 	brelse(oinfo->dqi_lqi_bh);
 	kfree(oinfo);
+	info->dqi_priv = NULL;
 	return status;
 }