Received-SPF: pass (google.com: domain of linux-kernel+bounces-122827-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <4ad908dc-ddc5-492e-8ed4-d304156b5810@huaweicloud.com>
Date: Thu, 28 Mar 2024 13:24:25 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: kernel crash in mknod
Content-Language: en-US
To: Christian Brauner <brauner@kernel.org>
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>
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>
 <20240328-raushalten-krass-cb040068bde9@brauner>
From: Roberto Sassu <roberto.sassu@huaweicloud.com>
In-Reply-To: <20240328-raushalten-krass-cb040068bde9@brauner>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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