Re: [bug report] integrity: implement get and set acl hook

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

 



On Fri, Oct 28, 2022 at 02:04:48PM +0300, Dan Carpenter wrote:
> Hello Christian Brauner,
> 
> The patch e61b135f7bfe: "integrity: implement get and set acl hook"
> from Sep 22, 2022, leads to the following Smatch static checker
> warning:
> 
> 	security/integrity/evm/evm_main.c:687 evm_inode_set_acl()
> 	warn: duplicate check 'evm_status' (previous on line 661)
> 
> security/integrity/evm/evm_main.c
>     649 int evm_inode_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
>     650                       const char *acl_name, struct posix_acl *kacl)
>     651 {
>     652         enum integrity_status evm_status;
>     653 
>     654         /* Policy permits modification of the protected xattrs even though
>     655          * there's no HMAC key loaded
>     656          */
>     657         if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
>     658                 return 0;
>     659 
>     660         evm_status = evm_verify_current_integrity(dentry);
>     661         if ((evm_status == INTEGRITY_PASS) ||
>     662             (evm_status == INTEGRITY_NOXATTRS))
>     663                 return 0;
> 
> If evm_status == INTEGRITY_PASS then this function returns here.
> 
>     664 
>     665         /* Exception if the HMAC is not going to be calculated. */
>     666         if (evm_hmac_disabled() && (evm_status == INTEGRITY_NOLABEL ||
>     667             evm_status == INTEGRITY_UNKNOWN))
>     668                 return 0;
>     669 
>     670         /*
>     671          * Writing other xattrs is safe for portable signatures, as portable
>     672          * signatures are immutable and can never be updated.
>     673          */
>     674         if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
>     675                 return 0;
>     676 
>     677         if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
>     678             !evm_inode_set_acl_change(mnt_userns, dentry, acl_name, kacl))
>     679                 return 0;
>     680 
>     681         if (evm_status != INTEGRITY_PASS &&
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Always true.
> 
>     682             evm_status != INTEGRITY_PASS_IMMUTABLE)
> 
> It feels like if evm_inode_set_acl_change() fails then it should trigger
> this audit message.  In other words just delete the if statement and
> always call integrity_audit_msg().
> 
>     683                 integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
>     684                                     dentry->d_name.name, "appraise_metadata",
>     685                                     integrity_status_msg[evm_status],
>     686                                     -EPERM, 0);
> --> 687         return evm_status == INTEGRITY_PASS ? 0 : -EPERM;
>                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is harmless dead code.  Just "return -EPERM;"
> 
>     688 }

Thanks for spotting this, Dan! I've added the following fix below to my
tree. If it shows no regression I'll likely fold it into the patch that
is fixed as we are pre -rc4 (Which is my somewhat arbitrarily chosen
cut-off point for rebases.):

>From 16257cf6658d5bde2a055caf48f143c255abade7 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@xxxxxxxxxx>
Date: Fri, 28 Oct 2022 15:41:31 +0200
Subject: [PATCH] evm: remove dead code in evm_inode_set_acl()

When evm_status is INTEGRITY_PASS then this function returns early and so
later codepaths that check for evm_status != INTEGRITY_PASS can be removed
as they are dead code.

Fixes: e61b135f7bfe ("integrity: implement get and set acl hook")
Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
---
 security/integrity/evm/evm_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index e074c2b4d499..e01cfd4ad896 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -678,13 +678,12 @@ int evm_inode_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
 	    !evm_inode_set_acl_change(mnt_userns, dentry, acl_name, kacl))
 		return 0;
 
-	if (evm_status != INTEGRITY_PASS &&
-	    evm_status != INTEGRITY_PASS_IMMUTABLE)
+	if (evm_status != INTEGRITY_PASS_IMMUTABLE)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
 				    integrity_status_msg[evm_status],
 				    -EPERM, 0);
-	return evm_status == INTEGRITY_PASS ? 0 : -EPERM;
+	return -EPERM;
 }
 
 static void evm_reset_status(struct inode *inode)
-- 
2.34.1




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux