Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <3bcb6e633f91d096cd0821a658c01cdb2f745cf6.camel@linux.ibm.com>
Subject: Re: [PATCH v5 06/12] evm: Ignore
 INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Roberto Sassu <roberto.sassu@huawei.com>,
        "mjg59@google.com" <mjg59@google.com>
Cc:     "linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>,
        "linux-security-module@vger.kernel.org" 
        <linux-security-module@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Date:   Mon, 03 May 2021 08:07:38 -0400
In-Reply-To: <c12f18094cc0479faa3f0f152b4964de@huawei.com>
References: <20210407105252.30721-1-roberto.sassu@huawei.com>
         <20210407105252.30721-7-roberto.sassu@huawei.com>
         <b8790b57e289980d4fe1133d15203ce016d2319d.camel@linux.ibm.com>
         <c12f18094cc0479faa3f0f152b4964de@huawei.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 2021-05-03 at 07:55 +0000, Roberto Sassu wrote:
> 
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c

> > > @@ -354,6 +372,8 @@ static int evm_protect_xattr(struct dentry *dentry,
> > const char *xattr_name,
> > >  				    -EPERM, 0);
> > >  	}
> > >  out:
> > > +	if (evm_ignore_error_safe(evm_status))
> > > +		return 0;
> > 
> > I agree with the concept, but the function name doesn't provide enough
> > context.  Perhaps defining a function more along the lines of
> > "evm_hmac_disabled()" would be more appropriate and at the same time
> > self documenting.
> 
> Since the function checks if the passed error can be ignored,
> would evm_ignore_error_hmac_disabled() also be ok?

The purpose of evm_protect_xattr() is to prevent allowing an invalid
security.evm xattr from being re-calculated and updated, making it
valid.   Refer to the first line of the function description.  That
hasn't changed.

One of the reasons for defining a new function is to avoid code
duplication, but it should not come at the expense of clear and easily
understood code.   In this case, the reason for "ignoring" certain
return codes needs to be highlighted, not hidden.  
(is_)evm_hmac_disabled() makes this very clear.

Please update the function description to include the reason why making
an exception is safe.

thanks,

Mimi

> > >  	if (evm_status != INTEGRITY_PASS)
> > >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry),
> > >  				    dentry->d_name.name,
> > "appraise_metadata",