[PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Robert Baronescu]

In case buffer length is a multiple of PAGE_SIZE,
the S/G table is incorrectly generated.
Fix this by handling buflen = k * PAGE_SIZE separately.

Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com>
---
 crypto/tcrypt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 0022a18..bd9b66c 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -221,11 +221,13 @@ static void sg_init_aead(struct scatterlist *sg, char *xbuf[XBUFSIZE],
 	}
 
 	sg_init_table(sg, np + 1);
-	np--;
+	if (rem)
+		np--;
 	for (k = 0; k < np; k++)
 		sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE);
 
-	sg_set_buf(&sg[k + 1], xbuf[k], rem);
+	if (rem)
+		sg_set_buf(&sg[k + 1], xbuf[k], rem);
 }
 
 static void test_aead_speed(const char *algo, int enc, unsigned int secs,
-- 
1.9.3

[PATCH 2/2] crypto: tcrypt - fix buffer lengths in test_aead_speed()

[Robert Baronescu]

Fix the way the length of the buffers used for
encryption / decryption are computed.
For e.g. in case of encryption, input buffer does not contain
an authentication tag.

Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com>
---
 crypto/tcrypt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index bd9b66c..e339960 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -342,7 +342,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int secs,
 			}
 
 			sg_init_aead(sg, xbuf,
-				    *b_size + (enc ? authsize : 0));
+				    *b_size + (enc ? 0 : authsize));
 
 			sg_init_aead(sgout, xoutbuf,
 				    *b_size + (enc ? authsize : 0));
@@ -350,7 +350,9 @@ static void test_aead_speed(const char *algo, int enc, unsigned int secs,
 			sg_set_buf(&sg[0], assoc, aad_size);
 			sg_set_buf(&sgout[0], assoc, aad_size);
 
-			aead_request_set_crypt(req, sg, sgout, *b_size, iv);
+			aead_request_set_crypt(req, sg, sgout,
+					       *b_size + (enc ? 0 : authsize),
+					       iv);
 			aead_request_set_ad(req, aad_size);
 
 			if (secs)
-- 
1.9.3

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Herbert Xu]

On Tue, Oct 10, 2017 at 01:21:59PM +0300, Robert Baronescu wrote:
> In case buffer length is a multiple of PAGE_SIZE,
> the S/G table is incorrectly generated.
> Fix this by handling buflen = k * PAGE_SIZE separately.
> 
> Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com>
> ---
>  crypto/tcrypt.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index 0022a18..bd9b66c 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -221,11 +221,13 @@ static void sg_init_aead(struct scatterlist *sg, char *xbuf[XBUFSIZE],
>  	}
>  
>  	sg_init_table(sg, np + 1);
> -	np--;
> +	if (rem)
> +		np--;
>  	for (k = 0; k < np; k++)
>  		sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE);
>  
> -	sg_set_buf(&sg[k + 1], xbuf[k], rem);
> +	if (rem)
> +		sg_set_buf(&sg[k + 1], xbuf[k], rem);

Sorry but I think this is still buggy because you have not moved the
end-of-table marking in the rem == 0 case.

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

Re: [PATCH 2/2] crypto: tcrypt - fix buffer lengths in test_aead_speed()

[Herbert Xu]

On Tue, Oct 10, 2017 at 01:22:00PM +0300, Robert Baronescu wrote:
> Fix the way the length of the buffers used for
> encryption / decryption are computed.
> For e.g. in case of encryption, input buffer does not contain
> an authentication tag.
> 
> Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com>

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Horia Geantă]

