Kernel panic - encryption/decryption failed when open file on Arm64
Subsystems (2)
A message that is not in the archive (sent off-list or not yet received): [email protected]
Herbert Xu
maintainerQuoted 35 lines · xiakaixu
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 failureThe 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 functionaes_ctr_encrypt()will be NULL. The
panic will occurs if we don't check the input parameter.
Quoted 1 line
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,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Sorry for resend this email, just add the [email protected] and [email protected].
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
Quoted 44 lines · xiakaixu
Quoted 35 lines
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 failureThe 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 functionaes_ctr_encrypt()will be NULL. The
panic will occurs if we don't check the input parameter.Quoted 1 line
Not sure about this and wish to get your opinions!
If the page allocation fails in
blkcipher_walk_nextit'll simply switch over to processing it block by block. so I don't think the warning is related to the crash.Cheers,
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
Quoted 44 lines · xiakaixu
Quoted 35 lines
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 failureThe 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 functionaes_ctr_encrypt()will be NULL. The
panic will occurs if we don't check the input parameter.Quoted 1 line
Not sure about this and wish to get your opinions!
If the page allocation fails in
blkcipher_walk_nextit'll simply switch over to processing it block by block. so I don't think the warning is related to the crash.Cheers,
Quoted 48 lines · xiakaixu
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 thewhile()loop and enter the if(nbytes) statment. But here the varibale tsrc is NULL and it is also the sencond input parameter of the functionaes_ctr_encrypt()... Kernel Panic...
Quoted 5 lines
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.
Quoted 49 lines · Ard Biesheuvel
Quoted 48 lines · xiakaixu
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 thewhile()loop and enter the if(nbytes) statment. But here the varibale tsrc is NULL and it is also the sencond input parameter of the functionaes_ctr_encrypt()... Kernel Panic...
Quoted 5 lines
Quoted 5 lines
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?
arch/arm64/crypto/aes-glue.c +0 -0
@@ -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];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
Quoted 89 lines
-----Original Message-----
From: Ard Biesheuvel [mailto:[email protected]]
Sent: Friday, September 09, 2016 6:57 PM
To: Xiakaixu
Cc: Herbert Xu; David S. Miller; Theodore Ts'o; Jaegeuk Kim; [email protected]; [email protected]; [email protected]; [email protected]; Wangbintian; liushuoran; Huxinwei; zhangzhibin
(C)
Subject: Re: Kernel panic - encryption/decryption failed when open file on Arm64Quoted 7 lines · Ard Biesheuvel
Quoted 6 lines · xiakaixu
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.Quoted 8 lines
Quoted 8 lines
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);--->
Quoted 5 lines
Quoted 5 lines
page allocation failed
...
while ((blocks = (walk.nbytes /
AES_BLOCK_SIZE))){ ---->
Quoted 3 lines
Quoted 3 lines
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,
Quoted 4 lines
Quoted 4 lines
first); ... err = blkcipher_walk_done(desc, &walk, walk.nbytes %
AES_BLOCK_SIZE);Quoted 2 lines
Quoted 2 lines
} if (nbytes){ ---->
Quoted 20 lines
Quoted 20 lines
enter this
if()statementu8 *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 thewhile()loop and enter the if(nbytes) statment. But here the varibale tsrc is NULL and it is also the sencond input parameter of the functionaes_ctr_encrypt()... Kernel Panic...Quoted 5 lines
Quoted 5 lines
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?
arch/arm64/crypto/aes-glue.c +1
-1@@ -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];
Quoted 6 lines · liushuoran
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_nextit'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.
Quoted 27 lines · liushuoran
Quoted 6 lines
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_nextit'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; }
Quoted 6 lines
where
blkcipher_next_copy()unconditionally callsmemcpy()withwalk->pageas destination (even though we ended up here due to the fact thatwalk->page== NULL)So to me, it seems like we should be taking the
blkcipher_next_slow()path, which does akmalloc()and bails with-ENOMEMif 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.
crypto/blkcipher.c +2 -1
@@ -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
Quoted 3 lines
.
Herbert Xu
maintainerQuoted 3 lines · Ard Biesheuvel
So to me, it seems like we should be taking the
blkcipher_next_slow()path, which does akmalloc()and bails with-ENOMEMif 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.
crypto/blkcipher.c +2 -1
@@ -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);
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Ard Biesheuvel
Tested-byQuoted 12 lines · Herbert Xu
Quoted 3 lines · Ard Biesheuvel
So to me, it seems like we should be taking the
blkcipher_next_slow()path, which does akmalloc()and bails with-ENOMEMif that fails.Indeed. This was broken a long time ago. It does seem to be fixed in the new
skcipher_walkcode but here is a patch to fix it for older kernels.---8<---
Subject: crypto: skcipher - Fix blkcipher walk OOM crash
Quoted 8 lines
When we need to allocate a temporary
blkcipher_walk_nextand 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 ^^^
Quoted 6 lines
Fixes 7607bd8ff03b ("[CRYPTO] blkcipher: Added blkcipher_walk_virt_block")Reported-by xiakaixu <[email protected]>Reported-by Ard Biesheuvel <[email protected]>Signed-off-by Herbert Xu <[email protected]>
This fixes the issue for me
I will follow up with fixes for the ARM and arm64 CTR code shortly.
Thanks,
Ard.
Quoted 25 lines
crypto/blkcipher.c +2
-2@@ -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 <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt