Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Andreas Dilger <adilger@dilger.ca>
Message-Id: <2FEE2257-0486-4C62-8482-731AA796AB2E@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_C3DF419D-EEF3-4EE1-B844-992BDF0D6B18";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH] ext4: improve xattr consistency checking and error
 reporting
Date:   Wed, 14 Dec 2022 15:00:34 -0700
In-Reply-To: <20221214200818.870087-1-tytso@mit.edu>
Cc:     Ext4 Developers List <linux-ext4@vger.kernel.org>,
        yebin@huaweicloud.com
To:     Theodore Ts'o <tytso@mit.edu>
References: <20221214200818.870087-1-tytso@mit.edu>
Precedence: bulk


--Apple-Mail=_C3DF419D-EEF3-4EE1-B844-992BDF0D6B18
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Dec 14, 2022, at 1:08 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>=20
> Refactor the in-inode and xattr block consistency checking, and report
> more fine-grained reports of the consistency problems.  Also add more
> consistency checks for ea_inode number.
>=20
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

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

A minor nit below, but no reason not to hold up the patch.

> static int
> -ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
> -			 void *value_start)
> +check_xattrs(struct inode *inode, struct buffer_head *bh,
> +	     struct ext4_xattr_entry *entry, void *end, void =
*value_start,
> +	     const char *function, unsigned int line)
> {
> 	struct ext4_xattr_entry *e =3D entry;

"e" isn't a great variable name (though it predates the patch), maybe =
"last"
or "last_entry" is better since that is what it is used for later?

> +	int err =3D -EFSCORRUPTED;
> +	char *err_str;
> +
> +	if (bh) {
> +		if (BHDR(bh)->h_magic !=3D cpu_to_le32(EXT4_XATTR_MAGIC) =
||
> +		    BHDR(bh)->h_blocks !=3D cpu_to_le32(1)) {
> +			err_str =3D "invalid header";
> +			goto errout;
> +		}
> +		if (buffer_verified(bh))
> +			return 0;
> +		if (!ext4_xattr_block_csum_verify(inode, bh)) {
> +			err =3D -EFSBADCRC;
> +			err_str =3D "invalid checksum";
> +			goto errout;
> +		}
> +	} else {
> +		struct ext4_xattr_ibody_header *header =3D value_start;
> +
> +		header -=3D 1;
> +		if (end - (void *)header < sizeof(*header) + =
sizeof(u32)) {
> +			err_str =3D "in-inode xattr block too small";
> +			goto errout;
> +		}
> +		if (header->h_magic !=3D cpu_to_le32(EXT4_XATTR_MAGIC)) =
{
> +			err_str =3D "bad magic number in in-inode =
xattr";
> +			goto errout;
> +		}
> +	}
>=20
> 	/* Find the end of the names list */
> 	while (!IS_LAST_ENTRY(e)) {
> 		struct ext4_xattr_entry *next =3D EXT4_XATTR_NEXT(e);
> -		if ((void *)next >=3D end)
> -			return -EFSCORRUPTED;
> -		if (strnlen(e->e_name, e->e_name_len) !=3D =
e->e_name_len)
> -			return -EFSCORRUPTED;
> +		if ((void *)next >=3D end) {
> +			err_str =3D "e_name out of bounds";
> +			goto errout;
> +		}
> +		if (strnlen(e->e_name, e->e_name_len) !=3D =
e->e_name_len) {
> +			err_str =3D "bad e_name length";
> +			goto errout;
> +		}
> 		e =3D next;
> 	}
>=20
> 	/* Check the values */
> 	while (!IS_LAST_ENTRY(entry)) {
> 		u32 size =3D le32_to_cpu(entry->e_value_size);
> +		unsigned long ea_ino =3D =
le32_to_cpu(entry->e_value_inum);
>=20
> -		if (size > EXT4_XATTR_SIZE_MAX)
> -			return -EFSCORRUPTED;
> +		if (!ext4_has_feature_ea_inode(inode->i_sb) && ea_ino) {
> +			err_str =3D "ea_inode specified without ea_inode =
feature enabled";
> +			goto errout;
> +		}
> +		if (ea_ino && ((ea_ino =3D=3D EXT4_ROOT_INO) ||
> +			       !ext4_valid_inum(inode->i_sb, ea_ino))) {
> +			err_str =3D "invalid ea_ino";
> +			goto errout;
> +		}
> +		if (size > EXT4_XATTR_SIZE_MAX) {
> +			err_str =3D "e_value size too large";
> +			goto errout;
> +		}
>=20
> 		if (size !=3D 0 && entry->e_value_inum =3D=3D 0) {
> 			u16 offs =3D le16_to_cpu(entry->e_value_offs);
> @@ -214,66 +260,54 @@ ext4_xattr_check_entries(struct ext4_xattr_entry =
*entry, void *end,
> 			 * the padded and unpadded sizes, since the size =
may
> 			 * overflow to 0 when adding padding.
> 			 */
> -			if (offs > end - value_start)
> -				return -EFSCORRUPTED;
> +			if (offs > end - value_start) {
> +				err_str =3D "e_value out of bounds";
> +				goto errout;
> +			}
> 			value =3D value_start + offs;
> 			if (value < (void *)e + sizeof(u32) ||
> 			    size > end - value ||
> -			    EXT4_XATTR_SIZE(size) > end - value)
> -				return -EFSCORRUPTED;
> +			    EXT4_XATTR_SIZE(size) > end - value) {
> +				err_str =3D "overlapping e_value ";
> +				goto errout;
> +			}
> 		}
> 		entry =3D EXT4_XATTR_NEXT(entry);
> 	}
> -
> +	if (bh)
> +		set_buffer_verified(bh);
> 	return 0;
> +
> +errout:
> +	if (bh)
> +		__ext4_error_inode(inode, function, line, 0, -err,
> +				   "corrupted xattr block %llu: %s",
> +				   (unsigned long long) bh->b_blocknr,
> +				   err_str);
> +	else
> +		__ext4_error_inode(inode, function, line, 0, -err,
> +				   "corrupted in-inode xattr: %s", =
err_str);
> +	return err;
> }
>=20
> static inline int
> __ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh,
> 			 const char *function, unsigned int line)
> {
> -	int error =3D -EFSCORRUPTED;
> -
> -	if (BHDR(bh)->h_magic !=3D cpu_to_le32(EXT4_XATTR_MAGIC) ||
> -	    BHDR(bh)->h_blocks !=3D cpu_to_le32(1))
> -		goto errout;
> -	if (buffer_verified(bh))
> -		return 0;
> -
> -	error =3D -EFSBADCRC;
> -	if (!ext4_xattr_block_csum_verify(inode, bh))
> -		goto errout;
> -	error =3D ext4_xattr_check_entries(BFIRST(bh), bh->b_data + =
bh->b_size,
> -					 bh->b_data);
> -errout:
> -	if (error)
> -		__ext4_error_inode(inode, function, line, 0, -error,
> -				   "corrupted xattr block %llu",
> -				   (unsigned long long) bh->b_blocknr);
> -	else
> -		set_buffer_verified(bh);
> -	return error;
> +	return check_xattrs(inode, bh, BFIRST(bh), bh->b_data + =
bh->b_size,
> +			    bh->b_data, function, line);
> }
>=20
> #define ext4_xattr_check_block(inode, bh) \
> 	__ext4_xattr_check_block((inode), (bh),  __func__, __LINE__)
>=20
>=20
> -static int
> +static inline int
> __xattr_check_inode(struct inode *inode, struct =
ext4_xattr_ibody_header *header,
> 			 void *end, const char *function, unsigned int =
line)
> {
> -	int error =3D -EFSCORRUPTED;
> -
> -	if (end - (void *)header < sizeof(*header) + sizeof(u32) ||
> -	    (header->h_magic !=3D cpu_to_le32(EXT4_XATTR_MAGIC)))
> -		goto errout;
> -	error =3D ext4_xattr_check_entries(IFIRST(header), end, =
IFIRST(header));
> -errout:
> -	if (error)
> -		__ext4_error_inode(inode, function, line, 0, -error,
> -				   "corrupted in-inode xattr");
> -	return error;
> +	return check_xattrs(inode, NULL, IFIRST(header), end, =
IFIRST(header),
> +			    function, line);
> }
>=20
> #define xattr_check_inode(inode, header, end) \
> --
> 2.31.0
>=20


