| Message ID | 20190617132440.2721536-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drbd: dynamically allocate shash descriptor | expand |
On Mon, Jun 17, 2019 at 03:24:13PM +0200, Arnd Bergmann wrote: > Building with clang and KASAN, we get a warning about an overly large > stack frame on 32-bit architectures: > > drivers/block/drbd/drbd_receiver.c:921:31: error: stack frame size of 1280 bytes in function 'conn_connect' > [-Werror,-Wframe-larger-than=] > > We already allocate other data dynamically in this function, so > just do the same for the shash descriptor, which makes up most of > this memory. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/block/drbd/drbd_receiver.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c > index 90ebfcae0ce6..10fb26e862d7 100644 > --- a/drivers/block/drbd/drbd_receiver.c > +++ b/drivers/block/drbd/drbd_receiver.c > @@ -5417,7 +5417,7 @@ static int drbd_do_auth(struct drbd_connection *connection) > unsigned int key_len; > char secret[SHARED_SECRET_MAX]; /* 64 byte */ > unsigned int resp_size; > - SHASH_DESC_ON_STACK(desc, connection->cram_hmac_tfm); > + struct shash_desc *desc; > struct packet_info pi; > struct net_conf *nc; > int err, rv; > @@ -5430,6 +5430,13 @@ static int drbd_do_auth(struct drbd_connection *connection) > memcpy(secret, nc->shared_secret, key_len); > rcu_read_unlock(); > > + desc = kmalloc(sizeof(struct shash_desc) + > + crypto_shash_descsize(connection->cram_hmac_tfm), > + GFP_KERNEL); > + if (!desc) { > + rv = -1; > + goto fail; > + } > desc->tfm = connection->cram_hmac_tfm; > > rv = crypto_shash_setkey(connection->cram_hmac_tfm, (u8 *)secret, key_len); > @@ -5572,6 +5579,7 @@ static int drbd_do_auth(struct drbd_connection *connection) > kfree(response); > kfree(right_response); > shash_desc_zero(desc); > + kfree(desc); > > return rv; > } Hi Arnd, are you sure your cleanup is okay? > shash_desc_zero(desc); > + kfree(desc); You shash_desc_zero() a potential NULL pointer. memzero_expicit() in the function then dereferences it: memzero_explicit(desc, sizeof(*desc) + crypto_shash_descsize(desc->tfm)); Maybe some if (desc) guard? Best, rck
On Mon, Jun 17, 2019 at 03:24:13PM +0200, Arnd Bergmann wrote: > Building with clang and KASAN, we get a warning about an overly large > stack frame on 32-bit architectures: > > drivers/block/drbd/drbd_receiver.c:921:31: error: stack frame size of 1280 bytes in function 'conn_connect' > [-Werror,-Wframe-larger-than=] > > We already allocate other data dynamically in this function, so > just do the same for the shash descriptor, which makes up most of > this memory. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/block/drbd/drbd_receiver.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) Does this patch fix the warning as well? diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c index 90ebfcae0ce6..ead13a6b3887 100644 --- a/drivers/block/drbd/drbd_receiver.c +++ b/drivers/block/drbd/drbd_receiver.c @@ -5401,6 +5401,35 @@ static int drbd_do_auth(struct drbd_connection *connection) #else #define CHALLENGE_LEN 64 +static char *drbd_get_response(struct drbd_connection *connection, + const char *challenge, unsigned int len) +{ + unsigned dlen = crypto_shash_digestsize(connection->cram_hmac_tfm); + SHASH_DESC_ON_STACK(desc, connection->cram_hmac_tfm); + char *response; + int err; + + desc->tfm = connection->cram_hmac_tfm; + + response = kmalloc(dlen, GFP_NOIO); + if (!response) { + drbd_err(connection, "kmalloc of response failed\n"); + goto out; + } + + err = crypto_shash_digest(desc, challenge, len, response); + if (err) { + drbd_err(connection, "crypto_shash_digest() failed with %d\n", + err); + kfree(response); + response = NULL; + } + +out: + shash_desc_zero(desc); + return response; +} + /* Return value: 1 - auth succeeded, 0 - failed, try again (network error), @@ -5417,7 +5446,6 @@ static int drbd_do_auth(struct drbd_connection *connection) unsigned int key_len; char secret[SHARED_SECRET_MAX]; /* 64 byte */ unsigned int resp_size; - SHASH_DESC_ON_STACK(desc, connection->cram_hmac_tfm); struct packet_info pi; struct net_conf *nc; int err, rv; @@ -5430,8 +5458,6 @@ static int drbd_do_auth(struct drbd_connection *connection) memcpy(secret, nc->shared_secret, key_len); rcu_read_unlock(); - desc->tfm = connection->cram_hmac_tfm; - rv = crypto_shash_setkey(connection->cram_hmac_tfm, (u8 *)secret, key_len); if (rv) { drbd_err(connection, "crypto_shash_setkey() failed with %d\n", rv); @@ -5496,16 +5522,8 @@ static int drbd_do_auth(struct drbd_connection *connection) } resp_size = crypto_shash_digestsize(connection->cram_hmac_tfm); - response = kmalloc(resp_size, GFP_NOIO); + response = drbd_get_response(connection, peers_ch, pi.size); if (response == NULL) { - drbd_err(connection, "kmalloc of response failed\n"); - rv = -1; - goto fail; - } - - rv = crypto_shash_digest(desc, peers_ch, pi.size, response); - if (rv) { - drbd_err(connection, "crypto_hash_digest() failed with %d\n", rv); rv = -1; goto fail; } @@ -5544,17 +5562,9 @@ static int drbd_do_auth(struct drbd_connection *connection) goto fail; } - right_response = kmalloc(resp_size, GFP_NOIO); + right_response = drbd_get_response(connection, my_challenge, + CHALLENGE_LEN); if (right_response == NULL) { - drbd_err(connection, "kmalloc of right_response failed\n"); - rv = -1; - goto fail; - } - - rv = crypto_shash_digest(desc, my_challenge, CHALLENGE_LEN, - right_response); - if (rv) { - drbd_err(connection, "crypto_hash_digest() failed with %d\n", rv); rv = -1; goto fail; } @@ -5571,7 +5581,6 @@ static int drbd_do_auth(struct drbd_connection *connection) kfree(peers_ch); kfree(response); kfree(right_response); - shash_desc_zero(desc); return rv; }
On Mon, Jun 17, 2019 at 4:36 PM Roland Kammerer <roland.kammerer@linbit.com> wrote: > > @@ -5572,6 +5579,7 @@ static int drbd_do_auth(struct drbd_connection *connection) > > kfree(response); > > kfree(right_response); > > shash_desc_zero(desc); > > + kfree(desc); > > > > return rv; > > } > > Hi Arnd, > > are you sure your cleanup is okay? > > > shash_desc_zero(desc); > > + kfree(desc); > > You shash_desc_zero() a potential NULL pointer. memzero_expicit() in the > function then dereferences it: > > memzero_explicit(desc, > sizeof(*desc) + crypto_shash_descsize(desc->tfm)); > > Maybe some if (desc) guard? Good catch. I guess kzfree() would have been the right thing to call. Arnd
On Mon, Jun 17, 2019 at 4:43 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Mon, Jun 17, 2019 at 03:24:13PM +0200, Arnd Bergmann wrote: > > Building with clang and KASAN, we get a warning about an overly large > > stack frame on 32-bit architectures: > > > > drivers/block/drbd/drbd_receiver.c:921:31: error: stack frame size of 1280 bytes in function 'conn_connect' > > [-Werror,-Wframe-larger-than=] > > > > We already allocate other data dynamically in this function, so > > just do the same for the shash descriptor, which makes up most of > > this memory. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > drivers/block/drbd/drbd_receiver.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > Does this patch fix the warning as well? The warning is gone with this patch. Instead of 1280 bytes for drbd_receiver, I now see 512 bytes, and 768 bytes for drbd_get_response, everything else is under 160 bytes in this file. However, with the call chain of drbd_receiver conn_connect drbd_do_auth drbd_get_response This still adds up to as much as before, so it only shuts up the warning but does not reduce the maximum stack usage. If we are sure that is ok, then your patch would be nicer, possibly with a 'noinline_for_stack' tag on drbd_get_response. Arnd
On Mon, Jun 17, 2019 at 10:58:58PM +0200, Arnd Bergmann wrote: > > The warning is gone with this patch. Instead of 1280 bytes for drbd_receiver, > I now see 512 bytes, and 768 bytes for drbd_get_response, everything else is > under 160 bytes in this file. > > However, with the call chain of > > drbd_receiver > conn_connect > drbd_do_auth > drbd_get_response > > This still adds up to as much as before, so it only shuts up the > warning but does not reduce the maximum stack usage. OK so it doesn't really reduce it. Let's just go with your patch. Thanks,
diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c index 90ebfcae0ce6..10fb26e862d7 100644 --- a/drivers/block/drbd/drbd_receiver.c +++ b/drivers/block/drbd/drbd_receiver.c @@ -5417,7 +5417,7 @@ static int drbd_do_auth(struct drbd_connection *connection) unsigned int key_len; char secret[SHARED_SECRET_MAX]; /* 64 byte */ unsigned int resp_size; - SHASH_DESC_ON_STACK(desc, connection->cram_hmac_tfm); + struct shash_desc *desc; struct packet_info pi; struct net_conf *nc; int err, rv; @@ -5430,6 +5430,13 @@ static int drbd_do_auth(struct drbd_connection *connection) memcpy(secret, nc->shared_secret, key_len); rcu_read_unlock(); + desc = kmalloc(sizeof(struct shash_desc) + + crypto_shash_descsize(connection->cram_hmac_tfm), + GFP_KERNEL); + if (!desc) { + rv = -1; + goto fail; + } desc->tfm = connection->cram_hmac_tfm; rv = crypto_shash_setkey(connection->cram_hmac_tfm, (u8 *)secret, key_len); @@ -5572,6 +5579,7 @@ static int drbd_do_auth(struct drbd_connection *connection) kfree(response); kfree(right_response); shash_desc_zero(desc); + kfree(desc); return rv; }
Building with clang and KASAN, we get a warning about an overly large stack frame on 32-bit architectures: drivers/block/drbd/drbd_receiver.c:921:31: error: stack frame size of 1280 bytes in function 'conn_connect' [-Werror,-Wframe-larger-than=] We already allocate other data dynamically in this function, so just do the same for the shash descriptor, which makes up most of this memory. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/block/drbd/drbd_receiver.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)