| Message ID | 20210507114150.139102-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | debugfs: fix security_locked_down() call for SELinux | expand |
On Fri, May 07, 2021 at 01:41:50PM +0200, Ondrej Mosnacek wrote: > Make sure that security_locked_down() is checked last so that a bogus > denial is not reported by SELinux when (ia->ia_valid & (ATTR_MODE | > ATTR_UID | ATTR_GID)) is zero. Why would this be "bogus"? > Note: this was introduced by commit 5496197f9b08 ("debugfs: Restrict > debugfs when the kernel is locked down"), but it didn't matter at that > time, as the SELinux support came in later. > > Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown") What does this "fix"? What is happening in selinux that it can not handle this sequence now? That commit showed up a long time ago, this feels "odd"... thanks, greg k-h
On Fri, May 07, 2021 at 02:03:04PM +0200, Greg Kroah-Hartman wrote: > On Fri, May 07, 2021 at 01:41:50PM +0200, Ondrej Mosnacek wrote: > > Make sure that security_locked_down() is checked last so that a bogus > > denial is not reported by SELinux when (ia->ia_valid & (ATTR_MODE | > > ATTR_UID | ATTR_GID)) is zero. > > Why would this be "bogus"? I presume selinux is logging a denial ... but we don't then actually deny the operation. > > Note: this was introduced by commit 5496197f9b08 ("debugfs: Restrict > > debugfs when the kernel is locked down"), but it didn't matter at that > > time, as the SELinux support came in later. > > > > Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown") > > What does this "fix"? > > What is happening in selinux that it can not handle this sequence now? > That commit showed up a long time ago, this feels "odd"... > > thanks, > > greg k-h
On Fri, May 07, 2021 at 01:12:18PM +0100, Matthew Wilcox wrote: > On Fri, May 07, 2021 at 02:03:04PM +0200, Greg Kroah-Hartman wrote: > > On Fri, May 07, 2021 at 01:41:50PM +0200, Ondrej Mosnacek wrote: > > > Make sure that security_locked_down() is checked last so that a bogus > > > denial is not reported by SELinux when (ia->ia_valid & (ATTR_MODE | > > > ATTR_UID | ATTR_GID)) is zero. > > > > Why would this be "bogus"? > > I presume selinux is logging a denial ... but we don't then actually > deny the operation. That would be nice to note here...
On Fri, May 7, 2021 at 2:16 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Fri, May 07, 2021 at 01:12:18PM +0100, Matthew Wilcox wrote: > > On Fri, May 07, 2021 at 02:03:04PM +0200, Greg Kroah-Hartman wrote: > > > On Fri, May 07, 2021 at 01:41:50PM +0200, Ondrej Mosnacek wrote: > > > > Make sure that security_locked_down() is checked last so that a bogus > > > > denial is not reported by SELinux when (ia->ia_valid & (ATTR_MODE | > > > > ATTR_UID | ATTR_GID)) is zero. > > > > > > Why would this be "bogus"? > > > > I presume selinux is logging a denial ... but we don't then actually > > deny the operation. > > That would be nice to note here... Granted, I didn't do a good job of describing the issue in the patch description... I'll send a v2 with hopefully a better description.
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index 22e86ae4dd5a..bbfc7898c1aa 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -45,10 +45,13 @@ static unsigned int debugfs_allow = DEFAULT_DEBUGFS_ALLOW_BITS; static int debugfs_setattr(struct user_namespace *mnt_userns, struct dentry *dentry, struct iattr *ia) { - int ret = security_locked_down(LOCKDOWN_DEBUGFS); + int ret; - if (ret && (ia->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))) - return ret; + if (ia->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) { + ret = security_locked_down(LOCKDOWN_DEBUGFS); + if (ret) + return ret; + } return simple_setattr(&init_user_ns, dentry, ia); }
Make sure that security_locked_down() is checked last so that a bogus denial is not reported by SELinux when (ia->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) is zero. Note: this was introduced by commit 5496197f9b08 ("debugfs: Restrict debugfs when the kernel is locked down"), but it didn't matter at that time, as the SELinux support came in later. Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown") Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- fs/debugfs/inode.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)