linux-hardening - Re: [PATCH] libxfs: Redefine 1-element arrays as flexible arrays

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZK6mC1npjONMoGMD@infradead.org>
Date:   Wed, 12 Jul 2023 06:09:31 -0700
From:   Christoph Hellwig <hch@...radead.org>
To:     "Darrick J. Wong" <djwong@...nel.org>
Cc:     Kees Cook <keescook@...omium.org>,
        Carlos Maiolino <cem@...nel.org>,
        Dave Chinner <david@...morbit.com>,
        Jeff Layton <jlayton@...nel.org>,
        Eric Biggers <ebiggers@...nel.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        linux-xfs@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] libxfs: Redefine 1-element arrays as flexible arrays

On Tue, Jul 11, 2023 at 10:37:38PM -0700, Darrick J. Wong wrote:
> Here's my version, where I go for a straight a[1] -> a[] conversion and
> let downstream attrlist ioctl users eat their lumps.  What do you think
> of the excess-documentation approach?

I think this is roughtly the right thing, with one big caveat:

> +	/* In Linux 6.5 this flex array was changed from list[1] to list[]. */

For all the format headers there's no need for these comments.  We've
done this for various other structures that had the old one size arrays
and never bothered.

> diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
> index 9c60ebb3..8927ecb2 100644
> --- a/libxfs/xfs_fs.h
> +++ b/libxfs/xfs_fs.h
> @@ -588,16 +588,19 @@ typedef struct xfs_attrlist_cursor {
>   *
>   * NOTE: struct xfs_attrlist must match struct attrlist defined in libattr, and
>   * struct xfs_attrlist_ent must match struct attrlist_ent defined in libattr.
> + *
> + * WARNING: In Linux 6.5, al_offset and a_name were changed from array[1] to
> + * array[].  Anyone using sizeof is (already) broken!
>   */
>  struct xfs_attrlist {
>  	__s32	al_count;	/* number of entries in attrlist */
>  	__s32	al_more;	/* T/F: more attrs (do call again) */
> -	__s32	al_offset[1];	/* byte offsets of attrs [var-sized] */
> +	__s32	al_offset[];	/* byte offsets of attrs [var-sized] */
>  };
>  
>  struct xfs_attrlist_ent {	/* data from attr_list() */
>  	__u32	a_valuelen;	/* number bytes in value of attr */
> -	char	a_name[1];	/* attr name (NULL terminated) */
> +	char	a_name[];	/* attr name (NULL terminated) */
>  };

Now these are currently exposed in a xfsprogs headeer and IFF someone
is using them it would break them in nasty ways.  That being said,
xfsprogs itself doesn't use them as it relies on identically layed
out but differntly named structures from libattr.

So I think we should just move these two out of xfs_fs.h, because
while they document a UAPI, they aren't actually used by userspace.

With that the need for any caution goes away and we can just fix the
definition without any caveats.