linux-kernel - Re: [PATCH v2 1/7] crypto: sun4i-ss: linearize buffers content must be kept

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200925073055.GA18879@gondor.apana.org.au>
Date:   Fri, 25 Sep 2020 17:30:55 +1000
From:   Herbert Xu <herbert@...dor.apana.org.au>
To:     Corentin Labbe <clabbe@...libre.com>
Cc:     arnd@...db.de, davem@...emloft.net, mripard@...nel.org,
        wens@...e.org, linux-arm-kernel@...ts.infradead.org,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-sunxi@...glegroups.com
Subject: Re: [PATCH v2 1/7] crypto: sun4i-ss: linearize buffers content must
 be kept

On Sun, Sep 20, 2020 at 06:37:12PM +0000, Corentin Labbe wrote:
> When running the non-optimized cipher function, SS produce partial random
> output.
> This is due to linearize buffers being reseted after each loop.
> 
> Fixes: 8d3bcb9900ca ("crypto: sun4i-ss - reduce stack usage")
> Signed-off-by: Corentin Labbe <clabbe@...libre.com>
> ---
>  drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> index b72de8939497..b92d175b5d2a 100644
> --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> @@ -163,6 +163,8 @@ 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 */
> @@ -233,8 +235,6 @@ 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

Ouch.  So this is obviously correct but I wonder if the stack
usage would be an issue again?

How about moving this code into another function so that it sits
at the same level as the fallback function, which would mean that
the buffers do not double up with the one for the fallback?

Cheers,
-- 
Email: Herbert Xu <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt