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: <10f9ab52c487b9dcde000f8aee77c8e04979a485.camel@linux.ibm.com>
Subject: Re: [PATCH v5 07/12] evm: Allow xattr/attr operations for portable
 signatures
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:   Tue, 04 May 2021 10:49:53 -0400
In-Reply-To: <f26f4b6fd3074bb4a6f0f0ff4911a202@huawei.com>
References: <20210407105252.30721-1-roberto.sassu@huawei.com>
         <20210407105252.30721-8-roberto.sassu@huawei.com>
         <75e8a4f70dfbbfa4cf5b923ab0ac92768e1e2de5.camel@linux.ibm.com>
         <f26f4b6fd3074bb4a6f0f0ff4911a202@huawei.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk

On Tue, 2021-05-04 at 14:28 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, May 3, 2021 2:13 AM
> > Hi Roberto,
> > 
> > > diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> > > index 2271939c5c31..2ea0f2f65ab6 100644
> > > --- a/include/linux/integrity.h
> > > +++ b/include/linux/integrity.h
> > >
> > > @@ -238,9 +241,12 @@ static enum integrity_status
> > evm_verify_hmac(struct dentry *dentry,
> > >  		break;
> > >  	}
> > >
> > > -	if (rc)
> > > -		evm_status = (rc == -ENODATA) ?
> > > -				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> > > +	if (rc) {
> > > +		evm_status = INTEGRITY_NOXATTRS;
> > > +		if (rc != -ENODATA)
> > > +			evm_status = evm_immutable ?
> > > +				     INTEGRITY_FAIL_IMMUTABLE :
> > INTEGRITY_FAIL;
> > 
> > The original code made an exception for the -ENODATA case.   Using a
> > ternary operator made sense in that case.   Inverting the test makes
> > the code less readable.  Please use the standard "if" statement
> > instead.
> 
> Did I understand correctly that the code should be:
> 
>                 evm_status = INTEGRITY_NOXATTRS;
>                 if (rc != -ENODA
>                         evm_status = INTEGRITY_FAIL;
>                         if (evm_immutable)
>                                 evm_status = INTEGRITY_FAIL_IMMUTABLE;
>                 }
> 
 
                if (rc == -ENODATA)
                        evm_status = INTEGRITY_NOXATTRS;
                else if (evm_status == evm_immutable)
                        evm_status = INTEGRITY_FAIL_IMMUTABLE;
                else
                        evm_status = INTEGRITY_FAIL;

I think keeping it simple makes it really clear that ENODATA is an
exception.

thanks,

Mimi