drbd: dynamically allocate shash descriptor
diff mbox series

Message ID 20190617132440.2721536-1-arnd@arndb.de
State New
Headers show
Series
  • drbd: dynamically allocate shash descriptor
Related show

Commit Message

Arnd Bergmann June 17, 2019, 1:24 p.m. UTC
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(-)

Comments

Roland Kammerer June 17, 2019, 2:36 p.m. UTC | #1
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
Herbert Xu June 17, 2019, 2:43 p.m. UTC | #2
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;
 }
Arnd Bergmann June 17, 2019, 8:38 p.m. UTC | #3
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
Arnd Bergmann June 17, 2019, 8:58 p.m. UTC | #4
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
Herbert Xu June 18, 2019, 3:42 a.m. UTC | #5
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,

Patch
diff mbox series

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;
 }