| Message ID | 20240221-idmap-fscap-refactor-v2-15-3039364623bd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | fs: use type-safe uid representation for filesystem capabilities | expand |
On Wed, Feb 21, 2024 at 4:25 PM Seth Forshee (DigitalOcean) <sforshee@kernel.org> wrote: > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > --- > security/security.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) First off, you've got to write *something* for the commit description, even if it is just a single sentence. > diff --git a/security/security.c b/security/security.c > index 0d210da9862c..f515d8430318 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2365,9 +2365,14 @@ int security_inode_remove_acl(struct mnt_idmap *idmap, > int security_inode_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, > const struct vfs_caps *caps, int flags) > { > + int ret; > + > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return 0; > - return call_int_hook(inode_set_fscaps, 0, idmap, dentry, caps, flags); > + ret = call_int_hook(inode_set_fscaps, 0, idmap, dentry, caps, flags); > + if (ret) > + return ret; > + return evm_inode_set_fscaps(idmap, dentry, caps, flags); > } > > /** > @@ -2387,6 +2392,7 @@ void security_inode_post_set_fscaps(struct mnt_idmap *idmap, > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return; > call_void_hook(inode_post_set_fscaps, idmap, dentry, caps, flags); > + evm_inode_post_set_fscaps(idmap, dentry, caps, flags); > } > > /** > @@ -2415,9 +2421,14 @@ int security_inode_get_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) > */ > int security_inode_remove_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) > { > + int ret; > + > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > return 0; > - return call_int_hook(inode_remove_fscaps, 0, idmap, dentry); > + ret = call_int_hook(inode_remove_fscaps, 0, idmap, dentry); > + if (ret) > + return ret; > + return evm_inode_remove_fscaps(dentry); > } If you take a look at linux-next or the LSM tree's dev branch you'll see that we've gotten rid of the dedicated IMA and EVM hooks, promoting both IMA and EVM to "proper" LSMs that leverage the existing LSM hook infrastructure. In this patchset, and moving forward, please don't add dedicated IMA/EVM hooks like this, instead register them as LSM hook implementations with LSM_HOOK_INIT().
On Wed, Feb 21, 2024 at 06:43:43PM -0500, Paul Moore wrote: > On Wed, Feb 21, 2024 at 4:25 PM Seth Forshee (DigitalOcean) > <sforshee@kernel.org> wrote: > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > --- > > security/security.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > First off, you've got to write *something* for the commit description, > even if it is just a single sentence. > > > diff --git a/security/security.c b/security/security.c > > index 0d210da9862c..f515d8430318 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -2365,9 +2365,14 @@ int security_inode_remove_acl(struct mnt_idmap *idmap, > > int security_inode_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, > > const struct vfs_caps *caps, int flags) > > { > > + int ret; > > + > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > return 0; > > - return call_int_hook(inode_set_fscaps, 0, idmap, dentry, caps, flags); > > + ret = call_int_hook(inode_set_fscaps, 0, idmap, dentry, caps, flags); > > + if (ret) > > + return ret; > > + return evm_inode_set_fscaps(idmap, dentry, caps, flags); > > } > > > > /** > > @@ -2387,6 +2392,7 @@ void security_inode_post_set_fscaps(struct mnt_idmap *idmap, > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > return; > > call_void_hook(inode_post_set_fscaps, idmap, dentry, caps, flags); > > + evm_inode_post_set_fscaps(idmap, dentry, caps, flags); > > } > > > > /** > > @@ -2415,9 +2421,14 @@ int security_inode_get_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) > > */ > > int security_inode_remove_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) > > { > > + int ret; > > + > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > return 0; > > - return call_int_hook(inode_remove_fscaps, 0, idmap, dentry); > > + ret = call_int_hook(inode_remove_fscaps, 0, idmap, dentry); > > + if (ret) > > + return ret; > > + return evm_inode_remove_fscaps(dentry); > > } > > If you take a look at linux-next or the LSM tree's dev branch you'll > see that we've gotten rid of the dedicated IMA and EVM hooks, > promoting both IMA and EVM to "proper" LSMs that leverage the existing > LSM hook infrastructure. In this patchset, and moving forward, please > don't add dedicated IMA/EVM hooks like this, instead register them as > LSM hook implementations with LSM_HOOK_INIT(). Yeah, I'm aware that work was going on and got applied recently. I've been assuming this change will go in through the vfs tree though, and I wasn't sure how you and Al/Christian would want to handle that dependency between your trees, so I held off on updating based off the LSM tree. I'm happy to update this for the next round though. Thanks, Seth
On Wed, Feb 21, 2024 at 7:20 PM Seth Forshee (DigitalOcean) <sforshee@kernel.org> wrote: > On Wed, Feb 21, 2024 at 06:43:43PM -0500, Paul Moore wrote: > > On Wed, Feb 21, 2024 at 4:25 PM Seth Forshee (DigitalOcean) > > <sforshee@kernel.org> wrote: > > > > > > Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> > > > --- > > > security/security.c | 15 +++++++++++++-- > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > First off, you've got to write *something* for the commit description, > > even if it is just a single sentence. > > > > > diff --git a/security/security.c b/security/security.c > > > index 0d210da9862c..f515d8430318 100644 > > > --- a/security/security.c > > > +++ b/security/security.c > > > @@ -2365,9 +2365,14 @@ int security_inode_remove_acl(struct mnt_idmap *idmap, > > > int security_inode_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, > > > const struct vfs_caps *caps, int flags) > > > { > > > + int ret; > > > + > > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > > return 0; > > > - return call_int_hook(inode_set_fscaps, 0, idmap, dentry, caps, flags); > > > + ret = call_int_hook(inode_set_fscaps, 0, idmap, dentry, caps, flags); > > > + if (ret) > > > + return ret; > > > + return evm_inode_set_fscaps(idmap, dentry, caps, flags); > > > } > > > > > > /** > > > @@ -2387,6 +2392,7 @@ void security_inode_post_set_fscaps(struct mnt_idmap *idmap, > > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > > return; > > > call_void_hook(inode_post_set_fscaps, idmap, dentry, caps, flags); > > > + evm_inode_post_set_fscaps(idmap, dentry, caps, flags); > > > } > > > > > > /** > > > @@ -2415,9 +2421,14 @@ int security_inode_get_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) > > > */ > > > int security_inode_remove_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) > > > { > > > + int ret; > > > + > > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > > return 0; > > > - return call_int_hook(inode_remove_fscaps, 0, idmap, dentry); > > > + ret = call_int_hook(inode_remove_fscaps, 0, idmap, dentry); > > > + if (ret) > > > + return ret; > > > + return evm_inode_remove_fscaps(dentry); > > > } > > > > If you take a look at linux-next or the LSM tree's dev branch you'll > > see that we've gotten rid of the dedicated IMA and EVM hooks, > > promoting both IMA and EVM to "proper" LSMs that leverage the existing > > LSM hook infrastructure. In this patchset, and moving forward, please > > don't add dedicated IMA/EVM hooks like this, instead register them as > > LSM hook implementations with LSM_HOOK_INIT(). > > Yeah, I'm aware that work was going on and got applied recently. I've > been assuming this change will go in through the vfs tree though, and I > wasn't sure how you and Al/Christian would want to handle that > dependency between your trees, so I held off on updating based off the > LSM tree. I'm happy to update this for the next round though. Okay, good, I just wanted to make sure you were aware of the changes. Since the merge window is only a couple of weeks away I'm guessing this isn't something we'll need to worry about in Linus' tree as the LSM/IMA/EVM changes are slated to go up during the next merge window and I'm guessing this will likely go in after that, targeting the following merge window at the earliest.
diff --git a/security/security.c b/security/security.c index 0d210da9862c..f515d8430318 100644 --- a/security/security.c +++ b/security/security.c @@ -2365,9 +2365,14 @@ int security_inode_remove_acl(struct mnt_idmap *idmap, int security_inode_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry, const struct vfs_caps *caps, int flags) { + int ret; + if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) return 0; - return call_int_hook(inode_set_fscaps, 0, idmap, dentry, caps, flags); + ret = call_int_hook(inode_set_fscaps, 0, idmap, dentry, caps, flags); + if (ret) + return ret; + return evm_inode_set_fscaps(idmap, dentry, caps, flags); } /** @@ -2387,6 +2392,7 @@ void security_inode_post_set_fscaps(struct mnt_idmap *idmap, if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) return; call_void_hook(inode_post_set_fscaps, idmap, dentry, caps, flags); + evm_inode_post_set_fscaps(idmap, dentry, caps, flags); } /** @@ -2415,9 +2421,14 @@ int security_inode_get_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) */ int security_inode_remove_fscaps(struct mnt_idmap *idmap, struct dentry *dentry) { + int ret; + if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) return 0; - return call_int_hook(inode_remove_fscaps, 0, idmap, dentry); + ret = call_int_hook(inode_remove_fscaps, 0, idmap, dentry); + if (ret) + return ret; + return evm_inode_remove_fscaps(dentry); } /**
Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org> --- security/security.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)