[PATCH 1/2] security: Handle dentries without inode in security_path_post_mknod()

[PATCH 1/2] security: Handle dentries without inode in security_path_post_mknod()

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Commit 08abce60d63fi ("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() is
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.

Add an extra check to first verify if there is an inode attached to the
dentry, before checking if the inode is private. Also add the same check to
the current users of the path_post_mknod hook, ima_post_path_mknod() and
evm_post_path_mknod().

Finally, use the proper helper, d_backing_inode(), to retrieve the inode
from the dentry in ima_post_path_mknod().

Cc: stable@vger.kernel.org # 6.8.x
Reported-by: Steve French <smfrench@gmail.com>
Closes: https://lore.kernel.org/linux-kernel/CAH2r5msAVzxCUHHG8VKrMPUKQHmBpE6K9_vjhgDa1uAvwx4ppw@mail.gmail.com/
Fixes: 08abce60d63fi ("security: Introduce path_post_mknod hook")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 6 ++++--
 security/integrity/ima/ima_main.c | 5 +++--
 security/security.c               | 4 +++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 81dbade5b9b3..ec1659273fcf 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -1037,11 +1037,13 @@ static void evm_file_release(struct file *file)
 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);
+	struct evm_iint_cache *iint;
 
-	if (!S_ISREG(inode->i_mode))
+	/* path_post_mknod hook might pass dentries without attached inode. */
+	if (!inode || !S_ISREG(inode->i_mode))
 		return;
 
+	iint = evm_iint_inode(inode);
 	if (iint)
 		iint->flags |= EVM_NEW_FILE;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c84e8c55333d..afc883e60cf3 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -719,10 +719,11 @@ static void ima_post_create_tmpfile(struct mnt_idmap *idmap,
 static void ima_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
 {
 	struct ima_iint_cache *iint;
-	struct inode *inode = dentry->d_inode;
+	struct inode *inode = d_backing_inode(dentry);
 	int must_appraise;
 
-	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+	/* path_post_mknod hook might pass dentries without attached inode. */
+	if (!ima_policy_flag || !inode || !S_ISREG(inode->i_mode))
 		return;
 
 	must_appraise = ima_must_appraise(idmap, inode, MAY_ACCESS,
diff --git a/security/security.c b/security/security.c
index 7e118858b545..455f0749e1b0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1801,7 +1801,9 @@ 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))))
+	/* Not all dentries have an inode attached after mknod. */
+	if (d_backing_inode(dentry) &&
+	    unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return;
 	call_void_hook(path_post_mknod, idmap, dentry);
 }
-- 
2.34.1

[PATCH 2/2] ima: evm: Rename *_post_path_mknod() to *_path_post_mknod()

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Rename ima_post_path_mknod() and evm_post_path_mknod() respectively to
ima_path_post_mknod() and evm_path_post_mknod(), to facilitate finding
users of the path_post_mknod LSM hook.

Cc: stable@vger.kernel.org # 6.8.x
Reported-by: Christian Brauner <christian@brauner.io>
Closes: https://lore.kernel.org/linux-kernel/20240328-raushalten-krass-cb040068bde9@brauner/
Fixes: 05d1a717ec04 ("ima: add support for creating files using the mknodat syscall")
Fixes: cd3cec0a02c7 ("ima: Move to LSM infrastructure")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 4 ++--
 security/integrity/ima/ima_main.c | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index ec1659273fcf..b4dd6e960203 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -1034,7 +1034,7 @@ static void evm_file_release(struct file *file)
 		iint->flags &= ~EVM_NEW_FILE;
 }
 
