All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: Kernel panic - encryption/decryption failed when open file on Arm64
       [not found] <57D15BD3.40903@huawei.com>
@ 2016-09-08 12:47 ` Herbert Xu
  2016-09-09  4:08   ` xiakaixu
  2016-09-09 10:19   ` xiakaixu
  0 siblings, 2 replies; 10+ messages in thread
From: Herbert Xu @ 2016-09-08 12:47 UTC (permalink / raw)
  To: xiakaixu
  Cc: davem, tytso, jaegeuk, nhorman, ard.biesheuvel, mh1,
	linux-crypto, linux-kernel, Bintian, liushuoran, Huxinwei,
	zhangzhibin.zhang

On Thu, Sep 08, 2016 at 08:38:43PM +0800, xiakaixu wrote:
> Hi,
> 
> I am using the encryption/decryption feature on arm64 board and a kernel
> panic occurs just when open a file.  As the memory size of the board
> is limited
> and there are some page allocation failures before the panic.
> 
> Seems it is a kernel bug from the call trace log.
> 
>     ...
>     - fscrypt_get_encryption_info
>       - get_crypt_info.part.1
>        - validate_user_key.isra.0
>         - derive_aes_gcm_key
>          - crypto_gcm_decrypt
>           - ablk_decrypt
>            - ctr_encrypt
>             - blkcipher_walk_done
>               - blkcipher_walk_next
>                -  __get_free_pages
> ----------------------------------> page allocation failure
>       ...
>            - aes_ctr_encrypt
> -----------------------------------------> the input parameter is
> NULL pointer as the page allocation failure
> 
> 
> The input parameter of function aes_ctr_encrypt() comes from the
> /struct blkcipher_walk//
> //walk/, and this variable /walk /is allocated by the function
> __get_free_pages(). So if this
> page allocate failed, the input parameter of function
> aes_ctr_encrypt() will be NULL. The
> panic will occurs if we don't check the input parameter.
> 
> Not sure about this and wish to get your opinions!

If the page allocation fails in blkcipher_walk_next it'll simply
switch over to processing it block by block. so I don't think the
warning is related to the crash.

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

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

* Re: Kernel panic - encryption/decryption failed when open file on Arm64
  2016-09-08 12:47 ` Kernel panic - encryption/decryption failed when open file on Arm64 Herbert Xu
@ 2016-09-09  4:08   ` xiakaixu
  2016-09-09 10:19   ` xiakaixu
  1 sibling, 0 replies; 10+ messages in thread
From: xiakaixu @ 2016-09-09  4:08 UTC (permalink / raw)
  To: Herbert Xu
  Cc: davem, tytso, jaegeuk, nhorman, ard.biesheuvel, mh1,
	linux-crypto, linux-kernel, Bintian, liushuoran, Huxinwei,
	zhangzhibin.zhang

Sorry for resend this email, just add the linux-crypto@vger.kernel.org
and linux-kernel@vger.kernel.org.


Hi,

Firstly, thanks for your reply!

