[RESEND,v3] security: Place security_path_post_mknod() where the original IMA call was

Commit Message

Roberto Sassu April 3, 2024, 9:07 a.m. UTC

From: Roberto Sassu <[e-mail]>

Commit 08abce60d63f ("security: Introduce path_post_mknod hook")
introduced security_path_post_mknod(), to replace the IMA-specific call to
ima_post_path_mknod().

For symmetry with security_path_mknod(), security_path_post_mknod() was
called after a successful mknod operation, for any file type, rather than
only for regular files at the time there was the IMA call.

However, as reported by VFS maintainers, successful mknod operation does
not mean that the dentry always has an inode attached to it (for example,
not for FIFOs on a SAMBA mount).

If that condition happens, the kernel crashes when
security_path_post_mknod() attempts to verify if the inode associated to
the dentry is private.

Move security_path_post_mknod() where the ima_post_path_mknod() call was,
which is obviously correct from IMA/EVM perspective. IMA/EVM are the only
in-kernel users, and only need to inspect regular files.

Reported-by: Steve French <[e-mail]>
Closes: https://lore.kernel.org/linux-kernel/[e-mail]/
Suggested-by: Al Viro <[e-mail]>
Fixes: 08abce60d63f ("security: Introduce path_post_mknod hook")
Signed-off-by: Roberto Sassu <[e-mail]>
---
 fs/namei.c          | 7 ++-----
 security/security.c | 4 ++--
 2 files changed, 4 insertions(+), 7 deletions(-)

Comments

Mimi Zohar April 3, 2024, 1:11 p.m. UTC | #1

Hi Roberto,

Subject: -> security: Limit security_path_post_mknod() to regular files

This patch description was written for the previous patch version with minor
changes.  The discussion was more about making LSM hooks more generic than
currently needed.  The patch description should somehow reflect that discussion.

On Wed, 2024-04-03 at 11:07 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <[e-mail]>
> 
> Commit 08abce60d63f ("security: Introduce path_post_mknod hook")
> introduced security_path_post_mknod(), to replace the IMA-specific call to
> ima_post_path_mknod().
> 
> For symmetry with security_path_mknod(), security_path_post_mknod() was
> called after a successful mknod operation, for any file type, rather than
> only for regular files at the time there was the IMA call.

-> rather than only for regular files.
> 
> However, as reported by VFS maintainers, successful mknod operation does
> not mean that the dentry always has an inode attached to it (for example,
> not for FIFOs on a SAMBA mount).
> 
> If that condition happens, the kernel crashes when
> security_path_post_mknod() attempts to verify if the inode associated to
> the dentry is private.

This is an example of why making the LSM hook more generic than needed didn't
work.  Based on the discussion there is no valid reason for making the hook more
generic.

> 
> Move security_path_post_mknod() where the ima_post_path_mknod() call was,
> which is obviously correct from IMA/EVM perspective. IMA/EVM are the only
> in-kernel users, and only need to inspect regular files.

-> Move the security_path_post_mknod() back to the original placement of the
ima_post_path_mknod(), so that it is only called for regular files.

> 
> Reported-by: Steve French <[e-mail]>
> Closes: 
> https://lore.kernel.org/linux-kernel/[e-mail]/
> Suggested-by: Al Viro <[e-mail]>
> Fixes: 08abce60d63f ("security: Introduce path_post_mknod hook")
> Signed-off-by: Roberto Sassu <[e-mail]>

Reviewed-by: Mimi Zohar <[e-mail]>

Paul Moore April 3, 2024, 3:02 p.m. UTC | #2

On Wed, Apr 3, 2024 at 9:11 AM Mimi Zohar <[e-mail]> wrote:
> On Wed, 2024-04-03 at 11:07 +0200, Roberto Sassu wrote:
> >
> > However, as reported by VFS maintainers, successful mknod operation does
> > not mean that the dentry always has an inode attached to it (for example,
> > not for FIFOs on a SAMBA mount).
> >
> > If that condition happens, the kernel crashes when
> > security_path_post_mknod() attempts to verify if the inode associated to
> > the dentry is private.
>
> This is an example of why making the LSM hook more generic than needed didn't
> work.  Based on the discussion there is no valid reason for making the hook more
> generic.

