linux-kernel - Re: [PATCH 4.4 123/124] crypto: padlock-aes

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]

Date:   Fri, 07 Sep 2018 02:54:10 +0100
From:   Ben Hutchings <ben.hutchings@...ethink.co.uk>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     stable@...r.kernel.org,
        Jamie Heilman <jamie@...ible.transient.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4.4 123/124] crypto: padlock-aes - Fix Nano workaround
 data corruption

On Sat, 2018-08-04 at 11:01 +0200, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Herbert Xu <herbert@...dor.apana.org.au>
> 
> commit 46d8c4b28652d35dc6cfb5adf7f54e102fc04384 upstream.
> 
> This was detected by the self-test thanks to Ard's chunking patch.
> 
> I finally got around to testing this out on my ancient Via box.  It
> turns out that the workaround got the assembly wrong and we end up
> doing count + initial cycles of the loop instead of just count.
> 
> This obviously causes corruption, either by overwriting the source
> that is yet to be processed, or writing over the end of the buffer.
> 
> On CPUs that don't require the workaround only ECB is affected.
> On Nano CPUs both ECB and CBC are affected.
> 
> This patch fixes it by doing the subtraction prior to the assembly.
[...]
> --- a/drivers/crypto/padlock-aes.c
> +++ b/drivers/crypto/padlock-aes.c
> @@ -266,6 +266,8 @@ static inline void padlock_xcrypt_ecb(co
> >  		return;
> >  	}
>  
> +	count -= initial;
> +
>  	if (initial)
>  		asm volatile (".byte 0xf3,0x0f,0xa7,0xc8"	/* rep xcryptecb */
>  			      : "+S"(input), "+D"(output)
> @@ -273,7 +275,7 @@ static inline void padlock_xcrypt_ecb(co
>  
>  	asm volatile (".byte 0xf3,0x0f,0xa7,0xc8"	/* rep xcryptecb */
>  		      : "+S"(input), "+D"(output)
> -		      : "d"(control_word), "b"(key), "c"(count - initial));
> +		      : "d"(control_word), "b"(key), "c"(count));
>  }
>  
>  static inline u8 *padlock_xcrypt_cbc(const u8 *input, u8 *output, void *key,
[...]

On the face of it, this change shouldn't make any difference.  But I
think what's going on is that the compiler stores "initial" in register
ecx and nowhere else, because it has no idea that the first inline
assembly block will update ecx.

This change evidently works around that problem for the specific
compiler and configuration you tested with, but it seems fragile.  I
think the assembly constraints should be updated to properly fix this.

Ben.

-- 
Ben Hutchings, Software Developer                         Codethink Ltd
https://www.codethink.co.uk/                 Dale House, 35 Dale Street
                                     Manchester, M1 2HF, United Kingdom