* Re: Kernel panic - encryption/decryption failed when open file on Arm64
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@tuxdriver.com, mh1@iki.fi,
linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
Wangbintian, Huxinwei, zhangzhibin (C)
In-Reply-To: <CAKv+Gu9jaVyJ4PYsxZfiZXB3Uezr26YWhSyE-g+n6-AuWFcneQ@mail.gmail.com>
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
* Re: [PATCH] crypto: qce: Initialize core src clock @100Mhz
From: Bjorn Andersson @ 2016-09-13 4:00 UTC (permalink / raw)
To: Iaroslav Gridin
Cc: herbert, davem, linux-crypto, linux-kernel, andy.gross,
david.brown, linux-arm-msm, linux-soc
In-Reply-To: <20160903164535.1118-1-voker57@gmail.com>
On Sat 03 Sep 09:45 PDT 2016, Iaroslav Gridin wrote:
> Without that, QCE performance is about 2x less.
>
> Signed-off-by: Iaroslav Gridin <voker57@gmail.com>
> ---
> drivers/crypto/qce/core.c | 18 +++++++++++++++++-
> drivers/crypto/qce/core.h | 2 +-
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
[..]
> @@ -205,10 +209,20 @@ static int qce_crypto_probe(struct platform_device *pdev)
> if (IS_ERR(qce->bus))
> return PTR_ERR(qce->bus);
>
> - ret = clk_prepare_enable(qce->core);
> + ret = clk_prepare_enable(qce->core_src);
> if (ret)
> return ret;
>
> + ret = clk_set_rate(qce->core_src, 100000000);
> + if (ret) {
> + dev_warn(qce->dev, "Unable to set QCE core src clk @100Mhz, performance might be degraded\n");
This warning is misleading as you return a failure from probe() when it
happens.
> + goto err_clks_core_src;
> + }
> +
[..]
> +err_clks_core_src:
> + clk_disable_unprepare(qce->core_src);
> return ret;
> }
>
Regards,
Bjorn
^ permalink raw reply
* Re: Kernel panic - encryption/decryption failed when open file on Arm64
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@tuxdriver.com, mh1@iki.fi, linux-crypto@vger.kernel.org,
linux-kernel@vger.kernel.org, Wangbintian, Huxinwei,
zhangzhibin (C)
In-Reply-To: <CAKv+Gu9jaVyJ4PYsxZfiZXB3Uezr26YWhSyE-g+n6-AuWFcneQ@mail.gmail.com>
> 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
* Re: [PATCH] crypto: squash lines for simple wrapper functions
From: Joe Perches @ 2016-09-12 19:44 UTC (permalink / raw)
To: Masahiro Yamada, linux-crypto; +Cc: Herbert Xu, linux-kernel, David S. Miller
In-Reply-To: <1473708474-32359-1-git-send-email-yamada.masahiro@socionext.com>
On Tue, 2016-09-13 at 04:27 +0900, Masahiro Yamada wrote:
> Remove unneeded variables and assignments.
Was this found by visual inspection or some tool?
If it's via a tool, it's good to mention that in the changelog.
^ permalink raw reply
* [PATCH] crypto: squash lines for simple wrapper functions
From: Masahiro Yamada @ 2016-09-12 19:27 UTC (permalink / raw)
To: linux-crypto; +Cc: Masahiro Yamada, Herbert Xu, linux-kernel, David S. Miller
Remove unneeded variables and assignments.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
crypto/crct10dif_generic.c | 5 +----
crypto/mcryptd.c | 7 +------
drivers/crypto/hifn_795x.c | 12 ++----------
3 files changed, 4 insertions(+), 20 deletions(-)
diff --git a/crypto/crct10dif_generic.c b/crypto/crct10dif_generic.c
index c1229614..8e94e29 100644
--- a/crypto/crct10dif_generic.c
+++ b/crypto/crct10dif_generic.c
@@ -107,10 +107,7 @@ static struct shash_alg alg = {
static int __init crct10dif_mod_init(void)
{
- int ret;
-
- ret = crypto_register_shash(&alg);
- return ret;
+ return crypto_register_shash(&alg);
}
static void __exit crct10dif_mod_fini(void)
diff --git a/crypto/mcryptd.c b/crypto/mcryptd.c
index 86fb59b..94ee44a 100644
--- a/crypto/mcryptd.c
+++ b/crypto/mcryptd.c
@@ -612,12 +612,7 @@ EXPORT_SYMBOL_GPL(mcryptd_alloc_ahash);
int ahash_mcryptd_digest(struct ahash_request *desc)
{
- int err;
-
- err = crypto_ahash_init(desc) ?:
- ahash_mcryptd_finup(desc);
-
- return err;
+ return crypto_ahash_init(desc) ?: ahash_mcryptd_finup(desc);
}
int ahash_mcryptd_update(struct ahash_request *desc)
diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
index eee2c7e..e09d405 100644
--- a/drivers/crypto/hifn_795x.c
+++ b/drivers/crypto/hifn_795x.c
@@ -636,20 +636,12 @@ struct hifn_request_context {
static inline u32 hifn_read_0(struct hifn_device *dev, u32 reg)
{
- u32 ret;
-
- ret = readl(dev->bar[0] + reg);
-
- return ret;
+ return readl(dev->bar[0] + reg);
}
static inline u32 hifn_read_1(struct hifn_device *dev, u32 reg)
{
- u32 ret;
-
- ret = readl(dev->bar[1] + reg);
-
- return ret;
+ return readl(dev->bar[1] + reg);
}
static inline void hifn_write_0(struct hifn_device *dev, u32 reg, u32 val)
--
1.9.1
^ permalink raw reply related
* Re: Kernel panic - encryption/decryption failed when open file on Arm64
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@tuxdriver.com, mh1@iki.fi,
linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
Wangbintian, Huxinwei, zhangzhibin (C)
In-Reply-To: <00B10D30F2BAA743B48953A4D86C96D54C8A8A@SZXEMI506-MBS.china.huawei.com>
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
* Memory corruption in algif_skciper AIO sendpage with multiple iocb
From: Stephan Mueller @ 2016-09-12 12:43 UTC (permalink / raw)
To: herbert; +Cc: linux-crypto
Hi Herbert,
after getting the AIO code working on sendmsg, tried it with vmsplice/splice
and I get a memory corruption. Interestingly, the stack trace is partially
garbled too. Thus, tracking this one down may be a bit of a challenge.
Ciao
Stephan
^ permalink raw reply
* Re: [STLinux Kernel] [PATCH -next] hwrng: st - Fix missing clk_disable_unprepare() on error in st_rng_probe()
From: Peter Griffin @ 2016-09-12 8:31 UTC (permalink / raw)
To: Wei Yongjun
Cc: Patrice Chotard, Matt Mackall, Herbert Xu, Wei Yongjun, kernel,
linux-arm-kernel, linux-crypto
In-Reply-To: <1473509022-3478-1-git-send-email-weiyj.lk@gmail.com>
Hi Wei,
On Sat, 10 Sep 2016, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> Fix the missing clk_disable_unprepare() before return
> from st_rng_probe() in the error handling case.
>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> drivers/char/hw_random/st-rng.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
> index 7e8aa6b..938ec10 100644
> --- a/drivers/char/hw_random/st-rng.c
> +++ b/drivers/char/hw_random/st-rng.c
> @@ -108,6 +108,7 @@ static int st_rng_probe(struct platform_device *pdev)
> ret = hwrng_register(&ddata->ops);
> if (ret) {
> dev_err(&pdev->dev, "Failed to register HW RNG\n");
> + clk_disable_unprepare(clk);
> return ret;
> }
>
>
Acked-by: Peter Griffin <peter.griffin@linaro.org>
^ permalink raw reply
* Re: [PATCH -next] hwrng: st - Fix missing clk_disable_unprepare() on error in st_rng_probe()
From: Patrice Chotard @ 2016-09-12 7:27 UTC (permalink / raw)
To: Wei Yongjun, Matt Mackall, Herbert Xu
Cc: Wei Yongjun, linux-arm-kernel, kernel, linux-crypto
In-Reply-To: <1473509022-3478-1-git-send-email-weiyj.lk@gmail.com>
Hi Wey
On 09/10/2016 02:03 PM, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> Fix the missing clk_disable_unprepare() before return
> from st_rng_probe() in the error handling case.
>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> drivers/char/hw_random/st-rng.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
> index 7e8aa6b..938ec10 100644
> --- a/drivers/char/hw_random/st-rng.c
> +++ b/drivers/char/hw_random/st-rng.c
> @@ -108,6 +108,7 @@ static int st_rng_probe(struct platform_device *pdev)
> ret = hwrng_register(&ddata->ops);
> if (ret) {
> dev_err(&pdev->dev, "Failed to register HW RNG\n");
> + clk_disable_unprepare(clk);
> return ret;
> }
>
>
>
>
Acked-by: Patrice Chotard <patrice.chotard@st.com>
Thanks
^ permalink raw reply
* RE: Kernel panic - encryption/decryption failed when open file on Arm64
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@tuxdriver.com, mh1@iki.fi, linux-crypto@vger.kernel.org,
linux-kernel@vger.kernel.org, Wangbintian, Huxinwei,
zhangzhibin (C)
In-Reply-To: <CAKv+Gu8w+BuwxQjOtpnFPHnJNUzq7m0K+KJ8=FG2wHigaB54ng@mail.gmail.com>
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
* [PATCH] hwrng: geode-rng - Use linux/io.h instead of asm/io.h
From: PrasannaKumar Muralidharan @ 2016-09-11 15:24 UTC (permalink / raw)
To: herbert, mpm, linux-geode, linux-crypto; +Cc: PrasannaKumar Muralidharan
Fix checkpatch.pl warning by changing from asm/io.h to linux/io.h. In
the mean time arrange the includes in alphabetical order.
Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
---
drivers/char/hw_random/geode-rng.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/char/hw_random/geode-rng.c b/drivers/char/hw_random/geode-rng.c
index 79e7482..0cae210 100644
--- a/drivers/char/hw_random/geode-rng.c
+++ b/drivers/char/hw_random/geode-rng.c
@@ -24,12 +24,12 @@
* warranty of any kind, whether express or implied.
*/
-#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/hw_random.h>
+#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/pci.h>
-#include <linux/hw_random.h>
-#include <linux/delay.h>
-#include <asm/io.h>
#define GEODE_RNG_DATA_REG 0x50
#define GEODE_RNG_STATUS_REG 0x54
--
2.5.0
^ permalink raw reply related
* [PATCH] hwrng: geode-rng - Migrate to managed API
From: PrasannaKumar Muralidharan @ 2016-09-11 15:23 UTC (permalink / raw)
To: herbert, mpm, linux-geode, linux-crypto; +Cc: PrasannaKumar Muralidharan
Use devm_ioremap and devm_hwrng_register instead of ioremap and
hwrng_register. This removes error handling code. Also moved code around
by removing goto statements. This improves code readability.
Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
---
drivers/char/hw_random/geode-rng.c | 50 ++++++++++++--------------------------
1 file changed, 15 insertions(+), 35 deletions(-)
diff --git a/drivers/char/hw_random/geode-rng.c b/drivers/char/hw_random/geode-rng.c
index 0d0579f..79e7482 100644
--- a/drivers/char/hw_random/geode-rng.c
+++ b/drivers/char/hw_random/geode-rng.c
@@ -31,9 +31,6 @@
#include <linux/delay.h>
#include <asm/io.h>
-
-#define PFX KBUILD_MODNAME ": "
-
#define GEODE_RNG_DATA_REG 0x50
#define GEODE_RNG_STATUS_REG 0x54
@@ -85,7 +82,6 @@ static struct hwrng geode_rng = {
static int __init mod_init(void)
{
- int err = -ENODEV;
struct pci_dev *pdev = NULL;
const struct pci_device_id *ent;
void __iomem *mem;
@@ -93,43 +89,27 @@ static int __init mod_init(void)
for_each_pci_dev(pdev) {
ent = pci_match_id(pci_tbl, pdev);
- if (ent)
- goto found;
- }
- /* Device not found. */
- goto out;
-
-found:
- rng_base = pci_resource_start(pdev, 0);
- if (rng_base == 0)
- goto out;
- err = -ENOMEM;
- mem = ioremap(rng_base, 0x58);
- if (!mem)
- goto out;
- geode_rng.priv = (unsigned long)mem;
-
- pr_info("AMD Geode RNG detected\n");
- err = hwrng_register(&geode_rng);
- if (err) {
- pr_err(PFX "RNG registering failed (%d)\n",
- err);
- goto err_unmap;
+ if (ent) {
+ rng_base = pci_resource_start(pdev, 0);
+ if (rng_base == 0)
+ return -ENODEV;
+
+ mem = devm_ioremap(&pdev->dev, rng_base, 0x58);
+ if (IS_ERR(mem))
+ return PTR_ERR(mem);
+ geode_rng.priv = (unsigned long)mem;
+
+ pr_info("AMD Geode RNG detected\n");
+ return devm_hwrng_register(&pdev->dev, &geode_rng);
+ }
}
-out:
- return err;
-err_unmap:
- iounmap(mem);
- goto out;
+ /* Device not found. */
+ return -ENODEV;
}
static void __exit mod_exit(void)
{
- void __iomem *mem = (void __iomem *)geode_rng.priv;
-
- hwrng_unregister(&geode_rng);
- iounmap(mem);
}
module_init(mod_init);
--
2.5.0
^ permalink raw reply related
* Re: algif_aead: AIO broken with more than one iocb
From: Stephan Mueller @ 2016-09-11 13:41 UTC (permalink / raw)
To: noloader; +Cc: Herbert Xu, linux-crypto
In-Reply-To: <CAH8yC8knD0U4-BhgzkUfyHMm-KyNyDCx-3PPqQOm0bdVh4qjiA@mail.gmail.com>
Am Sonntag, 11. September 2016, 08:43:00 CEST schrieb Jeffrey Walton:
Hi Jeffrey,
> > The AIO support for algif_aead is broken when submitting more than one
> > iocb.
> > The break happens in aead_recvmsg_async at the following code:
> I think the kernel needs to take a half step back, and add the missing
> self tests and test cases to be more proactive in detecting breaks
> earlier. Speaking first hand, some of these breaks have existed for
> months.
>
> I don't take the position you can't break things. I believe you can't
> make an omelet without breaking eggs; and if you're not breaking
> something, then you're probably not getting anything done. The
> engineering defect is not detecting the break.
The testing that is implemented for libkcapi should cover almost all code
paths of AF_ALG in the kernel. However, I just added the AIO support to the
library in the last few days as this logic is not straight forward. Thus these
issues show up now.
If you wish to analyze the AIO support more, I can certainly push my current
development branch of libkcapi to my github tree so that you would have a
working AIO user space component.
Ciao
Stephan
^ permalink raw reply
* Re: algif_aead: AIO broken with more than one iocb
From: Jeffrey Walton @ 2016-09-11 12:43 UTC (permalink / raw)
To: Stephan Mueller; +Cc: Herbert Xu, linux-crypto
In-Reply-To: <6245755.LbXSUvPjJL@positron.chronox.de>
> The AIO support for algif_aead is broken when submitting more than one iocb.
> The break happens in aead_recvmsg_async at the following code:
>
I think the kernel needs to take a half step back, and add the missing
self tests and test cases to be more proactive in detecting breaks
earlier. Speaking first hand, some of these breaks have existed for
months.
I don't take the position you can't break things. I believe you can't
make an omelet without breaking eggs; and if you're not breaking
something, then you're probably not getting anything done. The
engineering defect is not detecting the break.
Jeff
^ permalink raw reply
* algif_aead: AIO broken with more than one iocb
From: Stephan Mueller @ 2016-09-11 2:59 UTC (permalink / raw)
To: herbert; +Cc: linux-crypto
Hi Herbert,
The AIO support for algif_aead is broken when submitting more than one iocb.
The break happens in aead_recvmsg_async at the following code:
/* ensure output buffer is sufficiently large */
if (usedpages < outlen)
goto free;
The reason is that when submitting, say, two iocb, ctx->used contains the
buffer length for two AEAD operations (as expected). However, the recvmsg code
is invoked for each iocb individually and thus usedpages should only be
expected to point to memory for one AEAD operation. But this violates the
check above.
For example, I have two independent AEAD operations that I want to trigger.
The input to each is 48 bytes (including space for AAD and tag). The output
buffer that I have for each AEAD operation is also 48 bytes and thus
sufficient for the AEAD operation. Yet, when submitting the two AEAD
operations in one io_submit (i.e. using two iocb), ctx->used indicates that
the kernel has 96 bytes to process. This is correct, but only half of it
should be processed in one recvmsg_async invocation.
Note, the AIO operation works perfectly well, when io_submit only sends one
iocb.
Do you have any idea on how to fix that?
Ciao
Stephan
^ permalink raw reply
* [PATCH -next] hwrng: st - Fix missing clk_disable_unprepare() on error in st_rng_probe()
From: Wei Yongjun @ 2016-09-10 12:03 UTC (permalink / raw)
To: Patrice Chotard, Matt Mackall, Herbert Xu
Cc: Wei Yongjun, linux-arm-kernel, kernel, linux-crypto
From: Wei Yongjun <weiyongjun1@huawei.com>
Fix the missing clk_disable_unprepare() before return
from st_rng_probe() in the error handling case.
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
drivers/char/hw_random/st-rng.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
index 7e8aa6b..938ec10 100644
--- a/drivers/char/hw_random/st-rng.c
+++ b/drivers/char/hw_random/st-rng.c
@@ -108,6 +108,7 @@ static int st_rng_probe(struct platform_device *pdev)
ret = hwrng_register(&ddata->ops);
if (ret) {
dev_err(&pdev->dev, "Failed to register HW RNG\n");
+ clk_disable_unprepare(clk);
return ret;
}
^ permalink raw reply related
* [PATCH] crypto: call put_page on used pages only
From: Stephan Mueller @ 2016-09-10 11:50 UTC (permalink / raw)
To: herbert; +Cc: linux-crypto
In-Reply-To: <8008966.Q0OrxOpoA1@positron.chronox.de>
Hi Herbert,
This patch fixes the reported BUG reliably that I was able to
create with my (faulty) libkcapi test code.
However, I am yet unable to pinpoint the code that allocates an
SG without an associated page that would trigger the BUG.
In any case, if you approve, I would recommend that this patch
should go to 4.8 and to stable as well.
---8<---
Ensure that put_page is only invoked on pages that were used by
algif_skcipher.
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
crypto/algif_skcipher.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index d7acb73..bc36a9a 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -179,7 +179,7 @@ static void skcipher_pull_sgl(struct sock *sk, size_t used, int put)
if (sg[i].length)
return;
- if (put)
+ if (put && page_ref_count(sg_page(sg + i)))
put_page(sg_page(sg + i));
sg_assign_page(sg + i, NULL);
}
--
2.7.4
^ permalink raw reply related
* Company Agent Needed
From: Shougang Group @ 2016-09-10 8:37 UTC (permalink / raw)
--
Shougang Group
45 Huagong Road Xinji City,
Hebei Province China.
webpage: www.shougang.com.cn
This is an official request for Professional/consultants who will stand as
our regional representative to run logistics on behalf of Shougang Group.
We are only looking for individual or company from USA and Canada. You
will be entitle to ten percent (10%) of every payment you receive from our
customers in your region on behalf of the company.
NOTE: You are not require to travel, all communications with our customers
will be through email or phone. All fees, including taxes will be handled
by us.
You can apply for this position by filling the information below and send
back to us;
Full Name:
Residencial or office address:
Town:
State:
Zipcode:
Country:
Current Job:
If you own a company, please state the company name:
Prefered Email:
Gender:
Age:
Telephone:
If you have a current job, you can still be part of our business, as
services for us will not bother with your hours of work.
Respectfully,
Mr. Zhu Jimin (Chief Executive Officer)
Shougang Group
^ permalink raw reply
* [PATCH v2] crypto: only call put_page used pages
From: Stephan Mueller @ 2016-09-10 3:48 UTC (permalink / raw)
To: herbert; +Cc: linux-crypto
In-Reply-To: <2555546.QkgIzR2raW@positron.chronox.de>
Hi Herbert,
Changes v2: use sg_page(sg) only once
---8<---
For asynchronous operation, SGs are allocated without a page mapped to
them. If the SGL is freed, the code must only call put_page for an SG if
there was a page assigned to it in the first place.
This fixes a kernel crash when using io_submit with more than one iocb.
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
crypto/algif_skcipher.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 28556fc..d7acb73 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -86,8 +86,13 @@ static void skcipher_free_async_sgls(struct skcipher_async_req *sreq)
}
sgl = sreq->tsg;
n = sg_nents(sgl);
- for_each_sg(sgl, sg, n, i)
- put_page(sg_page(sg));
+ for_each_sg(sgl, sg, n, i) {
+ struct page *page = sg_page(sg);
+
+ /* some SGs may not have a page mapped */
+ if (page_ref_count(page))
+ put_page(page);
+ }
kfree(sreq->tsg);
}
--
2.7.4
^ permalink raw reply related
* [PATCH] crypto: only call put_page used pages
From: Stephan Mueller @ 2016-09-10 3:42 UTC (permalink / raw)
To: herbert; +Cc: linux-crypto
In-Reply-To: <2637498.CLV37KecV5@positron.chronox.de>
Hi Herbert,
This patch was tested with up to 64 iocb submitted with one io_submit call.
If you approve of this fix, I recommend that should go to the current 4.8 development cycle and to stable.
---8<---
For asynchronous operation, SGs are allocated without a page mapped to
them. If the SGL is freed, the code must only call put_page for an SG if
there was a page assigned to it in the first place.
This fixes a kernel crash when using io_submit with more than one iocb.
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
crypto/algif_skcipher.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 28556fc..58ec57a 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -86,8 +86,13 @@ static void skcipher_free_async_sgls(struct skcipher_async_req *sreq)
}
sgl = sreq->tsg;
n = sg_nents(sgl);
- for_each_sg(sgl, sg, n, i)
- put_page(sg_page(sg));
+ for_each_sg(sgl, sg, n, i) {
+ struct page *page = sg_page(sg);
+
+ /* some SGs may not have a page mapped */
+ if (page_ref_count(page))
+ put_page(sg_page(sg));
+ }
kfree(sreq->tsg);
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] softirq: fix tasklet_kill() and its users
From: Santosh Shilimkar @ 2016-09-10 1:41 UTC (permalink / raw)
To: Santosh Shilimkar, linux-kernel
Cc: qat-linux, linux-crypto, netdev, Greg Kroah-Hartman,
Andrew Morton, Thomas Gleixner, Tadeusz Struk, Herbert Xu,
David S. Miller, Paul Bolle, Giovanni Cabiddu,
Salvatore Benedetto, Karsten Keil, Peter Zijlstra (Intel)
In-Reply-To: <1472089950-2724-1-git-send-email-ssantosh@kernel.org>
Ping !!
On 8/24/2016 6:52 PM, Santosh Shilimkar wrote:
> Semantically the expectation from the tasklet init/kill API
> should be as below.
>
> tasklet_init() == Init and Enable scheduling
> tasklet_kill() == Disable scheduling and Destroy
>
> tasklet_init() API exibit above behavior but not the
> tasklet_kill(). The tasklet handler can still get scheduled
> and run even after the tasklet_kill().
>
> There are 2, 3 places where drivers are working around
> this issue by calling tasklet_disable() which will add an
> usecount and there by avoiding the handlers being called.
>
> tasklet_enable/tasklet_disable is a pair API and expected
> to be used together. Usage of tasklet_disable() *just* to
> workround tasklet scheduling after kill is probably not the
> correct and inteded use of the API as done the API.
> We also happen to see similar issue where in shutdown path
> the tasklet_handler was getting called even after the
> tasklet_kill().
>
> We fix this be making sure tasklet_kill() does right
> thing and there by ensuring tasklet handler won't run after
> tasklet_kil() with very simple change. Patch fixes the tasklet
> code and also few drivers workarounds.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tadeusz Struk <tadeusz.struk@intel.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paul Bolle <pebolle@tiscali.nl>
> Cc: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Cc: Salvatore Benedetto <salvatore.benedetto@intel.com>
> Cc: Karsten Keil <isdn@linux-pingi.de>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
>
> Signed-off-by: Santosh Shilimkar <ssantosh@kernel.org>
> ---
> Removed RFC tag from last post and dropped atmel serial
> driver which seems to have been fixed in 4.8
>
> https://lkml.org/lkml/2016/8/7/7
>
> drivers/crypto/qat/qat_common/adf_isr.c | 1 -
> drivers/crypto/qat/qat_common/adf_sriov.c | 1 -
> drivers/crypto/qat/qat_common/adf_vf_isr.c | 2 --
> drivers/isdn/gigaset/interface.c | 1 -
> kernel/softirq.c | 7 ++++---
> 5 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/crypto/qat/qat_common/adf_isr.c b/drivers/crypto/qat/qat_common/adf_isr.c
> index 06d4901..fd5e900 100644
> --- a/drivers/crypto/qat/qat_common/adf_isr.c
> +++ b/drivers/crypto/qat/qat_common/adf_isr.c
> @@ -296,7 +296,6 @@ static void adf_cleanup_bh(struct adf_accel_dev *accel_dev)
> int i;
>
> for (i = 0; i < hw_data->num_banks; i++) {
> - tasklet_disable(&priv_data->banks[i].resp_handler);
> tasklet_kill(&priv_data->banks[i].resp_handler);
> }
> }
> diff --git a/drivers/crypto/qat/qat_common/adf_sriov.c b/drivers/crypto/qat/qat_common/adf_sriov.c
> index 9320ae1..bc7c2fa 100644
> --- a/drivers/crypto/qat/qat_common/adf_sriov.c
> +++ b/drivers/crypto/qat/qat_common/adf_sriov.c
> @@ -204,7 +204,6 @@ void adf_disable_sriov(struct adf_accel_dev *accel_dev)
> }
>
> for (i = 0, vf = accel_dev->pf.vf_info; i < totalvfs; i++, vf++) {
> - tasklet_disable(&vf->vf2pf_bh_tasklet);
> tasklet_kill(&vf->vf2pf_bh_tasklet);
> mutex_destroy(&vf->pf2vf_lock);
> }
> diff --git a/drivers/crypto/qat/qat_common/adf_vf_isr.c b/drivers/crypto/qat/qat_common/adf_vf_isr.c
> index bf99e11..6e38bff 100644
> --- a/drivers/crypto/qat/qat_common/adf_vf_isr.c
> +++ b/drivers/crypto/qat/qat_common/adf_vf_isr.c
> @@ -191,7 +191,6 @@ static int adf_setup_pf2vf_bh(struct adf_accel_dev *accel_dev)
>
> static void adf_cleanup_pf2vf_bh(struct adf_accel_dev *accel_dev)
> {
> - tasklet_disable(&accel_dev->vf.pf2vf_bh_tasklet);
> tasklet_kill(&accel_dev->vf.pf2vf_bh_tasklet);
> mutex_destroy(&accel_dev->vf.vf2pf_lock);
> }
> @@ -268,7 +267,6 @@ static void adf_cleanup_bh(struct adf_accel_dev *accel_dev)
> {
> struct adf_etr_data *priv_data = accel_dev->transport;
>
> - tasklet_disable(&priv_data->banks[0].resp_handler);
> tasklet_kill(&priv_data->banks[0].resp_handler);
> }
>
> diff --git a/drivers/isdn/gigaset/interface.c b/drivers/isdn/gigaset/interface.c
> index 600c79b..2ce63b6 100644
> --- a/drivers/isdn/gigaset/interface.c
> +++ b/drivers/isdn/gigaset/interface.c
> @@ -524,7 +524,6 @@ void gigaset_if_free(struct cardstate *cs)
> if (!drv->have_tty)
> return;
>
> - tasklet_disable(&cs->if_wake_tasklet);
> tasklet_kill(&cs->if_wake_tasklet);
> cs->tty_dev = NULL;
> tty_unregister_device(drv->tty, cs->minor_index);
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 17caf4b..21397eb 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -498,7 +498,7 @@ static void tasklet_action(struct softirq_action *a)
> list = list->next;
>
> if (tasklet_trylock(t)) {
> - if (!atomic_read(&t->count)) {
> + if (atomic_read(&t->count) == 1) {
> if (!test_and_clear_bit(TASKLET_STATE_SCHED,
> &t->state))
> BUG();
> @@ -534,7 +534,7 @@ static void tasklet_hi_action(struct softirq_action *a)
> list = list->next;
>
> if (tasklet_trylock(t)) {
> - if (!atomic_read(&t->count)) {
> + if (atomic_read(&t->count) == 1) {
> if (!test_and_clear_bit(TASKLET_STATE_SCHED,
> &t->state))
> BUG();
> @@ -559,7 +559,7 @@ void tasklet_init(struct tasklet_struct *t,
> {
> t->next = NULL;
> t->state = 0;
> - atomic_set(&t->count, 0);
> + atomic_set(&t->count, 1);
> t->func = func;
> t->data = data;
> }
> @@ -576,6 +576,7 @@ void tasklet_kill(struct tasklet_struct *t)
> } while (test_bit(TASKLET_STATE_SCHED, &t->state));
> }
> tasklet_unlock_wait(t);
> + atomic_dec(&t->count);
> clear_bit(TASKLET_STATE_SCHED, &t->state);
> }
> EXPORT_SYMBOL(tasklet_kill);
>
^ permalink raw reply
* Re: Kernel panic - encryption/decryption failed when open file on Arm64
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@vger.kernel.org,
linux-kernel@vger.kernel.org, Bintian, liushuoran, Huxinwei,
zhangzhibin.zhang
In-Reply-To: <CAKv+Gu9JDCHDteOvgpEg4SJP4OKgM6=euF3pzBcXDJxTt=dAYg@mail.gmail.com>
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
* Re: Kernel panic - encryption/decryption failed when open file on Arm64
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@vger.kernel.org,
linux-kernel@vger.kernel.org, Bintian, liushuoran, Huxinwei,
zhangzhibin.zhang
In-Reply-To: <57D28CB8.4080904@huawei.com>
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
* Re: Kernel panic - encryption/decryption failed when open file on Arm64
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
In-Reply-To: <20160908124709.GA26586@gondor.apana.org.au>
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
* [PATCH] hwrng: amd-rng - Migrate to managed API
From: PrasannaKumar Muralidharan @ 2016-09-09 7:58 UTC (permalink / raw)
To: mpm, herbert, clabbe.montjoie, linux-crypto; +Cc: PrasannaKumar Muralidharan
Managed API eliminates error handling code, thus reduces several lines
of code.
Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
---
drivers/char/hw_random/amd-rng.c | 48 +++++++++-------------------------------
1 file changed, 11 insertions(+), 37 deletions(-)
diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
index 9959c76..4dbc5aa 100644
--- a/drivers/char/hw_random/amd-rng.c
+++ b/drivers/char/hw_random/amd-rng.c
@@ -55,7 +55,6 @@ MODULE_DEVICE_TABLE(pci, pci_tbl);
struct amd768_priv {
void __iomem *iobase;
struct pci_dev *pcidev;
- u32 pmbase;
};
static int amd_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
@@ -149,58 +148,33 @@ found:
if (pmbase == 0)
return -EIO;
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (IS_ERR(priv))
+ return PTR_ERR(priv);
- if (!request_region(pmbase + PMBASE_OFFSET, PMBASE_SIZE, DRV_NAME)) {
+ if (!devm_request_region(&pdev->dev, pmbase + PMBASE_OFFSET,
+ PMBASE_SIZE, DRV_NAME)) {
dev_err(&pdev->dev, DRV_NAME " region 0x%x already in use!\n",
pmbase + 0xF0);
- err = -EBUSY;
- goto out;
+ return -EBUSY;
}
- priv->iobase = ioport_map(pmbase + PMBASE_OFFSET, PMBASE_SIZE);
- if (!priv->iobase) {
+ priv->iobase = devm_ioport_map(&pdev->dev, pmbase + PMBASE_OFFSET,
+ PMBASE_SIZE);
+ if (IS_ERR(priv->iobase)) {
pr_err(DRV_NAME "Cannot map ioport\n");
- err = -EINVAL;
- goto err_iomap;
+ return PTR_ERR(priv->iobase);
}
amd_rng.priv = (unsigned long)priv;
- priv->pmbase = pmbase;
priv->pcidev = pdev;
pr_info(DRV_NAME " detected\n");
- err = hwrng_register(&amd_rng);
- if (err) {
- pr_err(DRV_NAME " registering failed (%d)\n", err);
- goto err_hwrng;
- }
- return 0;
-
-err_hwrng:
- ioport_unmap(priv->iobase);
-err_iomap:
- release_region(pmbase + PMBASE_OFFSET, PMBASE_SIZE);
-out:
- kfree(priv);
- return err;
+ return devm_hwrng_register(&pdev->dev, &amd_rng);
}
static void __exit mod_exit(void)
{
- struct amd768_priv *priv;
-
- priv = (struct amd768_priv *)amd_rng.priv;
-
- hwrng_unregister(&amd_rng);
-
- ioport_unmap(priv->iobase);
-
- release_region(priv->pmbase + PMBASE_OFFSET, PMBASE_SIZE);
-
- kfree(priv);
}
module_init(mod_init);
--
2.5.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox