Re: [PATCH 1/4] evm: skip replacing EVM signature with HMAC on read-only filesystem

Re: [PATCH 1/4] evm: skip replacing EVM signature with HMAC on read-only filesystem

[Mimi Zohar]

On Wed, 2014-09-24 at 15:07 +0300, Dmitry Kasatkin wrote: 
> If filesystem is mounted read-only or file is immutable, updating
> xattr will fail. This is a usual case during early boot until
> filesystem is remount read-write. This patch verifies conditions
> to skip unnecessary attempt to calculate HMAC and set xattr.
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> ---
>  security/integrity/evm/evm_main.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 9685af3..a30be77 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -162,9 +162,14 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  					(const char *)xattr_data, xattr_len,
>  					calc.digest, sizeof(calc.digest));
>  		if (!rc) {
> -			/* we probably want to replace rsa with hmac here */
> -			evm_update_evmxattr(dentry, xattr_name, xattr_value,
> -				   xattr_value_len);
> +			/* Replace RSA with HMAC if not mounted readonly and
> +			 * not immutable
> +			 */
> +			if (!IS_RDONLY(dentry->d_inode) &&
> +					!IS_IMMUTABLE(dentry->d_inode))

Previously patches conformed to Lindent, unless there was a valid reason
not to use it, like conflicting with checkpatch.pl.  Joe Perches
submitted a patch to remove it from the Documentation/CodingStyle a
while ago -  https://lkml.org/lkml/2013/2/11/390 and recommends using
"checkpatch.pl --fix" instead.

Andrew, Greg, what is the current best practice?

thanks,

Mimi

> +				evm_update_evmxattr(dentry, xattr_name,
> +						    xattr_value,
> +						    xattr_value_len);
>  		}
>  		break;
>  	default:

Re: [PATCH 1/4] evm: skip replacing EVM signature with HMAC on read-only filesystem

[Greg KH]

On Mon, Sep 29, 2014 at 12:14:31PM -0400, Mimi Zohar wrote:
> On Wed, 2014-09-24 at 15:07 +0300, Dmitry Kasatkin wrote: 
> > If filesystem is mounted read-only or file is immutable, updating
> > xattr will fail. This is a usual case during early boot until
> > filesystem is remount read-write. This patch verifies conditions
> > to skip unnecessary attempt to calculate HMAC and set xattr.
> > 
> > Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > ---
> >  security/integrity/evm/evm_main.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 9685af3..a30be77 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -162,9 +162,14 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> >  					(const char *)xattr_data, xattr_len,
> >  					calc.digest, sizeof(calc.digest));
> >  		if (!rc) {
> > -			/* we probably want to replace rsa with hmac here */
> > -			evm_update_evmxattr(dentry, xattr_name, xattr_value,
> > -				   xattr_value_len);
> > +			/* Replace RSA with HMAC if not mounted readonly and
> > +			 * not immutable
> > +			 */
> > +			if (!IS_RDONLY(dentry->d_inode) &&
> > +					!IS_IMMUTABLE(dentry->d_inode))
> 
> Previously patches conformed to Lindent, unless there was a valid reason
> not to use it, like conflicting with checkpatch.pl.  Joe Perches
> submitted a patch to remove it from the Documentation/CodingStyle a
> while ago -  https://lkml.org/lkml/2013/2/11/390 and recommends using
> "checkpatch.pl --fix" instead.
> 
> Andrew, Greg, what is the current best practice?

I don't understand, what is wrong with the formatting of this patch?  It
looks ok to me.  If you want to indent the second line of the if to the
left some more, that's fine, but just minor nits, nothing major at all.

thanks,

greg k-h

Re: [PATCH 1/4] evm: skip replacing EVM signature with HMAC on read-only filesystem

[Mimi Zohar]

On Mon, 2014-09-29 at 12:23 -0400, Greg KH wrote: 
> On Mon, Sep 29, 2014 at 12:14:31PM -0400, Mimi Zohar wrote:
> > On Wed, 2014-09-24 at 15:07 +0300, Dmitry Kasatkin wrote: 
> > > If filesystem is mounted read-only or file is immutable, updating
> > > xattr will fail. This is a usual case during early boot until
> > > filesystem is remount read-write. This patch verifies conditions
> > > to skip unnecessary attempt to calculate HMAC and set xattr.
> > > 
> > > Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > > ---
> > >  security/integrity/evm/evm_main.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > > index 9685af3..a30be77 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -162,9 +162,14 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> > >  					(const char *)xattr_data, xattr_len,
> > >  					calc.digest, sizeof(calc.digest));
> > >  		if (!rc) {
> > > -			/* we probably want to replace rsa with hmac here */
> > > -			evm_update_evmxattr(dentry, xattr_name, xattr_value,
> > > -				   xattr_value_len);
> > > +			/* Replace RSA with HMAC if not mounted readonly and
> > > +			 * not immutable
> > > +			 */
> > > +			if (!IS_RDONLY(dentry->d_inode) &&
> > > +					!IS_IMMUTABLE(dentry->d_inode))
> > 
> > Previously patches conformed to Lindent, unless there was a valid reason
> > not to use it, like conflicting with checkpatch.pl.  Joe Perches
> > submitted a patch to remove it from the Documentation/CodingStyle a
> > while ago -  https://lkml.org/lkml/2013/2/11/390 and recommends using
> > "checkpatch.pl --fix" instead.
> > 
> > Andrew, Greg, what is the current best practice?
> 
> I don't understand, what is wrong with the formatting of this patch?  It
> looks ok to me.  If you want to indent the second line of the if to the
> left some more, that's fine, but just minor nits, nothing major at all.

Right, scripts/Lindent is only about formatting, nothing major.

Mimi