Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Andreas Dilger <adilger@dilger.ca>
Message-Id: <459EE6E3-1CB2-4898-8C5F-283E821B2A75@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_4D06B8B6-5BA9-44A7-A0A1-4E0109678C8C";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: PROBLEM: potential concurrency bug in swap_inode_boot_loader()
Date:   Fri, 4 Sep 2020 14:46:48 -0600
In-Reply-To: <59AE9CA8-074C-4971-A857-175CA0E86420@purdue.edu>
Cc:     "tytso@mit.edu" <tytso@mit.edu>,
        "linux-ext4@vger.kernel.org" <linux-ext4@vger.kernel.org>,
        "Sousa da Fonseca, Pedro Jose" <pfonseca@purdue.edu>
To:     "Gong, Sishuai" <sishuai@purdue.edu>
References: <59AE9CA8-074C-4971-A857-175CA0E86420@purdue.edu>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk


--Apple-Mail=_4D06B8B6-5BA9-44A7-A0A1-4E0109678C8C
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

On Sep 4, 2020, at 9:57 AM, Gong, Sishuai <sishuai@purdue.edu> wrote:
> We found a potential concurrency bug in linux kernel 5.3.11. We were =
able to reproduce this bug in x86 under specific thread interleavings. =
This bug causes a =E2=80=9Cchecksum invalid=E2=80=9D EXT4-fs error.

I think there are two separate problems here, one that your test =
exposes,
and a larger an architectural conflict between swap_inode_boot_loader()
and metadata_csum.

The easier problem that your test code exposes is that the inode swap is
not "atomic" w.r.t. changing the data and updating the *inode* checksum, =
as
no sane user is going to be doing concurrent swaps on their bootloader =
inode
(race conditions would make it hard to know what the end result is).  =
This
also has an easy fix - add a mutex to ext4_sb_info and held at the start =
of swap_inode_boot_loader() to provide exclusion for the whole thing to =
avoid
access to the inode until the process is complete.  This is not =
performance
critical code.


The more complex issue (which you haven't hit, but initially I thought =
was
the culprit) is that there are checksums in the extent/index blocks that
use the inode number as part of the checksum seed, to allow detection of
incorrectly accessing extent metadata from the wrong inode, even if the
checksum of the contents would otherwise appear correct.

This means that the checksums for extents on the swapped inodes may need
to be updated as part of the swap to ensure that they are again valid, =
but
I don't see that being done anywhere.  The "swap" is merely copying the
contents of the in-inode block pointers between the two inodes, so it =
may
have the wrong checksums, if that feature is enabled.

If the bootloader inode is very large (over 512MB), or the allocation is
very fragmented and needs many extents to describe, it may also overflow
the maximum 4 extents that can fit directly into the inode.  That means
a traversal of the extent tree could be needed to update the checksums
in the index/extent blocks outside the inode itself.  However, this is
unlikely to be an issue for most systems, but could be hit in real =
usage.

One important question is whether the boot loader inode is actually used
by (m)any distros or not?  This would indicate how important this bug =
is,
or if this is currently only an academic exercise.  The easy workaround
would be to disable metadata_csum on the boot filesystem, but that isn't
very appealing.  Since concurrent swaps are already only a 0.000001%
issue, the mutex will fix those cases, but is unlikely to affect anyone.

The "fragmented and large" issue is more work to fix, and potentially an
issue that could be hit in real life.  I checked one of my systems and
see that /boot has kernels and initrd/initramfs in the tens of MB, and
filefrag shows them as having 1-4 extents.  Granted that my /boot fs is
small (190MB) it is harder to allocate contiguous extents.  When using
a boot inode inside a larger filesystem, that may be less of a concern,
but could still be hit, so will eventually also need to be fixed.

Cheers, Andreas

