Re: [PATCH v3] crypto: talitos - fix ablkcipher for CONFIG_VMAP_STACK

2019-01-08 Thread Michael Ellerman 
Christophe Leroy  writes:
> Le 04/01/2019 à 16:24, Horia Geanta a écrit :
>> On 1/4/2019 5:17 PM, Horia Geanta wrote:
>>> On 12/21/2018 10:07 AM, Christophe Leroy wrote:
>>> [snip]
 IV cannot be on stack when CONFIG_VMAP_STACK is selected because the stack
 cannot be DMA mapped anymore.
 This looks better, thanks.
>>>
 This patch copies the IV into the extended descriptor when iv is not
 a valid linear address.

>>> Though I am not sure the checks in place are enough.
>>>
 Fixes: 4de9d0b547b9 ("crypto: talitos - Add ablkcipher algorithms")
 Cc: sta...@vger.kernel.org
 Signed-off-by: Christophe Leroy 
 ---
   v3: Using struct edesc buffer.

   v2: Using per-request context.
>>> [snip]
 +  if (ivsize && !virt_addr_valid(iv))
 +  alloc_len += ivsize;
>>> [snip]
   
 +  if (ivsize && !virt_addr_valid(iv))
>>> A more precise condition would be (!is_vmalloc_addr || is_vmalloc_addr(iv))
>>>
>> Sorry for the typo, I meant:
>> (!virt_addr_valid(iv) || is_vmalloc_addr(iv))
>
> As far as I know, virt_addr_valid() means the address is in the linear 
> memory space. So it cannot be a vmalloc_addr if it is a linear space 
> addr, can it ?

That matches my understanding.

This is one of those historical macros that has no documentation and its
behaviour is only defined in terms of other things, so it's kind of hard
to track down.

In commit e41e53cd4fe3 ("powerpc/mm: Fix virt_addr_valid() etc. on
64-bit hash")

https://git.kernel.org/torvalds/c/e41e53cd4fe3

I said:

  virt_addr_valid() is supposed to tell you if it's OK to call virt_to_page() on
  an address. What this means in practice is that it should only return true for
  addresses in the linear mapping which are backed by a valid PFN.


Each arch can define virt_to_page(), so presumably you *could* set
things up such that virt_to_page() worked on vmalloc addresses.

Originally virt_to_page() would basically just mask and right shift the
address and then use that as an array index to get the page. Things are
more complicated now that we have SPARSEMEM etc. but it still only works
for linear mapping addresses.

Hopefully someone who knows mm stuff can chime in and tell us if we're
missing anything :)

cheers

Re: [PATCH v3] crypto: talitos - fix ablkcipher for CONFIG_VMAP_STACK

