All of lore.kernel.org
 help / color / mirror / Atom feed
* [bug report] integrity: implement get and set acl hook
@ 2022-10-28 11:04 Dan Carpenter
  2022-10-28 13:47 ` Christian Brauner
  0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2022-10-28 11:04 UTC (permalink / raw)
  To: brauner; +Cc: linux-integrity

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

^ permalink raw reply	[flat|nested] 2+ messages in thread
* Re: [bug report] integrity: implement get and set acl hook
  2022-10-28 11:04 [bug report] integrity: implement get and set acl hook Dan Carpenter
@ 2022-10-28 13:47 ` Christian Brauner
  0 siblings, 0 replies; 2+ messages in thread
From: Christian Brauner @ 2022-10-28 13:47 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: linux-integrity

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@kernel.org>
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@oracle.com>
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 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


^ permalink raw reply related	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-10-28 13:50 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-28 11:04 [bug report] integrity: implement get and set acl hook Dan Carpenter
2022-10-28 13:47 ` Christian Brauner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.