* [RESEND] evm: Refector struct evm_xattr
@ 2022-05-18 1:14 GUO Zihua
2022-05-18 5:13 ` Gustavo A. R. Silva
0 siblings, 1 reply; 3+ messages in thread
From: GUO Zihua @ 2022-05-18 1:14 UTC (permalink / raw)
To: linux-integrity; +Cc: zohar, dmitry.kasatkin, gustavoars, linux-hardening
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:
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
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@huawei.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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [RESEND] evm: Refector struct evm_xattr
2022-05-18 1:14 [RESEND] evm: Refector struct evm_xattr GUO Zihua
@ 2022-05-18 5:13 ` Gustavo A. R. Silva
2022-05-25 1:50 ` Guozihua (Scott)
0 siblings, 1 reply; 3+ messages in thread
From: Gustavo A. R. Silva @ 2022-05-18 5:13 UTC (permalink / raw)
To: GUO Zihua; +Cc: linux-integrity, zohar, dmitry.kasatkin, linux-hardening
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@huawei.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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [RESEND] evm: Refector struct evm_xattr
2022-05-18 5:13 ` Gustavo A. R. Silva
@ 2022-05-25 1:50 ` Guozihua (Scott)
0 siblings, 0 replies; 3+ messages in thread
From: Guozihua (Scott) @ 2022-05-25 1:50 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: linux-integrity, zohar, dmitry.kasatkin, linux-hardening
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@huawei.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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-25 1:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-18 1:14 [RESEND] evm: Refector struct evm_xattr GUO Zihua
2022-05-18 5:13 ` Gustavo A. R. Silva
2022-05-25 1:50 ` Guozihua (Scott)
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.