Re: [PATCH v1] xfs_spaceman: add fsuuid command

[Dave Chinner <david@fromorbit.com>]

On Thu, Nov 17, 2022 at 12:37:33PM -0800, Darrick J. Wong wrote:
> 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.

So, follow common convention and you'll get it wrong, eh? That a
score of -4 on Rusty's API Design scale.

http://sweng.the-davies.net/Home/rustys-api-design-manifesto

Flex arrays in user APIs like this just look plain dangerous to me.

Really, this says that the FSUUID API should have a fixed length
buffer size defined in the API and the length used can be anything
up to the maximum.

We already have this being added for the ioctl API:

#define UUID_SIZE 16

So why isn't the API definition this:

struct fsuuid {
    __u32   fsu_len;
    __u32   fsu_flags;
    __u8    fsu_uuid[UUID_SIZE];
};

Or if we want to support larger ID structures:

#define MAX_FSUUID_SIZE 256

struct fsuuid {
    __u32   fsu_len;
    __u32   fsu_flags;
    __u8    fsu_uuid[MAX_FSUUID_SIZE];
};

Then the structure can be safely placed on the stack, which means
"the obvious use is (probably) the correct one" (a score of 7 on
Rusty's API Design scale). It also gives the kernel a fixed upper
bound that it can use to validate the incoming fsu_len variable
against...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com