Re: [Patch V3] crypto: x86/sha1 : Fix reads beyond the number of blocks passed

[Megha Dey]

On Wed, 2017-08-02 at 10:13 -0700, Megha Dey wrote:
> It was reported that the sha1 AVX2 function(sha1_transform_avx2) is
> reading ahead beyond its intended data, and causing a crash if the next
> block is beyond page boundary:
> http://marc.info/?l=linux-crypto-vger&m=149373371023377
> 
> This patch makes sure that there is no overflow for any buffer length.
> 
> It passes the tests written by Jan Stancek that revealed this problem:
> https://github.com/jstancek/sha1-avx2-crash

Hi Jan,

Is it ok to add your Tested-by?

> 
> I have re-enabled sha1-avx2 by reverting commit
> b82ce24426a4071da9529d726057e4e642948667
> 
> Originally-by: Ilya Albrekht <ilya.albrekht@xxxxxxxxx>
> Signed-off-by: Megha Dey <megha.dey@xxxxxxxxxxxxxxx>
> Reported-by: Jan Stancek <jstancek@xxxxxxxxxx>
> ---
>  arch/x86/crypto/sha1_avx2_x86_64_asm.S | 67 ++++++++++++++++++----------------
>  arch/x86/crypto/sha1_ssse3_glue.c      |  2 +-
>  2 files changed, 37 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/crypto/sha1_avx2_x86_64_asm.S b/arch/x86/crypto/sha1_avx2_x86_64_asm.S
> index 1cd792d..1eab79c 100644
> --- a/arch/x86/crypto/sha1_avx2_x86_64_asm.S
> +++ b/arch/x86/crypto/sha1_avx2_x86_64_asm.S
> @@ -117,11 +117,10 @@
>  	.set T1, REG_T1
>  .endm
>  
> -#define K_BASE		%r8
>  #define HASH_PTR	%r9
> +#define BLOCKS_CTR	%r8
>  #define BUFFER_PTR	%r10
>  #define BUFFER_PTR2	%r13
> -#define BUFFER_END	%r11
>  
>  #define PRECALC_BUF	%r14
>  #define WK_BUF		%r15
> @@ -205,14 +204,14 @@
>  		 * blended AVX2 and ALU instruction scheduling
>  		 * 1 vector iteration per 8 rounds
>  		 */
> -		vmovdqu ((i * 2) + PRECALC_OFFSET)(BUFFER_PTR), W_TMP
> +		vmovdqu (i * 2)(BUFFER_PTR), W_TMP
>  	.elseif ((i & 7) == 1)
> -		vinsertf128 $1, (((i-1) * 2)+PRECALC_OFFSET)(BUFFER_PTR2),\
> +		vinsertf128 $1, ((i-1) * 2)(BUFFER_PTR2),\
>  			 WY_TMP, WY_TMP
>  	.elseif ((i & 7) == 2)
>  		vpshufb YMM_SHUFB_BSWAP, WY_TMP, WY
>  	.elseif ((i & 7) == 4)
> -		vpaddd  K_XMM(K_BASE), WY, WY_TMP
> +		vpaddd  K_XMM + K_XMM_AR(%rip), WY, WY_TMP
>  	.elseif ((i & 7) == 7)
>  		vmovdqu  WY_TMP, PRECALC_WK(i&~7)
>  
> @@ -255,7 +254,7 @@
>  		vpxor	WY, WY_TMP, WY_TMP
>  	.elseif ((i & 7) == 7)
>  		vpxor	WY_TMP2, WY_TMP, WY
> -		vpaddd	K_XMM(K_BASE), WY, WY_TMP
> +		vpaddd  K_XMM + K_XMM_AR(%rip), WY, WY_TMP
>  		vmovdqu	WY_TMP, PRECALC_WK(i&~7)
>  
>  		PRECALC_ROTATE_WY
> @@ -291,7 +290,7 @@
>  		vpsrld	$30, WY, WY
>  		vpor	WY, WY_TMP, WY
>  	.elseif ((i & 7) == 7)
> -		vpaddd	K_XMM(K_BASE), WY, WY_TMP
> +		vpaddd  K_XMM + K_XMM_AR(%rip), WY, WY_TMP
>  		vmovdqu	WY_TMP, PRECALC_WK(i&~7)
>  
>  		PRECALC_ROTATE_WY
> @@ -446,6 +445,16 @@
>  
>  .endm
>  
> +/* Add constant only if (%2 > %3) condition met (uses RTA as temp)
> + * %1 + %2 >= %3 ? %4 : 0
> + */
> +.macro ADD_IF_GE a, b, c, d
> +	mov     \a, RTA
> +	add     $\d, RTA
> +	cmp     $\c, \b
> +	cmovge  RTA, \a
> +.endm
> +
>  /*
>   * macro implements 80 rounds of SHA-1, for multiple blocks with s/w pipelining
>   */
> @@ -463,13 +472,16 @@
>  	lea	(2*4*80+32)(%rsp), WK_BUF
>  
>  	# Precalc WK for first 2 blocks
> -	PRECALC_OFFSET = 0
> +	ADD_IF_GE BUFFER_PTR2, BLOCKS_CTR, 2, 64
>  	.set i, 0
>  	.rept    160
>  		PRECALC i
>  		.set i, i + 1
>  	.endr
> -	PRECALC_OFFSET = 128
> +
> +	/* Go to next block if needed */
> +	ADD_IF_GE BUFFER_PTR, BLOCKS_CTR, 3, 128
> +	ADD_IF_GE BUFFER_PTR2, BLOCKS_CTR, 4, 128
>  	xchg	WK_BUF, PRECALC_BUF
>  
>  	.align 32
> @@ -479,8 +491,8 @@ _loop:
>  	 * we use K_BASE value as a signal of a last block,
>  	 * it is set below by: cmovae BUFFER_PTR, K_BASE
>  	 */
> -	cmp	K_BASE, BUFFER_PTR
> -	jne	_begin
> +	test BLOCKS_CTR, BLOCKS_CTR
> +	jnz _begin
>  	.align 32
>  	jmp	_end
>  	.align 32
> @@ -512,10 +524,10 @@ _loop0:
>  		.set j, j+2
>  	.endr
>  
> -	add	$(2*64), BUFFER_PTR       /* move to next odd-64-byte block */
> -	cmp	BUFFER_END, BUFFER_PTR    /* is current block the last one? */
> -	cmovae	K_BASE, BUFFER_PTR	/* signal the last iteration smartly */
> -
> +	/* Update Counter */
> +	sub $1, BLOCKS_CTR
> +	/* Move to the next block only if needed*/
> +	ADD_IF_GE BUFFER_PTR, BLOCKS_CTR, 4, 128
>  	/*
>  	 * rounds
>  	 * 60,62,64,66,68
> @@ -532,8 +544,8 @@ _loop0:
>  	UPDATE_HASH	12(HASH_PTR), D
>  	UPDATE_HASH	16(HASH_PTR), E
>  
> -	cmp	K_BASE, BUFFER_PTR	/* is current block the last one? */
> -	je	_loop
> +	test	BLOCKS_CTR, BLOCKS_CTR
> +	jz	_loop
>  
>  	mov	TB, B
>  
> @@ -575,10 +587,10 @@ _loop2:
>  		.set j, j+2
>  	.endr
>  
> -	add	$(2*64), BUFFER_PTR2      /* move to next even-64-byte block */
> -
> -	cmp	BUFFER_END, BUFFER_PTR2   /* is current block the last one */
> -	cmovae	K_BASE, BUFFER_PTR       /* signal the last iteration smartly */
> +	/* update counter */
> +	sub     $1, BLOCKS_CTR
> +	/* Move to the next block only if needed*/
> +	ADD_IF_GE BUFFER_PTR2, BLOCKS_CTR, 4, 128
>  
>  	jmp	_loop3
>  _loop3:
> @@ -641,19 +653,12 @@ _loop3:
>  
>  	avx2_zeroupper
>  
> -	lea	K_XMM_AR(%rip), K_BASE
> -
> +	/* Setup initial values */
>  	mov	CTX, HASH_PTR
>  	mov	BUF, BUFFER_PTR
> -	lea	64(BUF), BUFFER_PTR2
> -
> -	shl	$6, CNT			/* mul by 64 */
> -	add	BUF, CNT
> -	add	$64, CNT
> -	mov	CNT, BUFFER_END
>  
> -	cmp	BUFFER_END, BUFFER_PTR2
> -	cmovae	K_BASE, BUFFER_PTR2
> +	mov	BUF, BUFFER_PTR2
> +	mov	CNT, BLOCKS_CTR
>  
>  	xmm_mov	BSWAP_SHUFB_CTL(%rip), YMM_SHUFB_BSWAP
>  
> diff --git a/arch/x86/crypto/sha1_ssse3_glue.c b/arch/x86/crypto/sha1_ssse3_glue.c
> index f960a04..fc61739 100644
> --- a/arch/x86/crypto/sha1_ssse3_glue.c
> +++ b/arch/x86/crypto/sha1_ssse3_glue.c
> @@ -201,7 +201,7 @@ asmlinkage void sha1_transform_avx2(u32 *digest, const char *data,
>  
>  static bool avx2_usable(void)
>  {
> -	if (false && avx_usable() && boot_cpu_has(X86_FEATURE_AVX2)
> +	if (avx_usable() && boot_cpu_has(X86_FEATURE_AVX2)
>  		&& boot_cpu_has(X86_FEATURE_BMI1)
>  		&& boot_cpu_has(X86_FEATURE_BMI2))
>  		return true;