2019-01-07 Thread Herbert Xu 
On Fri, Dec 21, 2018 at 08:07:52AM +, Christophe Leroy wrote:
> [2.364486] WARNING: CPU: 0 PID: 60 at ./arch/powerpc/include/asm/io.h:837 
> dma_nommu_map_page+0x44/0xd4
> [2.373579] CPU: 0 PID: 60 Comm: cryptomgr_test Tainted: GW
>  4.20.0-rc5-00560-g6bfb52e23a00-dirty #531
> [2.384740] NIP:  c000c540 LR: c000c584 CTR: 
> [2.389743] REGS: c95abab0 TRAP: 0700   Tainted: GW  
> (4.20.0-rc5-00560-g6bfb52e23a00-dirty)
> [2.400042] MSR:  00029032   CR: 24042204  XER: 
> [2.406669]
> [2.406669] GPR00: c02f2244 c95abb60 c6262990 c95abd80 256a 0001 
> 0001 0001
> [2.406669] GPR08:  2000 0010 0010 24042202  
> 0100 c95abd88
> [2.406669] GPR16:  c05569d4 0001 0010 c95abc88 c0615664 
> 0004 
> [2.406669] GPR24: 0010 c95abc88 c95abc88  c61ae210 c7ff6d40 
> c61ae210 3d68
> [2.441559] NIP [c000c540] dma_nommu_map_page+0x44/0xd4
> [2.446720] LR [c000c584] dma_nommu_map_page+0x88/0xd4
> [2.451762] Call Trace:
> [2.454195] [c95abb60] [82000808] 0x82000808 (unreliable)
> [2.459572] [c95abb80] [c02f2244] talitos_edesc_alloc+0xbc/0x3c8
> [2.465493] [c95abbb0] [c02f2600] ablkcipher_edesc_alloc+0x4c/0x5c
> [2.471606] [c95abbd0] [c02f4ed0] ablkcipher_encrypt+0x20/0x64
> [2.477389] [c95abbe0] [c02023b0] __test_skcipher+0x4bc/0xa08
> [2.483049] [c95abe00] [c0204b60] test_skcipher+0x2c/0xcc
> [2.488385] [c95abe20] [c0204c48] alg_test_skcipher+0x48/0xbc
> [2.494064] [c95abe40] [c0205cec] alg_test+0x164/0x2e8
> [2.499142] [c95abf00] [c0200dec] cryptomgr_test+0x48/0x50
> [2.504558] [c95abf10] [c0039ff4] kthread+0xe4/0x110
> [2.509471] [c95abf40] [c000e1d0] ret_from_kernel_thread+0x14/0x1c
> [2.515532] Instruction dump:
> [2.518468] 7c7e1b78 7c9d2378 7cbf2b78 41820054 3d20c076 8089c200 3d20c076 
> 7c84e850
> [2.526127] 8129c204 7c842e70 7f844840 419c0008 <0fe0> 2f9e 
> 54847022 7c84fa14
> [2.533960] ---[ end trace bf78d94af73fe3b8 ]---
> [2.539123] talitos ff02.crypto: master data transfer error
> [2.544775] talitos ff02.crypto: TEA error: ISR 0x2000_0040
> [2.551625] alg: skcipher: encryption failed on test 1 for 
> ecb-aes-talitos: ret=22
> 
> IV cannot be on stack when CONFIG_VMAP_STACK is selected because the stack
> cannot be DMA mapped anymore.
> 
> This patch copies the IV into the extended descriptor when iv is not
> a valid linear address.

Please make the copy unconditional.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] crypto: talitos - fix ablkcipher for CONFIG_VMAP_STACK

2019-01-07 Thread Christophe Leroy 




Le 04/01/2019 à 16:24, Horia Geanta a écrit :

On 1/4/2019 5:17 PM, Horia Geanta wrote:

On 12/21/2018 10:07 AM, Christophe Leroy wrote:
[snip]

IV cannot be on stack when CONFIG_VMAP_STACK is selected because the stack
cannot be DMA mapped anymore.
This looks better, thanks.



This patch copies the IV into the extended descriptor when iv is not
a valid linear address.


Though I am not sure the checks in place are enough.


Fixes: 4de9d0b547b9 ("crypto: talitos - Add ablkcipher algorithms")
Cc: sta...@vger.kernel.org
Signed-off-by: Christophe Leroy 
---
  v3: Using struct edesc buffer.

  v2: Using per-request context.

[snip]

+   if (ivsize && !virt_addr_valid(iv))
+   alloc_len += ivsize;

[snip]
  
+	if (ivsize && !virt_addr_valid(iv))

A more precise condition would be (!is_vmalloc_addr || is_vmalloc_addr(iv))


Sorry for the typo, I meant:
(!virt_addr_valid(iv) || is_vmalloc_addr(iv))


As far as I know, virt_addr_valid() means the address is in the linear 
memory space. So it cannot be a vmalloc_addr if it is a linear space 
addr, can it ?


At least, it is that way on powerpc which is the arch embedding the 
talitos crypto engine. virt_addr_valid() means we are under max_pfn, 
while VMALLOC_START is above max_pfn.


Christophe




It matches the checks in debug_dma_map_single() helper, though I am not sure
they are enough to rule out all exceptions of DMA API.

Re: [PATCH v3] crypto: talitos - fix ablkcipher for CONFIG_VMAP_STACK

