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
