linux-ext4 - Re: Fwd: Remounting ext4 filesystem from ro to rw fails when quotas are enabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230608044056.GA1418535@mit.edu>
Date:   Thu, 8 Jun 2023 00:40:56 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Bagas Sanjaya <bagasdotme@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Regressions <regressions@...ts.linux.dev>,
        Linux ext4 Development <linux-ext4@...r.kernel.org>,
        Nikolas Kraetzschmar <nikolas.kraetzschmar@....com>,
        Linux Stable <stable@...r.kernel.org>, Jan Kara <jack@...e.cz>,
        syzbot+6385d7d3065524c5ca6d@...kaller.appspotmail.com,
        syzkaller-bugs@...glegroups.com
Subject: Re: Fwd: Remounting ext4 filesystem from ro to rw fails when quotas
 are enabled

On Wed, Jun 07, 2023 at 12:51:26PM +0700, Bagas Sanjaya wrote:
> I notice a regression report on Bugzilla [1]. Quoting from it:
> 
> > Since commit a44be64, remounting a read-only ext4 filesystem to
> > become read-write fails when quotas are enabled. The mount syscall
> > returns -EROFS and outputs the following in dmesg:

Yeah, and I think all we can do is revert this commit:

    ext4: don't clear SB_RDONLY when remounting r/w until quota is re-enabled

I think I saw some messages go by about this getting queued for the
stable kernel; if so, could it please be dropped?

> > 
> > The problem can be traced back to the changes introduced in commit
> > a44be64. It appears that the issue arises because the SB_RDONLY
> > bit of the s_flags field is now only cleared after executing the
> > ext4_enable_quotas function. However, the vfs_setup_quota_inode
> > function, called by ext4_enable_quotas, checks whether this bit is
> > set (fs/quota/dquot.c:2331):

The problem that we're trying to solve is the malicious syzbot
reproducer is in one thread, twiddling the file system state from r/o
to rw and back.  In another thread, it's attempt to create files and
directories.   And occasionally, we're tripping this warning:

	WARN_ON_ONCE(dquot_initialize_needed(inode));

That's because we're racing with the quota getting initialized, and
the moment we clear the SB_RDONLY bit the thread which is trying to
create a directory or file will proceed with the operation --- even
though the quota subsystem hasn't been initialized yet.  That's why
the patch attempted to move the clearing the SB_RDONLY bit ahead of
reiniitalization of the quota subsystem.

Since this is screwing up the ability to remount the file system rw,
we need to revert this commit, at which point, we'll be able to
trigger this warning again.

So how do we fix the warning?  Well, we could just drop the
WARN_ON_ONCE(); the downside is that when this race gets hit, the
quota operations to allocate the block and inode will silently become
a no-op, which means the quota will get out of sync with reality.

Alternatively, we could add a call to the beginning to
ext4_xattr_block_Set():

	if (dquot_initialize_needed(inode))
		reutrn -EROFS;

... on the theory that the only time we should hit this is when there
is a quota setup racing with inode creation, and it's better that we
just let the mkdir or open with O_CREAT fail than to succeed, and
allocate blocks before the quota subsystem has been initialized.  I'm
not sure how safe this would be on older quota setups (pre-ext4 quota
feature), since I suspect the race window is a quite a bit bigger if I
understand correctly how things worked with the legacy quota support.

The final really hacky thing I could imagine is to hack
dquot_initialize() to something like this:

int dquot_initialize(struct inode *inode)
{
	ret = __dquot_initialize(inode, -1);
	if (ret)
		return ret;
	if (dquot_initialize_needed(inode)) {
		msleep(1000)
		return __dquot_initialize(inode, -1);
	}
	return 0;
}

But I threw up a little in my mouth while writing it....

So I'm tempted to just remove the WARN_ON's, and just accept that if
superuser wants to twiddle the read/only state of a file system with
quota, at high rates, while something else is trying to create
files/directories, most of which will fail while the file system is
read-only, then the quota may gets out of sync, and... ¯\_(ツ)_/¯

Since this is only something that crazy people or malicious Syzbot
reproducers would try to do, I'm really having a hard time bringing
myself to care.  Especially since we have plenty of other places where
we aren't doing the dquot_initialize_needed() check, so the
opportunities for the quota to get out of sync already exist in other
code paths.

Jan, what do you think?

					- Ted