> ------------------------------------------
> Kernel console output
>=20
> EXT4-fs error (device sda1): swap_inode_boot_loader:124: inode #5: =
comm ski-executor:iget: checksum invalid
>=20
> ------------------------------------------
> Test input
>=20
> This bug occurs when a kernel test program is executed twice in =
different threads and ran concurrently. Our analysis has located that it =
happens when syscall ioctl with the EXT4_IOC_SWAP_BOOT flag is called =
twice and interleaves with itself.
> The test program is generated by Syzkaller as follows:
> r0 =3D creat(&(0x7f0000000080)=3D'./file0\x00', 0x0)
> ioctl$FS_IOC_SETFLAGS(r0, 0x40046602, &(0x7f0000000040))
> r1 =3D creat(&(0x7f0000000000)=3D'./file0\x00', 0x0)
> pwrite64(r1, &(0x7f00000000c0)=3D'\x00', 0x1, 0x1010000)
> r2 =3D creat(&(0x7f0000000000)=3D'./file0\x00', 0x0)
> ioctl$EXT4_IOC_SWAP_BOOT(r2, 0x6611)
>=20
> ------------------------------------------
> Interleaving
>=20
> Our analysis revealed that the following interleaving triggers the =
bug.
> CPU0								CPU1
> 									=
swap_inode_boot_loader()
> 									=
=E2=80=A6
> 										=
bytes =3D inode_bl->i_bytes;
>  										=
inode_bl->i_blocks =3D inode->i_blocks;
>  										=
inode_bl->i_bytes =3D inode->i_bytes;
> 								--->     =
	err =3D ext4_mark_inode_dirty(handle, inode_bl);
>=20
> 										=
ext4_mark_iloc_dirty() (fs/ext4/ioctl.c: 223)
> 										=
	ext4_do_update_inode()
> 										=
		ext4_inode_csum_set()
> 										=
			ext4_has_metadata_sum()
> 										=
				ext4_inode_csum()
> 										=
					ext4_chksum()
> 										=
						crypto_shash_update()
> 										=
							chksum_update()
> 									=
[context switch]
> swap_inode_boot_loader()
> 	ext4_iget()
> 		ext4_inode_csum_verify(fs/ext4/inode.c:4927)
> [EXT4-fs error]
>=20
>=20
>=20
>=20
> Thanks,
> Sishuai
>=20


Cheers, Andreas


--Apple-Mail=_4D06B8B6-5BA9-44A7-A0A1-4E0109678C8C
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iQIzBAEBCAAdFiEEDb73u6ZejP5ZMprvcqXauRfMH+AFAl9Sp7kACgkQcqXauRfM
H+CCVhAAvzlnHObYZbz2goc9s5FvHBsjOpjatcSVskiK9BLiO6hno7Fw+mApNqu+
FJVX0kGdd3z5GwIhOvGzcG2g2ghepUIMBAPTU5ZM8LvdKI0YJ2qTZCcDxanjgMcK
5w+937lxAmgctE0I7ersGCBOxel8vuAZscECKU0zsREaWeRx+YKFON/nDRflbWvm
i3shfI+5/abRKndoeKb9RG036/4kjOUkW6sTQ/LbxtL4nhcbrscjDO+TKmGaxKTK
dWdQp5JjlLkUZF9YpfujHjKV0FinYwIXu7B6qmlNbVjSpeHoRILYh3OuEFMqgFeo
Foh20UGXiQM1a0ED7LmgNymjyFos8/YXAObHDIFJDM92eSsu70KBqs2WveDjp0Fk
JOrElyCn/WhQnkjdFeAunWDlNvqPIwMWIet37OBjCuYxBFklzjUo4mWg21F9MsJN
d9N89JjWPBbuGd3QR2kEa7yuzUYXVBORJOGNQrchuQ5uHJIZTvhjZSbiXYkb+ZME
LKKSGwd4/1mRz3/chqeyIXST+sOKMkEaz23FcInZarApe4UIR0mMCWmrHQPSWMKH
EPTr2RnW8TGjPbEitwliiXqgorTYjpF6GXJXF1ZURDW+iXf794VIcCyl+sIUkn49
pEdVlyVq60atQ+NeZLS6CbMedqq5tlx1QzwJuCQN+L99j1sWUYo=
=fzy8
-----END PGP SIGNATURE-----

--Apple-Mail=_4D06B8B6-5BA9-44A7-A0A1-4E0109678C8C--