From patchwork Wed Aug 2 17:17:05 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thiago Jung Bauermann X-Patchwork-Id: 9877235 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id AAE6660360 for ; Wed, 2 Aug 2017 17:17:44 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8F4E928803 for ; Wed, 2 Aug 2017 17:17:44 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 83DF02880F; Wed, 2 Aug 2017 17:17:44 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EAE5628803 for ; Wed, 2 Aug 2017 17:17:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752958AbdHBRR3 (ORCPT ); Wed, 2 Aug 2017 13:17:29 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:58428 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751923AbdHBRRZ (ORCPT ); Wed, 2 Aug 2017 13:17:25 -0400 Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v72HE8Fw099074 for ; Wed, 2 Aug 2017 13:17:24 -0400 Received: from e17.ny.us.ibm.com (e17.ny.us.ibm.com [129.33.205.207]) by mx0a-001b2d01.pphosted.com with ESMTP id 2c3g8hgbg4-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 02 Aug 2017 13:17:24 -0400 Received: from localhost by e17.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 2 Aug 2017 13:17:23 -0400 Received: from b01cxnp23034.gho.pok.ibm.com (9.57.198.29) by e17.ny.us.ibm.com (146.89.104.204) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 2 Aug 2017 13:17:20 -0400 Received: from d24av05.br.ibm.com (d24av05.br.ibm.com [9.18.232.44]) by b01cxnp23034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v72HHJTS22282294; Wed, 2 Aug 2017 17:17:19 GMT Received: from d24av05.br.ibm.com (localhost [127.0.0.1]) by d24av05.br.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id v72EHHSV012288; Wed, 2 Aug 2017 11:17:19 -0300 Received: from morokweng ([9.85.207.249]) by d24av05.br.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id v72EH7in012272 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 2 Aug 2017 11:17:10 -0300 References: <20170706221753.17380-1-bauerman@linux.vnet.ibm.com> <20170706221753.17380-2-bauerman@linux.vnet.ibm.com> <1501245566.10288.35.camel@linux.vnet.ibm.com> From: Thiago Jung Bauermann To: Mimi Zohar Cc: linux-security-module@vger.kernel.org, linux-ima-devel@lists.sourceforge.net, keyrings@vger.kernel.org, linux-crypto@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Dmitry Kasatkin , James Morris , "Serge E. Hallyn" , David Howells , David Woodhouse , Jessica Yu , Rusty Russell , Herbert Xu , "David S. Miller" , "AKASHI\, Takahiro" Subject: Re: [PATCH v3 1/7] integrity: Introduce struct evm_hmac_xattr In-reply-to: <1501245566.10288.35.camel@linux.vnet.ibm.com> Date: Wed, 02 Aug 2017 14:17:05 -0300 MIME-Version: 1.0 X-TM-AS-MML: disable x-cbid: 17080217-0040-0000-0000-000003897EE7 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007472; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000216; SDB=6.00896569; UDB=6.00448525; IPR=6.00676756; BA=6.00005506; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016500; XFM=3.00000015; UTC=2017-08-02 17:17:22 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17080217-0041-0000-0000-0000077DA9C2 Message-Id: <87h8xpyjn2.fsf@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-08-02_09:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000 definitions=main-1708020278 Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP Hello Mimi, Thanks for your review! The patch at the end of the email implements your suggestions, what do you think? Mimi Zohar writes: > On Thu, 2017-07-06 at 19:17 -0300, Thiago Jung Bauermann wrote: >> A separate struct evm_hmac_xattr is introduced, with the original >> definition of evm_ima_xattr_data to be used in the places that actually >> expect that definition. > > The new structure name implies that the xattr can only be an HMAC. By > moving the new structure to evm.h we also loose the connection that it > has to theevm_ima_xattr_type enumeration. Ok, I renamed it to struct evm_xattr. > Instead, how about defining the new struct in terms of the modified > evm_ima_xattr_data struct? IMHO IMA and EVM's practice of mixing and matching structs to compose its data structures makes the code somewhat confusing and harder to see where the actual storage used by a given field is actually allocated. But I don't feel strongly about it, so the patch at the end of this emails defines it as: struct evm_xattr { struct evm_ima_xattr_data data; u8 digest[SHA1_DIGEST_SIZE]; } __packed; Another disadvantage of the above is that you have two fields pointing to the same place: evm_xattr.data.data and evm_xattr.digest. > Please leave the new structure in integrity.h. I think that moving the structure in evm.h is helpful. It immediately conveys that nothing outside of security/integrity/evm/ interprets the xattr data in the way defined by struct evm_xattr. But I also don't feel strongly about it, so I moved it to integrity.h. diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index d7f282d75cc1..d902a18ef66f 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -252,13 +252,13 @@ int 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; + struct evm_xattr xattr_data; int rc = 0; rc = evm_calc_hmac(dentry, xattr_name, xattr_value, xattr_value_len, xattr_data.digest); if (rc == 0) { - xattr_data.type = EVM_XATTR_HMAC; + xattr_data.data.type = EVM_XATTR_HMAC; rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM, &xattr_data, sizeof(xattr_data), 0); diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 063d38aef64e..536694499515 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -116,7 +116,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, struct integrity_iint_cache *iint) { struct evm_ima_xattr_data *xattr_data = NULL; - struct evm_ima_xattr_data calc; + struct evm_xattr calc; enum integrity_status evm_status = INTEGRITY_PASS; int rc, xattr_len; @@ -147,7 +147,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, /* check value type */ switch (xattr_data->type) { case EVM_XATTR_HMAC: - if (xattr_len != sizeof(struct evm_ima_xattr_data)) { + if (xattr_len != sizeof(struct evm_xattr)) { evm_status = INTEGRITY_FAIL; goto out; } @@ -155,7 +155,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, xattr_value_len, calc.digest); if (rc) break; - rc = crypto_memneq(xattr_data->digest, calc.digest, + rc = crypto_memneq(xattr_data->data, calc.digest, sizeof(calc.digest)); if (rc) rc = -EINVAL; @@ -467,7 +467,7 @@ int evm_inode_init_security(struct inode *inode, const struct xattr *lsm_xattr, struct xattr *evm_xattr) { - struct evm_ima_xattr_data *xattr_data; + struct evm_xattr *xattr_data; int rc; if (!evm_initialized || !evm_protected_xattr(lsm_xattr->name)) @@ -477,7 +477,7 @@ int evm_inode_init_security(struct inode *inode, if (!xattr_data) return -ENOMEM; - xattr_data->type = EVM_XATTR_HMAC; + xattr_data->data.type = EVM_XATTR_HMAC; rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest); if (rc < 0) goto out; diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 809ba70fbbbf..87d2b601cf8e 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -156,7 +156,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, return sig->hash_algo; break; case IMA_XATTR_DIGEST_NG: - ret = xattr_value->digest[0]; + /* first byte contains algorithm id */ + ret = xattr_value->data[0]; if (ret < HASH_ALGO__LAST) return ret; break; @@ -164,7 +165,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, /* this is for backward compatibility */ if (xattr_len == 21) { unsigned int zero = 0; - if (!memcmp(&xattr_value->digest[16], &zero, 4)) + if (!memcmp(&xattr_value->data[16], &zero, 4)) return HASH_ALGO_MD5; else return HASH_ALGO_SHA1; @@ -253,7 +254,7 @@ int ima_appraise_measurement(enum ima_hooks func, /* xattr length may be longer. md5 hash in previous version occupied 20 bytes in xattr, instead of 16 */ - rc = memcmp(&xattr_value->digest[hash_start], + rc = memcmp(&xattr_value->data[hash_start], iint->ima_hash->digest, iint->ima_hash->length); else diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index a53e7e4ab06c..9b1762076f38 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -63,6 +63,11 @@ enum evm_ima_xattr_type { struct evm_ima_xattr_data { u8 type; + u8 data[]; +} __packed; + +struct evm_xattr { + struct evm_ima_xattr_data data; u8 digest[SHA1_DIGEST_SIZE]; } __packed;