integrity: implement get and set acl hook

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 }

regards,
dan carpenter

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 <[email protected]>
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.

---
 security/integrity/evm/evm_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index e074c2b4d499..e01cfd4ad896 100644
@@ -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