2019-01-04 Thread Horia Geanta 
On 1/4/2019 5:17 PM, Horia Geanta wrote:
> On 12/21/2018 10:07 AM, Christophe Leroy wrote:
> [snip]
>> IV cannot be on stack when CONFIG_VMAP_STACK is selected because the stack
>> cannot be DMA mapped anymore.
>> This looks better, thanks.
> 
>> This patch copies the IV into the extended descriptor when iv is not
>> a valid linear address.
>>
> Though I am not sure the checks in place are enough.
> 
>> Fixes: 4de9d0b547b9 ("crypto: talitos - Add ablkcipher algorithms")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Christophe Leroy 
>> ---
>>  v3: Using struct edesc buffer.
>>
>>  v2: Using per-request context.
> [snip]
>> +if (ivsize && !virt_addr_valid(iv))
>> +alloc_len += ivsize;
> [snip]
>>  
>> +if (ivsize && !virt_addr_valid(iv))
> A more precise condition would be (!is_vmalloc_addr || is_vmalloc_addr(iv))
>
Sorry for the typo, I meant:
(!virt_addr_valid(iv) || is_vmalloc_addr(iv))

> It matches the checks in debug_dma_map_single() helper, though I am not sure
> they are enough to rule out all exceptions of DMA API.

Re: [PATCH v3] crypto: talitos - fix ablkcipher for CONFIG_VMAP_STACK

2019-01-04 Thread Horia Geanta 
On 12/21/2018 10:07 AM, Christophe Leroy wrote:
[snip]
> IV cannot be on stack when CONFIG_VMAP_STACK is selected because the stack
> cannot be DMA mapped anymore.
> This looks better, thanks.

> This patch copies the IV into the extended descriptor when iv is not
> a valid linear address.
> 
Though I am not sure the checks in place are enough.

> Fixes: 4de9d0b547b9 ("crypto: talitos - Add ablkcipher algorithms")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Christophe Leroy 
> ---
>  v3: Using struct edesc buffer.
> 
>  v2: Using per-request context.
[snip]
> + if (ivsize && !virt_addr_valid(iv))
> + alloc_len += ivsize;
[snip]
>  
> + if (ivsize && !virt_addr_valid(iv))
A more precise condition would be (!is_vmalloc_addr || is_vmalloc_addr(iv))

It matches the checks in debug_dma_map_single() helper, though I am not sure
they are enough to rule out all exceptions of DMA API.

> + iv = memcpy(((u8 *)edesc) + alloc_len - ivsize, iv, ivsize);
> +
>   edesc->src_nents = src_nents;
>   edesc->dst_nents = dst_nents;
> - edesc->iv_dma = iv_dma;
> + if (ivsize)
> + edesc->iv_dma = dma_map_single(dev, iv, ivsize, DMA_TO_DEVICE);
> + else
> + edesc->iv_dma = 0;
> +

[PATCH v3] crypto: talitos - fix ablkcipher for CONFIG_VMAP_STACK

