Message-ID: <20230712053738.GD108251@frogsfrogsfrogs>
Date:   Tue, 11 Jul 2023 22:37:38 -0700
From:   "Darrick J. Wong" <djwong@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     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 03:20:28PM -0700, Kees Cook wrote:
> To allow for code bases that include libxfs (e.g. the Linux kernel) and
> build with strict flexible array handling (-fstrict-flex-arrays=3),
> FORTIFY_SOURCE, and/or UBSAN bounds checking, redefine the remaining
> 1-element trailing arrays as true flexible arrays, but without changing
> their structure sizes. This is done via a union to retain a single element
> (named "legacy_padding"). As not all distro headers may yet have the
> UAPI stddef.h __DECLARE_FLEX_ARRAY macro, include it explicitly in
> platform_defs.h.in.
> 
> Addresses the warnings being seen under Linux:
> https://lore.kernel.org/all/20230710170243.GF11456@frogsfrogsfrogs
> 
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
>  include/platform_defs.h.in | 18 ++++++++++++++++++
>  libxfs/xfs_da_format.h     | 38 ++++++++++++++++++++++++++++----------
>  libxfs/xfs_fs.h            | 12 ++++++++++--
>  3 files changed, 56 insertions(+), 12 deletions(-)
> 
> diff --git a/include/platform_defs.h.in b/include/platform_defs.h.in
> index 315ad77cfb78..a55965ce050d 100644
> --- a/include/platform_defs.h.in
> +++ b/include/platform_defs.h.in
> @@ -134,6 +134,24 @@ static inline size_t __ab_c_size(size_t a, size_t b, size_t c)
>  #    define fallthrough                    do {} while (0)  /* fallthrough */
>  #endif
>  
> +#ifndef __DECLARE_FLEX_ARRAY
> +/**
> + * __DECLARE_FLEX_ARRAY() - Declare a flexible array usable in a union
> + *
> + * @type: The type of each flexible array element
> + * @name: The name of the flexible array member
> + *
> + * In order to have a flexible array member in a union or alone in a
> + * struct, it needs to be wrapped in an anonymous struct with at least 1
> + * named member, but that member can be empty.
> + */
> +#define __DECLARE_FLEX_ARRAY(type, name)	\
> +	struct { \
> +		struct { } __empty_ ## name; \
> +		type name[]; \
> +	}
> +#endif

Hmm.  This is tempting, since the struct sizes stay the same, which
avoids all the potential sizeof breakage problems and whatnot.  I think
the downside of doing it this way is that xfs_fs.h ships in xfslibs-dev
but platform_defs.h does not, which means that random user programs can
pick up a *user* of __DECLARE_FLEX_ARRAY from /usr/include/xfs/xfs_fs.h.

I guess this could be a problem for anyone who might (a) compile against
xfs_fs.h from a newer xfslibs-dev package on a system with (b) old
kernel headers that don't define __DECLARE_FLEX_ARRAY.

More on this at the bottom.

> +
>  /* Only needed for the kernel. */
>  #define __init
>  
> diff --git a/libxfs/xfs_da_format.h b/libxfs/xfs_da_format.h
> index 25e2841084e1..4af92f16d5cd 100644
> --- a/libxfs/xfs_da_format.h
> +++ b/libxfs/xfs_da_format.h
> @@ -580,18 +580,23 @@ xfs_dir2_block_leaf_p(struct xfs_dir2_block_tail *btp)
>  /*
>   * Entries are packed toward the top as tight as possible.
>   */
> +struct xfs_attr_sf_entry {
> +	uint8_t namelen;	/* actual length of name (no NULL) */
> +	uint8_t valuelen;	/* actual length of value (no NULL) */
> +	uint8_t flags;		/* flags bits (see xfs_attr_leaf.h) */
> +	uint8_t nameval[];	/* name & value bytes concatenated */
> +};

(Yeah, these ought to be broken out...)