On 11/3/2017 2:42 PM, Herbert Xu wrote:
> On Tue, Oct 10, 2017 at 01:21:59PM +0300, Robert Baronescu wrote:
>> In case buffer length is a multiple of PAGE_SIZE,
>> the S/G table is incorrectly generated.
>> Fix this by handling buflen = k * PAGE_SIZE separately.
>>
>> Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com>
>> ---
>>  crypto/tcrypt.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
>> index 0022a18..bd9b66c 100644
>> --- a/crypto/tcrypt.c
>> +++ b/crypto/tcrypt.c
>> @@ -221,11 +221,13 @@ static void sg_init_aead(struct scatterlist *sg, char *xbuf[XBUFSIZE],
>>  	}
>>  
>>  	sg_init_table(sg, np + 1);
sg_mark_end() marks sg[np].

>> -	np--;
>> +	if (rem)
>> +		np--;
>>  	for (k = 0; k < np; k++)
>>  		sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE);
In case rem == 0, last k value is np-1, thus sg[np-1+1] will be filled
here with xbuf[np-1].

>>  
>> -	sg_set_buf(&sg[k + 1], xbuf[k], rem);
>> +	if (rem)
>> +		sg_set_buf(&sg[k + 1], xbuf[k], rem);
In case rem !=0, sg[np] will be filled here with xbuf[np-1].

> 
> Sorry but I think this is still buggy because you have not moved the
> end-of-table marking in the rem == 0 case.
IIUC this is correct, see above comments.
Could you please take a look again?

Thanks,
Horia

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Herbert Xu]

On Thu, Nov 09, 2017 at 02:37:29PM +0000, Horia Geantă wrote:
>
> >>  	sg_init_table(sg, np + 1);
> sg_mark_end() marks sg[np].
> 
> >> -	np--;
> >> +	if (rem)
> >> +		np--;
> >>  	for (k = 0; k < np; k++)
> >>  		sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE);
> In case rem == 0, last k value is np-1, thus sg[np-1+1] will be filled
> here with xbuf[np-1].

No, if rem == 0, then the last k value is np-2.

The whole point of the patch is to shrink the SG table by one
entry when rem == 0.  How can you shrink it without moving the
end-of-table marker?

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

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Horia Geantă]

On 11/10/2017 12:21 AM, Herbert Xu wrote:
> On Thu, Nov 09, 2017 at 02:37:29PM +0000, Horia Geantă wrote:
>>
>>>>  	sg_init_table(sg, np + 1);
>> sg_mark_end() marks sg[np].
>>
>>>> -	np--;
>>>> +	if (rem)
>>>> +		np--;
>>>>  	for (k = 0; k < np; k++)
>>>>  		sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE);
>> In case rem == 0, last k value is np-1, thus sg[np-1+1] will be filled
>> here with xbuf[np-1].
> 
> No, if rem == 0, then the last k value is np-2.
> 
Notice that np-- above the for loop is done conditionally, so in the for
loop k takes values in [0, np-1].
This means the for loop fills sg[1]...sg[np].

Thanks,
Horia

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Herbert Xu]

On Fri, Nov 10, 2017 at 06:37:22AM +0000, Horia Geantă wrote:
> On 11/10/2017 12:21 AM, Herbert Xu wrote:
> > On Thu, Nov 09, 2017 at 02:37:29PM +0000, Horia Geantă wrote:
> >>
> >>>>  	sg_init_table(sg, np + 1);
> >> sg_mark_end() marks sg[np].
> >>
> >>>> -	np--;
> >>>> +	if (rem)
> >>>> +		np--;
> >>>>  	for (k = 0; k < np; k++)
> >>>>  		sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE);
> >> In case rem == 0, last k value is np-1, thus sg[np-1+1] will be filled
> >> here with xbuf[np-1].
> > 
> > No, if rem == 0, then the last k value is np-2.
> > 
> Notice that np-- above the for loop is done conditionally, so in the for
> loop k takes values in [0, np-1].
> This means the for loop fills sg[1]...sg[np].

I must be missing something.  In the case rem == 0, let's say
the original value of np is npo.  Then at the start of the loop,
np = npo - 1, and at the last iteration, k = npo - 2, so we do

	sg_set_buf(&sg[npo - 1], xbuf[npo - 2], PAGE_SIZE);