2018-12-21 Thread Christophe Leroy 
[2.364486] WARNING: CPU: 0 PID: 60 at ./arch/powerpc/include/asm/io.h:837 
dma_nommu_map_page+0x44/0xd4
[2.373579] CPU: 0 PID: 60 Comm: cryptomgr_test Tainted: GW 
4.20.0-rc5-00560-g6bfb52e23a00-dirty #531
[2.384740] NIP:  c000c540 LR: c000c584 CTR: 
[2.389743] REGS: c95abab0 TRAP: 0700   Tainted: GW  
(4.20.0-rc5-00560-g6bfb52e23a00-dirty)
[2.400042] MSR:  00029032   CR: 24042204  XER: 
[2.406669]
[2.406669] GPR00: c02f2244 c95abb60 c6262990 c95abd80 256a 0001 
0001 0001
[2.406669] GPR08:  2000 0010 0010 24042202  
0100 c95abd88
[2.406669] GPR16:  c05569d4 0001 0010 c95abc88 c0615664 
0004 
[2.406669] GPR24: 0010 c95abc88 c95abc88  c61ae210 c7ff6d40 
c61ae210 3d68
[2.441559] NIP [c000c540] dma_nommu_map_page+0x44/0xd4
[2.446720] LR [c000c584] dma_nommu_map_page+0x88/0xd4
[2.451762] Call Trace:
[2.454195] [c95abb60] [82000808] 0x82000808 (unreliable)
[2.459572] [c95abb80] [c02f2244] talitos_edesc_alloc+0xbc/0x3c8
[2.465493] [c95abbb0] [c02f2600] ablkcipher_edesc_alloc+0x4c/0x5c
[2.471606] [c95abbd0] [c02f4ed0] ablkcipher_encrypt+0x20/0x64
[2.477389] [c95abbe0] [c02023b0] __test_skcipher+0x4bc/0xa08
[2.483049] [c95abe00] [c0204b60] test_skcipher+0x2c/0xcc
[2.488385] [c95abe20] [c0204c48] alg_test_skcipher+0x48/0xbc
[2.494064] [c95abe40] [c0205cec] alg_test+0x164/0x2e8
[2.499142] [c95abf00] [c0200dec] cryptomgr_test+0x48/0x50
[2.504558] [c95abf10] [c0039ff4] kthread+0xe4/0x110
[2.509471] [c95abf40] [c000e1d0] ret_from_kernel_thread+0x14/0x1c
[2.515532] Instruction dump:
[2.518468] 7c7e1b78 7c9d2378 7cbf2b78 41820054 3d20c076 8089c200 3d20c076 
7c84e850
[2.526127] 8129c204 7c842e70 7f844840 419c0008 <0fe0> 2f9e 54847022 
7c84fa14
[2.533960] ---[ end trace bf78d94af73fe3b8 ]---
[2.539123] talitos ff02.crypto: master data transfer error
[2.544775] talitos ff02.crypto: TEA error: ISR 0x2000_0040
[2.551625] alg: skcipher: encryption failed on test 1 for ecb-aes-talitos: 
ret=22

IV cannot be on stack when CONFIG_VMAP_STACK is selected because the stack
cannot be DMA mapped anymore.

This patch copies the IV into the extended descriptor when iv is not
a valid linear address.

Fixes: 4de9d0b547b9 ("crypto: talitos - Add ablkcipher algorithms")
Cc: sta...@vger.kernel.org
Signed-off-by: Christophe Leroy 
---
 v3: Using struct edesc buffer.

 v2: Using per-request context.

 drivers/crypto/talitos.c | 35 +++
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 6988012deca4..160702b119bb 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1355,29 +1355,23 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
 {
struct talitos_edesc *edesc;
int src_nents, dst_nents, alloc_len, dma_len, src_len, dst_len;
-   dma_addr_t iv_dma = 0;
gfp_t flags = cryptoflags & CRYPTO_TFM_REQ_MAY_SLEEP ? GFP_KERNEL :
  GFP_ATOMIC;
struct talitos_private *priv = dev_get_drvdata(dev);
bool is_sec1 = has_ftr_sec1(priv);
int max_len = is_sec1 ? TALITOS1_MAX_DATA_LEN : TALITOS2_MAX_DATA_LEN;
-   void *err;
 
if (cryptlen + authsize > max_len) {
dev_err(dev, "length exceeds h/w max limit\n");
return ERR_PTR(-EINVAL);
}
 
-   if (ivsize)
-   iv_dma = dma_map_single(dev, iv, ivsize, DMA_TO_DEVICE);
-
if (!dst || dst == src) {
src_len = assoclen + cryptlen + authsize;
src_nents = sg_nents_for_len(src, src_len);
if (src_nents < 0) {
dev_err(dev, "Invalid number of src SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
+   return ERR_PTR(-EINVAL);
}
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_nents = dst ? src_nents : 0;
@@ -1387,16 +1381,14 @@ static struct talitos_edesc *talitos_edesc_alloc(struct 
device *dev,
src_nents = sg_nents_for_len(src, src_len);
if (src_nents < 0) {
dev_err(dev, "Invalid number of src SG.\n");
-   err = ERR_PTR(-EINVAL);
-   goto error_sg;
+   return ERR_PTR(-EINVAL);
}
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_len = assoclen + cryptlen + (encrypt ? authsize : 0);
dst_nents = sg_nents_for_len(dst, dst_len);
if (dst_nents < 0) {
dev_err(dev, "Invalid number of dst SG.\n");
-   err = ERR_PTR(-EINVAL);
-