* [PATCH] evm: allow metadata changes for inode without xattr support
@ 2017-11-03 7:26 Mikhail Kurinnoi
2017-11-03 16:54 ` Mimi Zohar
0 siblings, 1 reply; 7+ messages in thread
From: Mikhail Kurinnoi @ 2017-11-03 7:26 UTC (permalink / raw)
To: linux-integrity
This patch provide changes in order to allow metadata changes for
inode without xattr support.
Signed-off-by: Mikhail Kurinnoi <viewizard@viewizard.com>
security/integrity/evm/evm_main.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 9826c02e2db8..51151c43433d 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -294,8 +294,7 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
if (!posix_xattr_acl(xattr_name))
return 0;
evm_status = evm_verify_current_integrity(dentry);
- if ((evm_status == INTEGRITY_PASS) ||
- (evm_status == INTEGRITY_NOXATTRS))
+ if (evm_status == INTEGRITY_NOXATTRS)
return 0;
goto out;
}
@@ -319,12 +318,15 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
-EPERM, 0);
}
out:
- if (evm_status != INTEGRITY_PASS)
- 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;
+ if ((evm_status == INTEGRITY_PASS) ||
+ (evm_status == INTEGRITY_UNKNOWN))
+ return 0;
+
+ integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
+ dentry->d_name.name, "appraise_metadata",
+ integrity_status_msg[evm_status],
+ -EPERM, 0);
+ return -EPERM;
}
/**
@@ -435,7 +437,8 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
return 0;
evm_status = evm_verify_current_integrity(dentry);
if ((evm_status == INTEGRITY_PASS) ||
- (evm_status == INTEGRITY_NOXATTRS))
+ (evm_status == INTEGRITY_NOXATTRS) ||
+ (evm_status == INTEGRITY_UNKNOWN))
return 0;
integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
dentry->d_name.name, "appraise_metadata",
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] evm: allow metadata changes for inode without xattr support
2017-11-03 7:26 [PATCH] evm: allow metadata changes for inode without xattr support Mikhail Kurinnoi
@ 2017-11-03 16:54 ` Mimi Zohar
2017-11-03 17:06 ` Mikhail Kurinnoi
0 siblings, 1 reply; 7+ messages in thread
From: Mimi Zohar @ 2017-11-03 16:54 UTC (permalink / raw)
To: Mikhail Kurinnoi, linux-integrity
On Fri, 2017-11-03 at 10:26 +0300, Mikhail Kurinnoi wrote:
> This patch provide changes in order to allow metadata changes for
> inode without xattr support.
>
>
> Signed-off-by: Mikhail Kurinnoi <viewizard@viewizard.com>
>
> security/integrity/evm/evm_main.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 9826c02e2db8..51151c43433d 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -294,8 +294,7 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
> if (!posix_xattr_acl(xattr_name))
> return 0;
> evm_status = evm_verify_current_integrity(dentry);
> - if ((evm_status == INTEGRITY_PASS) ||
> - (evm_status == INTEGRITY_NOXATTRS))
> + if (evm_status == INTEGRITY_NOXATTRS)
> return 0;
> goto out;
> }
> @@ -319,12 +318,15 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
> -EPERM, 0);
> }
> out:
> - if (evm_status != INTEGRITY_PASS)
> - 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;
> + if ((evm_status == INTEGRITY_PASS) ||
> + (evm_status == INTEGRITY_UNKNOWN))
> + return 0;
> +
> + integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
> + dentry->d_name.name, "appraise_metadata",
> + integrity_status_msg[evm_status],
> + -EPERM, 0);
> + return -EPERM;
> }
>
> /**
> @@ -435,7 +437,8 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
> return 0;
> evm_status = evm_verify_current_integrity(dentry);
> if ((evm_status == INTEGRITY_PASS) ||
> - (evm_status == INTEGRITY_NOXATTRS))
> + (evm_status == INTEGRITY_NOXATTRS) ||
> + (evm_status == INTEGRITY_UNKNOWN))
> return 0;
> integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
> dentry->d_name.name, "appraise_metadata",
>
Since this change is limited to setattr, perhaps it would be simpler
to test the i_opflags directly, without modifying evm_protect_xattr().
Mimi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] evm: allow metadata changes for inode without xattr support
2017-11-03 16:54 ` Mimi Zohar
@ 2017-11-03 17:06 ` Mikhail Kurinnoi
2017-11-03 17:15 ` Mimi Zohar
0 siblings, 1 reply; 7+ messages in thread
From: Mikhail Kurinnoi @ 2017-11-03 17:06 UTC (permalink / raw)
To: Mimi Zohar; +Cc: linux-integrity
? Fri, 03 Nov 2017 12:54:08 -0400
Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
> On Fri, 2017-11-03 at 10:26 +0300, Mikhail Kurinnoi wrote:
> > This patch provide changes in order to allow metadata changes for
> > inode without xattr support.
> >
> >
> > Signed-off-by: Mikhail Kurinnoi <viewizard@viewizard.com>
> >
> > security/integrity/evm/evm_main.c | 21 ++++++++++++---------
> > 1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c index
> > 9826c02e2db8..51151c43433d 100644 ---
> > a/security/integrity/evm/evm_main.c +++
> > b/security/integrity/evm/evm_main.c @@ -294,8 +294,7 @@ static int
> > evm_protect_xattr(struct dentry *dentry, const char *xattr_name, if
> > (!posix_xattr_acl(xattr_name)) return 0;
> > evm_status = evm_verify_current_integrity(dentry);
> > - if ((evm_status == INTEGRITY_PASS) ||
> > - (evm_status == INTEGRITY_NOXATTRS))
> > + if (evm_status == INTEGRITY_NOXATTRS)
> > return 0;
> > goto out;
> > }
> > @@ -319,12 +318,15 @@ static int evm_protect_xattr(struct dentry
> > *dentry, const char *xattr_name, -EPERM, 0);
> > }
> > out:
> > - if (evm_status != INTEGRITY_PASS)
> > - 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;
> > + if ((evm_status == INTEGRITY_PASS) ||
> > + (evm_status == INTEGRITY_UNKNOWN))
> > + return 0;
> > +
> > + integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry),
> > + dentry->d_name.name,
> > "appraise_metadata",
> > + integrity_status_msg[evm_status],
> > + -EPERM, 0);
> > + return -EPERM;
> > }
> >
> > /**
> > @@ -435,7 +437,8 @@ int evm_inode_setattr(struct dentry *dentry,
> > struct iattr *attr) return 0;
> > evm_status = evm_verify_current_integrity(dentry);
> > if ((evm_status == INTEGRITY_PASS) ||
> > - (evm_status == INTEGRITY_NOXATTRS))
> > + (evm_status == INTEGRITY_NOXATTRS) ||
> > + (evm_status == INTEGRITY_UNKNOWN))
> > return 0;
> > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata",
> >
>
> Since this change is limited to setattr, perhaps it would be simpler
> to test the i_opflags directly, without modifying evm_protect_xattr().
In case of set/remove xattr (evm_inode_setxattr(),
evm_inode_removexattr()), evm should not interact fs module work, that
will provide proper error code.
As I see in __vfs_setxattr_noperm(), error code could be -EOPNOTSUPP
or -EIO, but evm will override it by error code -EPERM. I think, this is
wrong. If we don't have xattr support, let fs module handle the error
code.
--
Best regards,
Mikhail Kurinnoi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] evm: allow metadata changes for inode without xattr support
2017-11-03 17:06 ` Mikhail Kurinnoi
@ 2017-11-03 17:15 ` Mimi Zohar
2017-11-03 18:11 ` Mikhail Kurinnoi
0 siblings, 1 reply; 7+ messages in thread
From: Mimi Zohar @ 2017-11-03 17:15 UTC (permalink / raw)
To: Mikhail Kurinnoi; +Cc: linux-integrity
On Fri, 2017-11-03 at 20:06 +0300, Mikhail Kurinnoi wrote:
> ? Fri, 03 Nov 2017 12:54:08 -0400
> Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
>
> > On Fri, 2017-11-03 at 10:26 +0300, Mikhail Kurinnoi wrote:
> > > This patch provide changes in order to allow metadata changes for
> > > inode without xattr support.
> > >
> > >
> > > Signed-off-by: Mikhail Kurinnoi <viewizard@viewizard.com>
> > >
> > > security/integrity/evm/evm_main.c | 21 ++++++++++++---------
> > > 1 file changed, 12 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/security/integrity/evm/evm_main.c
> > > b/security/integrity/evm/evm_main.c index
> > > 9826c02e2db8..51151c43433d 100644 ---
> > > a/security/integrity/evm/evm_main.c +++
> > > b/security/integrity/evm/evm_main.c @@ -294,8 +294,7 @@ static int
> > > evm_protect_xattr(struct dentry *dentry, const char *xattr_name, if
> > > (!posix_xattr_acl(xattr_name)) return 0;
> > > evm_status = evm_verify_current_integrity(dentry);
> > > - if ((evm_status == INTEGRITY_PASS) ||
> > > - (evm_status == INTEGRITY_NOXATTRS))
> > > + if (evm_status == INTEGRITY_NOXATTRS)
> > > return 0;
> > > goto out;
> > > }
> > > @@ -319,12 +318,15 @@ static int evm_protect_xattr(struct dentry
> > > *dentry, const char *xattr_name, -EPERM, 0);
> > > }
> > > out:
> > > - if (evm_status != INTEGRITY_PASS)
> > > - 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;
> > > + if ((evm_status == INTEGRITY_PASS) ||
> > > + (evm_status == INTEGRITY_UNKNOWN))
> > > + return 0;
> > > +
> > > + integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > d_backing_inode(dentry),
> > > + dentry->d_name.name,
> > > "appraise_metadata",
> > > + integrity_status_msg[evm_status],
> > > + -EPERM, 0);
> > > + return -EPERM;
> > > }
> > >
> > > /**
> > > @@ -435,7 +437,8 @@ int evm_inode_setattr(struct dentry *dentry,
> > > struct iattr *attr) return 0;
> > > evm_status = evm_verify_current_integrity(dentry);
> > > if ((evm_status == INTEGRITY_PASS) ||
> > > - (evm_status == INTEGRITY_NOXATTRS))
> > > + (evm_status == INTEGRITY_NOXATTRS) ||
> > > + (evm_status == INTEGRITY_UNKNOWN))
> > > return 0;
> > > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata",
> > >
> >
> > Since this change is limited to setattr, perhaps it would be simpler
> > to test the i_opflags directly, without modifying evm_protect_xattr().
>
> In case of set/remove xattr (evm_inode_setxattr(),
> evm_inode_removexattr()), evm should not interact fs module work, that
> will provide proper error code.
> As I see in __vfs_setxattr_noperm(), error code could be -EOPNOTSUPP
> or -EIO, but evm will override it by error code -EPERM. I think, this is
> wrong. If we don't have xattr support, let fs module handle the error
> code.
The patch description described a specific reason for the change. If
there is another reason for the change, then either include it in the
patch description or provide a separate patch.
Mimi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] evm: allow metadata changes for inode without xattr support
2017-11-03 17:15 ` Mimi Zohar
@ 2017-11-03 18:11 ` Mikhail Kurinnoi
2017-11-03 18:26 ` Mimi Zohar
0 siblings, 1 reply; 7+ messages in thread
From: Mikhail Kurinnoi @ 2017-11-03 18:11 UTC (permalink / raw)
To: Mimi Zohar; +Cc: linux-integrity
? Fri, 03 Nov 2017 13:15:31 -0400
Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
> On Fri, 2017-11-03 at 20:06 +0300, Mikhail Kurinnoi wrote:
> > ? Fri, 03 Nov 2017 12:54:08 -0400
> > Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
> >
> > > On Fri, 2017-11-03 at 10:26 +0300, Mikhail Kurinnoi wrote:
> > > > This patch provide changes in order to allow metadata changes
> > > > for inode without xattr support.
> > > >
> > > >
> > > > Signed-off-by: Mikhail Kurinnoi <viewizard@viewizard.com>
> > > >
> > > > security/integrity/evm/evm_main.c | 21 ++++++++++++---------
> > > > 1 file changed, 12 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/security/integrity/evm/evm_main.c
> > > > b/security/integrity/evm/evm_main.c index
> > > > 9826c02e2db8..51151c43433d 100644 ---
> > > > a/security/integrity/evm/evm_main.c +++
> > > > b/security/integrity/evm/evm_main.c @@ -294,8 +294,7 @@ static
> > > > int evm_protect_xattr(struct dentry *dentry, const char
> > > > *xattr_name, if (!posix_xattr_acl(xattr_name)) return 0;
> > > > evm_status =
> > > > evm_verify_current_integrity(dentry);
> > > > - if ((evm_status == INTEGRITY_PASS) ||
> > > > - (evm_status == INTEGRITY_NOXATTRS))
> > > > + if (evm_status == INTEGRITY_NOXATTRS)
> > > > return 0;
> > > > goto out;
> > > > }
> > > > @@ -319,12 +318,15 @@ static int evm_protect_xattr(struct dentry
> > > > *dentry, const char *xattr_name, -EPERM, 0);
> > > > }
> > > > out:
> > > > - if (evm_status != INTEGRITY_PASS)
> > > > - 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;
> > > > + if ((evm_status == INTEGRITY_PASS) ||
> > > > + (evm_status == INTEGRITY_UNKNOWN))
> > > > + return 0;
> > > > +
> > > > + integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > > d_backing_inode(dentry),
> > > > + dentry->d_name.name,
> > > > "appraise_metadata",
> > > > + integrity_status_msg[evm_status],
> > > > + -EPERM, 0);
> > > > + return -EPERM;
> > > > }
> > > >
> > > > /**
> > > > @@ -435,7 +437,8 @@ int evm_inode_setattr(struct dentry *dentry,
> > > > struct iattr *attr) return 0;
> > > > evm_status = evm_verify_current_integrity(dentry);
> > > > if ((evm_status == INTEGRITY_PASS) ||
> > > > - (evm_status == INTEGRITY_NOXATTRS))
> > > > + (evm_status == INTEGRITY_NOXATTRS) ||
> > > > + (evm_status == INTEGRITY_UNKNOWN))
> > > > return 0;
> > > > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > > d_backing_inode(dentry), dentry->d_name.name,
> > > > "appraise_metadata",
> > >
> > > Since this change is limited to setattr, perhaps it would be
> > > simpler to test the i_opflags directly, without modifying
> > > evm_protect_xattr().
> >
> > In case of set/remove xattr (evm_inode_setxattr(),
> > evm_inode_removexattr()), evm should not interact fs module work,
> > that will provide proper error code.
> > As I see in __vfs_setxattr_noperm(), error code could be -EOPNOTSUPP
> > or -EIO, but evm will override it by error code -EPERM. I think,
> > this is wrong. If we don't have xattr support, let fs module handle
> > the error code.
>
> The patch description described a specific reason for the change. If
> there is another reason for the change, then either include it in the
> patch description or provide a separate patch.
You are right, this is really poor description that don't describe
evm_protect_xattr() changes. I will provide patch v2 with extended
patch description.
Mimi, is it appropriate changes for evm_inode_setattr(), or should I
correct patch to test the i_opflags directly instead?
--
Best regards,
Mikhail Kurinnoi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] evm: allow metadata changes for inode without xattr support
2017-11-03 18:11 ` Mikhail Kurinnoi
@ 2017-11-03 18:26 ` Mimi Zohar
2017-11-03 19:00 ` Mikhail Kurinnoi
0 siblings, 1 reply; 7+ messages in thread
From: Mimi Zohar @ 2017-11-03 18:26 UTC (permalink / raw)
To: Mikhail Kurinnoi; +Cc: linux-integrity
On Fri, 2017-11-03 at 21:11 +0300, Mikhail Kurinnoi wrote:
> ? Fri, 03 Nov 2017 13:15:31 -0400
> Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
>
> > On Fri, 2017-11-03 at 20:06 +0300, Mikhail Kurinnoi wrote:
> > > ? Fri, 03 Nov 2017 12:54:08 -0400
> > > Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
> > >
> > > > On Fri, 2017-11-03 at 10:26 +0300, Mikhail Kurinnoi wrote:
> > > > > This patch provide changes in order to allow metadata changes
> > > > > for inode without xattr support.
> > > > >
> > > > >
> > > > > Signed-off-by: Mikhail Kurinnoi <viewizard@viewizard.com>
> > > > >
> > > > > security/integrity/evm/evm_main.c | 21 ++++++++++++---------
> > > > > 1 file changed, 12 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/security/integrity/evm/evm_main.c
> > > > > b/security/integrity/evm/evm_main.c index
> > > > > 9826c02e2db8..51151c43433d 100644 ---
> > > > > a/security/integrity/evm/evm_main.c +++
> > > > > b/security/integrity/evm/evm_main.c @@ -294,8 +294,7 @@ static
> > > > > int evm_protect_xattr(struct dentry *dentry, const char
> > > > > *xattr_name, if (!posix_xattr_acl(xattr_name)) return 0;
> > > > > evm_status =
> > > > > evm_verify_current_integrity(dentry);
> > > > > - if ((evm_status == INTEGRITY_PASS) ||
> > > > > - (evm_status == INTEGRITY_NOXATTRS))
> > > > > + if (evm_status == INTEGRITY_NOXATTRS)
> > > > > return 0;
> > > > > goto out;
> > > > > }
> > > > > @@ -319,12 +318,15 @@ static int evm_protect_xattr(struct dentry
> > > > > *dentry, const char *xattr_name, -EPERM, 0);
> > > > > }
> > > > > out:
> > > > > - if (evm_status != INTEGRITY_PASS)
> > > > > - 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;
> > > > > + if ((evm_status == INTEGRITY_PASS) ||
> > > > > + (evm_status == INTEGRITY_UNKNOWN))
> > > > > + return 0;
> > > > > +
> > > > > + integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > > > d_backing_inode(dentry),
> > > > > + dentry->d_name.name,
> > > > > "appraise_metadata",
> > > > > + integrity_status_msg[evm_status],
> > > > > + -EPERM, 0);
> > > > > + return -EPERM;
> > > > > }
> > > > >
> > > > > /**
> > > > > @@ -435,7 +437,8 @@ int evm_inode_setattr(struct dentry *dentry,
> > > > > struct iattr *attr) return 0;
> > > > > evm_status = evm_verify_current_integrity(dentry);
> > > > > if ((evm_status == INTEGRITY_PASS) ||
> > > > > - (evm_status == INTEGRITY_NOXATTRS))
> > > > > + (evm_status == INTEGRITY_NOXATTRS) ||
> > > > > + (evm_status == INTEGRITY_UNKNOWN))
> > > > > return 0;
> > > > > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > > > d_backing_inode(dentry), dentry->d_name.name,
> > > > > "appraise_metadata",
> > > >
> > > > Since this change is limited to setattr, perhaps it would be
> > > > simpler to test the i_opflags directly, without modifying
> > > > evm_protect_xattr().
> > >
> > > In case of set/remove xattr (evm_inode_setxattr(),
> > > evm_inode_removexattr()), evm should not interact fs module work,
> > > that will provide proper error code.
> > > As I see in __vfs_setxattr_noperm(), error code could be -EOPNOTSUPP
> > > or -EIO, but evm will override it by error code -EPERM. I think,
> > > this is wrong. If we don't have xattr support, let fs module handle
> > > the error code.
> >
> > The patch description described a specific reason for the change. If
> > there is another reason for the change, then either include it in the
> > patch description or provide a separate patch.
>
> You are right, this is really poor description that don't describe
> evm_protect_xattr() changes. I will provide patch v2 with extended
> patch description.
>
> Mimi, is it appropriate changes for evm_inode_setattr(), or should I
> correct patch to test the i_opflags directly instead?
I could be wrong, but I think these are two separate issues and should
be addressed separately. From your explanation, it sounds like you
want to return the real setxattr/removexattr failure status. Perhaps
after making this change, I'll think differently.
Mimi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] evm: allow metadata changes for inode without xattr support
2017-11-03 18:26 ` Mimi Zohar
@ 2017-11-03 19:00 ` Mikhail Kurinnoi
0 siblings, 0 replies; 7+ messages in thread
From: Mikhail Kurinnoi @ 2017-11-03 19:00 UTC (permalink / raw)
To: Mimi Zohar; +Cc: linux-integrity
? Fri, 03 Nov 2017 14:26:18 -0400
Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
> On Fri, 2017-11-03 at 21:11 +0300, Mikhail Kurinnoi wrote:
> > ? Fri, 03 Nov 2017 13:15:31 -0400
> > Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
> >
> > > On Fri, 2017-11-03 at 20:06 +0300, Mikhail Kurinnoi wrote:
> > > > ? Fri, 03 Nov 2017 12:54:08 -0400
> > > > Mimi Zohar <zohar@linux.vnet.ibm.com> ?????:
> > > >
> > > > > On Fri, 2017-11-03 at 10:26 +0300, Mikhail Kurinnoi wrote:
> > > > > > This patch provide changes in order to allow metadata
> > > > > > changes for inode without xattr support.
> > > > > >
> > > > > >
> > > > > > Signed-off-by: Mikhail Kurinnoi <viewizard@viewizard.com>
> > > > > >
> > > > > > security/integrity/evm/evm_main.c | 21
> > > > > > ++++++++++++--------- 1 file changed, 12 insertions(+), 9
> > > > > > deletions(-)
> > > > > >
> > > > > > diff --git a/security/integrity/evm/evm_main.c
> > > > > > b/security/integrity/evm/evm_main.c index
> > > > > > 9826c02e2db8..51151c43433d 100644 ---
> > > > > > a/security/integrity/evm/evm_main.c +++
> > > > > > b/security/integrity/evm/evm_main.c @@ -294,8 +294,7 @@
> > > > > > static int evm_protect_xattr(struct dentry *dentry, const
> > > > > > char *xattr_name, if (!posix_xattr_acl(xattr_name)) return
> > > > > > 0; evm_status =
> > > > > > evm_verify_current_integrity(dentry);
> > > > > > - if ((evm_status == INTEGRITY_PASS) ||
> > > > > > - (evm_status == INTEGRITY_NOXATTRS))
> > > > > > + if (evm_status == INTEGRITY_NOXATTRS)
> > > > > > return 0;
> > > > > > goto out;
> > > > > > }
> > > > > > @@ -319,12 +318,15 @@ static int evm_protect_xattr(struct
> > > > > > dentry *dentry, const char *xattr_name, -EPERM, 0);
> > > > > > }
> > > > > > out:
> > > > > > - if (evm_status != INTEGRITY_PASS)
> > > > > > -
> > > > > > 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;
> > > > > > + if ((evm_status == INTEGRITY_PASS) ||
> > > > > > + (evm_status == INTEGRITY_UNKNOWN))
> > > > > > + return 0;
> > > > > > +
> > > > > > + integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > > > > d_backing_inode(dentry),
> > > > > > + dentry->d_name.name,
> > > > > > "appraise_metadata",
> > > > > > +
> > > > > > integrity_status_msg[evm_status],
> > > > > > + -EPERM, 0);
> > > > > > + return -EPERM;
> > > > > > }
> > > > > >
> > > > > > /**
> > > > > > @@ -435,7 +437,8 @@ int evm_inode_setattr(struct dentry
> > > > > > *dentry, struct iattr *attr) return 0;
> > > > > > evm_status = evm_verify_current_integrity(dentry);
> > > > > > if ((evm_status == INTEGRITY_PASS) ||
> > > > > > - (evm_status == INTEGRITY_NOXATTRS))
> > > > > > + (evm_status == INTEGRITY_NOXATTRS) ||
> > > > > > + (evm_status == INTEGRITY_UNKNOWN))
> > > > > > return 0;
> > > > > > integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > > > > d_backing_inode(dentry), dentry->d_name.name,
> > > > > > "appraise_metadata",
> > > > >
> > > > > Since this change is limited to setattr, perhaps it would be
> > > > > simpler to test the i_opflags directly, without modifying
> > > > > evm_protect_xattr().
> > > >
> > > > In case of set/remove xattr (evm_inode_setxattr(),
> > > > evm_inode_removexattr()), evm should not interact fs module
> > > > work, that will provide proper error code.
> > > > As I see in __vfs_setxattr_noperm(), error code could be
> > > > -EOPNOTSUPP or -EIO, but evm will override it by error code
> > > > -EPERM. I think, this is wrong. If we don't have xattr support,
> > > > let fs module handle the error code.
> > >
> > > The patch description described a specific reason for the change.
> > > If there is another reason for the change, then either include
> > > it in the patch description or provide a separate patch.
> >
> > You are right, this is really poor description that don't describe
> > evm_protect_xattr() changes. I will provide patch v2 with extended
> > patch description.
> >
> > Mimi, is it appropriate changes for evm_inode_setattr(), or should I
> > correct patch to test the i_opflags directly instead?
>
> I could be wrong, but I think these are two separate issues and should
> be addressed separately.
Ok.
> From your explanation, it sounds like you
> want to return the real setxattr/removexattr failure status.
Yes. That was the idea of evm_protect_xattr() changes.
I think, in this way we highlight the real point of
issue (why we can't set/remove xattr).
--
Best regards,
Mikhail Kurinnoi
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-03 19:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-03 7:26 [PATCH] evm: allow metadata changes for inode without xattr support Mikhail Kurinnoi
2017-11-03 16:54 ` Mimi Zohar
2017-11-03 17:06 ` Mikhail Kurinnoi
2017-11-03 17:15 ` Mimi Zohar
2017-11-03 18:11 ` Mikhail Kurinnoi
2017-11-03 18:26 ` Mimi Zohar
2017-11-03 19:00 ` Mikhail Kurinnoi
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.