Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 18 Jun 2019 15:12:34 +0200
From:   Corentin Labbe <clabbe.montjoie@gmail.com>
To:     Arnd Bergmann <arnd@arndb.de>
Cc:     Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        Chen-Yu Tsai <wens@csie.org>,
        Eric Biggers <ebiggers@google.com>,
        linux-crypto@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] crypto: sun4i-ss - reduce stack usage
Message-ID: <20190618131234.GA8474@Red>
References: <20190617132538.2759714-1-arnd@arndb.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190617132538.2759714-1-arnd@arndb.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 17, 2019 at 03:25:17PM +0200, Arnd Bergmann wrote:
> After the latest addition, the stack usage of sun4i_ss_cipher_poll
> grew beyond the warning limit when KASAN is enabled:
> 
> drivers/crypto/sunxi-ss/sun4i-ss-cipher.c:118:12: error: stack frame size of 1152 bytes in function 'sun4i_ss_cipher_poll' [-Werror,-Wframe-larger-than=]
> static int sun4i_ss_cipher_poll(struct skcipher_request *areq)
> 
> Reduce it in three ways:
> 
> - split out the new code into a separate function so its stack
>   usage can overlap that of the sun4i_ss_opti_poll() code path
> - mark both special cases as noinline_for_stack, which should
>   ideally result in a tail call that frees the rest of the
>   stack
> - move the buf and obuf variables into the code blocks in
>   which they are used.
> 
> The three separate functions now use 144, 640 and 304 bytes of kernel
> stack, respectively.
> 
> Fixes: 0ae1f46c55f8 ("crypto: sun4i-ss - fallback when length is not multiple of blocksize")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/crypto/sunxi-ss/sun4i-ss-cipher.c | 47 +++++++++++++++--------
>  1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-cipher.c b/drivers/crypto/sunxi-ss/sun4i-ss-cipher.c
> index 7b0c42882830..4ab14d58e85b 100644
> --- a/drivers/crypto/sunxi-ss/sun4i-ss-cipher.c
> +++ b/drivers/crypto/sunxi-ss/sun4i-ss-cipher.c
> @@ -12,7 +12,7 @@
>   */
>  #include "sun4i-ss.h"
>  
> -static int sun4i_ss_opti_poll(struct skcipher_request *areq)
> +static int noinline_for_stack sun4i_ss_opti_poll(struct skcipher_request *areq)
>  {
>  	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq);
>  	struct sun4i_tfm_ctx *op = crypto_skcipher_ctx(tfm);
> @@ -114,6 +114,29 @@ static int sun4i_ss_opti_poll(struct skcipher_request *areq)
>  	return err;
>  }
>  
> +
> +static int noinline_for_stack sun4i_ss_cipher_poll_fallback(struct skcipher_request *areq)
> +{
> +	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq);
> +	struct sun4i_tfm_ctx *op = crypto_skcipher_ctx(tfm);
> +	struct sun4i_cipher_req_ctx *ctx = skcipher_request_ctx(areq);
> +	SYNC_SKCIPHER_REQUEST_ON_STACK(subreq, op->fallback_tfm);
> +	int err;
> +
> +	skcipher_request_set_sync_tfm(subreq, op->fallback_tfm);
> +	skcipher_request_set_callback(subreq, areq->base.flags, NULL,
> +				      NULL);
> +	skcipher_request_set_crypt(subreq, areq->src, areq->dst,
> +				   areq->cryptlen, areq->iv);
> +	if (ctx->mode & SS_DECRYPTION)
> +		err = crypto_skcipher_decrypt(subreq);
> +	else
> +		err = crypto_skcipher_encrypt(subreq);
> +	skcipher_request_zero(subreq);
> +
> +	return err;
> +}
> +
>  /* Generic function that support SG with size not multiple of 4 */
>  static int sun4i_ss_cipher_poll(struct skcipher_request *areq)
>  {
> @@ -140,8 +163,6 @@ static int sun4i_ss_cipher_poll(struct skcipher_request *areq)
>  	unsigned int todo;
>  	struct sg_mapping_iter mi, mo;
>  	unsigned int oi, oo;	/* offset for in and out */
> -	char buf[4 * SS_RX_MAX];/* buffer for linearize SG src */
> -	char bufo[4 * SS_TX_MAX]; /* buffer for linearize SG dst */
>  	unsigned int ob = 0;	/* offset in buf */
>  	unsigned int obo = 0;	/* offset in bufo*/
>  	unsigned int obl = 0;	/* length of data in bufo */
> @@ -178,20 +199,8 @@ static int sun4i_ss_cipher_poll(struct skcipher_request *areq)
>  	if (no_chunk == 1 && !need_fallback)
>  		return sun4i_ss_opti_poll(areq);
>  
> -	if (need_fallback) {
> -		SYNC_SKCIPHER_REQUEST_ON_STACK(subreq, op->fallback_tfm);
> -		skcipher_request_set_sync_tfm(subreq, op->fallback_tfm);
> -		skcipher_request_set_callback(subreq, areq->base.flags, NULL,
> -					      NULL);
> -		skcipher_request_set_crypt(subreq, areq->src, areq->dst,
> -					   areq->cryptlen, areq->iv);
> -		if (ctx->mode & SS_DECRYPTION)
> -			err = crypto_skcipher_decrypt(subreq);
> -		else
> -			err = crypto_skcipher_encrypt(subreq);
> -		skcipher_request_zero(subreq);
> -		return err;
> -	}
> +	if (need_fallback)
> +		return sun4i_ss_cipher_poll_fallback(areq);
>  
>  	spin_lock_irqsave(&ss->slock, flags);
>  
> @@ -224,6 +233,8 @@ static int sun4i_ss_cipher_poll(struct skcipher_request *areq)
>  
>  	while (oleft) {
>  		if (ileft) {
> +			char buf[4 * SS_RX_MAX];/* buffer for linearize SG src */
> +
>  			/*
>  			 * todo is the number of consecutive 4byte word that we
>  			 * can read from current SG
> @@ -281,6 +292,8 @@ static int sun4i_ss_cipher_poll(struct skcipher_request *areq)
>  				oo = 0;
>  			}
>  		} else {
> +			char bufo[4 * SS_TX_MAX]; /* buffer for linearize SG dst */
> +
>  			/*
>  			 * read obl bytes in bufo, we read at maximum for
>  			 * emptying the device
> -- 
> 2.20.0
> 

Tested-by: Corentin LABBE <clabbe.montjoie@gmail.com>