While the sg_init_table call sets the end-of-table at

	sg_init_table(sg, npo + 1);

I can't see how this can be right.

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

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Horia Geantă]

On 11/10/2017 9:43 AM, Herbert Xu wrote:
> On Fri, Nov 10, 2017 at 06:37:22AM +0000, Horia Geantă wrote:
>> On 11/10/2017 12:21 AM, Herbert Xu wrote:
>>> On Thu, Nov 09, 2017 at 02:37:29PM +0000, Horia Geantă wrote:
>>>>
>>>>>>  	sg_init_table(sg, np + 1);
>>>> sg_mark_end() marks sg[np].
>>>>
>>>>>> -	np--;
>>>>>> +	if (rem)
>>>>>> +		np--;
>>>>>>  	for (k = 0; k < np; k++)
>>>>>>  		sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE);
>>>> In case rem == 0, last k value is np-1, thus sg[np-1+1] will be filled
>>>> here with xbuf[np-1].
>>>
>>> No, if rem == 0, then the last k value is np-2.
>>>
>> Notice that np-- above the for loop is done conditionally, so in the for
>> loop k takes values in [0, np-1].
>> This means the for loop fills sg[1]...sg[np].
> 
> I must be missing something.  In the case rem == 0, let's say
> the original value of np is npo.  Then at the start of the loop,
> np = npo - 1, and at the last iteration, k = npo - 2, so we do
IIUC at the start of the loop np = npo (and not npo - 1), since np is no
longer decremented in the rem == 0 case:
-	np--;
+	if (rem)
+		np--;

> 
> 	sg_set_buf(&sg[npo - 1], xbuf[npo - 2], PAGE_SIZE);
> 
and accordingly last iteration is for k = npo - 1:
 	sg_set_buf(&sg[npo], xbuf[npo - 1], PAGE_SIZE);

> While the sg_init_table call sets the end-of-table at
> 
> 	sg_init_table(sg, npo + 1);
> 
while this marks sg[npo] as last SG table entry.

Thanks,
Horia

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Herbert Xu]

On Fri, Nov 10, 2017 at 09:17:33AM +0000, Horia Geantă wrote:
>
> > I must be missing something.  In the case rem == 0, let's say
> > the original value of np is npo.  Then at the start of the loop,
> > np = npo - 1, and at the last iteration, k = npo - 2, so we do
> IIUC at the start of the loop np = npo (and not npo - 1), since np is no
> longer decremented in the rem == 0 case:
> -	np--;
> +	if (rem)
> +		np--;
> 
> > 
> > 	sg_set_buf(&sg[npo - 1], xbuf[npo - 2], PAGE_SIZE);
> > 
> and accordingly last iteration is for k = npo - 1:
>  	sg_set_buf(&sg[npo], xbuf[npo - 1], PAGE_SIZE);
> 
> > While the sg_init_table call sets the end-of-table at
> > 
> > 	sg_init_table(sg, npo + 1);
> > 
> while this marks sg[npo] as last SG table entry.

OK, we're both sort of right.  You're correct that this generates
a valid SG list in that the number of entries match the end-of-table
marking.

But the thing that prompted to check this patch in the first place
is the semantics behind it.  For the case rem == 0, it means that
buflen is a multiple of PAGE_SIZE.  In that case, the code with
your patch will create an SG list that's one page longer than
buflen.

AFAICS it should be harmless but it would still be better if we can
generate an SG list that's exactly buflen long rather than one page
longer.

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

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Horia Geantă]

