lkml.org 
[lkml]   [2024]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [GIT PULL] security changes for v6.9-rc3
On Tue, 2 Apr 2024 at 07:12, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> A single bug fix to address a kernel panic in the newly introduced function
> security_path_post_mknod.

So I've pulled from you before, but I still don't have a signature
chain for your key (not that I can even find the key itself, much less
a signature chain).

Last time I pulled, it was after having everybody else just verify the
actual commit.

This time, the commit looks like a valid "avoid NULL", but I have to
say that I also think the security layer code in question is ENTIRELY
WRONG.

IOW, as far as I can tell, the mknod() system call may indeed leave
the dentry unhashed, and rely on anybody who then wants to use the new
special file to just do a "lookup()" to actually use it.

HOWEVER.

That also means that the whole notion of "post_path_mknod() is
complete and utter hoghwash. There is not anything that the security
layer can possibly validly do.

End result: instead of checking the 'inode' for NULL, I think the
right fix is to remove that meaningless security hook. It cannot do
anything sane, since one option is always 'the inode hasn't been
initialized yet".

Put another way: any security hook that checks inode in
security_path_post_mknod() seems simply buggy.

But if we really want to do this ("if mknod creates a positive dentry,
I won't see it in lookup, so I want to appraise it now"), then we
should just deal with this in the generic layer with some hack like
this:

--- a/security/security.c
+++ b/security/security.c
@@ -1801,7 +1801,8 @@ EXPORT_SYMBOL(security_path_mknod);
*/
void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
{
- if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
+ struct inode *inode = d_backing_inode(dentry);
+ if (unlikely(!inode || IS_PRIVATE(inode)))
return;
call_void_hook(path_post_mknod, idmap, dentry);
}

and IMA and EVM would have to do any validation at lookup() time for
the cases where the dentry wasn't hashed by ->mknod.

Anyway, all of this is to say that I don't feel like I can pull this without
(a) more acks by people
and
(b) explanations for why the simpler fix to just
security_path_post_mknod() isn't the right fix.

Linus

\
 
 \ /
  Last update: 2024-05-27 16:19    [W:0.151 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site