diff mbox series

[1/3] xfs: convert flex-array declarations in struct xfs_attrlist*

Message ID 168934591095.3368057.15849162788748534581.stgit@frogsfrogsfrogs (mailing list archive)
State Accepted, archived
Headers show
Series xfsprogs: ubsan fixes for 6.5-rc2 | expand

Commit Message

Darrick J. Wong July 14, 2023, 2:45 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Source kernel commit: d6da44c23ec357f132717301ea3319b8aa95274e

As of 6.5-rc1, UBSAN trips over the attrlist ioctl definitions using an
array length of 1 to pretend to be a flex array.  Kernel compilers have
to support unbounded array declarations, so let's correct this.  This
may cause friction with userspace header declarations, but suck is life.

================================================================================
UBSAN: array-index-out-of-bounds in fs/xfs/xfs_ioctl.c:345:18
index 1 is out of range for type '__s32 [1]'
Call Trace:
<TASK>
dump_stack_lvl+0x33/0x50
__ubsan_handle_out_of_bounds+0x9c/0xd0
xfs_ioc_attr_put_listent+0x413/0x420 [xfs 4a986a89a77bb77402ab8a87a37da369ef6a3f09]
xfs_attr_list_ilocked+0x170/0x850 [xfs 4a986a89a77bb77402ab8a87a37da369ef6a3f09]
xfs_attr_list+0xb7/0x120 [xfs 4a986a89a77bb77402ab8a87a37da369ef6a3f09]
xfs_ioc_attr_list+0x13b/0x2e0 [xfs 4a986a89a77bb77402ab8a87a37da369ef6a3f09]
xfs_attrlist_by_handle+0xab/0x120 [xfs 4a986a89a77bb77402ab8a87a37da369ef6a3f09]
xfs_file_ioctl+0x1ff/0x15e0 [xfs 4a986a89a77bb77402ab8a87a37da369ef6a3f09]
vfs_ioctl+0x1f/0x60

The kernel and xfsprogs code that uses these structures will not have
problems, but the long tail of external user programs might.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 libxfs/xfs_fs.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig July 14, 2023, 2:46 p.m. UTC | #1 
The change itself looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

But as mentioned in the last thread leaving these out in the UAPI
is a bit dangerous, and at the same time they really shouldn't
be in the uapi.  Do you want me to send an incremental patch for
that?
Darrick J. Wong July 14, 2023, 2:54 p.m. UTC | #2 
On Fri, Jul 14, 2023 at 04:46:38PM +0200, Christoph Hellwig wrote:
> The change itself looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But as mentioned in the last thread leaving these out in the UAPI
> is a bit dangerous, and at the same time they really shouldn't
> be in the uapi.  Do you want me to send an incremental patch for
> that?

Yes please. :)

FWIW I tried removing the attrlist ioctl structs, but I couldn't find
anywhere else in the kernel uapi headers that defines them so that the
ioctl code can actually format the buffer.

--D
Christoph Hellwig July 14, 2023, 2:56 p.m. UTC | #3 
On Fri, Jul 14, 2023 at 07:54:39AM -0700, Darrick J. Wong wrote:
> FWIW I tried removing the attrlist ioctl structs, but I couldn't find
> anywhere else in the kernel uapi headers that defines them so that the
> ioctl code can actually format the buffer.

We need to keep them somewhere.  But I'd rather not do that in xfs_fs.h
which through xfsprogs goes into /usr/include/.

I'd probably do it locally in xfs_ioctl.c with an extended version of
the comment about matching the libattr structures.
diff mbox series

Patch

diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
index 9c60ebb3..2cbf9ea3 100644
--- a/libxfs/xfs_fs.h
+++ b/libxfs/xfs_fs.h
@@ -592,12 +592,12 @@  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] */
+	__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 {