On 11/10/2017 1:23 PM, Herbert Xu wrote:
> On Fri, Nov 10, 2017 at 09:17:33AM +0000, Horia Geantă wrote:
>>
>>> I must be missing something.  In the case rem == 0, let's say
>>> the original value of np is npo.  Then at the start of the loop,
>>> np = npo - 1, and at the last iteration, k = npo - 2, so we do
>> IIUC at the start of the loop np = npo (and not npo - 1), since np is no
>> longer decremented in the rem == 0 case:
>> -	np--;
>> +	if (rem)
>> +		np--;
>>
>>>
>>> 	sg_set_buf(&sg[npo - 1], xbuf[npo - 2], PAGE_SIZE);
>>>
>> and accordingly last iteration is for k = npo - 1:
>>  	sg_set_buf(&sg[npo], xbuf[npo - 1], PAGE_SIZE);
>>
>>> While the sg_init_table call sets the end-of-table at
>>>
>>> 	sg_init_table(sg, npo + 1);
>>>
>> while this marks sg[npo] as last SG table entry.
> 
> OK, we're both sort of right.  You're correct that this generates
> a valid SG list in that the number of entries match the end-of-table
> marking.
> 
> But the thing that prompted to check this patch in the first place
> is the semantics behind it.  For the case rem == 0, it means that
> buflen is a multiple of PAGE_SIZE.  In that case, the code with
> your patch will create an SG list that's one page longer than
> buflen.
> 
SG table always has 1 entry more than what's needed strictly for input data.

Let's say buflen = npo * PAGE_SIZE.
SG table generated by the code will have npo + 1 entries:
-sg[0] - (1 entry) reserved for associated data, filled outside
sg_init_aead()
-sg[1]..sg[npo] (npo entries) - input data, entries pointing to
xbuf[0]..xbuf[npo-1]

Horia

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Tudor Ambarus]

Hi,

On 10/10/2017 01:21 PM, Robert Baronescu wrote:
> In case buffer length is a multiple of PAGE_SIZE,
> the S/G table is incorrectly generated.
> Fix this by handling buflen = k * PAGE_SIZE separately.
> 
> Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com>
> ---
>   crypto/tcrypt.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

This patch fixes the segmentation fault listed below. The NULL
dereference can be seen starting with:
7aacbfc crypto: tcrypt - fix buffer lengths in test_aead_speed()

Cheers,
ta

# insmod tcrypt.ko mode=212