Cheers, Andreas


--Apple-Mail=_C3DF419D-EEF3-4EE1-B844-992BDF0D6B18
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+AFAmOaR4IACgkQcqXauRfM
H+AarQ/+LMhkNlIpR7HMTo8eKEJJ9C7wxVhJ0UL2HdPivrXUg0DDfu177HL57P89
6tCd8AcTWRnvHp9Qpq0+r0aSbQvXYH+ATMi919fYW6wdIXDRIF4G7Jv3AKP4RAzP
4pSq5nvipa6rrW9JuJLpdDX39wKZaAYTCtwf9442RilaO9rEx/IdypLkIkXyRPuC
qHiHRtfFEBDFUDFCGdLgS+sIFzOpcAvoBkbodzo8H8y1cksqIY1DaYHz2kf5sTOA
RNxfyMJKIgj8AnpDcNSl9lb9CloFux4DuqqYEGOOGC+RQ5BDg8+G0bfCICzp43jX
rM5kXL/UvZCoB0bc4L4KGH+/RQMOEO3zVeEZhlmP597av4Bv+8PAl4zSw69XFQwO
i8kcsCke242MDA72KGy5RSJsAkKA0/dSYoMfxkJ6n07B4tqOxk6Goylw4Me+HPH0
QndrCAOY0tfAznejHCbg2kpJmwuOjWgP/L1DcNc2KmvW/+nTE7dXtt+P3boDlbsE
nkWuTq2AFOn8zMvYnteJK/xp2j7jRiUt4EfBN2EAlq5Khj8sQeAP1puzB2OAOiW7
gfGhp2RcDAhKfxiefpCeWBgeMzQwfE9IFo+yo8AVnq4YXXCsSlcXPYNoEbyjns18
eBnZ3CvyJvUF10zRUHfVWJ4G7j5XGF2IXrZbSsm5zFSCmEFprY0=
=DsKs
-----END PGP SIGNATURE-----

--Apple-Mail=_C3DF419D-EEF3-4EE1-B844-992BDF0D6B18--