Received-SPF: pass (google.com: domain of linux-kernel+bounces-143934-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Andreas Dilger <adilger@dilger.ca>
Message-Id: <22E65CA5-A2C0-44A3-AB01-7514916A18FC@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_7C671C3E-4468-48AE-8BCA-0F78F2D6442B";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: riscv32 EXT4 splat, 6.8 regression?
Date: Sat, 13 Apr 2024 19:46:03 -0600
In-Reply-To: <20240413164318.7260c5ef@namcao>
Cc: =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
 linux-fsdevel <linux-fsdevel@vger.kernel.org>,
 Alexander Viro <viro@zeniv.linux.org.uk>,
 Christian Brauner <brauner@kernel.org>,
 Jan Kara <jack@suse.cz>,
 Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
 linux-riscv@lists.infradead.org,
 Theodore Ts'o <tytso@mit.edu>,
 Ext4 Developers List <linux-ext4@vger.kernel.org>,
 Conor Dooley <conor@kernel.org>
To: Nam Cao <namcao@linutronix.de>
References: <878r1ibpdn.fsf@all.your.base.are.belong.to.us>
 <20240413164318.7260c5ef@namcao>


--Apple-Mail=_7C671C3E-4468-48AE-8BCA-0F78F2D6442B
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

On Apr 13, 2024, at 8:43 AM, Nam Cao <namcao@linutronix.de> wrote:
>=20
> On 2024-04-12 Bj=C3=B6rn T=C3=B6pel wrote:
>>=20
>> What I see in ext4_search_dir() is that search_buf is 0xfffff000, and =
at
>> some point the address wraps to zero, and boom. I doubt that =
0xfffff000
>> is a sane address.
>=20
> I have zero knowledge about file system, but I think it's an integer
> overflow problem. The calculation of "dlimit" overflow and dlimit =
wraps
> around, this leads to wrong comparison later on.
>=20
> I guess that explains why your bisect and Conor's bisect results are
> strange: the bug has been here for quite some time, but it only =
appears
> when "dlimit" happens to overflow.
>=20
> It can be fixed by re-arrange the comparisons a bit. Can you give the
> below patch a try?
>=20
> Best regards,
> Nam
>=20
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 05b647e6bc19..71b88b33b676 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1532,15 +1532,13 @@ int ext4_search_dir(struct buffer_head *bh, =
char *search_buf, int buf_size,
> 		    unsigned int offset, struct ext4_dir_entry_2 =
**res_dir)
> {
> 	struct ext4_dir_entry_2 * de;
> -	char * dlimit;
> 	int de_len;
>=20
> 	de =3D (struct ext4_dir_entry_2 *)search_buf;
> -	dlimit =3D search_buf + buf_size;
> -	while ((char *) de < dlimit - EXT4_BASE_DIR_LEN) {
> +	while ((char *) de - search_buf < buf_size - EXT4_BASE_DIR_LEN) =
{
> 		/* this code is executed quadratically often */
> 		/* do minimal checking `by hand' */
> -		if (de->name + de->name_len <=3D dlimit &&
> +		if (de->name + de->name_len - search_buf <=3D buf_size =
&&
> 		    ext4_match(dir, fname, de)) {
> 			/* found a match - just to be sure, do
> 			 * a full check */

This looks like a straight-forward mathematical substitution of "dlimit"
with "search_buf + buf_size" and rearranging of the terms to make the
while loop offset "zero based" rather than "address based" and would
avoid overflow if "search_buf" was within one 4kB block of overflow:

   dlimit =3D search_buf + buf_size =3D 0xfffff000 + 0x1000 =3D =
0x00000000

If (char *)de is signed then this loop would run for a long time.

It doesn't look like it would have any issues with *underflow* (since
"de =3D=3D search_buf" at the start and is only incremented, and there =
is
no valid filesystem where "buf_size < EXT4_BASE_DIR_LEN".

As such, I think this change would likely address the issue.

As to whether the 0xfffff000 address itself is valid for riscv32 is
outside my realm, but given that RAM is cheap it doesn't seem unlikely
to have 4GB+ of RAM and want to use it all.  The riscv32 might consider
reserving this page address from allocation to avoid similar issues in
other parts of the code, as is done with the NULL/0 page address.

If you submit this as a proper patch you could add my:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Cheers, Andreas


--Apple-Mail=_7C671C3E-4468-48AE-8BCA-0F78F2D6442B
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+AFAmYbNVsACgkQcqXauRfM
H+AK5Q//QbC+ePTcnzq7IXWpTH8Xglce/mu5s1AdjdqqeMB2rmYxAKkK5a4ngScC
AH4bFT8nPG34bNT95oZu0R0BOrsVj+K40JulHjHaOePH+qlRAlq4RvHFFRJ4BY4I
P+Ec5I587w72rrWaPTyBfyq/O5ylBH4V8sXJBXJvuCDbgOJRBxQG35f8plmWOPxn
vOX86TRauoBf30k2vg2f/U/zz0FhzAdrFB8R+/KVHcfj6Ch5HzDbXGF8vQhl4t1P
xlgJYOJrVPdeTPgB3b41Y3rFwFNI/ZLSPBkIBCPi7ytY57XxyB/ddQHg7q45AkRm
zxuug2hE16TvA7aNcaMqlEElmrIrU6wnuQ8WGOPxEPZwfklDJbmPTHkBI2dN6KBi
xxMaxPNB6xVWEsDtwXSTgTROfxLLElNWyXLKMRKMNwwdB+FrsOMRmBhOeKz0Kkef
ScAswy7giDHK3hs0cFuXdCxAHB7f9OoxrC5SITPwbvMqkmOMvhwhF6BjdlF8qpt9
XPciHAtUa5H1hf5KZ1EuwS/wdyoyjPf97VprjZSfGhupS4DO+uVdR5cMl0LMm45U
Ba1lpxXOLUDHAG7NI/i6R9WmLInKPkENBXj69PeRTi8HsT6fidACJ6iVPqoy4te4
KKTy+EV0NI87Z6Exu5nqamdHpsoP+NAeZWAtH57AcSIDm0AWPcE=
=gGOA
-----END PGP SIGNATURE-----

--Apple-Mail=_7C671C3E-4468-48AE-8BCA-0F78F2D6442B--