testing speed of rfc4309(ccm(aes)) 
(rfc4309(ccm_base(ctr(aes-generic),cbcmac(aes-generic)))) encryption
test 0 (152 bit key, 16 byte blocks):
1 operation in 0 cycles (16 bytes)
test 1 (152 bit key, 64 byte blocks):
1 operation in 0 cycles (64 bytes)
test 2 (152 bit key, 256 byte blocks):
1 operation in 0 cycles (256 bytes)
test 3 (152 bit key, 512 byte blocks):
1 operation in 0 cycles (512 bytes)
test 4 (152 bit key, 1024 byte blocks):
1 operation in 0 cycles (1024 bytes)
test 5 (152 bit key, 2048 byte blocks):
1 operation in 0 cycles (2048 bytes)
test 6 (152 bit key, 4096 byte blocks):
Unable to handle kernel NULL pointer dereference at virtual address 00000004
pgd = deee0000
[00000004] *pgd=3f6b8831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] ARM
Modules linked in: tcrypt(+)
CPU: 0 PID: 795 Comm: insmod Not tainted 4.14.0-rc3+ #15
Hardware name: Atmel SAMA5
task: def4d000 task.stack: def4a000
PC is at scatterwalk_copychunks+0x14c/0x18c
LR is at scatterwalk_copychunks+0x144/0x18c
pc : [<c02c2d84>]    lr : [<c02c2d7c>]    psr: 20000013
sp : def4bbf8  ip : 00000000  fp : def4bcb4
r10: c02d1e5c  r9 : 00000000  r8 : def4a000
r7 : defd0090  r6 : def4bc58  r5 : 00000010  r4 : 00000000
r3 : dffe71e2  r2 : def4d000  r1 : 00000000  r0 : 00000000
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c53c7d  Table: 3eee0059  DAC: 00000051
Process insmod (pid: 795, stack limit = 0xdef4a208)
Stack: (0xdef4bbf8 to 0xdef4c000)
bbe0:                                                       def4bc48 
00000010
bc00: def4bcbc ffffffff 00000010 00000000 c02d1e5c c02c47f0 00000010 
def4bc28
bc20: deefe110 00000000 deefe200 def11800 c02d1e5c c02cc178 000000e7 
def4bc38
bc40: 00000010 def4bcbc dffd8fc0 defd0090 dffd8fc0 defd0080 00000000 
00000000
bc60: 00001000 def7e2a0 00000000 00001000 00000000 defd0080 deefe200 
00000010
bc80: 00000000 00000010 00000001 00000000 00000000 c02cc0bc 00000000 
ded1a4c0
bca0: 00001000 deefe200 deefe0c0 deefe134 deefe164 c02c509c 00001000 
deda5280
bcc0: deefe200 00000400 deefe100 c02cec9c def4bd70 deefe000 00000000 
deefe000
bce0: 00000000 00000004 00000000 def7e200 bf007144 ded19300 00000000 
bf001950
bd00: 014000c0 bf007234 00000000 00000010 bf0075c0 def7e290 deda7a80 
00000006
bd20: c0a4bd38 00001000 00000000 ded19300 bf007140 ded19340 00000000 
defd0f00
bd40: 00000000 def4bd44 def4bd44 c0176ea4 df60f000 def5c000 def5e000 
deff1000
bd60: df4a5000 df651000 df648000 df646000 deebe000 dee59000 deeae000 
defd1000
bd80: deda0000 defd3000 de806000 def82000 def63000 def78000 deec7000 
deeff000
bda0: deeb9000 deef2000 deeba000 deebd000 00000000 00000000 00000004 
bf0075c0
bdc0: bf007440 defd0f00 bf007488 00000001 2102f11c bf005238 df4ac000 
000075c0
bde0: 00000003 bf0075c0 bf007440 bf0075c0 00000004 bf0075c0 bf007440 
defd0f00
be00: bf007488 bf00a054 bf007440 bf00a000 00000000 c01018e8 00000000 
ded17780
be20: df4ac000 c0a3a72c df420000 c0844a4c c07df704 c01a5054 bf007488 
c0684d38
be40: 00000012 deda7440 defd0f08 a0000013 deda7640 e0a7e000 00000001 
defd0f00
be60: bf007440 defd0f08 deda7640 defd0f00 bf007488 c016203c bf007488 
00000001
be80: def4bf50 defd0f08 00000001 c0161390 bf00744c 00007fff bf007440 
c015ea8c
bea0: 00000000 bf007590 00000578 bf007528 c0844c7c c07018f0 c01b1060 
bf000000
bec0: 0000dcfb 0000dcfb 00000000 00000000 00000000 00000000 00000000 
00000000
bee0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
00000000
bf00: 00000000 00000000 7fffffff 00000000 00000003 00099008 0000017b 
c0107964
bf20: def4a000 00000000 00000000 c0161a68 7fffffff 00000000 00000003 
a0000013
bf40: dedd1c00 e0a7e000 0000dcfb 00000000 e0a83d03 e0a7e000 0000dcfb 
e0a85238
bf60: e0a850dd e0a8b258 00008000 000081d0 00000000 00000000 00000000 
00002e84
bf80: 00000021 00000022 00000019 00000000 00000013 00000000 00099008 
bebd1f45
bfa0: 00000003 c01077a0 00099008 bebd1f45 00000003 00099008 00000000 
bebd1f45
bfc0: 00099008 bebd1f45 00000003 0000017b bebd1f45 00000000 00000000 
00000000
bfe0: bebd1ca8 bebd1c98 0001f99d b6f3f2c4 80000030 00000003 00000000 
00000000
[<c02c2d84>] (scatterwalk_copychunks) from [<c02c47f0>] 
(blkcipher_walk_next+0x3a0/0x44c)
[<c02c47f0>] (blkcipher_walk_next) from [<c02cc178>] 
(crypto_ctr_crypt+0xbc/0x1cc)
[<c02cc178>] (crypto_ctr_crypt) from [<c02c509c>] 
(skcipher_encrypt_blkcipher+0x44/0x4c)
[<c02c509c>] (skcipher_encrypt_blkcipher) from [<c02cec9c>] 
(crypto_ccm_encrypt+0xc8/0xf8)
[<c02cec9c>] (crypto_ccm_encrypt) from [<bf001950>] 
(test_aead_speed.constprop.2+0x3e8/0x5a8 [tcrypt])
[<bf001950>] (test_aead_speed.constprop.2 [tcrypt]) from [<bf005238>] 
(do_test+0x3728/0x3e88 [tcrypt])
[<bf005238>] (do_test [tcrypt]) from [<bf00a054>] 
(tcrypt_mod_init+0x54/0x1000 [tcrypt])
[<bf00a054>] (tcrypt_mod_init [tcrypt]) from [<c01018e8>] 
(do_one_initcall+0x40/0x16c)
[<c01018e8>] (do_one_initcall) from [<c016203c>] (do_init_module+0x60/0x1d8)
[<c016203c>] (do_init_module) from [<c0161390>] (load_module+0x1c4c/0x214c)
[<c0161390>] (load_module) from [<c0161a68>] (SyS_finit_module+0x8c/0x9c)
[<c0161a68>] (SyS_finit_module) from [<c01077a0>] 
(ret_fast_syscall+0x0/0x48)
Code: e1a00001 eb00da5e e5860000 e1a01000 (e590c004)
---[ end trace d97c437cd566fdf4 ]---
Segmentation fault

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Tudor Ambarus]

