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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 12 Jul 2023 22:44:50 -0700
From:   "Darrick J. Wong" <djwong@...nel.org>
To:     Christoph Hellwig <hch@...radead.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 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