I agree, I think we all do, but I don't think we want to get into
process discussions in the patch description.  The description
explains the original motivation for the buggy commit, the problem it
caused, and the solution; that's enough IMHO.

Linus Torvalds April 3, 2024, 4:59 p.m. UTC | #3

On Wed, 3 Apr 2024 at 02:10, Roberto Sassu
<[e-mail]> wrote:
>
> Move security_path_post_mknod() where the ima_post_path_mknod() call was,
> which is obviously correct from IMA/EVM perspective. IMA/EVM are the only
> in-kernel users, and only need to inspect regular files.

Thanks, applied,

              Linus

Al Viro April 3, 2024, 10:21 p.m. UTC | #4

On Wed, Apr 03, 2024 at 11:07:49AM +0200, Roberto Sassu wrote:
> From: Roberto Sassu <[e-mail]>
> 
> Commit 08abce60d63f ("security: Introduce path_post_mknod hook")
> introduced security_path_post_mknod(), to replace the IMA-specific call to
> ima_post_path_mknod().
> 
> For symmetry with security_path_mknod(), security_path_post_mknod() was
> called after a successful mknod operation, for any file type, rather than
> only for regular files at the time there was the IMA call.
> 
> However, as reported by VFS maintainers, successful mknod operation does
> not mean that the dentry always has an inode attached to it (for example,
> not for FIFOs on a SAMBA mount).
> 
> If that condition happens, the kernel crashes when
> security_path_post_mknod() attempts to verify if the inode associated to
> the dentry is private.
> 
> Move security_path_post_mknod() where the ima_post_path_mknod() call was,
> which is obviously correct from IMA/EVM perspective. IMA/EVM are the only
> in-kernel users, and only need to inspect regular files.
> 
> Reported-by: Steve French <[e-mail]>
> Closes: https://lore.kernel.org/linux-kernel/[e-mail]/
> Suggested-by: Al Viro <[e-mail]>
> Fixes: 08abce60d63f ("security: Introduce path_post_mknod hook")
> Signed-off-by: Roberto Sassu <[e-mail]>

LGTM...

Patch

diff --git a/fs/namei.c b/fs/namei.c
index ceb9ddf8dfdd..c5b2a25be7d0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4050,6 +4050,8 @@  static int do_mknodat(int dfd, struct filename *name, umode_t mode,
 		case 0: case S_IFREG:
 			error = vfs_create(idmap, path.dentry->d_inode,
 					   dentry, mode, true);
+			if (!error)
+				security_path_post_mknod(idmap, dentry);
 			break;
 		case S_IFCHR: case S_IFBLK:
 			error = vfs_mknod(idmap, path.dentry->d_inode,
@@ -4060,11 +4062,6 @@  static int do_mknodat(int dfd, struct filename *name, umode_t mode,
 					  dentry, mode, 0);
 			break;
 	}
-
-	if (error)
-		goto out2;
-
-	security_path_post_mknod(idmap, dentry);
 out2:
 	done_path_create(&path, dentry);
 	if (retry_estale(error, lookup_flags)) {
diff --git a/security/security.c b/security/security.c
index 7e118858b545..0a9a0ac3f266 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1793,11 +1793,11 @@  int security_path_mknod(const struct path *dir, struct dentry *dentry,
 EXPORT_SYMBOL(security_path_mknod);
 
 /**
- * security_path_post_mknod() - Update inode security field after file creation
+ * security_path_post_mknod() - Update inode security after reg file creation
  * @idmap: idmap of the mount
  * @dentry: new file
  *
- * Update inode security field after a file has been created.
+ * Update inode security field after a regular file has been created.
  */
 void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
 {