It's trivial to trigger a use-after-free bug in the ocfs2 quotas code using
fstest generic/452. After mounting a filesystem as read-only, quotas are
suspended and ocfs2_mem_dqinfo is freed through ->ocfs2_local_free_info(). When
unmounting the filesystem, an UAF access to the oinfo will eventually cause a
crash.
Cc: <[email protected]>
Signed-off-by: Luís Henriques <[email protected]>
---
fs/ocfs2/super.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 0b0e6a132101..988d1c076861 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -952,8 +952,10 @@ static void ocfs2_disable_quotas(struct ocfs2_super *osb)
for (type = 0; type < OCFS2_MAXQUOTAS; type++) {
if (!sb_has_quota_loaded(sb, type))
continue;
- oinfo = sb_dqinfo(sb, type)->dqi_priv;
- cancel_delayed_work_sync(&oinfo->dqi_sync_work);
+ if (!sb_has_quota_suspended(sb, type)) {
+ oinfo = sb_dqinfo(sb, type)->dqi_priv;
+ cancel_delayed_work_sync(&oinfo->dqi_sync_work);
+ }
inode = igrab(sb->s_dquot.files[type]);
/* Turn off quotas. This will remove all dquot structures from
* memory and so they will be automatically synced to global
On 5/22/23 6:25 PM, Luís Henriques wrote:
> It's trivial to trigger a use-after-free bug in the ocfs2 quotas code using
> fstest generic/452. After mounting a filesystem as read-only, quotas are
generic/452 is for testing ext4 mounted with dax and ro.
But ocfs2 doesn't support dax yet.
> suspended and ocfs2_mem_dqinfo is freed through ->ocfs2_local_free_info(). When
> unmounting the filesystem, an UAF access to the oinfo will eventually cause a
> crash.
In ocfs2_fill_super(), it won't enable quota if is a readonly mount.
Do you mean remount as readonly?
Thanks,
Joseph
>
> Cc: <[email protected]>
> Signed-off-by: Luís Henriques <[email protected]>
> ---
> fs/ocfs2/super.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index 0b0e6a132101..988d1c076861 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -952,8 +952,10 @@ static void ocfs2_disable_quotas(struct ocfs2_super *osb)
> for (type = 0; type < OCFS2_MAXQUOTAS; type++) {
> if (!sb_has_quota_loaded(sb, type))
> continue;
> - oinfo = sb_dqinfo(sb, type)->dqi_priv;
> - cancel_delayed_work_sync(&oinfo->dqi_sync_work);
> + if (!sb_has_quota_suspended(sb, type)) {
> + oinfo = sb_dqinfo(sb, type)->dqi_priv;
> + cancel_delayed_work_sync(&oinfo->dqi_sync_work);
> + }
> inode = igrab(sb->s_dquot.files[type]);
> /* Turn off quotas. This will remove all dquot structures from
> * memory and so they will be automatically synced to global
Joseph Qi <[email protected]> writes:
> On 5/22/23 6:25 PM, Luís Henriques wrote:
>> It's trivial to trigger a use-after-free bug in the ocfs2 quotas code using
>> fstest generic/452. After mounting a filesystem as read-only, quotas are
>
> generic/452 is for testing ext4 mounted with dax and ro.
> But ocfs2 doesn't support dax yet.
Right, but I think it's still useful to run the 'generic' test-suite in a
filesystem. We can always find issues in the test itself or, in this
case, a bug in the filesystem.
>> suspended and ocfs2_mem_dqinfo is freed through ->ocfs2_local_free_info(). When
>> unmounting the filesystem, an UAF access to the oinfo will eventually cause a
>> crash.
>
> In ocfs2_fill_super(), it won't enable quota if is a readonly mount.
> Do you mean remount as readonly?
Yes, sorry. Instead of "mounting", the patch changelog should say
"After remounting a filesystem as read-only..."
Cheers,
--
Luís
>
> Thanks,
> Joseph
>
>>
>> Cc: <[email protected]>
>> Signed-off-by: Luís Henriques <[email protected]>
>> ---
>> fs/ocfs2/super.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>> index 0b0e6a132101..988d1c076861 100644
>> --- a/fs/ocfs2/super.c
>> +++ b/fs/ocfs2/super.c
>> @@ -952,8 +952,10 @@ static void ocfs2_disable_quotas(struct ocfs2_super *osb)
>> for (type = 0; type < OCFS2_MAXQUOTAS; type++) {
>> if (!sb_has_quota_loaded(sb, type))
>> continue;
>> - oinfo = sb_dqinfo(sb, type)->dqi_priv;
>> - cancel_delayed_work_sync(&oinfo->dqi_sync_work);
>> + if (!sb_has_quota_suspended(sb, type)) {
>> + oinfo = sb_dqinfo(sb, type)->dqi_priv;
>> + cancel_delayed_work_sync(&oinfo->dqi_sync_work);
>> + }
>> inode = igrab(sb->s_dquot.files[type]);
>> /* Turn off quotas. This will remove all dquot structures from
>> * memory and so they will be automatically synced to global
On 5/22/23 8:23 PM, Luís Henriques wrote:
> Joseph Qi <[email protected]> writes:
>
>> On 5/22/23 6:25 PM, Luís Henriques wrote:
>>> It's trivial to trigger a use-after-free bug in the ocfs2 quotas code using
>>> fstest generic/452. After mounting a filesystem as read-only, quotas are
>>
>> generic/452 is for testing ext4 mounted with dax and ro.
>> But ocfs2 doesn't support dax yet.
>
> Right, but I think it's still useful to run the 'generic' test-suite in a
> filesystem. We can always find issues in the test itself or, in this
> case, a bug in the filesystem.
>
>>> suspended and ocfs2_mem_dqinfo is freed through ->ocfs2_local_free_info(). When
>>> unmounting the filesystem, an UAF access to the oinfo will eventually cause a
>>> crash.
>>
>> In ocfs2_fill_super(), it won't enable quota if is a readonly mount.
>> Do you mean remount as readonly?
>
> Yes, sorry. Instead of "mounting", the patch changelog should say
>
> "After remounting a filesystem as read-only..."
>
> Cheers,
Okay, look into the code flow, it does have problem when remount with
read-only.
BTW, it seems that we can't call into quota_disable() again, which will
call free_file_info().
Thanks,
Joseph