linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drbd: dynamically allocate shash descriptor
@ 2019-06-17 13:24 Arnd Bergmann
  2019-06-17 14:36 ` Roland Kammerer
  2019-06-17 14:43 ` Herbert Xu
  0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2019-06-17 13:24 UTC (permalink / raw)
  To: Philipp Reisner, Lars Ellenberg, Jens Axboe
  Cc: Arnd Bergmann, Roland Kammerer, Eric Biggers, Herbert Xu,
	Gustavo A. R. Silva, Kees Cook, drbd-dev, linux-block,
	linux-kernel

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] drbd: dynamically allocate shash descriptor
  2019-06-17 13:24 [PATCH] drbd: dynamically allocate shash descriptor Arnd Bergmann
@ 2019-06-17 14:36 ` Roland Kammerer
  2019-06-17 20:38   ` Arnd Bergmann
  2019-06-17 14:43 ` Herbert Xu
  1 sibling, 1 reply; 6+ messages in thread
From: Roland Kammerer @ 2019-06-17 14:36 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Philipp Reisner, Lars Ellenberg, Jens Axboe, Roland Kammerer,
	Eric Biggers, Herbert Xu, Gustavo A. R. Silva, Kees Cook,
	drbd-dev, linux-block, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drbd: dynamically allocate shash descriptor
  2019-06-17 13:24 [PATCH] drbd: dynamically allocate shash descriptor Arnd Bergmann
  2019-06-17 14:36 ` Roland Kammerer
@ 2019-06-17 14:43 ` Herbert Xu
  2019-06-17 20:58   ` Arnd Bergmann
  1 sibling, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2019-06-17 14:43 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Philipp Reisner, Lars Ellenberg, Jens Axboe, Roland Kammerer,
	Eric Biggers, Gustavo A. R. Silva, Kees Cook, drbd-dev,
	linux-block, linux-kernel

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;
 }
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] drbd: dynamically allocate shash descriptor
  2019-06-17 14:36 ` Roland Kammerer
@ 2019-06-17 20:38   ` Arnd Bergmann
  0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2019-06-17 20:38 UTC (permalink / raw)
  To: Roland Kammerer
  Cc: Philipp Reisner, Lars Ellenberg, Jens Axboe, Eric Biggers,
	Herbert Xu, Gustavo A. R. Silva, Kees Cook, drbd-dev,
	linux-block, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drbd: dynamically allocate shash descriptor
  2019-06-17 14:43 ` Herbert Xu
@ 2019-06-17 20:58   ` Arnd Bergmann
  2019-06-18  3:42     ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2019-06-17 20:58 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Philipp Reisner, Lars Ellenberg, Jens Axboe, Roland Kammerer,
	Eric Biggers, Gustavo A. R. Silva, Kees Cook, drbd-dev,
	linux-block, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] drbd: dynamically allocate shash descriptor
  2019-06-17 20:58   ` Arnd Bergmann
@ 2019-06-18  3:42     ` Herbert Xu
  0 siblings, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2019-06-18  3:42 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Philipp Reisner, Lars Ellenberg, Jens Axboe, Roland Kammerer,
	Eric Biggers, Gustavo A. R. Silva, Kees Cook, drbd-dev,
	linux-block, Linux Kernel Mailing List

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-06-18  3:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 13:24 [PATCH] drbd: dynamically allocate shash descriptor Arnd Bergmann
2019-06-17 14:36 ` Roland Kammerer
2019-06-17 20:38   ` Arnd Bergmann
2019-06-17 14:43 ` Herbert Xu
2019-06-17 20:58   ` Arnd Bergmann
2019-06-18  3:42     ` Herbert Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).