| Message ID | 20230711222025.never.220-kees@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | libxfs: Redefine 1-element arrays as flexible arrays | expand |
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@chromium.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 {
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.
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? This is fine by me, and I think it's much cleaner. Thanks for running with this! -Kees
On Wed, Jul 12, 2023 at 06:09:31AM -0700, Christoph Hellwig wrote: > 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. <nod> Dave looked at an earlier version and wanted the comments for xfs_da_format.h to steer people at the entsize helpers. I more or less agree that it's overkill everywhere else though. > > 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. So I looked at the libattr headers for Ubuntu 22.04 and saw this: /* * List the names and sizes of the values of all the attributes of an object. * "Cursor" must be allocated and zeroed before the first call, it is used * to maintain context between system calls if all the attribute names won't * fit into the buffer on the first system call. * The return value is -1 on error (w/errno set appropriately), 0 on success. */ extern int attr_list(const char *__path, char *__buffer, const int __buffersize, int __flags, attrlist_cursor_t *__cursor) __attribute__ ((deprecated ("Use listxattr or llistxattr instead"))); extern int attr_listf(int __fd, char *__buffer, const int __buffersize, int __flags, attrlist_cursor_t *__cursor) __attribute__ ((deprecated ("Use flistxattr instead"))); I take that as a sign that they could drop all these xfs-specific APIs one day, which means we ought to keep them in xfs_fs.h. --D
On Wed, Jul 12, 2023 at 10:44:50PM -0700, Darrick J. Wong wrote: > /* > * List the names and sizes of the values of all the attributes of an object. > * "Cursor" must be allocated and zeroed before the first call, it is used > * to maintain context between system calls if all the attribute names won't > * fit into the buffer on the first system call. > * The return value is -1 on error (w/errno set appropriately), 0 on success. > */ > extern int attr_list(const char *__path, char *__buffer, const int __buffersize, > int __flags, attrlist_cursor_t *__cursor) > __attribute__ ((deprecated ("Use listxattr or llistxattr instead"))); > extern int attr_listf(int __fd, char *__buffer, const int __buffersize, > int __flags, attrlist_cursor_t *__cursor) > __attribute__ ((deprecated ("Use flistxattr instead"))); > > I take that as a sign that they could drop all these xfs-specific APIs > one day, which means we ought to keep them in xfs_fs.h. Well... These APIs you quoted are in fact internally mapped to the normal xattr syscalls by libattr, and have been since the Linux xattr syscalls were created. The only thing that actually uses the definitions in Linux is the magic attrlist by handle ioctl that exists only in XFS and which is exported through libhandle in xfsprogs. But the libhandle API is based on the attrlist_cursor from libattr and doesn't use the xfs_ kernel definitions. (that struct attrlist/attrlist_ent in libattr have the same 1-sized array problem, but fortunately we don't need to solve that here..)
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 + /* 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 */ +}; + 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 {
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@chromium.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(-)