-static void evm_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
+static void evm_path_post_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
 {
 	struct inode *inode = d_backing_inode(dentry);
 	struct evm_iint_cache *iint;
@@ -1102,7 +1102,7 @@ static struct security_hook_list evm_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(inode_init_security, evm_inode_init_security),
 	LSM_HOOK_INIT(inode_alloc_security, evm_inode_alloc_security),
 	LSM_HOOK_INIT(file_release, evm_file_release),
-	LSM_HOOK_INIT(path_post_mknod, evm_post_path_mknod),
+	LSM_HOOK_INIT(path_post_mknod, evm_path_post_mknod),
 };
 
 static const struct lsm_id evm_lsmid = {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index afc883e60cf3..f33124ceece3 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -709,14 +709,14 @@ static void ima_post_create_tmpfile(struct mnt_idmap *idmap,
 }
 
 /**
- * ima_post_path_mknod - mark as a new inode
+ * ima_path_post_mknod - mark as a new inode
  * @idmap: idmap of the mount the inode was found from
  * @dentry: newly created dentry
  *
  * Mark files created via the mknodat syscall as new, so that the
  * file data can be written later.
  */
-static void ima_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
+static void ima_path_post_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
 {
 	struct ima_iint_cache *iint;
 	struct inode *inode = d_backing_inode(dentry);
@@ -1165,7 +1165,7 @@ static struct security_hook_list ima_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(kernel_post_load_data, ima_post_load_data),
 	LSM_HOOK_INIT(kernel_read_file, ima_read_file),
 	LSM_HOOK_INIT(kernel_post_read_file, ima_post_read_file),
-	LSM_HOOK_INIT(path_post_mknod, ima_post_path_mknod),
+	LSM_HOOK_INIT(path_post_mknod, ima_path_post_mknod),
 #ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
 	LSM_HOOK_INIT(key_post_create_or_update, ima_post_key_create_or_update),
 #endif
-- 
2.34.1

Re: [PATCH 1/2] security: Handle dentries without inode in security_path_post_mknod()

[Mimi Zohar]

On Fri, 2024-03-29 at 11:56 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Commit 08abce60d63fi ("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() is
> 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.
> 
> Add an extra check to first verify if there is an inode attached to the
> dentry, before checking if the inode is private. Also add the same check to
> the current users of the path_post_mknod hook, ima_post_path_mknod() and
> evm_post_path_mknod().
> 
> Finally, use the proper helper, d_backing_inode(), to retrieve the inode
> from the dentry in ima_post_path_mknod().
> 
> Cc: stable@vger.kernel.org # 6.8.x

Huh?  It doesn't need to be backported.

> Reported-by: Steve French <smfrench@gmail.com>
> Closes: 
> https://lore.kernel.org/linux-kernel/CAH2r5msAVzxCUHHG8VKrMPUKQHmBpE6K9_vjhgDa1uAvwx4ppw@mail.gmail.com/
> Fixes: 08abce60d63fi ("security: Introduce path_post_mknod hook")

-> 08abce60d63f

> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Acked-by: Mimi Zohar <zohar@linux.ibm.com>

Re: [PATCH 2/2] ima: evm: Rename *_post_path_mknod() to *_path_post_mknod()

[Mimi Zohar]

On Fri, 2024-03-29 at 11:56 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Rename ima_post_path_mknod() and evm_post_path_mknod() respectively to
> ima_path_post_mknod() and evm_path_post_mknod(), to facilitate finding
> users of the path_post_mknod LSM hook.
> 
> Cc: stable@vger.kernel.org # 6.8.x

Since commit cd3cec0a02c7 ("ima: Move to LSM infrastructure") was upstreamed in
this open window.  This change does not need to be packported and should be
limited to IMA and EVM full fledge LSMs.

> Reported-by: Christian Brauner <christian@brauner.io>
> Closes: 
> https://lore.kernel.org/linux-kernel/20240328-raushalten-krass-cb040068bde9@brauner/
> Fixes: 05d1a717ec04 ("ima: add support for creating files using the mknodat
> syscall")

"Fixes: 05d1a717ec04" should be removed.

> Fixes: cd3cec0a02c7 ("ima: Move to LSM infrastructure")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Acked-by: Mimi Zohar <zohar@linux.ibm.com>

Re: [PATCH 1/2] security: Handle dentries without inode in security_path_post_mknod()

[Roberto Sassu]

On 3/29/2024 4:05 PM, Mimi Zohar wrote:
> On Fri, 2024-03-29 at 11:56 +0100, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>
>> Commit 08abce60d63fi ("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() is
>> 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.
>>
>> Add an extra check to first verify if there is an inode attached to the
>> dentry, before checking if the inode is private. Also add the same check to
>> the current users of the path_post_mknod hook, ima_post_path_mknod() and
>> evm_post_path_mknod().
>>
>> Finally, use the proper helper, d_backing_inode(), to retrieve the inode
>> from the dentry in ima_post_path_mknod().
>>
>> Cc: stable@vger.kernel.org # 6.8.x
> 
> Huh?  It doesn't need to be backported.

Ehm, sorry. To be removed.

>> Reported-by: Steve French <smfrench@gmail.com>
>> Closes:
>> https://lore.kernel.org/linux-kernel/CAH2r5msAVzxCUHHG8VKrMPUKQHmBpE6K9_vjhgDa1uAvwx4ppw@mail.gmail.com/
>> Fixes: 08abce60d63fi ("security: Introduce path_post_mknod hook")
> 
> -> 08abce60d63f

Ok.

Thanks

Roberto

>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Acked-by: Mimi Zohar <zohar@linux.ibm.com>
> 

Re: [PATCH 2/2] ima: evm: Rename *_post_path_mknod() to *_path_post_mknod()

[Roberto Sassu]

On 3/29/2024 4:16 PM, Mimi Zohar wrote:
> On Fri, 2024-03-29 at 11:56 +0100, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>
>> Rename ima_post_path_mknod() and evm_post_path_mknod() respectively to
>> ima_path_post_mknod() and evm_path_post_mknod(), to facilitate finding
>> users of the path_post_mknod LSM hook.
>>
>> Cc: stable@vger.kernel.org # 6.8.x
> 
> Since commit cd3cec0a02c7 ("ima: Move to LSM infrastructure") was upstreamed in
> this open window.  This change does not need to be packported and should be
> limited to IMA and EVM full fledge LSMs.

Yes, got it wrong.

>> Reported-by: Christian Brauner <christian@brauner.io>
>> Closes:
>> https://lore.kernel.org/linux-kernel/20240328-raushalten-krass-cb040068bde9@brauner/
>> Fixes: 05d1a717ec04 ("ima: add support for creating files using the mknodat
>> syscall")
> 
> "Fixes: 05d1a717ec04" should be removed.

Ok, I agree that it is not a necessary fix for stable kernels. We can 
reconsider it if there is a bug fix depending on it.

Thanks

Roberto

>> Fixes: cd3cec0a02c7 ("ima: Move to LSM infrastructure")
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Acked-by: Mimi Zohar <zohar@linux.ibm.com>
> 

Re: [PATCH 1/2] security: Handle dentries without inode in security_path_post_mknod()

[Paul Moore]

On Mar 29, 2024 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> 
> Commit 08abce60d63fi ("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() is
> 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.
> 
> Add an extra check to first verify if there is an inode attached to the
> dentry, before checking if the inode is private. Also add the same check to
> the current users of the path_post_mknod hook, ima_post_path_mknod() and
> evm_post_path_mknod().
> 
> Finally, use the proper helper, d_backing_inode(), to retrieve the inode
> from the dentry in ima_post_path_mknod().
> 
> Cc: stable@vger.kernel.org # 6.8.x
> Reported-by: Steve French <smfrench@gmail.com>
> Closes: https://lore.kernel.org/linux-kernel/CAH2r5msAVzxCUHHG8VKrMPUKQHmBpE6K9_vjhgDa1uAvwx4ppw@mail.gmail.com/
> Fixes: 08abce60d63fi ("security: Introduce path_post_mknod hook")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Acked-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  security/integrity/evm/evm_main.c | 6 ++++--
>  security/integrity/ima/ima_main.c | 5 +++--
>  security/security.c               | 4 +++-
>  3 files changed, 10 insertions(+), 5 deletions(-)

In addition to the stable marking that Mimi already pointed out, I've
got one small comment below, but otherwise this looks fine to me.
Also, just to confirm, you're going to send patch 1/2 up to Linus during
the v6.9-rc1 phase and hold patch 2/2 for the next merge window, right?

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/security/security.c b/security/security.c
> index 7e118858b545..455f0749e1b0 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1801,7 +1801,9 @@ 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))))
> +	/* Not all dentries have an inode attached after mknod. */
> +	if (d_backing_inode(dentry) &&
> +	    unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>  		return;

I don't know how much impact this would have on the compiled code, but
you could save yourself a call into d_backing_inode() by saving it to
a local variable:

  struct inode *inode = d_backing_inode(dentry);
  if (inode && unlikely(IS_PRIVATE(inode)))
    return;

>  	call_void_hook(path_post_mknod, idmap, dentry);
>  }
> -- 
> 2.34.1

--
paul-moore.com

Re: [PATCH 2/2] ima: evm: Rename *_post_path_mknod() to *_path_post_mknod()

[Paul Moore]

On Fri, Mar 29, 2024 at 11:17 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Fri, 2024-03-29 at 11:56 +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> >
> > Rename ima_post_path_mknod() and evm_post_path_mknod() respectively to
> > ima_path_post_mknod() and evm_path_post_mknod(), to facilitate finding
> > users of the path_post_mknod LSM hook.
> >
> > Cc: stable@vger.kernel.org # 6.8.x
>
> Since commit cd3cec0a02c7 ("ima: Move to LSM infrastructure") was upstreamed in
> this open window.  This change does not need to be packported and should be
> limited to IMA and EVM full fledge LSMs.
>
> > Reported-by: Christian Brauner <christian@brauner.io>
> > Closes:
> > https://lore.kernel.org/linux-kernel/20240328-raushalten-krass-cb040068bde9@brauner/
> > Fixes: 05d1a717ec04 ("ima: add support for creating files using the mknodat
> > syscall")
>
> "Fixes: 05d1a717ec04" should be removed.

I'd take it one step further and remove both 'Fixes' tags.  A 'Fixes'
tag implies a flaw in the functionality of the code, this is just a
function rename.

Another important thing to keep in mind about 'Fixes' tags, unless
you've told the stable kernel folks to only take patches that you've
explicitly marked for stable, they are likely going to attempt to
backport anything with a 'Fixes' tag.

Regardless, since I was looking at 1/2 I took a quick look at this
patch and it looks fine to me once the comments have been
incorporated.

Reviewed-by: Paul Moore <paul@paul-moore.com>

> > Fixes: cd3cec0a02c7 ("ima: Move to LSM infrastructure")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>
> Acked-by: Mimi Zohar <zohar@linux.ibm.com>

-- 
paul-moore.com

Re: [PATCH 2/2] ima: evm: Rename *_post_path_mknod() to *_path_post_mknod()

[Mimi Zohar]

[Cc: Sasha, Greg]

On Fri, 2024-03-29 at 15:12 -0400, Paul Moore wrote:
> I'd take it one step further and remove both 'Fixes' tags.  A 'Fixes'
> tag implies a flaw in the functionality of the code, this is just a
> function rename.

Totally agree.

> Another important thing to keep in mind about 'Fixes' tags, unless
> you've told the stable kernel folks to only take patches that you've
> explicitly marked for stable, they are likely going to attempt to
> backport anything with a 'Fixes' tag.

How do we go about doing that?  Do we just send an email to stable?

Is it disabled for security?  I thought new functionality won't be backported. 
Hopefully the changes for making IMA & EVM full fledged LSMs won't be
automatically backported to stable.

thanks,

Mimi

Re: [PATCH 2/2] ima: evm: Rename *_post_path_mknod() to *_path_post_mknod()

[Paul Moore]

On Fri, Mar 29, 2024 at 3:28 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Fri, 2024-03-29 at 15:12 -0400, Paul Moore wrote:
> > Another important thing to keep in mind about 'Fixes' tags, unless
> > you've told the stable kernel folks to only take patches that you've
> > explicitly marked for stable, they are likely going to attempt to
> > backport anything with a 'Fixes' tag.
>
> How do we go about doing that?  Do we just send an email to stable?

When I asked for a change to the stable policy, it was an email
exchange with Greg where we setup what is essentially a shell glob to
filter out the files to skip unless explicitly tagged:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/ignore_list

> Is it disabled for security?

I asked for it to be disabled for the LSM layer, SELinux, and audit.
I sent a note about it last year to the mailing list:

https://lore.kernel.org/linux-security-module/CAHC9VhQgzshziG2tvaQMd9jchAVMu39M4Ym9RCComgbXj+WF0Q@mail.gmail.com

> I thought new functionality won't be backported.

One thing I noticed fairly consistently in the trees I maintained is
that commits marked with a 'Fixes' tag were generally backported
regardless of if they were marked for stable.

> Hopefully the changes for making IMA & EVM full fledged LSMs won't be
> automatically backported to stable.

I haven't seen that happening, and I wouldn't expect it in the future
as none of those patches were explicitly marked for stable or had a
'Fixes' tag.

--
paul-moore.com