| Message ID | 20230505081200.254449-1-xiujianfeng@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | lsm: Change inode_setattr() to take struct | expand |
sorry, I forgot to add the link to preview discussion: https://lore.kernel.org/all/20220827111215.131442-1-xiujianfeng@huawei.com/ On 2023/5/5 16:11, 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 > > Some of these hooks only take struct dentry as a argument, However, for > path-based LSMs such Landlock, Apparmor and Tomoyo, struct path instead > of struct dentry required to make sense of attr/xattr accesses. So we > need to refactor these hooks to take a struct path argument. > > This patchset only refators inode_setattr hook as part of whole work. > > Also, I have a problem about file_dentry() in __file_remove_privs() of the > first patch, before changes in commit c1892c37769cf ("vfs: fix deadlock in > file_remove_privs() on overlayfs"), it gets dentry and inode as belows: > > struct dentry *dentry = file->f_path.dentry; > struct inode *inode = d_inode(dentry); > > That would be clear to change it to pass &file->f_path to > __remove_privs()->notify_change()->inode_setattr(). > After that commit, it has been changed to: > > struct dentry *dentry = file_dentry(file); > struct inode *inode = file_inode(file); > > If I understand correctly, the dentry from file_dentry() maybe the upper > or the lower, it can be different from file->f_path.dentry. It can't just > go back to use &file->f_path otherwise the bug will come back for > overlayfs. So for such scenario, how to get a path from file if the file > maybe or not from overlayfs, and which kind of overlayfs path is ok for > Landlock? > > Xiu Jianfeng (2): > fs: Change notify_change() to take struct path argument > lsm: Change inode_setattr hook to take struct path argument > > drivers/base/devtmpfs.c | 5 +++-- > fs/attr.c | 7 ++++--- > fs/cachefiles/interface.c | 4 ++-- > fs/coredump.c | 2 +- > fs/ecryptfs/inode.c | 18 +++++++++--------- > fs/fat/file.c | 2 +- > fs/inode.c | 8 +++++--- > fs/ksmbd/smb2pdu.c | 6 +++--- > fs/ksmbd/smbacl.c | 2 +- > fs/namei.c | 2 +- > fs/nfsd/vfs.c | 12 ++++++++---- > fs/open.c | 19 ++++++++++--------- > fs/overlayfs/overlayfs.h | 4 +++- > fs/utimes.c | 2 +- > include/linux/fs.h | 4 ++-- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/security.h | 4 ++-- > security/security.c | 10 +++++----- > security/selinux/hooks.c | 3 ++- > security/smack/smack_lsm.c | 5 +++-- > 20 files changed, 67 insertions(+), 54 deletions(-) >
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.
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?
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.
On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote: > 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. Agreed. Passing struct path into random places is not how the VFS works. > So the best way to achieve the landlock goal might be to add new hooks What is "the landlock goal", and why does it matter? > 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. Yes, I'm really worried about th amount of LSMs we have, and the weird things they do.
On 5/30/2023 7:28 AM, Christoph Hellwig wrote: > On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote: >> 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. > Agreed. Passing struct path into random places is not how the VFS works. > >> So the best way to achieve the landlock goal might be to add new hooks > What is "the landlock goal", and why does it matter? > >> 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. > Yes, I'm really worried about th amount of LSMs we have, and the weird > things they do. Which LSM(s) do you think ought to be deprecated? I only see one that I might consider a candidate. As for weird behavior, that's what LSMs are for, and the really weird ones proposed (e.g. pathname character set limitations) (and excepting for BPF, of course) haven't gotten far.
On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote: > On 5/30/2023 7:28 AM, Christoph Hellwig wrote: > > On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote: > >> 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. > > Agreed. Passing struct path into random places is not how the VFS works. > > > >> So the best way to achieve the landlock goal might be to add new hooks > > What is "the landlock goal", and why does it matter? > > > >> 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. > > Yes, I'm really worried about th amount of LSMs we have, and the weird > > things they do. > > Which LSM(s) do you think ought to be deprecated? I only see one that I I don't have a good insight into what LSMs are actively used or are effectively unused but I would be curious to hear what LSMs are considered actively used/maintained from the LSM maintainer's perspective. > might consider a candidate. As for weird behavior, that's what LSMs are > for, and the really weird ones proposed (e.g. pathname character set limitations) If this is effectively saying that LSMs are licensed to step outside the rules of the subsystem they're a guest in then it seems unlikely subsystems will be very excited to let new LSM changes go in important codepaths going forward. In fact this seems like a good argument against it.
On 5/30/2023 9:01 AM, Christian Brauner wrote: > On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote: >> On 5/30/2023 7:28 AM, Christoph Hellwig wrote: >>> On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote: >>>> 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. >>> Agreed. Passing struct path into random places is not how the VFS works. >>> >>>> So the best way to achieve the landlock goal might be to add new hooks >>> What is "the landlock goal", and why does it matter? >>> >>>> 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. >>> Yes, I'm really worried about th amount of LSMs we have, and the weird >>> things they do. >> Which LSM(s) do you think ought to be deprecated? I only see one that I > I don't have a good insight into what LSMs are actively used or are > effectively unused but I would be curious to hear what LSMs are > considered actively used/maintained from the LSM maintainer's > perspective. I'm not the LSM maintainer, but I've been working on the infrastructure for quite some time. All the existing LSMs save one can readily be associated with active systems, and the one that isn't is actively maintained. We have not gotten into the habit of accepting LSMs upstream that don't have a real world use. >> might consider a candidate. As for weird behavior, that's what LSMs are >> for, and the really weird ones proposed (e.g. pathname character set limitations) > If this is effectively saying that LSMs are licensed to step outside the > rules of the subsystem they're a guest in then it seems unlikely > subsystems will be very excited to let new LSM changes go in important > codepaths going forward. In fact this seems like a good argument against > it. This is an artifact of Linus' decision that security models should be supported as add-on modules. On the one hand, all that a subsystem maintainer needs to know about a security feature is what it needs in the way of hooks. On the other hand, the subsystem maintainer loses control over what kinds of things the security feature does with the available information. It's a tension that we've had to deal with since the Orange Book days of the late 1980's. The deal has always been: You can have your security feature if: 1. If I turn it off it has no performance impact 2. I don't have to do anything to maintain it 3. It doesn't interfere with any other system behavior 4. You'll leave me alone As a security developer from way back I would be delighted if maintainers of other subsystems took an active interest in some of what we've been trying to accomplish in the security space. If the VFS maintainers would like to see the LSM interfaces for file systems changed I, for one, would like very much to hear about what they'd prefer. We do a lot of crazy things to avoid interfering with the subsystems we interact with. A closer developer relationship would be most welcome, so long as it helps us achieve or goals. We get a lot of complaints about how LSM feature perform, but no one wants to hear that a good deal of that comes about because of what has to be done in support of 1, 2 and 3 above. Sometimes we do stoopid things, but usually it's to avoid changes "outside our swim lane".
On Tue, May 30, 2023 at 03:15:01PM -0700, Casey Schaufler wrote: > On 5/30/2023 9:01 AM, Christian Brauner wrote: > > On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote: > >> On 5/30/2023 7:28 AM, Christoph Hellwig wrote: > >>> On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote: > >>>> 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. > >>> Agreed. Passing struct path into random places is not how the VFS works. > >>> > >>>> So the best way to achieve the landlock goal might be to add new hooks > >>> What is "the landlock goal", and why does it matter? > >>> > >>>> 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. > >>> Yes, I'm really worried about th amount of LSMs we have, and the weird > >>> things they do. > >> Which LSM(s) do you think ought to be deprecated? I only see one that I > > I don't have a good insight into what LSMs are actively used or are > > effectively unused but I would be curious to hear what LSMs are > > considered actively used/maintained from the LSM maintainer's > > perspective. > > I'm not the LSM maintainer, but I've been working on the infrastructure > for quite some time. All the existing LSMs save one can readily be associated > with active systems, and the one that isn't is actively maintained. We have > not gotten into the habit of accepting LSMs upstream that don't have a real > world use. > > >> might consider a candidate. As for weird behavior, that's what LSMs are > >> for, and the really weird ones proposed (e.g. pathname character set limitations) > > If this is effectively saying that LSMs are licensed to step outside the > > rules of the subsystem they're a guest in then it seems unlikely > > subsystems will be very excited to let new LSM changes go in important > > codepaths going forward. In fact this seems like a good argument against > > it. > > This is an artifact of Linus' decision that security models should be > supported as add-on modules. On the one hand, all that a subsystem maintainer > needs to know about a security feature is what it needs in the way of hooks. > On the other hand, the subsystem maintainer loses control over what kinds of > things the security feature does with the available information. It's a > tension that we've had to deal with since the Orange Book days of the late > 1980's. The deal has always been: > > You can have your security feature if: > 1. If I turn it off it has no performance impact > 2. I don't have to do anything to maintain it > 3. It doesn't interfere with any other system behavior > 4. You'll leave me alone > > As a security developer from way back I would be delighted if maintainers of > other subsystems took an active interest in some of what we've been trying > to accomplish in the security space. If the VFS maintainers would like to > see the LSM interfaces for file systems changed I, for one, would like very > much to hear about what they'd prefer. What is important for us is that the security layer must understand and accept that some things cannot be done the way it envisions them to be done because it would involve design compromises in the fs layer that the fs maintainers are unwilling to make. The idea to pass struct path to almost every security hook is a good example. If the project is feature parity between inode and path based LSMs then it must be clear from the start that this won't be achieved at the cost of mixing up the layer where only dentries and inodes are relevant and the layer where struct paths are most relevant. > > We do a lot of crazy things to avoid interfering with the subsystems we > interact with. A closer developer relationship would be most welcome, so > long as it helps us achieve or goals. We get a lot of complaints about how > LSM feature perform, but no one wants to hear that a good deal of that comes > about because of what has to be done in support of 1, 2 and 3 above. Sometimes > we do stoopid things, but usually it's to avoid changes "outside our swim lane". I personally am not opposed to comment on patches but they will naturally have lower priority than other things.
On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote: > Which LSM(s) do you think ought to be deprecated? I have no idea. But what I want is less weirdo things messing with VFS semantics. > > I only see one that I > might consider a candidate. As for weird behavior, that's what LSMs are > for, and the really weird ones proposed (e.g. pathname character set limitations) > (and excepting for BPF, of course) haven't gotten far. They haven't gotten far for a reason usually. Trying to sneak things in through the back door is exactly what is the problem with LSMs. > ---end quoted text---
On 5/31/2023 6:22 AM, Christoph Hellwig wrote: > On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote: >> Which LSM(s) do you think ought to be deprecated? > I have no idea. But what I want is less weirdo things messing with > VFS semantics. I am curious what you consider a weirdo thing done by LSMs. Things like io_uring are much stranger than anything an LSM does. > >> I only see one that I >> might consider a candidate. As for weird behavior, that's what LSMs are >> for, and the really weird ones proposed (e.g. pathname character set limitations) >> (and excepting for BPF, of course) haven't gotten far. > They haven't gotten far for a reason usually. Trying to sneak things in > through the back door is exactly what is the problem with LSMs. Mostly developers play by the rules, and we don't let things sneak in.
On 30/05/2023 16:28, Christoph Hellwig wrote: > On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote: >> 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. > > Agreed. Passing struct path into random places is not how the VFS works. I understand, it makes sense for the FS layer to not get access to things not required. IIUC, the main issue is the layering, with LSM calls being sometime at the last layer. > >> So the best way to achieve the landlock goal might be to add new hooks > > What is "the landlock goal", and why does it matter? Landlock's goal is to enable (unprivileged) users to set their own access rights for their (ephemeral) processes (on top of the existing access-controls of course) i.e., to sandbox applications. Landlock rules are defined by users, and then according to the FS topology they see. This means that Landlock relies on inodes and mount points to define and enforce a policy. > >> 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. > > Yes, I'm really worried about th amount of LSMs we have, and the weird > things they do. About Landlock, it's a new LSM that fit an actual need. I'd be glad to hear about not recommended things and how to improve the situation. I don't know all the history between VFS and LSM.
On 5/31/2023 1:36 AM, Christian Brauner wrote: > On Tue, May 30, 2023 at 03:15:01PM -0700, Casey Schaufler wrote: >> On 5/30/2023 9:01 AM, Christian Brauner wrote: >>> On Tue, May 30, 2023 at 07:55:17AM -0700, Casey Schaufler wrote: >>>> On 5/30/2023 7:28 AM, Christoph Hellwig wrote: >>>>> On Tue, May 30, 2023 at 03:58:35PM +0200, Christian Brauner wrote: >>>>>> 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. >>>>> Agreed. Passing struct path into random places is not how the VFS works. >>>>> >>>>>> So the best way to achieve the landlock goal might be to add new hooks >>>>> What is "the landlock goal", and why does it matter? >>>>> >>>>>> 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. >>>>> Yes, I'm really worried about th amount of LSMs we have, and the weird >>>>> things they do. >>>> Which LSM(s) do you think ought to be deprecated? I only see one that I >>> I don't have a good insight into what LSMs are actively used or are >>> effectively unused but I would be curious to hear what LSMs are >>> considered actively used/maintained from the LSM maintainer's >>> perspective. >> I'm not the LSM maintainer, but I've been working on the infrastructure >> for quite some time. All the existing LSMs save one can readily be associated >> with active systems, and the one that isn't is actively maintained. We have >> not gotten into the habit of accepting LSMs upstream that don't have a real >> world use. >> >>>> might consider a candidate. As for weird behavior, that's what LSMs are >>>> for, and the really weird ones proposed (e.g. pathname character set limitations) >>> If this is effectively saying that LSMs are licensed to step outside the >>> rules of the subsystem they're a guest in then it seems unlikely >>> subsystems will be very excited to let new LSM changes go in important >>> codepaths going forward. In fact this seems like a good argument against >>> it. >> This is an artifact of Linus' decision that security models should be >> supported as add-on modules. On the one hand, all that a subsystem maintainer >> needs to know about a security feature is what it needs in the way of hooks. >> On the other hand, the subsystem maintainer loses control over what kinds of >> things the security feature does with the available information. It's a >> tension that we've had to deal with since the Orange Book days of the late >> 1980's. The deal has always been: >> >> You can have your security feature if: >> 1. If I turn it off it has no performance impact >> 2. I don't have to do anything to maintain it >> 3. It doesn't interfere with any other system behavior >> 4. You'll leave me alone >> >> As a security developer from way back I would be delighted if maintainers of >> other subsystems took an active interest in some of what we've been trying >> to accomplish in the security space. If the VFS maintainers would like to >> see the LSM interfaces for file systems changed I, for one, would like very >> much to hear about what they'd prefer. > What is important for us is that the security layer must understand and > accept that some things cannot be done the way it envisions them to be > done because it would involve design compromises in the fs layer that > the fs maintainers are unwilling to make. The idea to pass struct path > to almost every security hook is a good example. Yes, and that's completely acceptable. What would be really great is some guidance about what to do instead. Fishing for NAKs isn't fun for anybody. > If the project is feature parity between inode and path based LSMs then > it must be clear from the start that this won't be achieved at the cost > of mixing up the layer where only dentries and inodes are relevant and > the layer where struct paths are most relevant. Which is a fair point, and helps those of us who don't work in the VFS layer daily understand the rationale. > >> We do a lot of crazy things to avoid interfering with the subsystems we >> interact with. A closer developer relationship would be most welcome, so >> long as it helps us achieve or goals. We get a lot of complaints about how >> LSM feature perform, but no one wants to hear that a good deal of that comes >> about because of what has to be done in support of 1, 2 and 3 above. Sometimes >> we do stoopid things, but usually it's to avoid changes "outside our swim lane". > I personally am not opposed to comment on patches but they will > naturally have lower priority than other things. I can't say that I see how security features "naturally have lower priority", but everyone has to balance things.
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 Some of these hooks only take struct dentry as a argument, However, for path-based LSMs such Landlock, Apparmor and Tomoyo, struct path instead of struct dentry required to make sense of attr/xattr accesses. So we need to refactor these hooks to take a struct path argument. This patchset only refators inode_setattr hook as part of whole work. Also, I have a problem about file_dentry() in __file_remove_privs() of the first patch, before changes in commit c1892c37769cf ("vfs: fix deadlock in file_remove_privs() on overlayfs"), it gets dentry and inode as belows: struct dentry *dentry = file->f_path.dentry; struct inode *inode = d_inode(dentry); That would be clear to change it to pass &file->f_path to __remove_privs()->notify_change()->inode_setattr(). After that commit, it has been changed to: struct dentry *dentry = file_dentry(file); struct inode *inode = file_inode(file); If I understand correctly, the dentry from file_dentry() maybe the upper or the lower, it can be different from file->f_path.dentry. It can't just go back to use &file->f_path otherwise the bug will come back for overlayfs. So for such scenario, how to get a path from file if the file maybe or not from overlayfs, and which kind of overlayfs path is ok for Landlock? Xiu Jianfeng (2): fs: Change notify_change() to take struct path argument lsm: Change inode_setattr hook to take struct path argument drivers/base/devtmpfs.c | 5 +++-- fs/attr.c | 7 ++++--- fs/cachefiles/interface.c | 4 ++-- fs/coredump.c | 2 +- fs/ecryptfs/inode.c | 18 +++++++++--------- fs/fat/file.c | 2 +- fs/inode.c | 8 +++++--- fs/ksmbd/smb2pdu.c | 6 +++--- fs/ksmbd/smbacl.c | 2 +- fs/namei.c | 2 +- fs/nfsd/vfs.c | 12 ++++++++---- fs/open.c | 19 ++++++++++--------- fs/overlayfs/overlayfs.h | 4 +++- fs/utimes.c | 2 +- include/linux/fs.h | 4 ++-- include/linux/lsm_hook_defs.h | 2 +- include/linux/security.h | 4 ++-- security/security.c | 10 +++++----- security/selinux/hooks.c | 3 ++- security/smack/smack_lsm.c | 5 +++-- 20 files changed, 67 insertions(+), 54 deletions(-)