lkml.org 
[lkml]   [2023]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.

    \
     
     \ /
      Last update: 2023-05-30 15:59    [W:4.563 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site