> +
>  struct xfs_attr_shortform {
>  	struct xfs_attr_sf_hdr {	/* constant-structure header block */
>  		__be16	totsize;	/* total bytes in shortform list */
>  		__u8	count;	/* count of active entries */
>  		__u8	padding;
>  	} hdr;
> -	struct xfs_attr_sf_entry {
> -		uint8_t namelen;	/* actual length of name (no NULL) */
> -		uint8_t valuelen;	/* actual length of value (no NULL) */
> -		uint8_t flags;	/* flags bits (see xfs_attr_leaf.h) */
> -		uint8_t nameval[];	/* name & value bytes concatenated */
> -	} list[1];			/* variable sized array */
> +	union {
> +		struct xfs_attr_sf_entry legacy_padding;
> +		__DECLARE_FLEX_ARRAY(struct xfs_attr_sf_entry, list);
> +	};
>  };
>  
>  typedef struct xfs_attr_leaf_map {	/* RLE map of free bytes */
> @@ -620,19 +625,29 @@ typedef struct xfs_attr_leaf_entry {	/* sorted on key, not name */
>  typedef struct xfs_attr_leaf_name_local {
>  	__be16	valuelen;		/* number of bytes in value */
>  	__u8	namelen;		/* length of name bytes */
> -	__u8	nameval[1];		/* name/value bytes */
> +	union {
> +		__u8	legacy_padding;
> +		__DECLARE_FLEX_ARRAY(__u8, nameval);	/* name/value bytes */
> +	};
>  } xfs_attr_leaf_name_local_t;
>  
>  typedef struct xfs_attr_leaf_name_remote {
>  	__be32	valueblk;		/* block number of value bytes */
>  	__be32	valuelen;		/* number of bytes in value */
>  	__u8	namelen;		/* length of name bytes */
> -	__u8	name[1];		/* name bytes */
> +	union {
> +		__u8	legacy_padding;
> +		__DECLARE_FLEX_ARRAY(__u8, name);	/* name bytes */
> +	};
>  } xfs_attr_leaf_name_remote_t;
>  
>  typedef struct xfs_attr_leafblock {
>  	xfs_attr_leaf_hdr_t	hdr;	/* constant-structure header block */
> -	xfs_attr_leaf_entry_t	entries[1];	/* sorted on key, not name */
> +	union {
> +		xfs_attr_leaf_entry_t	legacy_padding;
> +		/* sorted on key, not name */
> +		__DECLARE_FLEX_ARRAY(xfs_attr_leaf_entry_t, entries);
> +	};
>  	/*
>  	 * The rest of the block contains the following structures after the
>  	 * leaf entries, growing from the bottom up. The variables are never
> @@ -664,7 +679,10 @@ struct xfs_attr3_leaf_hdr {
>  
>  struct xfs_attr3_leafblock {
>  	struct xfs_attr3_leaf_hdr	hdr;
> -	struct xfs_attr_leaf_entry	entries[1];
> +	union {
> +		struct xfs_attr_leaf_entry	legacy_padding;
> +		__DECLARE_FLEX_ARRAY(struct xfs_attr_leaf_entry, entries);
> +	};
>  
>  	/*
>  	 * The rest of the block contains the following structures after the
> diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
> index 1cfd5bc6520a..d6ec66ef0194 100644
> --- a/libxfs/xfs_fs.h
> +++ b/libxfs/xfs_fs.h
> @@ -590,12 +590,20 @@ typedef struct xfs_attrlist_cursor {
>  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] */
> +	union {
> +		__s32	legacy_padding;
> +		/* byte offsets of attrs [var-sized] */
> +		__DECLARE_FLEX_ARRAY(__s32, al_offset);
> +	};
>  };
>  
>  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) */
> +	union {
> +		char	legacy_padding;
> +		/* attr name (NULL terminated) */
> +		__DECLARE_FLEX_ARRAY(char, a_name);
> +	};
>  };
>  
>  typedef struct xfs_fsop_attrlist_handlereq {
> -- 
> 2.34.1

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?

--D

diff --git a/db/metadump.c b/db/metadump.c
index d9a616a9..3545124f 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -1475,7 +1475,7 @@ process_attr_block(
 			nlen = local->namelen;
 			vlen = be16_to_cpu(local->valuelen);
 			zlen = xfs_attr_leaf_entsize_local(nlen, vlen) -
-				(sizeof(xfs_attr_leaf_name_local_t) - 1 +
+				(offsetof(struct xfs_attr_leaf_name_local, nameval) +
 				 nlen + vlen);
 			if (zero_stale_data)
 				memset(&local->nameval[nlen + vlen], 0, zlen);
@@ -1497,7 +1497,7 @@ process_attr_block(
 			/* zero from end of name[] to next name start */
 			nlen = remote->namelen;
 			zlen = xfs_attr_leaf_entsize_remote(nlen) -
-				(sizeof(xfs_attr_leaf_name_remote_t) - 1 +
+				(offsetof(struct xfs_attr_leaf_name_remote, name) +
 				 nlen);
 			if (zero_stale_data)
 				memset(&remote->name[nlen], 0, zlen);
diff --git a/libxfs/xfs_da_format.h b/libxfs/xfs_da_format.h
index 25e28410..17a6d8be 100644
--- a/libxfs/xfs_da_format.h
+++ b/libxfs/xfs_da_format.h
@@ -586,12 +586,14 @@ struct xfs_attr_shortform {
 		__u8	count;	/* count of active entries */
 		__u8	padding;
 	} hdr;
+
+	/* In Linux 6.5 this flex array was changed from list[1] to list[]. */
 	struct xfs_attr_sf_entry {
 		uint8_t namelen;	/* actual length of name (no NULL) */
 		uint8_t valuelen;	/* actual length of value (no NULL) */
 		uint8_t flags;	/* flags bits (see xfs_attr_leaf.h) */
 		uint8_t nameval[];	/* name & value bytes concatenated */
-	} list[1];			/* variable sized array */
+	} list[];			/* variable sized array */
 };
 
 typedef struct xfs_attr_leaf_map {	/* RLE map of free bytes */
@@ -620,19 +622,29 @@ typedef struct xfs_attr_leaf_entry {	/* sorted on key, not name */
 typedef struct xfs_attr_leaf_name_local {
 	__be16	valuelen;		/* number of bytes in value */
 	__u8	namelen;		/* length of name bytes */
-	__u8	nameval[1];		/* name/value bytes */
+	/*
+	 * In Linux 6.5 this flex array was converted from nameval[1] to
+	 * nameval[].  Be very careful here about extra padding at the end;
+	 * see xfs_attr_leaf_entsize_local() for details.
+	 */
+	__u8	nameval[];		/* name/value bytes */
 } xfs_attr_leaf_name_local_t;
 
 typedef struct xfs_attr_leaf_name_remote {
 	__be32	valueblk;		/* block number of value bytes */
 	__be32	valuelen;		/* number of bytes in value */
 	__u8	namelen;		/* length of name bytes */
-	__u8	name[1];		/* name bytes */
+	/*
+	 * In Linux 6.5 this flex array was converted from name[1] to name[].
+	 * Be very careful here about extra padding at the end; see
+	 * xfs_attr_leaf_entsize_remote() for details.
+	 */
+	__u8	name[];			/* name bytes */
 } xfs_attr_leaf_name_remote_t;
 
 typedef struct xfs_attr_leafblock {
 	xfs_attr_leaf_hdr_t	hdr;	/* constant-structure header block */
-	xfs_attr_leaf_entry_t	entries[1];	/* sorted on key, not name */
+	xfs_attr_leaf_entry_t	entries[];	/* sorted on key, not name */
 	/*
 	 * The rest of the block contains the following structures after the
 	 * leaf entries, growing from the bottom up. The variables are never
@@ -641,6 +653,9 @@ typedef struct xfs_attr_leafblock {
 	 *
 	 * xfs_attr_leaf_name_local_t namelist;
 	 * xfs_attr_leaf_name_remote_t valuelist;
+	 *
+	 * In Linux 6.5 this flex array was converted from entries[1] to
+	 * entries[].
 	 */
 } xfs_attr_leafblock_t;
 
@@ -664,7 +679,7 @@ struct xfs_attr3_leaf_hdr {
 
 struct xfs_attr3_leafblock {
 	struct xfs_attr3_leaf_hdr	hdr;
-	struct xfs_attr_leaf_entry	entries[1];
+	struct xfs_attr_leaf_entry	entries[];
 
 	/*
 	 * The rest of the block contains the following structures after the
@@ -673,6 +688,9 @@ struct xfs_attr3_leafblock {
 	 *
 	 * struct xfs_attr_leaf_name_local
 	 * struct xfs_attr_leaf_name_remote
+	 *
+	 * In Linux 6.5 this flex array was converted from entries[1] to
+	 * entries[].
 	 */
 };
 
@@ -747,14 +765,61 @@ xfs_attr3_leaf_name_local(xfs_attr_leafblock_t *leafp, int idx)
  */
 static inline int xfs_attr_leaf_entsize_remote(int nlen)
 {
-	return round_up(sizeof(struct xfs_attr_leaf_name_remote) - 1 +
-			nlen, XFS_ATTR_LEAF_NAME_ALIGN);
+	/*
+	 * Prior to Linux 6.5, struct xfs_attr_leaf_name_remote ended with
+	 * name[1], which was used as a flexarray.  The layout of this struct
+	 * is 9 bytes of fixed-length fields followed by a __u8 flex array at
+	 * offset 9.
+	 *
+	 * On most architectures, struct xfs_attr_leaf_name_remote had two
+	 * bytes of implicit padding at the end of the struct to make the
+	 * struct length 12.  After converting name[1] to name[], there are
+	 * three implicit padding bytes and the struct size remains 12.
+	 * However, there are compiler configurations that do not add implicit
+	 * padding at all (m68k) and have been broken for years.
+	 *
+	 * This entsize computation historically added (the xattr name length)
+	 * to (the padded struct length - 1) and rounded that sum up to the
+	 * nearest multiple of 4 (NAME_ALIGN).  IOWs, round_up(11 + nlen, 4).
+	 * This is encoded in the ondisk format, so we cannot change this.
+	 *
+	 * Compute the entsize from offsetof of the flexarray and manually
+	 * adding bytes for the implicit padding.
+	 */
+	const size_t remotesize =
+			offsetof(struct xfs_attr_leaf_name_remote, name) + 2;
+
+	return round_up(remotesize + nlen, XFS_ATTR_LEAF_NAME_ALIGN);
 }
 
 static inline int xfs_attr_leaf_entsize_local(int nlen, int vlen)
 {
-	return round_up(sizeof(struct xfs_attr_leaf_name_local) - 1 +
-			nlen + vlen, XFS_ATTR_LEAF_NAME_ALIGN);
+	/*
+	 * Prior to Linux 6.5, struct xfs_attr_leaf_name_local ended with
+	 * nameval[1], which was used as a flexarray.  The layout of this
+	 * struct is 3 bytes of fixed-length fields followed by a __u8 flex
+	 * array at offset 3.
+	 *
+	 * struct xfs_attr_leaf_name_local had zero bytes of implicit padding
+	 * at the end of the struct to make the struct length 4.  On most
+	 * architectures, after converting nameval[1] to nameval[], there is
+	 * one implicit padding byte and the struct size remains 4.  However,
+	 * there are compiler configurations that do not add implicit padding
+	 * at all (m68k) and would break.
+	 *
+	 * This entsize computation historically added (the xattr name and
+	 * value length) to (the padded struct length - 1) and rounded that sum
+	 * up to the nearest multiple of 4 (NAME_ALIGN).  IOWs, the formula is
+	 * round_up(3 + nlen + vlen, 4).  This is encoded in the ondisk format,
+	 * so we cannot change this.
+	 *
+	 * Compute the entsize from offsetof of the flexarray and manually
+	 * adding bytes for the implicit padding.
+	 */
+	const size_t localsize =
+			offsetof(struct xfs_attr_leaf_name_local, nameval);
+
+	return round_up(localsize + nlen + vlen, XFS_ATTR_LEAF_NAME_ALIGN);
 }
 
 static inline int xfs_attr_leaf_entsize_local_max(int bsize)
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) */
 };
 
 typedef struct xfs_fsop_attrlist_handlereq {

Powered by blists - more mailing lists