linux-hardening.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: Kees Cook <keescook@chromium.org>,
	Carlos Maiolino <cem@kernel.org>,
	Dave Chinner <david@fromorbit.com>,
	Jeff Layton <jlayton@kernel.org>,
	Eric Biggers <ebiggers@kernel.org>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	linux-xfs@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] libxfs: Redefine 1-element arrays as flexible arrays
Date: Wed, 12 Jul 2023 06:09:31 -0700	[thread overview]
Message-ID: <ZK6mC1npjONMoGMD@infradead.org> (raw)
In-Reply-To: <20230712053738.GD108251@frogsfrogsfrogs>

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.

  reply	other threads:[~2023-07-12 13:09 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-11 22:20 [PATCH] libxfs: Redefine 1-element arrays as flexible arrays Kees Cook
2023-07-12  5:37 ` Darrick J. Wong
2023-07-12 13:09   ` Christoph Hellwig [this message]
2023-07-13  5:44     ` Darrick J. Wong
2023-07-13  5:54       ` Christoph Hellwig
2023-07-12 18:45   ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZK6mC1npjONMoGMD@infradead.org \
    --to=hch@infradead.org \
    --cc=cem@kernel.org \
    --cc=david@fromorbit.com \
    --cc=djwong@kernel.org \
    --cc=ebiggers@kernel.org \
    --cc=gustavoars@kernel.org \
    --cc=jlayton@kernel.org \
    --cc=keescook@chromium.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).