| Message ID | 20190617115838.2397872-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: dynamically allocate shash_desc | expand |
On Mon, 2019-06-17 at 13:20 +0200, Arnd Bergmann wrote: > On 32-bit ARM, we get a warning about excessive stack usage when > building with clang. > > security/integrity/ima/ima_crypto.c:504:5: error: stack frame size > of 1152 bytes in function 'ima_calc_field_array_hash' [-Werror,- > Wframe-larger-than=] I'm definitely not seeing this. Is this problem a result of non upstreamed patches? For sha1, currently the only possible hash algorithm, I'm seeing 664. Mimi > > Using kmalloc to get the descriptor reduces this to 320 bytes. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > security/integrity/ima/ima_crypto.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c > index d4c7b8e1b083..8a66bab4c435 100644 > --- a/security/integrity/ima/ima_crypto.c > +++ b/security/integrity/ima/ima_crypto.c > @@ -461,16 +461,21 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data, > struct ima_digest_data *hash, > struct crypto_shash *tfm) > { > - SHASH_DESC_ON_STACK(shash, tfm); > + struct shash_desc *shash; > int rc, i; > > + shash = kmalloc(sizeof(struct shash_desc) + crypto_shash_descsize(tfm), > + GFP_KERNEL); > + if (!shash) > + return -ENOMEM; > + > shash->tfm = tfm; > > hash->length = crypto_shash_digestsize(tfm); > > rc = crypto_shash_init(shash); > if (rc != 0) > - return rc; > + goto out; > > for (i = 0; i < num_fields; i++) { > u8 buffer[IMA_EVENT_NAME_LEN_MAX + 1] = { 0 }; > @@ -497,7 +502,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data, > > if (!rc) > rc = crypto_shash_final(shash, hash->digest); > - > +out: > + kfree(shash); > return rc; > } >
On Mon, 2019-06-17 at 11:55 -0400, Mimi Zohar wrote: > On Mon, 2019-06-17 at 13:20 +0200, Arnd Bergmann wrote: > > On 32-bit ARM, we get a warning about excessive stack usage when > > building with clang. > > > > security/integrity/ima/ima_crypto.c:504:5: error: stack frame size > > of 1152 bytes in function 'ima_calc_field_array_hash' [-Werror,- > > Wframe-larger-than=] > > I'm definitely not seeing this. Is this problem a result of non > upstreamed patches? For sha1, currently the only possible hash > algorithm, I'm seeing 664. Every time a measurement is added to the measurement list, the memory would be allocated/freed. The frequency of new measurements is policy dependent. For performance reasons, I'd prefer if the allocation remains on the stack. Mimi
On Mon, Jun 17, 2019 at 8:08 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Mon, 2019-06-17 at 11:55 -0400, Mimi Zohar wrote: > > On Mon, 2019-06-17 at 13:20 +0200, Arnd Bergmann wrote: > > > On 32-bit ARM, we get a warning about excessive stack usage when > > > building with clang. > > > > > > security/integrity/ima/ima_crypto.c:504:5: error: stack frame size > > > of 1152 bytes in function 'ima_calc_field_array_hash' [-Werror,- > > > Wframe-larger-than=] > > > > I'm definitely not seeing this. Is this problem a result of non > > upstreamed patches? For sha1, currently the only possible hash > > algorithm, I'm seeing 664. You won't see it with gcc, only with clang in some randconfig builds, I suppose only when KASAN is enabled. > Every time a measurement is added to the measurement list, the memory > would be allocated/freed. The frequency of new measurements is policy > dependent. For performance reasons, I'd prefer if the allocation > remains on the stack. Is there a way to preallocate the shash_desc instead? That would avoid the overhead. Arnd
On Mon, 2019-06-17 at 22:08 +0200, Arnd Bergmann wrote: > On Mon, Jun 17, 2019 at 8:08 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Mon, 2019-06-17 at 11:55 -0400, Mimi Zohar wrote: > > > On Mon, 2019-06-17 at 13:20 +0200, Arnd Bergmann wrote: > > > > On 32-bit ARM, we get a warning about excessive stack usage when > > > > building with clang. > > > > > > > > security/integrity/ima/ima_crypto.c:504:5: error: stack frame size > > > > of 1152 bytes in function 'ima_calc_field_array_hash' [-Werror,- > > > > Wframe-larger-than=] > > > > > > I'm definitely not seeing this. Is this problem a result of non > > > upstreamed patches? For sha1, currently the only possible hash > > > algorithm, I'm seeing 664. > > You won't see it with gcc, only with clang in some randconfig builds, > I suppose only when KASAN is enabled. > > > Every time a measurement is added to the measurement list, the memory > > would be allocated/freed. The frequency of new measurements is policy > > dependent. For performance reasons, I'd prefer if the allocation > > remains on the stack. > > Is there a way to preallocate the shash_desc instead? That would > avoid the overhead. There are 3 other SHASH_DESC_ON_STACK definitions in just ima_crypto.c, with a total of ~55 other places in the kernel. Before fixing this particular function, I'd like to know if the "excessive stack usage" warning is limited to ima_calc_field_array_hash_tfm(). If so, what is so special about its usage of SHASH_DESC_ON_STACK? thanks, Mimi
On Tue, Jun 18, 2019 at 3:55 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Mon, 2019-06-17 at 22:08 +0200, Arnd Bergmann wrote: > > On Mon, Jun 17, 2019 at 8:08 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > On Mon, 2019-06-17 at 11:55 -0400, Mimi Zohar wrote: > > > > On Mon, 2019-06-17 at 13:20 +0200, Arnd Bergmann wrote: > > > > > On 32-bit ARM, we get a warning about excessive stack usage when > > > > > building with clang. > > > > > > > > > > security/integrity/ima/ima_crypto.c:504:5: error: stack frame size > > > > > of 1152 bytes in function 'ima_calc_field_array_hash' [-Werror,- > > > > > Wframe-larger-than=] > > > > > > > > I'm definitely not seeing this. Is this problem a result of non > > > > upstreamed patches? For sha1, currently the only possible hash > > > > algorithm, I'm seeing 664. > > > > You won't see it with gcc, only with clang in some randconfig builds, > > I suppose only when KASAN is enabled. > > > > > Every time a measurement is added to the measurement list, the memory > > > would be allocated/freed. The frequency of new measurements is policy > > > dependent. For performance reasons, I'd prefer if the allocation > > > remains on the stack. > > > > Is there a way to preallocate the shash_desc instead? That would > > avoid the overhead. > > There are 3 other SHASH_DESC_ON_STACK definitions in just > ima_crypto.c, with a total of ~55 other places in the kernel. Before > fixing this particular function, I'd like to know if the "excessive > stack usage" warning is limited to ima_calc_field_array_hash_tfm(). > If so, what is so special about its usage of SHASH_DESC_ON_STACK? SHASH_DESC_ON_STACK() uses at least 512 bytes of stack everywhere, which is half of the warning limit for a function on 32 bit kernels. With KASAN, a small redzone is put around it so we can detect out of bounds access to a variable that is passed by reference. clang makes that buffer larger than gcc, so we end up with something like 768 bytes for each instance of SHASH_DESC_ON_STACK(). Most other users still stay below the 1024 byte warning level though, because typical functions only use a few bytes of stack space. In case of ima_calc_field_array_hash_tfm(), the is also the buffer[] array of 255 bytes that gets another large redzone. I fixed up all the (randconfig) warnings I get for arm32, arm64 and x86 kernels, and I think there were four to five that were because of SHASH_DESC_ON_STACK(). It might make sense to convert all three instances in ima to preallocate the descriptor if we do it for one of them, even when it's not actually needed. Arnd
On Tue, 2019-06-18 at 20:06 +0200, Arnd Bergmann wrote: > On Tue, Jun 18, 2019 at 3:55 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Mon, 2019-06-17 at 22:08 +0200, Arnd Bergmann wrote: > > > On Mon, Jun 17, 2019 at 8:08 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > > > On Mon, 2019-06-17 at 11:55 -0400, Mimi Zohar wrote: > > > > > On Mon, 2019-06-17 at 13:20 +0200, Arnd Bergmann wrote: > > > > > > On 32-bit ARM, we get a warning about excessive stack usage when > > > > > > building with clang. > > > > > > > > > > > > security/integrity/ima/ima_crypto.c:504:5: error: stack frame size > > > > > > of 1152 bytes in function 'ima_calc_field_array_hash' [-Werror,- > > > > > > Wframe-larger-than=] > > > > > > > > > > I'm definitely not seeing this. Is this problem a result of non > > > > > upstreamed patches? For sha1, currently the only possible hash > > > > > algorithm, I'm seeing 664. > > > > > > You won't see it with gcc, only with clang in some randconfig builds, > > > I suppose only when KASAN is enabled. > > > > > > > Every time a measurement is added to the measurement list, the memory > > > > would be allocated/freed. The frequency of new measurements is policy > > > > dependent. For performance reasons, I'd prefer if the allocation > > > > remains on the stack. > > > > > > Is there a way to preallocate the shash_desc instead? That would > > > avoid the overhead. > > > > There are 3 other SHASH_DESC_ON_STACK definitions in just > > ima_crypto.c, with a total of ~55 other places in the kernel. Before > > fixing this particular function, I'd like to know if the "excessive > > stack usage" warning is limited to ima_calc_field_array_hash_tfm(). > > If so, what is so special about its usage of SHASH_DESC_ON_STACK? > > SHASH_DESC_ON_STACK() uses at least 512 bytes of stack > everywhere, which is half of the warning limit for a function on > 32 bit kernels. > > With KASAN, a small redzone is put around it so we can detect out > of bounds access to a variable that is passed by reference. > clang makes that buffer larger than gcc, so we end up with something > like 768 bytes for each instance of SHASH_DESC_ON_STACK(). > > Most other users still stay below the 1024 byte warning level though, > because typical functions only use a few bytes of stack space. > In case of ima_calc_field_array_hash_tfm(), the is also the buffer[] > array of 255 bytes that gets another large redzone. > > I fixed up all the (randconfig) warnings I get for arm32, arm64 and > x86 kernels, and I think there were four to five that were because of > SHASH_DESC_ON_STACK(). It might make sense to convert all > three instances in ima to preallocate the descriptor if we do it for > one of them, even when it's not actually needed. "buffer" is only used for the original "ima" template format, which is limited to sha1. Rather than allocating shash, I would prefer "buffer" be allocated, if needed, and only the first time. Mimi
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index d4c7b8e1b083..8a66bab4c435 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -461,16 +461,21 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data, struct ima_digest_data *hash, struct crypto_shash *tfm) { - SHASH_DESC_ON_STACK(shash, tfm); + struct shash_desc *shash; int rc, i; + shash = kmalloc(sizeof(struct shash_desc) + crypto_shash_descsize(tfm), + GFP_KERNEL); + if (!shash) + return -ENOMEM; + shash->tfm = tfm; hash->length = crypto_shash_digestsize(tfm); rc = crypto_shash_init(shash); if (rc != 0) - return rc; + goto out; for (i = 0; i < num_fields; i++) { u8 buffer[IMA_EVENT_NAME_LEN_MAX + 1] = { 0 }; @@ -497,7 +502,8 @@ static int ima_calc_field_array_hash_tfm(struct ima_field_data *field_data, if (!rc) rc = crypto_shash_final(shash, hash->digest); - +out: + kfree(shash); return rc; }
On 32-bit ARM, we get a warning about excessive stack usage when building with clang. security/integrity/ima/ima_crypto.c:504:5: error: stack frame size of 1152 bytes in function 'ima_calc_field_array_hash' [-Werror,-Wframe-larger-than=] Using kmalloc to get the descriptor reduces this to 320 bytes. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- security/integrity/ima/ima_crypto.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)