linux-kernel - Re: [PATCH 5/6] fsmount: do not use legacy MS

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]

Message-ID: <20180922132106.wfa6xaxwbu276qka@brauner.io>
Date:   Sat, 22 Sep 2018 15:21:09 +0200
From:   Christian Brauner <christian@...uner.io>
To:     David Howells <dhowells@...hat.com>
Cc:     Miklos Szeredi <mszeredi@...hat.com>,
        Al Viro <viro@...IV.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

On Fri, Sep 21, 2018 at 05:52:36PM +0100, David Howells wrote:
> Christian Brauner <christian@...uner.io> wrote:
> 
> > So from reading the patch I got the impression that superblock mount
> > options passed via fsconfig() are passed as strings like "ro" and are
> > translated into approriate objects (e.g. flags etc.) by the kernel.
> 
> I'm having second throughts about that - at least for "ro": that one is
> particularly problematic as it governs how source devices are opened.  I'm
> kind of tempted to pass that as a flag to fsopen().
> 
> What I'd like to do in btrfs, ext4, etc. is open blockdevs as I get the
> parameters that enumerate them - but I have to hold the looked-up paths till
> the validate/get_tree stage so that I know the final ro/rw state before I can
> do the opening.
> 
> > That seems like a future proof mechanism to extend mount options for
> > userspace without having to worry about exceeding any integer types in the
> > future. Maybe this would make sense for non-superblock options as well? I
> > can see that this is less performant that checking flags and string parsing
> > is a thing that is not particularly nice but it would be one option to solve
> > the running out of flags problem.
> 
> Al disliked the idea of setting up a separate context to define the mount
> options.

I hope thinking about mount flag exhaustion at this point is not too
hijacking this thread too much. But another option I was thinking about was to
split the mount flags into different sets where the sets can be
differentiated by a command.

enum {
        MOUNT_ATTR_PROPAGATION = 1
        MOUNT_ATTR_SECURITY
        MOUNT_ATTR_FSTIME
}

Let's say split mount propagation flags into a set identified by
MOUNT_ATTR_PROPAGATION:
#define MOUNT_ATTR_UNBINDABLE   (1<<0)  /* change to unbindable */
#define MOUNT_ATTR_PRIVATE      (1<<1)  /* change to private */
#define MOUNT_ATTR_SLAVE        (1<<2)  /* change to slave */
#define MOUNT_ATTR_SHARED       (1<<3)  /* change to shared */

and another set
MOUNT_ATTR_SECURITY:
#define MOUNT_ATTR_RDONLY        (1<<0) /* Mount read-only */
#define MOUNT_ATTR_NOSUID        (1<<1) /* Ignore suid and sgid bits */
#define MOUNT_ATTR_NODEV         (1<<2) /* Disallow access to device special files */
#define MOUNT_ATTR_NOEXEC        (1<<3) /* Disallow program execution */

and another set
MOUNT_ATTR_FSTIME:
#define MOUNT_ATTR_NOATIME      (1<<0)  /* Do not update access times. */
#define MOUNT_ATTR_NODIRATIME   (1<<1)  /* Do not update directory access times */

and so on...

So you could e.g. add another unsigned int cmd argument to
mount_setattr():

mount_setattr(int dfd, const char *path, unsigned int atflags,
              unsigned int attr_cmd,
	      unsigned int attr_values,
	      unsigned int attr_mask);

Then - similiar to fsconfig()'s FS_CONFIG_SET_{FLAG,STRING,...} - you
would pass:

/* set propagation */
mount_setattr(dfd, path, atflags,
              MOUNT_ATTR_PROPAGATION,
	      vals,
	      propagation_flagmask);

/* set security */
mount_setattr(dfd, path, atflags,
              MOUNT_ATTR_SECURITY,
	      vals,
	      security_flagmask);

/* set time */
mount_setattr(dfd, path, atflags,
              MOUNT_ATTR_FSTIME,
	      vals,
	      fstime_flagmask);

and then finally have an additional command like:

/* apply changes */
mount_setattr(dfd, path, atflags, MOUNT_ATTR_APPLY, vals, 0);

that applies all the properties.

In kernel you could then either have separate flags in the corresponding
structs of interest or bit arrays of arbitrary length or whatever.
Each of the flag setting commands before the MOUNT_ATTR_APPLY would then
perform verification whether the combination of flags makes sense and
immediately return back an error to userspace but only the
MOUNT_ATTR_APPLY call would actually commit the changes.

> 
> > > 	mount_setattr(int dfd, const char *path, unsigned int atflags,
> > > 		      unsigned int attr_values,
> > > 		      unsigned int attr_mask);
> > 
> > If we go with the flag arguments wouldn't it make sense to use a larger
> > integer type?
> 
> You can't - at least not directly through syscall args.  They are 32-bit on a
> 32-bit system.

Ah, 32bit systems. I tend to forget that they exist. :)

> 
> > > where atflags can potentially include AT_RECURSIVE.
> > 
> > Very much in favor of having this operate recursively!
> 
> It gets complicated with propagation.

Sure, I totally understand.

Christian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)