lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 25 May 2022 09:50:16 +0800 From: "Guozihua (Scott)" <guozihua@...wei.com> To: "Gustavo A. R. Silva" <gustavoars@...nel.org> CC: <linux-integrity@...r.kernel.org>, <zohar@...ux.ibm.com>, <dmitry.kasatkin@...il.com>, <linux-hardening@...r.kernel.org> Subject: Re: [RESEND] evm: Refector struct evm_xattr On 2022/5/18 13:13, Gustavo A. R. Silva wrote: > On Wed, May 18, 2022 at 09:14:39AM +0800, GUO Zihua wrote: >> struct evm_xattr is only used for EVM_XATTR_HMAC type evm digest and is >> glued together one flexible array and one fixed length array. The >> original intention seems to be shortening the code as the "data" field >> would always be a SHA1 digest. >> >> This implementation is not complying with GCC's specification about >> flexible array and spars yield the following warning: > > The sparse warning has nothing to do with any GCC specification. It's > perfectly fine to apply the sizeof operator to a struct-with-flex-array. > However, it might be suspicious if the intention is to also get the > _dynamic_ size of the flexible array, because in that case the size of > the flex array is always zero. See the example below: > > struct foo { > uint8_t len; > struct boo data[]; > }; > > sizeof(struct foo) == 1 > > Also, you sent this patch twice in the last 24 hours. Give the maintainers > time to review your patch (usually a couple of weeks) before resending it. > >> >> security/integrity/evm/evm_main.c:852:30: warning: using sizeof on a flexible >> structure >> security/integrity/evm/evm_main.c:862:32: warning: using sizeof on a flexible >> structure > > Regarding the warnings above, please take a look at my response to your > other patch (the same applies in this case). :) > > Thanks > -- > Gustavo > >> >> Fix it by: >> 1. Remove struct evm_xattr and use struct evm_ima_xattr_data directly. >> 2. Get array size with struct_size instead of sizeof. >> >> Reference: https://github.com/KSPP/linux/issues/174 >> >> Fixes: 6be5cc5246f80 ("evm: add support for different security.evm data types") >> Signed-off-by: GUO Zihua <guozihua@...wei.com> >> --- >> security/integrity/evm/evm_main.c | 14 ++++++++------ >> security/integrity/integrity.h | 6 ------ >> 2 files changed, 8 insertions(+), 12 deletions(-) >> >> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c >> index 7d87772f0ce6..f2c4501a287a 100644 >> --- a/security/integrity/evm/evm_main.c >> +++ b/security/integrity/evm/evm_main.c >> @@ -211,7 +211,8 @@ 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_xattr)) { >> + if (xattr_len != struct_size(*xattr_data, data, >> + SHA1_DIGEST_SIZE)) { >> evm_status = INTEGRITY_FAIL; >> goto out; >> } >> @@ -842,24 +843,25 @@ int evm_inode_init_security(struct inode *inode, >> const struct xattr *lsm_xattr, >> struct xattr *evm_xattr) >> { >> - struct evm_xattr *xattr_data; >> + struct evm_ima_xattr_data *xattr_data; >> int rc; >> >> if (!(evm_initialized & EVM_INIT_HMAC) || >> !evm_protected_xattr(lsm_xattr->name)) >> return 0; >> >> - xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS); >> + xattr_data = kzalloc(struct_size(*xattr_data, data, >> + SHA1_DIGEST_SIZE), GFP_NOFS); >> if (!xattr_data) >> return -ENOMEM; >> >> - xattr_data->data.type = EVM_XATTR_HMAC; >> - rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest); >> + xattr_data->type = EVM_XATTR_HMAC; >> + rc = evm_init_hmac(inode, lsm_xattr, xattr_data->data); >> if (rc < 0) >> goto out; >> >> evm_xattr->value = xattr_data; >> - evm_xattr->value_len = sizeof(*xattr_data); >> + evm_xattr->value_len = struct_size(*xattr_data, data, SHA1_DIGEST_SIZE); >> evm_xattr->name = XATTR_EVM_SUFFIX; >> return 0; >> out: >> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h >> index 3510e413ea17..91b16d620dd9 100644 >> --- a/security/integrity/integrity.h >> +++ b/security/integrity/integrity.h >> @@ -86,12 +86,6 @@ struct evm_ima_xattr_data { >> u8 data[]; >> } __packed; >> >> -/* Only used in the EVM HMAC code. */ >> -struct evm_xattr { >> - struct evm_ima_xattr_data data; >> - u8 digest[SHA1_DIGEST_SIZE]; >> -} __packed; >> - >> #define IMA_MAX_DIGEST_SIZE 64 >> >> struct ima_digest_data { >> -- >> 2.36.0 >> > . Thanks Gustavo! I'm still pretty new to the community so your feedback is much appreciated! -- Best GUO Zihua
Powered by blists - more mailing lists