All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed
@ 2017-08-23  0:41 Megha Dey
  2017-08-23  0:47 ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Megha Dey @ 2017-08-23  0:41 UTC (permalink / raw)
  To: stable; +Cc: Megha Dey

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

This patch fixes reads beyond the number of blocks in the same way it
was done in commit 8861249c740fc4af9ddc5aee321eafefb960d7c6
("crypto: x86/sha1 : Fix reads beyond the number of blocks passed").

Originally-by: Ilya Albrekht <ilya.albrekht@intel.com>
Tested-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Megha Dey <megha.dey@linux.intel.com>
Reported-by: Jan Stancek <jstancek@redhat.com>
---
 arch/x86/crypto/sha1_avx2_x86_64_asm.S | 67 ++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 31 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
 
-- 
1.9.1

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

* Re: [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed
  2017-08-23  0:41 [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed Megha Dey
@ 2017-08-23  0:47 ` Greg KH
  2017-08-29 17:08   ` Megha Dey
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2017-08-23  0:47 UTC (permalink / raw)
  To: Megha Dey; +Cc: stable

On Tue, Aug 22, 2017 at 05:41:03PM -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
> 
> This patch fixes reads beyond the number of blocks in the same way it
> was done in commit 8861249c740fc4af9ddc5aee321eafefb960d7c6
> ("crypto: x86/sha1 : Fix reads beyond the number of blocks passed").

So all you really want is that specific commit added to the stable
kernels?  If so, what kernel tree(s)?

thanks,

greg k-h

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

* Re: [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed
  2017-08-23  0:47 ` Greg KH
@ 2017-08-29 17:08   ` Megha Dey
  2017-08-31  6:03     ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Megha Dey @ 2017-08-29 17:08 UTC (permalink / raw)
  To: Greg KH; +Cc: stable

On Tue, 2017-08-22 at 17:47 -0700, Greg KH wrote:
> On Tue, Aug 22, 2017 at 05:41:03PM -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
> > 
> > This patch fixes reads beyond the number of blocks in the same way it
> > was done in commit 8861249c740fc4af9ddc5aee321eafefb960d7c6
> > ("crypto: x86/sha1 : Fix reads beyond the number of blocks passed").
> 
> So all you really want is that specific commit added to the stable
> kernels?  If so, what kernel tree(s)?

Hi Greg,
The commit 8861249c740fc4af9ddc5aee321eafefb960d7c6 present in the
mainline kernel does not apply cleanly to the stable kernel tree. Hence,
I have submitted this patch with some minor changes for the stable tree.

I am not sure what you mean by which kernel trees.
> 
> thanks,
> 
> greg k-h

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

* Re: [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed
  2017-08-29 17:08   ` Megha Dey
@ 2017-08-31  6:03     ` Greg KH
  2017-08-31  6:06       ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2017-08-31  6:03 UTC (permalink / raw)
  To: Megha Dey; +Cc: stable

On Tue, Aug 29, 2017 at 10:08:31AM -0700, Megha Dey wrote:
> On Tue, 2017-08-22 at 17:47 -0700, Greg KH wrote:
> > On Tue, Aug 22, 2017 at 05:41:03PM -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
> > > 
> > > This patch fixes reads beyond the number of blocks in the same way it
> > > was done in commit 8861249c740fc4af9ddc5aee321eafefb960d7c6
> > > ("crypto: x86/sha1 : Fix reads beyond the number of blocks passed").
> > 
> > So all you really want is that specific commit added to the stable
> > kernels?  If so, what kernel tree(s)?
> 
> Hi Greg,
> The commit 8861249c740fc4af9ddc5aee321eafefb960d7c6 present in the
> mainline kernel does not apply cleanly to the stable kernel tree. Hence,
> I have submitted this patch with some minor changes for the stable tree.
> 
> I am not sure what you mean by which kernel trees.

There are lots of stable kernel trees being maintained at the moment,
see:
	https://www.kernel.org/category/releases.html

I was asking if any of those are applicable for this patch as well.

thanks,

greg k-h

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

* Re: [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed
  2017-08-31  6:03     ` Greg KH
@ 2017-08-31  6:06       ` Greg KH
  2017-08-31 17:35         ` Megha Dey
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2017-08-31  6:06 UTC (permalink / raw)
  To: Megha Dey; +Cc: stable

On Thu, Aug 31, 2017 at 08:03:59AM +0200, Greg KH wrote:
> On Tue, Aug 29, 2017 at 10:08:31AM -0700, Megha Dey wrote:
> > On Tue, 2017-08-22 at 17:47 -0700, Greg KH wrote:
> > > On Tue, Aug 22, 2017 at 05:41:03PM -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
> > > > 
> > > > This patch fixes reads beyond the number of blocks in the same way it
> > > > was done in commit 8861249c740fc4af9ddc5aee321eafefb960d7c6
> > > > ("crypto: x86/sha1 : Fix reads beyond the number of blocks passed").
> > > 
> > > So all you really want is that specific commit added to the stable
> > > kernels?  If so, what kernel tree(s)?
> > 
> > Hi Greg,
> > The commit 8861249c740fc4af9ddc5aee321eafefb960d7c6 present in the
> > mainline kernel does not apply cleanly to the stable kernel tree. Hence,
> > I have submitted this patch with some minor changes for the stable tree.
> > 
> > I am not sure what you mean by which kernel trees.
> 
> There are lots of stable kernel trees being maintained at the moment,
> see:
> 	https://www.kernel.org/category/releases.html
> 
> I was asking if any of those are applicable for this patch as well.

Even more confusing, commit 8861249c740fc4af9ddc5aee321eafefb960d7c6 is
already in the stable kernel releases.  It showed up in 4.4.84, 4.9.45,
and 4.12.9.  You should have gotten notifications about all of these.

So I don't really understand what needs to be done here.

totally confused,

greg k-h

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

* Re: [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed
  2017-08-31  6:06       ` Greg KH
@ 2017-08-31 17:35         ` Megha Dey
  2017-09-04  9:34           ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Megha Dey @ 2017-08-31 17:35 UTC (permalink / raw)
  To: Greg KH; +Cc: stable

On Thu, 2017-08-31 at 08:06 +0200, Greg KH wrote:
> On Thu, Aug 31, 2017 at 08:03:59AM +0200, Greg KH wrote:
> > On Tue, Aug 29, 2017 at 10:08:31AM -0700, Megha Dey wrote:
> > > On Tue, 2017-08-22 at 17:47 -0700, Greg KH wrote:
> > > > On Tue, Aug 22, 2017 at 05:41:03PM -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
> > > > > 
> > > > > This patch fixes reads beyond the number of blocks in the same way it
> > > > > was done in commit 8861249c740fc4af9ddc5aee321eafefb960d7c6
> > > > > ("crypto: x86/sha1 : Fix reads beyond the number of blocks passed").
> > > > 
> > > > So all you really want is that specific commit added to the stable
> > > > kernels?  If so, what kernel tree(s)?
> > > 
> > > Hi Greg,
> > > The commit 8861249c740fc4af9ddc5aee321eafefb960d7c6 present in the
> > > mainline kernel does not apply cleanly to the stable kernel tree. Hence,
> > > I have submitted this patch with some minor changes for the stable tree.
> > > 
> > > I am not sure what you mean by which kernel trees.
> > 
> > There are lots of stable kernel trees being maintained at the moment,
> > see:
> > 	https://www.kernel.org/category/releases.html
> > 
> > I was asking if any of those are applicable for this patch as well.
> 
> Even more confusing, commit 8861249c740fc4af9ddc5aee321eafefb960d7c6 is
> already in the stable kernel releases.  It showed up in 4.4.84, 4.9.45,
> and 4.12.9.  You should have gotten notifications about all of these.
> 
> So I don't really understand what needs to be done here.
> 
> totally confused,
Hi Greg,

Sorry for the confusion. 

I had got an email from you: 
"The patch below does not apply to the 3.18-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>."

This new patch applies cleanly on the 3.18 stable kernel unlike commit
8861249. So to answer your question, this patch needs to be added to the
3.18 stable kernel. 

-Megha
> 
> greg k-h

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

* Re: [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed
  2017-08-31 17:35         ` Megha Dey
@ 2017-09-04  9:34           ` Greg KH
  2017-09-18  6:42             ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2017-09-04  9:34 UTC (permalink / raw)
  To: Megha Dey; +Cc: stable

On Thu, Aug 31, 2017 at 10:35:31AM -0700, Megha Dey wrote:
> On Thu, 2017-08-31 at 08:06 +0200, Greg KH wrote:
> > On Thu, Aug 31, 2017 at 08:03:59AM +0200, Greg KH wrote:
> > > On Tue, Aug 29, 2017 at 10:08:31AM -0700, Megha Dey wrote:
> > > > On Tue, 2017-08-22 at 17:47 -0700, Greg KH wrote:
> > > > > On Tue, Aug 22, 2017 at 05:41:03PM -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
> > > > > > 
> > > > > > This patch fixes reads beyond the number of blocks in the same way it
> > > > > > was done in commit 8861249c740fc4af9ddc5aee321eafefb960d7c6
> > > > > > ("crypto: x86/sha1 : Fix reads beyond the number of blocks passed").
> > > > > 
> > > > > So all you really want is that specific commit added to the stable
> > > > > kernels?  If so, what kernel tree(s)?
> > > > 
> > > > Hi Greg,
> > > > The commit 8861249c740fc4af9ddc5aee321eafefb960d7c6 present in the
> > > > mainline kernel does not apply cleanly to the stable kernel tree. Hence,
> > > > I have submitted this patch with some minor changes for the stable tree.
> > > > 
> > > > I am not sure what you mean by which kernel trees.
> > > 
> > > There are lots of stable kernel trees being maintained at the moment,
> > > see:
> > > 	https://www.kernel.org/category/releases.html
> > > 
> > > I was asking if any of those are applicable for this patch as well.
> > 
> > Even more confusing, commit 8861249c740fc4af9ddc5aee321eafefb960d7c6 is
> > already in the stable kernel releases.  It showed up in 4.4.84, 4.9.45,
> > and 4.12.9.  You should have gotten notifications about all of these.
> > 
> > So I don't really understand what needs to be done here.
> > 
> > totally confused,
> Hi Greg,
> 
> Sorry for the confusion. 
> 
> I had got an email from you: 
> "The patch below does not apply to the 3.18-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>."
> 
> This new patch applies cleanly on the 3.18 stable kernel unlike commit
> 8861249. So to answer your question, this patch needs to be added to the
> 3.18 stable kernel. 

Ok, but commit 8861249 modifies a lot of files, while your "backport"
does not do that at all.

Again, I still do not know what is going on here, or what exactly you
want.  How about a backport of the original patch, to the 3.18-stable
kernel tree, that is what I think is needed, correct?

thanks,

greg k-h

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

* Re: [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed
  2017-09-04  9:34           ` Greg KH
@ 2017-09-18  6:42             ` Greg KH
  0 siblings, 0 replies; 9+ messages in thread
From: Greg KH @ 2017-09-18  6:42 UTC (permalink / raw)
  To: Megha Dey; +Cc: stable

On Mon, Sep 04, 2017 at 11:34:24AM +0200, Greg KH wrote:
> On Thu, Aug 31, 2017 at 10:35:31AM -0700, Megha Dey wrote:
> > On Thu, 2017-08-31 at 08:06 +0200, Greg KH wrote:
> > > On Thu, Aug 31, 2017 at 08:03:59AM +0200, Greg KH wrote:
> > > > On Tue, Aug 29, 2017 at 10:08:31AM -0700, Megha Dey wrote:
> > > > > On Tue, 2017-08-22 at 17:47 -0700, Greg KH wrote:
> > > > > > On Tue, Aug 22, 2017 at 05:41:03PM -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
> > > > > > > 
> > > > > > > This patch fixes reads beyond the number of blocks in the same way it
> > > > > > > was done in commit 8861249c740fc4af9ddc5aee321eafefb960d7c6
> > > > > > > ("crypto: x86/sha1 : Fix reads beyond the number of blocks passed").
> > > > > > 
> > > > > > So all you really want is that specific commit added to the stable
> > > > > > kernels?  If so, what kernel tree(s)?
> > > > > 
> > > > > Hi Greg,
> > > > > The commit 8861249c740fc4af9ddc5aee321eafefb960d7c6 present in the
> > > > > mainline kernel does not apply cleanly to the stable kernel tree. Hence,
> > > > > I have submitted this patch with some minor changes for the stable tree.
> > > > > 
> > > > > I am not sure what you mean by which kernel trees.
> > > > 
> > > > There are lots of stable kernel trees being maintained at the moment,
> > > > see:
> > > > 	https://www.kernel.org/category/releases.html
> > > > 
> > > > I was asking if any of those are applicable for this patch as well.
> > > 
> > > Even more confusing, commit 8861249c740fc4af9ddc5aee321eafefb960d7c6 is
> > > already in the stable kernel releases.  It showed up in 4.4.84, 4.9.45,
> > > and 4.12.9.  You should have gotten notifications about all of these.
> > > 
> > > So I don't really understand what needs to be done here.
> > > 
> > > totally confused,
> > Hi Greg,
> > 
> > Sorry for the confusion. 
> > 
> > I had got an email from you: 
> > "The patch below does not apply to the 3.18-stable tree.
> > If someone wants it applied there, or to any other stable or longterm
> > tree, then please email the backport, including the original git commit
> > id to <stable@vger.kernel.org>."
> > 
> > This new patch applies cleanly on the 3.18 stable kernel unlike commit
> > 8861249. So to answer your question, this patch needs to be added to the
> > 3.18 stable kernel. 
> 
> Ok, but commit 8861249 modifies a lot of files, while your "backport"
> does not do that at all.
> 
> Again, I still do not know what is going on here, or what exactly you
> want.  How about a backport of the original patch, to the 3.18-stable
> kernel tree, that is what I think is needed, correct?

Dropping this email thread from my "todo" stable queue due to a lack of
response, if you still want this applied, please fix up and make it
obvious what I need to do here.

thanks,

greg k-h

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

* [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed
@ 2017-08-02  0:03 Megha Dey
  0 siblings, 0 replies; 9+ messages in thread
From: Megha Dey @ 2017-08-02  0:03 UTC (permalink / raw)
  To: herbert
  Cc: tim.c.chen, davem, linux-crypto, linux-kernel, jstancek,
	megha.dey, Megha Dey

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

Jan, can you verify this fix?
Herbert, can you re-enable sha1-avx2 once Jan has checked it out and
revert commit b82ce24426a4071da9529d726057e4e642948667 ?

Signed-off-by: Megha Dey <megha.dey@linux.intel.com>
Reported-by: Jan Stancek <jstancek@redhat.com>
---
 arch/x86/crypto/sha1_avx2_x86_64_asm.S | 67 ++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 31 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
 
-- 
1.9.1

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

end of thread, other threads:[~2017-09-18  6:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-23  0:41 [PATCH] crypto: x86/sha1 : Fix reads beyond the number of blocks passed Megha Dey
2017-08-23  0:47 ` Greg KH
2017-08-29 17:08   ` Megha Dey
2017-08-31  6:03     ` Greg KH
2017-08-31  6:06       ` Greg KH
2017-08-31 17:35         ` Megha Dey
2017-09-04  9:34           ` Greg KH
2017-09-18  6:42             ` Greg KH
  -- strict thread matches above, loose matches on Subject: below --
2017-08-02  0:03 Megha Dey

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.