Message-ID: <20230530-mietfrei-zynisch-8b63a8566f66@brauner>
Date:   Tue, 30 May 2023 15:58:35 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Mickaël Salaün <mic@...ikod.net>
Cc:     Xiu Jianfeng <xiujianfeng@...wei.com>, gregkh@...uxfoundation.org,
        rafael@...nel.org, viro@...iv.linux.org.uk, dhowells@...hat.com,
        code@...icks.com, hirofumi@...l.parknet.co.jp,
        linkinjeon@...nel.org, sfrench@...ba.org, senozhatsky@...omium.org,
        tom@...pey.com, chuck.lever@...cle.com, jlayton@...nel.org,
        miklos@...redi.hu, paul@...l-moore.com, jmorris@...ei.org,
        serge@...lyn.com, stephen.smalley.work@...il.com,
        eparis@...isplace.org, casey@...aufler-ca.com, dchinner@...hat.com,
        john.johansen@...onical.com, mcgrof@...nel.org,
        mortonm@...omium.org, fred@...udflare.com, mpe@...erman.id.au,
        nathanl@...ux.ibm.com, gnoack3000@...il.com,
        roberto.sassu@...wei.com, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-cachefs@...hat.com,
        ecryptfs@...r.kernel.org, linux-cifs@...r.kernel.org,
        linux-nfs@...r.kernel.org, linux-unionfs@...r.kernel.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
        wangweiyang2@...wei.com, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH -next 0/2] lsm: Change inode_setattr() to take struct

On Fri, May 26, 2023 at 06:33:05PM +0200, Mickaël Salaün wrote:
> 
> On 15/05/2023 17:12, Christian Brauner wrote:
> > On Fri, May 05, 2023 at 04:11:58PM +0800, Xiu Jianfeng wrote:
> > > Hi,
> > > 
> > > I am working on adding xattr/attr support for landlock [1], so we can
> > > control fs accesses such as chmod, chown, uptimes, setxattr, etc.. inside
> > > landlock sandbox. the LSM hooks as following are invoved:
> > > 1.inode_setattr
> > > 2.inode_setxattr
> > > 3.inode_removexattr
> > > 4.inode_set_acl
> > > 5.inode_remove_acl
> > > which are controlled by LANDLOCK_ACCESS_FS_WRITE_METADATA.
> > > 
> > > and
> > > 1.inode_getattr
> > > 2.inode_get_acl
> > > 3.inode_getxattr
> > > 4.inode_listxattr
> > > which are controlled by LANDLOCK_ACCESS_FS_READ_METADATA
> > 
> > It would be helpful to get the complete, full picture.
> > 
> > Piecemeal extending vfs helpers with struct path arguments is costly,
> > will cause a lot of churn and will require a lot of review time from us.
> > 
> > Please give us the list of all security hooks to which you want to pass
> > a struct path (if there are more to come apart from the ones listed
> > here). Then please follow all callchains and identify the vfs helpers
> > that would need to be updated. Then please figure out where those
> > vfs helpers are called from and follow all callchains finding all
> > inode_operations that would have to be updated and passed a struct path
> > argument. So ultimately we'll end up with a list of vfs helpers and
> > inode_operations that would have to be changed.
> > 
> > I'm very reluctant to see anything merged without knowing _exactly_ what
> > you're getting us into.
> 
> Ultimately we'd like the path-based LSMs to reach parity with the
> inode-based LSMs. This proposal's goal is to provide users the ability to
> control (in a complete and easy way) file metadata access. For these we need
> to extend the inode_*attr hooks and inode_*acl hooks to handle paths. The
> chown/chmod hooks are already good.
> 
> In the future, I'd also like to be able to control directory traversals
> (e.g. chdir), which currently only calls inode_permission().
> 
> What would be the best way to reach this goal?

The main concern which was expressed on other patchsets before is that
modifying inode operations to take struct path is not the way to go.
Passing struct path into individual filesystems is a clear layering
violation for most inode operations, sometimes downright not feasible,
and in general exposing struct vfsmount to filesystems is a hard no. At
least as far as I'm concerned.

So the best way to achieve the landlock goal might be to add new hooks
in cases where you would be required to modify inode operations
otherwise. Taking the chdir() case as an example. That calls
path_permission(). Since inode_permission() and generic_permission() are
called in a lot of places where not even a dentry might be readily
available we will not extend them to take a struct path argument. This
would also involve extending the inode ->permission() method which is a
no go. That's neither feasible and would involve modifying a good chunk
of code for the sole purpose of an LSM.

So in path_permission() you might have the potential to add an LSM hook.
Or if you need to know what syscall this was called for you might have
to add a hook into chdir() itself. That is still unpleasant but since
the alternative to adding new LSM hooks might be endless layering
violations that's a compromise that at least I can live with. Ultimately
you have to convince more people.

Some concerns around passing struct path to LSM hooks in general that I
would like to just point out and ask you to keep in mind: As soon as
there's an LSM hook that takes a path argument it means all LSMs have
access to a struct path. At that point visibility into what's been done
to that struct path is lost for the fs layer.

One the one hand that's fine on the other hand sooner or later some LSM
will try to get creative and do things like starting to infer
relationships between mounts without understanding mount property and
mount handling enough, or start trying to infer the parent of a path and
perform permission checks on it in ways that aren't sane. And that sucks
because this only becomes obvious when fs wide changes are done that
affect LSM hooks as well.

And that's the other thing. The more objects the LSM layer gets access
to the greater the cost to do fs wide changes because the fs layer is
now even closer entangled with the LSM layer. For example, even simple
things like removing IOP_XATTR - even just for POSIX ACLs - suddenly
become complicated not because of the fs layer but because of how the
LSM layer makes use of it. It might start relying on internal flags that
would be revoked later and so on. That also goes for struct vfsmount. So
it means going through every LSM trying to figure out if a change is ok
or not. And we keep adding new LSMs without deprecating older ones (A
problem we also face in the fs layer.) and then they sit around but
still need to be taken into account when doing changes.

Powered by blists - more mailing lists