Re: kernel crash in mknod

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/28/2024 12:08 PM, Christian Brauner wrote:
On Thu, Mar 28, 2024 at 12:53:40PM +0200, Roberto Sassu wrote:
On 3/26/2024 12:40 PM, Christian Brauner wrote:
we can change the parameter of security_path_post_mknod() from
dentry to inode?

If all current callers only operate on the inode then it seems the best
to only pass the inode. If there's some reason someone later needs a
dentry the hook can always be changed.

Ok, so the crash is likely caused by:

void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry
*dentry)
{
         if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))

I guess we can also simply check if there is an inode attached to the
dentry, to minimize the changes. I can do both.

More technical question, do I need to do extra checks on the dentry before
calling security_path_post_mknod()?

Why do you need the dentry? The two users I see are ima in [1] and evm in [2].
Both of them don't care about the dentry. They only care about the
inode. So why is that hook not just:

Sure, I can definitely do that. Seems an easier fix to do an extra check in security_path_post_mknod(), rather than changing the parameter everywhere.

Next time, when we introduce new LSM hooks we can try to introduce more specific parameters.

Also, consider that the pre hook security_path_mknod() has the dentry as parameter. For symmetry, we could keep it in the post hook.

What I was also asking is if I can still call d_backing_inode() on the dentry without extra checks, and avoiding the IS_PRIVATE() check if the former returns NULL.

diff --git a/security/security.c b/security/security.c
index 7e118858b545..025689a7e912 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1799,11 +1799,11 @@ EXPORT_SYMBOL(security_path_mknod);
   *
   * Update inode security field after a file has been created.
   */
-void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
+void security_inode_post_mknod(struct mnt_idmap *idmap, struct inode *inode)
  {
-       if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
+       if (unlikely(IS_PRIVATE(inode)))
                 return;
-       call_void_hook(path_post_mknod, idmap, dentry);
+       call_void_hook(path_post_mknod, idmap, inode);
  }

  /**

And one another thing I'd like to point out is that the security hook is
called "security_path_post_mknod()" while the evm and ima hooks are
called evm_post_path_mknod() and ima_post_path_mknod() respectively. In
other words:

git grep _path_post_mknod() doesn't show the implementers of that hook
which is rather unfortunate. It would be better if the pattern were:

<specific LSM>_$some_$ordered_$words()

I know, yes. Didn't want to change just yet since people familiar with the IMA code know the current function name. I don't see any problem to rename the functions.

Thanks

Roberto

[1]:
static void evm_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
{
         struct inode *inode = d_backing_inode(dentry);
         struct evm_iint_cache *iint = evm_iint_inode(inode);

         if (!S_ISREG(inode->i_mode))
                 return;

         if (iint)
                 iint->flags |= EVM_NEW_FILE;
}

[2]:
static void ima_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
{
         struct ima_iint_cache *iint;
         struct inode *inode = dentry->d_inode;
         int must_appraise;

         if (!ima_policy_flag || !S_ISREG(inode->i_mode))
                 return;

         must_appraise = ima_must_appraise(idmap, inode, MAY_ACCESS,
                                           FILE_CHECK);
         if (!must_appraise)
                 return;

         /* Nothing to do if we can't allocate memory */
         iint = ima_inode_get(inode);
         if (!iint)
                 return;

         /* needed for re-opening empty files */
         iint->flags |= IMA_NEW_FILE;
}




Thanks

Roberto

For bigger changes it's also worthwhile if the object that's passed down
into the hook-based LSM layer is as specific as possible. If someone
does a change that affects lifetime rules of mounts then any hook that
takes a struct path argument that's unused means going through each LSM
that implements the hook only to find out it's not actually used.
Similar for dentry vs inode imho.






[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux