Received-SPF: pass (google.com: domain of linux-kernel+bounces-122814-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 28 Mar 2024 12:08:31 +0100
From: Christian Brauner <brauner@kernel.org>
To: Roberto Sassu <roberto.sassu@huaweicloud.com>
Cc: Roberto Sassu <roberto.sassu@huawei.com>, 
	Al Viro <viro@zeniv.linux.org.uk>, Steve French <smfrench@gmail.com>, 
	LKML <linux-kernel@vger.kernel.org>, linux-fsdevel <linux-fsdevel@vger.kernel.org>, 
	CIFS <linux-cifs@vger.kernel.org>, Paulo Alcantara <pc@manguebit.com>, 
	Christian Brauner <christian@brauner.io>, Mimi Zohar <zohar@linux.ibm.com>, 
	Paul Moore <paul@paul-moore.com>, 
	"linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>, 
	"linux-security-module@vger.kernel.org" <linux-security-module@vger.kernel.org>
Subject: Re: kernel crash in mknod
Message-ID: <20240328-raushalten-krass-cb040068bde9@brauner>
References: <CAH2r5msAVzxCUHHG8VKrMPUKQHmBpE6K9_vjhgDa1uAvwx4ppw@mail.gmail.com>
 <20240324054636.GT538574@ZenIV>
 <3441a4a1140944f5b418b70f557bca72@huawei.com>
 <20240325-beugen-kraftvoll-1390fd52d59c@brauner>
 <cb267d1c7988460094dbe19d1e7bcece@huawei.com>
 <20240326-halbkreis-wegstecken-8d5886e54d28@brauner>
 <4a0b28ba-be57-4443-b91e-1a744a0feabf@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <4a0b28ba-be57-4443-b91e-1a744a0feabf@huaweicloud.com>

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:

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()

[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.
>