To reproduce this kernel panic, I test the encryption/decryption feature 
on arm64 board with more memory. Just add the following
change:

     diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
     index 0122bec..10ef3f4 100644
     --- a/crypto/blkcipher.c
     +++ b/crypto/blkcipher.c
     @@ -240,6 +240,7 @@ static int blkcipher_walk_next(struct 
blkcipher_desc *desc,
                     walk->flags |= BLKCIPHER_WALK_COPY;
                     if (!walk->page) {
                             walk->page = (void 
*)__get_free_page(GFP_ATOMIC);
     +                      walk->page = NULL;
                             if (!walk->page)
                                     n = 0;
                     }


This change just set the walk->page to NULL manually.
I get the same crash when open file with the above change log. So I 
think this NULL page failure is not be handled correctly in current code.

Regards
Kaixu Xia

> On Thu, Sep 08, 2016 at 08:38:43PM +0800, xiakaixu wrote:
>> Hi,
>>
>> I am using the encryption/decryption feature on arm64 board and a kernel
>> panic occurs just when open a file.  As the memory size of the board
>> is limited
>> and there are some page allocation failures before the panic.
>>
>> Seems it is a kernel bug from the call trace log.
>>
>>      ...
>>      - fscrypt_get_encryption_info
>>        - get_crypt_info.part.1
>>         - validate_user_key.isra.0
>>          - derive_aes_gcm_key
>>           - crypto_gcm_decrypt
>>            - ablk_decrypt
>>             - ctr_encrypt
>>              - blkcipher_walk_done
>>                - blkcipher_walk_next
>>                 -  __get_free_pages
>> ----------------------------------> page allocation failure
>>        ...
>>             - aes_ctr_encrypt
>> -----------------------------------------> the input parameter is
>> NULL pointer as the page allocation failure
>>
>>
>> The input parameter of function aes_ctr_encrypt() comes from the
>> /struct blkcipher_walk//
>> //walk/, and this variable /walk /is allocated by the function
>> __get_free_pages(). So if this
>> page allocate failed, the input parameter of function
>> aes_ctr_encrypt() will be NULL. The
>> panic will occurs if we don't check the input parameter.
>>
>> Not sure about this and wish to get your opinions!
>
> If the page allocation fails in blkcipher_walk_next it'll simply
> switch over to processing it block by block. so I don't think the
> warning is related to the crash.
>
> Cheers,
>

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

* Re: Kernel panic - encryption/decryption failed when open file on Arm64
  2016-09-08 12:47 ` Kernel panic - encryption/decryption failed when open file on Arm64 Herbert Xu
  2016-09-09  4:08   ` xiakaixu
@ 2016-09-09 10:19   ` xiakaixu
  2016-09-09 10:31     ` Ard Biesheuvel
  1 sibling, 1 reply; 10+ messages in thread
From: xiakaixu @ 2016-09-09 10:19 UTC (permalink / raw)
  To: Herbert Xu
  Cc: davem, tytso, jaegeuk, nhorman, ard.biesheuvel, mh1,
	linux-crypto, linux-kernel, Bintian, liushuoran, Huxinwei,
	zhangzhibin.zhang

Hi,

After a deeply research about this crash, seems it is a specific
bug that only exists in armv8 board. And it occurs in this function
in arch/arm64/crypto/aes-glue.c.

static int ctr_encrypt(struct blkcipher_desc *desc, struct scatterlist *dst,
                        struct scatterlist *src, unsigned int nbytes)
{
        ...

         desc->flags &= ~CRYPTO_TFM_REQ_MAY_SLEEP;
         blkcipher_walk_init(&walk, dst, src, nbytes);
         err = blkcipher_walk_virt_block(desc, &walk, AES_BLOCK_SIZE); ---> page allocation failed

	...

         while ((blocks = (walk.nbytes / AES_BLOCK_SIZE))) {           ----> walk.nbytes = 0, and skip this loop
                 aes_ctr_encrypt(walk.dst.virt.addr, walk.src.virt.addr,
                                 (u8 *)ctx->key_enc, rounds, blocks, walk.iv,
                                 first);
	...
                 err = blkcipher_walk_done(desc, &walk,
                                           walk.nbytes % AES_BLOCK_SIZE);
         }
         if (nbytes) {                                                 ----> enter this if() statement
                 u8 *tdst = walk.dst.virt.addr + blocks * AES_BLOCK_SIZE;
                 u8 *tsrc = walk.src.virt.addr + blocks * AES_BLOCK_SIZE;
	...

                 aes_ctr_encrypt(tail, tsrc, (u8 *)ctx->key_enc, rounds,  ----> the the sencond input parameter is NULL, so crash...
                                 blocks, walk.iv, first);
	...
         }
         ...
}


If the page allocation failed in the function blkcipher_walk_virt_block(),
the variable walk.nbytes = 0, so it will skip the while() loop and enter
the if(nbytes) statment. But here the varibale tsrc is NULL and it is also
the sencond input parameter of the function aes_ctr_encrypt()... Kernel Panic...

I have also researched the similar function in other architectures, and
there if(walk.nbytes) is used, not this if(nbytes) statement in the armv8.
so I think this armv8 function ctr_encrypt() should deal with the page
allocation failed situation.

Regards
Kaixu Xia


> On Thu, Sep 08, 2016 at 08:38:43PM +0800, xiakaixu wrote:
>> Hi,
>>
>> I am using the encryption/decryption feature on arm64 board and a kernel
>> panic occurs just when open a file.  As the memory size of the board
>> is limited
>> and there are some page allocation failures before the panic.
>>
>> Seems it is a kernel bug from the call trace log.
>>
>>      ...
>>      - fscrypt_get_encryption_info
>>        - get_crypt_info.part.1
>>         - validate_user_key.isra.0
>>          - derive_aes_gcm_key
>>           - crypto_gcm_decrypt
>>            - ablk_decrypt
>>             - ctr_encrypt
>>              - blkcipher_walk_done
>>                - blkcipher_walk_next
>>                 -  __get_free_pages
>> ----------------------------------> page allocation failure
>>        ...
>>             - aes_ctr_encrypt
>> -----------------------------------------> the input parameter is
>> NULL pointer as the page allocation failure
>>
>>
>> The input parameter of function aes_ctr_encrypt() comes from the
>> /struct blkcipher_walk//
>> //walk/, and this variable /walk /is allocated by the function
>> __get_free_pages(). So if this
>> page allocate failed, the input parameter of function
>> aes_ctr_encrypt() will be NULL. The
>> panic will occurs if we don't check the input parameter.
>>
>> Not sure about this and wish to get your opinions!
>
> If the page allocation fails in blkcipher_walk_next it'll simply
> switch over to processing it block by block. so I don't think the
> warning is related to the crash.
>
> Cheers,
>

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

* Re: Kernel panic - encryption/decryption failed when open file on Arm64
  2016-09-09 10:19   ` xiakaixu
@ 2016-09-09 10:31     ` Ard Biesheuvel
  2016-09-09 10:56       ` Ard Biesheuvel
  0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-09 10:31 UTC (permalink / raw)
  To: xiakaixu
  Cc: Herbert Xu, David S. Miller, Theodore Ts'o, Jaegeuk Kim,
	nhorman, mh1, linux-crypto, linux-kernel, Bintian, liushuoran,
	Huxinwei, zhangzhibin.zhang

On 9 September 2016 at 11:19, xiakaixu <xiakaixu@huawei.com> wrote:
> Hi,
>
> After a deeply research about this crash, seems it is a specific
> bug that only exists in armv8 board. And it occurs in this function
> in arch/arm64/crypto/aes-glue.c.
>
> static int ctr_encrypt(struct blkcipher_desc *desc, struct scatterlist *dst,
>                        struct scatterlist *src, unsigned int nbytes)
> {
>        ...
>
>         desc->flags &= ~CRYPTO_TFM_REQ_MAY_SLEEP;
>         blkcipher_walk_init(&walk, dst, src, nbytes);
>         err = blkcipher_walk_virt_block(desc, &walk, AES_BLOCK_SIZE); --->
> page allocation failed
>
>         ...
>
>         while ((blocks = (walk.nbytes / AES_BLOCK_SIZE))) {           ---->
> walk.nbytes = 0, and skip this loop
>                 aes_ctr_encrypt(walk.dst.virt.addr, walk.src.virt.addr,
>                                 (u8 *)ctx->key_enc, rounds, blocks, walk.iv,
>                                 first);
>         ...
>                 err = blkcipher_walk_done(desc, &walk,
>                                           walk.nbytes % AES_BLOCK_SIZE);
>         }
>         if (nbytes) {                                                 ---->
> enter this if() statement
>                 u8 *tdst = walk.dst.virt.addr + blocks * AES_BLOCK_SIZE;
>                 u8 *tsrc = walk.src.virt.addr + blocks * AES_BLOCK_SIZE;
>         ...
>
>                 aes_ctr_encrypt(tail, tsrc, (u8 *)ctx->key_enc, rounds,
> ----> the the sencond input parameter is NULL, so crash...
>                                 blocks, walk.iv, first);
>         ...
>         }
>         ...
> }
>
>
> If the page allocation failed in the function blkcipher_walk_virt_block(),
> the variable walk.nbytes = 0, so it will skip the while() loop and enter
> the if(nbytes) statment. But here the varibale tsrc is NULL and it is also
> the sencond input parameter of the function aes_ctr_encrypt()... Kernel
> Panic...
>
> I have also researched the similar function in other architectures, and
> there if(walk.nbytes) is used, not this if(nbytes) statement in the armv8.
> so I think this armv8 function ctr_encrypt() should deal with the page
> allocation failed situation.
>

OK, thanks for the report, and for the analysis. I will investigate,
and propose a fix

Thanks,
Ard.

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

* Re: Kernel panic - encryption/decryption failed when open file on Arm64
  2016-09-09 10:31     ` Ard Biesheuvel
@ 2016-09-09 10:56       ` Ard Biesheuvel
  2016-09-12  2:16         ` liushuoran
  0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-09 10:56 UTC (permalink / raw)
  To: xiakaixu
  Cc: Herbert Xu, David S. Miller, Theodore Ts'o, Jaegeuk Kim,
	nhorman, mh1, linux-crypto, linux-kernel, Bintian, liushuoran,
	Huxinwei, zhangzhibin.zhang

On 9 September 2016 at 11:31, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 9 September 2016 at 11:19, xiakaixu <xiakaixu@huawei.com> wrote:
>> Hi,
>>
>> After a deeply research about this crash, seems it is a specific
>> bug that only exists in armv8 board. And it occurs in this function
>> in arch/arm64/crypto/aes-glue.c.
>>
>> static int ctr_encrypt(struct blkcipher_desc *desc, struct scatterlist *dst,
>>                        struct scatterlist *src, unsigned int nbytes)
>> {
>>        ...
>>
>>         desc->flags &= ~CRYPTO_TFM_REQ_MAY_SLEEP;
>>         blkcipher_walk_init(&walk, dst, src, nbytes);
>>         err = blkcipher_walk_virt_block(desc, &walk, AES_BLOCK_SIZE); --->
>> page allocation failed
>>
>>         ...
>>
>>         while ((blocks = (walk.nbytes / AES_BLOCK_SIZE))) {           ---->
>> walk.nbytes = 0, and skip this loop
>>                 aes_ctr_encrypt(walk.dst.virt.addr, walk.src.virt.addr,
>>                                 (u8 *)ctx->key_enc, rounds, blocks, walk.iv,
>>                                 first);
>>         ...
>>                 err = blkcipher_walk_done(desc, &walk,
>>                                           walk.nbytes % AES_BLOCK_SIZE);
>>         }
>>         if (nbytes) {                                                 ---->
>> enter this if() statement
>>                 u8 *tdst = walk.dst.virt.addr + blocks * AES_BLOCK_SIZE;
>>                 u8 *tsrc = walk.src.virt.addr + blocks * AES_BLOCK_SIZE;
>>         ...
>>
>>                 aes_ctr_encrypt(tail, tsrc, (u8 *)ctx->key_enc, rounds,
>> ----> the the sencond input parameter is NULL, so crash...
>>                                 blocks, walk.iv, first);
>>         ...
>>         }
>>         ...
>> }
>>
>>
>> If the page allocation failed in the function blkcipher_walk_virt_block(),
>> the variable walk.nbytes = 0, so it will skip the while() loop and enter
>> the if(nbytes) statment. But here the varibale tsrc is NULL and it is also
>> the sencond input parameter of the function aes_ctr_encrypt()... Kernel
>> Panic...
>>
>> I have also researched the similar function in other architectures, and
>> there if(walk.nbytes) is used, not this if(nbytes) statement in the armv8.
>> so I think this armv8 function ctr_encrypt() should deal with the page
>> allocation failed situation.
>>

Does this solve your problem?

diff --git a/arch/arm64/crypto/aes-glue.c b/arch/arm64/crypto/aes-glue.c
index 5c888049d061..6b2aa0fd6cd0 100644
--- a/arch/arm64/crypto/aes-glue.c
+++ b/arch/arm64/crypto/aes-glue.c
@@ -216,7 +216,7 @@ static int ctr_encrypt(struct blkcipher_desc
*desc, struct scatterlist *dst,
                err = blkcipher_walk_done(desc, &walk,
                                          walk.nbytes % AES_BLOCK_SIZE);
        }
-       if (nbytes) {
+       if (walk.nbytes % AES_BLOCK_SIZE) {
                u8 *tdst = walk.dst.virt.addr + blocks * AES_BLOCK_SIZE;
                u8 *tsrc = walk.src.virt.addr + blocks * AES_BLOCK_SIZE;
                u8 __aligned(8) tail[AES_BLOCK_SIZE];

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

* RE: Kernel panic - encryption/decryption failed when open file on Arm64
  2016-09-09 10:56       ` Ard Biesheuvel
@ 2016-09-12  2:16         ` liushuoran
  2016-09-12 17:40           ` Ard Biesheuvel
  0 siblings, 1 reply; 10+ messages in thread
From: liushuoran @ 2016-09-12  2:16 UTC (permalink / raw)
  To: Ard Biesheuvel, Xiakaixu
  Cc: Herbert Xu, David S. Miller, Theodore Ts'o, Jaegeuk Kim,
	nhorman, mh1, linux-crypto, linux-kernel, Wangbintian, Huxinwei,
	zhangzhibin (C)

Hi Ard,

Thanks for the prompt reply. With the patch, there is no panic anymore. But it seems that the encryption/decryption is not successful anyway.

As Herbert points out, "If the page allocation fails in blkcipher_walk_next it'll simply switch over to processing it block by block". So does that mean the encryption/decryption should be successful even if the page allocation fails? Please correct me if I misunderstand anything. Thanks in advance.

Regards,
Shuoran

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, September 09, 2016 6:57 PM
> To: Xiakaixu
> Cc: Herbert Xu; David S. Miller; Theodore Ts'o; Jaegeuk Kim;
> nhorman@tuxdriver.com; mh1@iki.fi; linux-crypto@vger.kernel.org;
> linux-kernel@vger.kernel.org; Wangbintian; liushuoran; Huxinwei; zhangzhibin
> (C)
> Subject: Re: Kernel panic - encryption/decryption failed when open file on
> Arm64
> 
> On 9 September 2016 at 11:31, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> wrote:
> > On 9 September 2016 at 11:19, xiakaixu <xiakaixu@huawei.com> wrote:
> >> Hi,
> >>
> >> After a deeply research about this crash, seems it is a specific
> >> bug that only exists in armv8 board. And it occurs in this function
> >> in arch/arm64/crypto/aes-glue.c.
> >>
> >> static int ctr_encrypt(struct blkcipher_desc *desc, struct scatterlist *dst,
> >>                        struct scatterlist *src, unsigned int nbytes)
> >> {
> >>        ...
> >>
> >>         desc->flags &= ~CRYPTO_TFM_REQ_MAY_SLEEP;
> >>         blkcipher_walk_init(&walk, dst, src, nbytes);
> >>         err = blkcipher_walk_virt_block(desc, &walk, AES_BLOCK_SIZE);
> --->
> >> page allocation failed
> >>
> >>         ...
> >>
> >>         while ((blocks = (walk.nbytes / AES_BLOCK_SIZE)))
> {           ---->
> >> walk.nbytes = 0, and skip this loop
> >>                 aes_ctr_encrypt(walk.dst.virt.addr, walk.src.virt.addr,
> >>                                 (u8 *)ctx->key_enc, rounds, blocks,
> walk.iv,
> >>                                 first);
> >>         ...
> >>                 err = blkcipher_walk_done(desc, &walk,
> >>                                           walk.nbytes %
> AES_BLOCK_SIZE);
> >>         }
> >>         if (nbytes)
> {                                                 ---->
> >> enter this if() statement
> >>                 u8 *tdst = walk.dst.virt.addr + blocks * AES_BLOCK_SIZE;
> >>                 u8 *tsrc = walk.src.virt.addr + blocks * AES_BLOCK_SIZE;
> >>         ...
> >>
> >>                 aes_ctr_encrypt(tail, tsrc, (u8 *)ctx->key_enc, rounds,
> >> ----> the the sencond input parameter is NULL, so crash...
> >>                                 blocks, walk.iv, first);
> >>         ...
> >>         }
> >>         ...
> >> }
> >>
> >>
> >> If the page allocation failed in the function blkcipher_walk_virt_block(),
> >> the variable walk.nbytes = 0, so it will skip the while() loop and enter
> >> the if(nbytes) statment. But here the varibale tsrc is NULL and it is also
> >> the sencond input parameter of the function aes_ctr_encrypt()... Kernel
> >> Panic...
> >>
> >> I have also researched the similar function in other architectures, and
> >> there if(walk.nbytes) is used, not this if(nbytes) statement in the armv8.
> >> so I think this armv8 function ctr_encrypt() should deal with the page
> >> allocation failed situation.
> >>
> 
> Does this solve your problem?
> 
> diff --git a/arch/arm64/crypto/aes-glue.c b/arch/arm64/crypto/aes-glue.c
> index 5c888049d061..6b2aa0fd6cd0 100644
> --- a/arch/arm64/crypto/aes-glue.c
> +++ b/arch/arm64/crypto/aes-glue.c
> @@ -216,7 +216,7 @@ static int ctr_encrypt(struct blkcipher_desc
> *desc, struct scatterlist *dst,
>                 err = blkcipher_walk_done(desc, &walk,
>                                           walk.nbytes % AES_BLOCK_SIZE);
>         }
> -       if (nbytes) {
> +       if (walk.nbytes % AES_BLOCK_SIZE) {
>                 u8 *tdst = walk.dst.virt.addr + blocks * AES_BLOCK_SIZE;
>                 u8 *tsrc = walk.src.virt.addr + blocks * AES_BLOCK_SIZE;
>                 u8 __aligned(8) tail[AES_BLOCK_SIZE];

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

* Re: Kernel panic - encryption/decryption failed when open file on Arm64
  2016-09-12  2:16         ` liushuoran
@ 2016-09-12 17:40           ` Ard Biesheuvel
  2016-09-13  2:05             ` xiakaixu
  2016-09-13  6:43             ` Herbert Xu
  0 siblings, 2 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-12 17:40 UTC (permalink / raw)
  To: liushuoran
  Cc: Xiakaixu, Herbert Xu, David S. Miller, Theodore Ts'o,
	Jaegeuk Kim, nhorman, mh1, linux-crypto, linux-kernel,
	Wangbintian, Huxinwei, zhangzhibin (C)

On 12 September 2016 at 03:16, liushuoran <liushuoran@huawei.com> wrote:
> Hi Ard,
>
> Thanks for the prompt reply. With the patch, there is no panic anymore. But it seems that the encryption/decryption is not successful anyway.
>
> As Herbert points out, "If the page allocation fails in blkcipher_walk_next it'll simply switch over to processing it block by block". So does that mean the encryption/decryption should be successful even if the page allocation fails? Please correct me if I misunderstand anything. Thanks in advance.
>

Perhaps Herbert can explain: I don't see how the 'n = 0' assignment
results in the correct path being taken; this chunk (blkcipher.c:252)

if (unlikely(n < bsize)) {
    err = blkcipher_next_slow(desc, walk, bsize, walk->alignmask);
    goto set_phys_lowmem;
}

is skipped due to the fact that n == 0 and therefore bsize == 0, and
so the condition is always false for n == 0

Therefore we end up here (blkcipher.c:257)

walk->nbytes = n;
if (walk->flags & BLKCIPHER_WALK_COPY) {
    err = blkcipher_next_copy(walk);
    goto set_phys_lowmem;
}

where blkcipher_next_copy() unconditionally calls memcpy() with
walk->page as destination (even though we ended up here due to the
fact that walk->page == NULL)

So to me, it seems like we should be taking the blkcipher_next_slow()
path, which does a kmalloc() and bails with -ENOMEM if that fails.

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

* Re: Kernel panic - encryption/decryption failed when open file on Arm64
  2016-09-12 17:40           ` Ard Biesheuvel
@ 2016-09-13  2:05             ` xiakaixu
  2016-09-13  6:43             ` Herbert Xu
  1 sibling, 0 replies; 10+ messages in thread
From: xiakaixu @ 2016-09-13  2:05 UTC (permalink / raw)
  To: Ard Biesheuvel, Herbert Xu
  Cc: liushuoran, David S. Miller, Theodore Ts'o, Jaegeuk Kim,
	nhorman, mh1, linux-crypto, linux-kernel, Wangbintian, Huxinwei,
	zhangzhibin (C)

> On 12 September 2016 at 03:16, liushuoran <liushuoran@huawei.com> wrote:
>> Hi Ard,
>>
>> Thanks for the prompt reply. With the patch, there is no panic anymore. But it seems that the encryption/decryption is not successful anyway.
>>
>> As Herbert points out, "If the page allocation fails in blkcipher_walk_next it'll simply switch over to processing it block by block". So does that mean the encryption/decryption should be successful even if the page allocation fails? Please correct me if I misunderstand anything. Thanks in advance.
>>
>
> Perhaps Herbert can explain: I don't see how the 'n = 0' assignment
> results in the correct path being taken; this chunk (blkcipher.c:252)
>
> if (unlikely(n < bsize)) {
>      err = blkcipher_next_slow(desc, walk, bsize, walk->alignmask);
>      goto set_phys_lowmem;
> }
>
> is skipped due to the fact that n == 0 and therefore bsize == 0, and
> so the condition is always false for n == 0
>
> Therefore we end up here (blkcipher.c:257)
>
> walk->nbytes = n;
> if (walk->flags & BLKCIPHER_WALK_COPY) {
>      err = blkcipher_next_copy(walk);
>      goto set_phys_lowmem;
> }
>
> where blkcipher_next_copy() unconditionally calls memcpy() with
> walk->page as destination (even though we ended up here due to the
> fact that walk->page == NULL)
>
> So to me, it seems like we should be taking the blkcipher_next_slow()
> path, which does a kmalloc() and bails with -ENOMEM if that fails.

Hi Ard,

Thanks for such a detailed reply.

According to your reply, I just make a little change to take the
blkcipher_next_slow() path. I test it on arm64 board, there is
no panic anymore and seems the encryption/decryption is successful.

diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
index 0122bec..5389d40 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -240,12 +240,13 @@ static int blkcipher_walk_next(struct blkcipher_desc *desc,
                 walk->flags |= BLKCIPHER_WALK_COPY;
                 if (!walk->page) {
                         walk->page = (void *)__get_free_page(GFP_ATOMIC);
+                       walk->page = NULL;
                         if (!walk->page)
                                 n = 0;
                 }
         }

-       bsize = min(walk->walk_blocksize, n);
+       bsize = walk->walk_blocksize;
         n = scatterwalk_clamp(&walk->in, n);
         n = scatterwalk_clamp(&walk->out, n);

It is just a trial and not sure it makes sense. But anyway, we can do
something here to fix the crash result from the page allocation failure.

What's your opinions, Herbert?

Regards
Kaixu Xia
>
> .
>

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

* Re: Kernel panic - encryption/decryption failed when open file on Arm64
  2016-09-12 17:40           ` Ard Biesheuvel
  2016-09-13  2:05             ` xiakaixu
@ 2016-09-13  6:43             ` Herbert Xu
  2016-09-13  7:56               ` Ard Biesheuvel
  1 sibling, 1 reply; 10+ messages in thread
From: Herbert Xu @ 2016-09-13  6:43 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: liushuoran, Xiakaixu, David S. Miller, Theodore Ts'o,
	Jaegeuk Kim, nhorman, mh1, linux-crypto, linux-kernel,
	Wangbintian, Huxinwei, zhangzhibin (C)

On Mon, Sep 12, 2016 at 06:40:15PM +0100, Ard Biesheuvel wrote:
>
> So to me, it seems like we should be taking the blkcipher_next_slow()
> path, which does a kmalloc() and bails with -ENOMEM if that fails.

Indeed.  This was broken a long time ago.  It does seem to be
fixed in the new skcipher_walk code but here is a patch to fix
it for older kernels.

---8<---
Subject: crypto: skcipher - Fix blkcipher walk OOM crash

When we need to allocate a temporary blkcipher_walk_next and it
fails, the code is supposed to take the slow path of processing
the data block by block.  However, due to an unrelated change
we instead end up dereferencing the NULL pointer.

This patch fixes it by moving the unrelated bsize setting out
of the way so that we enter the slow path as inteded.

Fixes: 7607bd8ff03b ("[CRYPTO] blkcipher: Added blkcipher_walk_virt_block")
Cc: stable@vger.kernel.org
Reported-by: xiakaixu <xiakaixu@huawei.com>
Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
index 3699995..a832426 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -233,6 +233,8 @@ static int blkcipher_walk_next(struct blkcipher_desc *desc,
 		return blkcipher_walk_done(desc, walk, -EINVAL);
 	}
 
+	bsize = min(walk->walk_blocksize, n);
+
 	walk->flags &= ~(BLKCIPHER_WALK_SLOW | BLKCIPHER_WALK_COPY |
 			 BLKCIPHER_WALK_DIFF);
 	if (!scatterwalk_aligned(&walk->in, walk->alignmask) ||
@@ -245,7 +247,6 @@ static int blkcipher_walk_next(struct blkcipher_desc *desc,
 		}
 	}
 
-	bsize = min(walk->walk_blocksize, n);
 	n = scatterwalk_clamp(&walk->in, n);
 	n = scatterwalk_clamp(&walk->out, n);
 
-- 
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

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

* Re: Kernel panic - encryption/decryption failed when open file on Arm64
  2016-09-13  6:43             ` Herbert Xu
@ 2016-09-13  7:56               ` Ard Biesheuvel
  0 siblings, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2016-09-13  7:56 UTC (permalink / raw)
  To: Herbert Xu
  Cc: liushuoran, Xiakaixu, David S. Miller, Theodore Ts'o,
	Jaegeuk Kim, nhorman, mh1, linux-crypto, linux-kernel,
	Wangbintian, Huxinwei, zhangzhibin (C)

On 13 September 2016 at 07:43, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Mon, Sep 12, 2016 at 06:40:15PM +0100, Ard Biesheuvel wrote:
>>
>> So to me, it seems like we should be taking the blkcipher_next_slow()
>> path, which does a kmalloc() and bails with -ENOMEM if that fails.
>
> Indeed.  This was broken a long time ago.  It does seem to be
> fixed in the new skcipher_walk code but here is a patch to fix
> it for older kernels.
>
> ---8<---
> Subject: crypto: skcipher - Fix blkcipher walk OOM crash
>
> When we need to allocate a temporary blkcipher_walk_next and it
> fails, the code is supposed to take the slow path of processing
> the data block by block.  However, due to an unrelated change
> we instead end up dereferencing the NULL pointer.
>
> This patch fixes it by moving the unrelated bsize setting out
> of the way so that we enter the slow path as inteded.
>
inteNded ^^^

> Fixes: 7607bd8ff03b ("[CRYPTO] blkcipher: Added blkcipher_walk_virt_block")
> Cc: stable@vger.kernel.org
> Reported-by: xiakaixu <xiakaixu@huawei.com>
> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>

This fixes the issue for me

Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

I will follow up with fixes for the ARM and arm64 CTR code shortly.

Thanks,
Ard.

> diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
> index 3699995..a832426 100644
> --- a/crypto/blkcipher.c
> +++ b/crypto/blkcipher.c
> @@ -233,6 +233,8 @@ static int blkcipher_walk_next(struct blkcipher_desc *desc,
>                 return blkcipher_walk_done(desc, walk, -EINVAL);
>         }
>
> +       bsize = min(walk->walk_blocksize, n);
> +
>         walk->flags &= ~(BLKCIPHER_WALK_SLOW | BLKCIPHER_WALK_COPY |
>                          BLKCIPHER_WALK_DIFF);
>         if (!scatterwalk_aligned(&walk->in, walk->alignmask) ||
> @@ -245,7 +247,6 @@ static int blkcipher_walk_next(struct blkcipher_desc *desc,
>                 }
>         }
>
> -       bsize = min(walk->walk_blocksize, n);
>         n = scatterwalk_clamp(&walk->in, n);
>         n = scatterwalk_clamp(&walk->out, n);
>
> --
> 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

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

end of thread, other threads:[~2016-09-13  7:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <57D15BD3.40903@huawei.com>
2016-09-08 12:47 ` Kernel panic - encryption/decryption failed when open file on Arm64 Herbert Xu
2016-09-09  4:08   ` xiakaixu
2016-09-09 10:19   ` xiakaixu
2016-09-09 10:31     ` Ard Biesheuvel
2016-09-09 10:56       ` Ard Biesheuvel
2016-09-12  2:16         ` liushuoran
2016-09-12 17:40           ` Ard Biesheuvel
2016-09-13  2:05             ` xiakaixu
2016-09-13  6:43             ` Herbert Xu
2016-09-13  7:56               ` Ard Biesheuvel

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.