Hi,

On 11/12/2017 06:26 PM, Horia Geantă wrote:

> -sg[0] - (1 entry) reserved for associated data, filled outside
> sg_init_aead()

Let's fill the sg[0] with aad inside sg_init_aead()!

Cheers,
ta

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Horia Geantă]

On 11/13/2017 8:24 PM, Tudor Ambarus wrote:
> Hi,
> 
> On 10/10/2017 01:21 PM, Robert Baronescu wrote:
>> In case buffer length is a multiple of PAGE_SIZE,
>> the S/G table is incorrectly generated.
>> Fix this by handling buflen = k * PAGE_SIZE separately.
>>
>> Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com>
>> ---
>>   crypto/tcrypt.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> This patch fixes the segmentation fault listed below. The NULL
> dereference can be seen starting with:
> 7aacbfc crypto: tcrypt - fix buffer lengths in test_aead_speed()
> 
Right, the order of the two fixes is unfortunate in the sense that first
fix (commit 7aacbfc) uncovers the issue you mention.

Thanks,
Horia

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Horia Geantă]

On 11/13/2017 8:28 PM, Tudor Ambarus wrote:
> Hi,
> 
> On 11/12/2017 06:26 PM, Horia Geantă wrote:
> 
>> -sg[0] - (1 entry) reserved for associated data, filled outside
>> sg_init_aead()
> 
> Let's fill the sg[0] with aad inside sg_init_aead()!
> 
This could be done, however I would not mix fixes with improvements.

Thanks,
Horia

Re: [PATCH 1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Herbert Xu]

On Sun, Nov 12, 2017 at 04:26:39PM +0000, Horia Geantă wrote:
>
> SG table always has 1 entry more than what's needed strictly for input data.
> 
> Let's say buflen = npo * PAGE_SIZE.
> SG table generated by the code will have npo + 1 entries:
> -sg[0] - (1 entry) reserved for associated data, filled outside
> sg_init_aead()
> -sg[1]..sg[npo] (npo entries) - input data, entries pointing to
> xbuf[0]..xbuf[npo-1]

