On Thu, Nov 17, 2022 at 03:58:06PM -0800, Darrick J. Wong wrote:
> On Fri, Nov 18, 2022 at 08:51:25AM +1100, Dave Chinner wrote:
> > 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@xxxxxxxxxx>
> > > > ---
> > > > 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...
>
> Too late now, this already shipped in 6.0. Changing the struct size
> would change the ioctl number, which is a totally new API. This was
> already discussed back in July on fsdevel/api.
It is certainly not too late - if we are going to lift this to the
VFS, then we can simply make it a new ioctl. The horrible ext4 ioctl
can ber left to rot in ext4 and nobody else ever needs to care that
it exists.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
