| Message ID | 20230522102506.9205-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ocfs2: fix use-after-free when unmounting read-only filesystem | expand |
On 5/22/23 8:23 PM, Luís Henriques wrote: > Joseph Qi <joseph.qi@linux.alibaba.com> 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
On 5/22/23 9:22 PM, Luís Henriques wrote: > Heming Zhao <heming.zhao@suse.com> writes: > >> On Mon, May 22, 2023 at 01:23:07PM +0100, Luís Henriques wrote: >>> Joseph Qi <joseph.qi@linux.alibaba.com> 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. >> >> It looks you did some special steps for 452. In my env, without changing >> anything, I could pass this case successfully. > > No, I haven't changed anything to the test. I just make sure there's a > scratch device to be used. > > Maybe you can try to enable KASAN to catch the UAF. I've found the bug > without KASAN (i.e. I saw a NULL pointer panic), but enabling it also > detects the issue -- see below. > > Cheers, We'd better append panic dmesg to patch description as well. Thanks, Joseph
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
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: <stable@vger.kernel.org> Signed-off-by: Luís Henriques <lhenriques@suse.de> --- fs/ocfs2/super.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)