[v2,10/18] evm: Turn evm_update_evmxattr into void function
diff mbox

Message ID 1463742875-9836-11-git-send-email-agruenba@redhat.com
State New
Headers show

Commit Message

Andreas Gruenbacher May 20, 2016, 11:14 a.m. UTC
The return value of evm_update_evmxattr is never used.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 security/integrity/evm/evm.h        |  7 +++----
 security/integrity/evm/evm_crypto.c | 14 ++++++--------
 2 files changed, 9 insertions(+), 12 deletions(-)

Comments

James Morris May 25, 2016, 5:30 a.m. UTC | #1
On Fri, 20 May 2016, Andreas Gruenbacher wrote:

> The return value of evm_update_evmxattr is never used.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

As I mentioned last time, the EVM code is silently ignoring errors here, 
and I'd prefer to see that fixed.

Mimi: any comment on this?
Mimi Zohar May 25, 2016, 11:08 a.m. UTC | #2
On Wed, 2016-05-25 at 15:30 +1000, James Morris wrote:
> On Fri, 20 May 2016, Andreas Gruenbacher wrote:
> 
> > The return value of evm_update_evmxattr is never used.
> > 
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> 
> As I mentioned last time, the EVM code is silently ignoring errors here, 
> and I'd prefer to see that fixed.

Agreed.   evm_update_evmxattr() is called as a result of a "protected"
xattr or some other file metadata having been modified.  The two actions
need to remain in sync, otherwise subsequent file access will be denied.
At the point that evm_update_evmxattr() fails, there isn't much that can
be done other than audit the failure.  The file metadata has already
been modified.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index f5f1272..8b1cef07 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -39,10 +39,9 @@  extern struct crypto_shash *hash_tfm;
 extern char *evm_config_xattrnames[];
 
 int evm_init_key(void);
-int evm_update_evmxattr(struct dentry *dentry,
-			const char *req_xattr_name,
-			const char *req_xattr_value,
-			size_t req_xattr_value_len);
+void evm_update_evmxattr(struct dentry *dentry, const char *req_xattr_name,
+			 const char *req_xattr_value,
+			 size_t req_xattr_value_len);
 int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
 		  const char *req_xattr_value,
 		  size_t req_xattr_value_len, char *digest);
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 30b6b7d0..3ac6407 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -239,24 +239,22 @@  int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
  *
  * Expects to be called with i_mutex locked.
  */
-int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
-			const char *xattr_value, size_t xattr_value_len)
+void evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
+			 const char *xattr_value, size_t xattr_value_len)
 {
 	struct inode *inode = d_backing_inode(dentry);
 	struct evm_ima_xattr_data xattr_data;
-	int rc = 0;
+	int rc;
 
 	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
 			   xattr_value_len, xattr_data.digest);
 	if (rc == 0) {
 		xattr_data.type = EVM_XATTR_HMAC;
-		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
-					   &xattr_data,
-					   sizeof(xattr_data), 0);
+		__vfs_setxattr_noperm(dentry, XATTR_NAME_EVM, &xattr_data,
+				      sizeof(xattr_data), 0);
 	} else if (rc == -ENODATA && inode->i_op->removexattr) {
-		rc = inode->i_op->removexattr(dentry, XATTR_NAME_EVM);
+		inode->i_op->removexattr(dentry, XATTR_NAME_EVM);
 	}
-	return rc;
 }
 
 int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,