Re: [PATCH v2 RESEND 1/2] xfs: reject out-of-range attribute value lengths in xfs_attr_copy_value

[Darrick J. Wong]

On Wed, Sep 23, 2026 at 08:58:22AM -0300, Aldo Ariel Panzardo wrote:
> xfs_attr_copy_value() takes the value length as a signed int and, for a
> remote xattr, is handed args->rmtvaluelen.  That field is filled from the
> on-disk __be32 xfs_attr_leaf_name_remote.valuelen in
> xfs_attr3_leaf_getvalue(), so a crafted length such as 0x80000000 is
> stored into the signed rmtvaluelen as a negative number.
> 
> The "buffer too small" guard in xfs_attr_copy_value() is a signed
> comparison:
> 
> 	if (args->valuelen < valuelen)
> 		return -ERANGE;
> 
> A negative valuelen therefore compares as smaller than the caller's
> buffer size, skips the -ERANGE path, and is then used as a copy length,
> leading to an out-of-bounds copy of a full remote block into a small
> getxattr(2) buffer on a mounted crafted image.
> 
> Reject a value length that is negative or larger than the maximum xattr
> size before it is used, so a bogus on-disk length can no longer slip
> through the value copier.
> 
> Fixes: 9df243a1a9e6 ("xfs: consolidate attribute value copying")
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@xxxxxxxxx>

I wonder if we /really/ need this check now that the leaf verifier is
getting fixed, but I do see the value in making sure that crazy sizes
don't get passed to kvmalloc later on, so:

Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>

--D

> ---
> v2: new patch (see the 0/2 cover letter).  Fixes the signed value-length
>     check in the consumer, xfs_attr_copy_value(), which is the root
>     cause Darrick pointed at in his review of the v1 verifier patch.
> 
>  fs/xfs/libxfs/xfs_attr_leaf.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 86c5c09a5db4..d0f7753659c9 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -628,6 +628,17 @@ xfs_attr_copy_value(
>  	unsigned char		*value,
>  	int			valuelen)
>  {
> +	/*
> +	 * A value length that is negative or larger than the maximum xattr
> +	 * size is on-disk corruption.  The remote value length is an on-disk
> +	 * __be32 stored into the signed args->rmtvaluelen, so a crafted value
> +	 * such as 0x80000000 becomes negative and would slip past the
> +	 * "args->valuelen < valuelen" check below and be used as a copy
> +	 * length.  Reject it before that can happen.
> +	 */
> +	if (valuelen < 0 || valuelen > XFS_XATTR_SIZE_MAX)
> +		return -EFSCORRUPTED;
> +
>  	/*
>  	 * Parent pointer lookups require the caller to specify the name and
>  	 * value, so don't copy anything.
> -- 
> 2.53.0
> 
>