Re: [PATCH v1] xfs_spaceman: add fsuuid command

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Nov 09, 2022 at 02:23:35PM -0800, Catherine Hoang wrote:
> Add support for the fsuuid command to retrieve the UUID of a mounted
> filesystem.
> 
> Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> ---
>  spaceman/Makefile |  4 +--
>  spaceman/fsuuid.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>  spaceman/init.c   |  1 +
>  spaceman/space.h  |  1 +
>  4 files changed, 67 insertions(+), 2 deletions(-)
>  create mode 100644 spaceman/fsuuid.c
> 
> diff --git a/spaceman/Makefile b/spaceman/Makefile
> index 1f048d54..901e4e6d 100644
> --- a/spaceman/Makefile
> +++ b/spaceman/Makefile
> @@ -7,10 +7,10 @@ include $(TOPDIR)/include/builddefs
>  
>  LTCOMMAND = xfs_spaceman
>  HFILES = init.h space.h
> -CFILES = info.c init.c file.c health.c prealloc.c trim.c
> +CFILES = info.c init.c file.c health.c prealloc.c trim.c fsuuid.c
>  LSRCFILES = xfs_info.sh
>  
> -LLDLIBS = $(LIBXCMD) $(LIBFROG)
> +LLDLIBS = $(LIBXCMD) $(LIBFROG) $(LIBUUID)
>  LTDEPENDENCIES = $(LIBXCMD) $(LIBFROG)
>  LLDFLAGS = -static
>  
> diff --git a/spaceman/fsuuid.c b/spaceman/fsuuid.c
> new file mode 100644
> index 00000000..be12c1ad
> --- /dev/null
> +++ b/spaceman/fsuuid.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Oracle.
> + * All Rights Reserved.
> + */
> +
> +#include "libxfs.h"
> +#include "libfrog/fsgeom.h"
> +#include "libfrog/paths.h"
> +#include "command.h"
> +#include "init.h"
> +#include "space.h"
> +#include <sys/ioctl.h>
> +
> +#ifndef FS_IOC_GETFSUUID
> +#define FS_IOC_GETFSUUID	_IOR('f', 44, struct fsuuid)
> +#define UUID_SIZE 16
> +struct fsuuid {
> +    __u32   fsu_len;
> +    __u32   fsu_flags;
> +    __u8    fsu_uuid[];

This is a flex array   ^^ which has no size.  struct fsuuid therefore
has a size of 8 bytes (i.e. enough to cover the two u32 fields) and no
more.  It's assumed that the caller will allocate the memory for
fsu_uuid...

> +};
> +#endif
> +
> +static cmdinfo_t fsuuid_cmd;
> +
> +static int
> +fsuuid_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	struct fsuuid	fsuuid;
> +	int		error;

...which makes this usage a problem, because we've not reserved any
space on the stack to hold the UUID.  The kernel will blindly assume
that there are fsuuid.fsu_len bytes after fsuuid and write to them,
which will clobber something on the stack.

If you're really unlucky, the C compiler will put the fsuuid right
before the call frame, which is how stack smashing attacks work.  It
might also lay out bp[] immediately afterwards, which will give you
weird results as the unparse function overwrites its source buffer.  The
C compiler controls the stack layout, which means this can go bad in
subtle ways.

Either way, gcc complains about this (albeit in an opaque manner)...

In file included from ../include/xfs.h:9,
                 from ../include/libxfs.h:15,
                 from fsuuid.c:7:
In function ‘platform_uuid_unparse’,
    inlined from ‘fsuuid_f’ at fsuuid.c:45:3:
../include/xfs/linux.h:100:9: error: ‘uuid_unparse’ reading 16 bytes from a region of size 0 [-Werror=stringop-overread]
  100 |         uuid_unparse(*uu, buffer);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~
../include/xfs/linux.h: In function ‘fsuuid_f’:
../include/xfs/linux.h:100:9: note: referencing argument 1 of type ‘const unsigned char *’
In file included from ../include/xfs/linux.h:13,
                 from ../include/xfs.h:9,
                 from ../include/libxfs.h:15,
                 from fsuuid.c:7:
/usr/include/uuid/uuid.h:107:13: note: in a call to function ‘uuid_unparse’
  107 | extern void uuid_unparse(const uuid_t uu, char *out);
      |             ^~~~~~~~~~~~
cc1: all warnings being treated as errors

...so please allocate the struct fsuuid object dynamically.

--D

> +	char		bp[40];
> +
> +	fsuuid.fsu_len = UUID_SIZE;
> +	fsuuid.fsu_flags = 0;
> +
> +	error = ioctl(file->xfd.fd, FS_IOC_GETFSUUID, &fsuuid);
> +
> +	if (error) {
> +		perror("fsuuid");
> +		exitcode = 1;
> +	} else {
> +		platform_uuid_unparse((uuid_t *)fsuuid.fsu_uuid, bp);
> +		printf("UUID = %s\n", bp);
> +	}
> +
> +	return 0;
> +}
> +
> +void
> +fsuuid_init(void)
> +{
> +	fsuuid_cmd.name = "fsuuid";
> +	fsuuid_cmd.cfunc = fsuuid_f;
> +	fsuuid_cmd.argmin = 0;
> +	fsuuid_cmd.argmax = 0;
> +	fsuuid_cmd.flags = CMD_FLAG_ONESHOT;
> +	fsuuid_cmd.oneline = _("get mounted filesystem UUID");
> +
> +	add_command(&fsuuid_cmd);
> +}
> diff --git a/spaceman/init.c b/spaceman/init.c
> index cf1ff3cb..efe1bf9b 100644
> --- a/spaceman/init.c
> +++ b/spaceman/init.c
> @@ -35,6 +35,7 @@ init_commands(void)
>  	trim_init();
>  	freesp_init();
>  	health_init();
> +	fsuuid_init();
>  }
>  
>  static int
> diff --git a/spaceman/space.h b/spaceman/space.h
> index 723209ed..dcbdca08 100644
> --- a/spaceman/space.h
> +++ b/spaceman/space.h
> @@ -33,5 +33,6 @@ extern void	freesp_init(void);
>  #endif
>  extern void	info_init(void);
>  extern void	health_init(void);
> +extern void	fsuuid_init(void);
>  
>  #endif /* XFS_SPACEMAN_SPACE_H_ */
> -- 
> 2.25.1
>