You are right.  I will apply the patch.

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

[PATCH] crypto: tcrypt - set assoc in sg_init_aead()

[Tudor Ambarus]

Results better code readability.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
Should be applied after:
    crypto: tcrypt - fix S/G table for test_aead_speed()

 crypto/tcrypt.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 3ced1ba..28b4882 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -185,7 +185,8 @@ static void testmgr_free_buf(char *buf[XBUFSIZE])
 }
 
 static void sg_init_aead(struct scatterlist *sg, char *xbuf[XBUFSIZE],
-			unsigned int buflen)
+			 unsigned int buflen, const void *assoc,
+			 unsigned int aad_size)
 {
 	int np = (buflen + PAGE_SIZE - 1)/PAGE_SIZE;
 	int k, rem;
@@ -198,6 +199,9 @@ static void sg_init_aead(struct scatterlist *sg, char *xbuf[XBUFSIZE],
 	}
 
 	sg_init_table(sg, np + 1);
+
+	sg_set_buf(&sg[0], assoc, aad_size);
+
 	if (rem)
 		np--;
 	for (k = 0; k < np; k++)
@@ -318,14 +322,12 @@ static void test_aead_speed(const char *algo, int enc, unsigned int secs,
 				goto out;
 			}
 
-			sg_init_aead(sg, xbuf,
-				    *b_size + (enc ? 0 : authsize));
+			sg_init_aead(sg, xbuf, *b_size + (enc ? 0 : authsize),
+				     assoc, aad_size);
 
 			sg_init_aead(sgout, xoutbuf,
-				    *b_size + (enc ? authsize : 0));
-
-			sg_set_buf(&sg[0], assoc, aad_size);
-			sg_set_buf(&sgout[0], assoc, aad_size);
+				     *b_size + (enc ? authsize : 0), assoc,
+				     aad_size);
 
 			aead_request_set_crypt(req, sg, sgout,
 					       *b_size + (enc ? 0 : authsize),
-- 
2.9.4

Re: [PATCH] crypto: tcrypt - set assoc in sg_init_aead()

[Horia Geantă]

On 11/14/2017 4:59 PM, Tudor Ambarus wrote:
> Results better code readability.
         ^^ *in* better
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>

Horia

Re: [1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Herbert Xu]

On Tue, Oct 10, 2017 at 01:21:59PM +0300, Robert Baronescu wrote:
> In case buffer length is a multiple of PAGE_SIZE,
> the S/G table is incorrectly generated.
> Fix this by handling buflen = k * PAGE_SIZE separately.
> 
> Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com>

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: crypto: tcrypt - set assoc in sg_init_aead()

[Herbert Xu]

On Tue, Nov 14, 2017 at 04:59:15PM +0200, Tudor-Dan Ambarus wrote:
> Results better code readability.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Reviewed-by: Horia Geantă <horia.geanta@nxp.com>

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Horia Geantă]

On 11/29/2017 8:28 AM, Herbert Xu wrote:
> On Tue, Oct 10, 2017 at 01:21:59PM +0300, Robert Baronescu wrote:
>> In case buffer length is a multiple of PAGE_SIZE,
>> the S/G table is incorrectly generated.
>> Fix this by handling buflen = k * PAGE_SIZE separately.
>>
>> Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com>
> 
> Patch applied.  Thanks.
> 
Thanks Herbert.

Considering this fixes the crash reported by Tudor:
https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg29172.html
I think it should be merged in this release cycle (v4.15).

Horia

Re: [1/2] crypto: tcrypt - fix S/G table for test_aead_speed()

[Herbert Xu]

On Wed, Nov 29, 2017 at 08:57:33AM +0000, Horia Geantă wrote:
>
> Considering this fixes the crash reported by Tudor:
> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg29172.html
> I think it should be merged in this release cycle (v4.15).

This only affects tcrypt right? Since tcrypt is merely a debugging
interface I don't think it's that critical.

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