* Re: [PATCH 1/2] crypto: tcrypt: Fix mixing printk/pr_err and obvious indentation issues
From: Herbert Xu @ 2016-06-28 8:37 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David S. Miller, linux-crypto, linux-kernel,
Bartlomiej Zolnierkiewicz
In-Reply-To: <1467098587-1038-1-git-send-email-k.kozlowski@samsung.com>
On Tue, Jun 28, 2016 at 09:23:06AM +0200, Krzysztof Kozlowski wrote:
> The recently added test_mb_ahash_speed() has clearly serious coding
> style issues. Try to fix some of them:
> 1. Don't mix pr_err() and printk();
> 2. Don't wrap strings;
> 3. Properly align goto statement in if() block;
> 4. Align wrapped arguments on new line;
> 5. Don't wrap functions on first argument;
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 2/2] crypto: tcrypt: Fix linkage error on ARM on division of s64
From: Herbert Xu @ 2016-06-28 8:41 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David S. Miller, linux-crypto, linux-kernel,
Bartlomiej Zolnierkiewicz
In-Reply-To: <1467098587-1038-2-git-send-email-k.kozlowski@samsung.com>
On Tue, Jun 28, 2016 at 09:23:07AM +0200, Krzysztof Kozlowski wrote:
> gcc 4.7.3 for ARM on Ubuntu couldn't link tcrypt module because of
> division of s64:
> ERROR: "__aeabi_ldivmod" [crypto/tcrypt.ko] undefined!
>
> Fixes: 087bcd225c56 ("crypto: tcrypt - Add speed tests for SHA multibuffer algorithms")
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
I'd prefer to be consistent here and just use unsigned long like
every other speed test in tcrypt. This code has some serious
issues, such as not waiting for completion in case of an error.
Let me try to fix them up.
Thanks!
---8<---
Subject: crypto: tcrypt - Use unsigned long for mb ahash cycle counter
For the timescales we are working against there is no need to
go beyond unsigned long.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 2f84583..1537a1c 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -415,7 +415,7 @@ char result[8][64];
struct ahash_request *req[8];
struct tcrypt_result tresult[8];
char *xbuf[8][XBUFSIZE];
-cycles_t start[8], end[8], mid;
+unsigned long start[8], end[8], mid;
static void test_mb_ahash_speed(const char *algo, unsigned int sec,
struct hash_speed *speed)
@@ -424,6 +424,7 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
void *hash_buff;
int ret = -ENOMEM;
struct crypto_ahash *tfm;
+ unsigned long cycles;
tfm = crypto_alloc_ahash(algo, 0, 0);
if (IS_ERR(tfm)) {
@@ -498,10 +499,9 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
end[k] = get_cycles();
}
- printk("\nBlock: %lld cycles (%lld cycles/byte), %d bytes\n",
- (s64) (end[7]-start[0])/1,
- (s64) (end[7]-start[0])/(8*speed[i].blen),
- 8*speed[i].blen);
+ cycles = end[7] - start[0];
+ printk("\nBlock: %6lu cycles (%4lu cycles/byte)\n",
+ cycles, cycles / (8 * speed[i].blen));
}
ret = 0;
--
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 2/2] crypto: tcrypt: Fix linkage error on ARM on division of s64
From: Krzysztof Kozlowski @ 2016-06-28 8:49 UTC (permalink / raw)
To: Herbert Xu
Cc: David S. Miller, linux-crypto, linux-kernel,
Bartlomiej Zolnierkiewicz
In-Reply-To: <20160628084138.GD15985@gondor.apana.org.au>
On 06/28/2016 10:41 AM, Herbert Xu wrote:
> On Tue, Jun 28, 2016 at 09:23:07AM +0200, Krzysztof Kozlowski wrote:
>> gcc 4.7.3 for ARM on Ubuntu couldn't link tcrypt module because of
>> division of s64:
>> ERROR: "__aeabi_ldivmod" [crypto/tcrypt.ko] undefined!
>>
>> Fixes: 087bcd225c56 ("crypto: tcrypt - Add speed tests for SHA multibuffer algorithms")
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
> I'd prefer to be consistent here and just use unsigned long like
> every other speed test in tcrypt. This code has some serious
> issues, such as not waiting for completion in case of an error.
>
> Let me try to fix them up.
>
> Thanks!
>
> ---8<---
> Subject: crypto: tcrypt - Use unsigned long for mb ahash cycle counter
>
> For the timescales we are working against there is no need to
> go beyond unsigned long.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
Works fine for me:
Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Best regards,
Krzysztof
^ permalink raw reply
* crypto: tcrypt - Fix memory leaks/crashes in multibuffer hash speed test
From: Herbert Xu @ 2016-06-28 9:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David S. Miller, linux-crypto, linux-kernel,
Bartlomiej Zolnierkiewicz, Megha Dey, Fenghua Yu, Tim Chen
In-Reply-To: <57723A08.1010704@samsung.com>
This patch resolves a number of issues with the mb speed test
function:
* The tfm is never freed.
* Memory is allocated even when we're not using mb.
* When an error occurs we don't wait for completion for other requests.
* When an error occurs during allocation we may leak memory.
* The test function ignores plen but still runs for plen != blen.
* The backlog flag is incorrectly used (may crash).
This patch tries to resolve all these issues as well as making
the code consistent with the existing hash speed testing function.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1537a1c..8208026 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -409,54 +409,61 @@ static inline int do_one_ahash_op(struct ahash_request *req, int ret)
return ret;
}
-char ptext[4096];
-struct scatterlist sg[8][8];
-char result[8][64];
-struct ahash_request *req[8];
-struct tcrypt_result tresult[8];
-char *xbuf[8][XBUFSIZE];
-unsigned long start[8], end[8], mid;
+struct test_mb_ahash_data {
+ struct scatterlist sg[TVMEMSIZE];
+ char result[64];
+ struct ahash_request *req;
+ struct tcrypt_result tresult;
+ char *xbuf[XBUFSIZE];
+};
static void test_mb_ahash_speed(const char *algo, unsigned int sec,
- struct hash_speed *speed)
+ struct hash_speed *speed)
{
- unsigned int i, j, k;
- void *hash_buff;
- int ret = -ENOMEM;
+ struct test_mb_ahash_data *data;
struct crypto_ahash *tfm;
+ unsigned long start, end;
unsigned long cycles;
+ unsigned int i, j, k;
+ int ret;
+
+ data = kzalloc(sizeof(*data) * 8, GFP_KERNEL);
+ if (!data)
+ return;
tfm = crypto_alloc_ahash(algo, 0, 0);
if (IS_ERR(tfm)) {
pr_err("failed to load transform for %s: %ld\n",
algo, PTR_ERR(tfm));
- return;
+ goto free_data;
}
+
for (i = 0; i < 8; ++i) {
- if (testmgr_alloc_buf(xbuf[i]))
- goto out_nobuf;
+ if (testmgr_alloc_buf(data[i].xbuf))
+ goto out;
- init_completion(&tresult[i].completion);
+ init_completion(&data[i].tresult.completion);
- req[i] = ahash_request_alloc(tfm, GFP_KERNEL);
- if (!req[i]) {
+ data[i].req = ahash_request_alloc(tfm, GFP_KERNEL);
+ if (!data[i].req) {
pr_err("alg: hash: Failed to allocate request for %s\n",
algo);
- goto out_noreq;
+ goto out;
}
- ahash_request_set_callback(req[i], CRYPTO_TFM_REQ_MAY_BACKLOG,
- tcrypt_complete, &tresult[i]);
- hash_buff = xbuf[i][0];
- memcpy(hash_buff, ptext, 4096);
+ ahash_request_set_callback(data[i].req, 0,
+ tcrypt_complete, &data[i].tresult);
+ test_hash_sg_init(data[i].sg);
}
- j = 0;
-
- pr_err("\ntesting speed of %s (%s)\n", algo,
- get_driver_name(crypto_ahash, tfm));
+ pr_info("\ntesting speed of multibuffer %s (%s)\n", algo,
+ get_driver_name(crypto_ahash, tfm));
for (i = 0; speed[i].blen != 0; i++) {
+ /* For some reason this only tests digests. */
+ if (speed[i].blen != speed[i].plen)
+ continue;
+
if (speed[i].blen > TVMEMSIZE * PAGE_SIZE) {
pr_err("template (%u) too big for tvmem (%lu)\n",
speed[i].blen, TVMEMSIZE * PAGE_SIZE);
@@ -466,53 +473,58 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
if (speed[i].klen)
crypto_ahash_setkey(tfm, tvmem[0], speed[i].klen);
- for (k = 0; k < 8; ++k) {
- sg_init_one(&sg[k][0], (void *) xbuf[k][0],
- speed[i].blen);
- ahash_request_set_crypt(req[k], sg[k],
- result[k], speed[i].blen);
- }
+ for (k = 0; k < 8; k++)
+ ahash_request_set_crypt(data[k].req, data[k].sg,
+ data[k].result, speed[i].blen);
- pr_err("test%3u (%5u byte blocks,%5u bytes per update,%4u updates): ",
+ pr_info("test%3u "
+ "(%5u byte blocks,%5u bytes per update,%4u updates): ",
i, speed[i].blen, speed[i].plen,
speed[i].blen / speed[i].plen);
- for (k = 0; k < 8; ++k) {
- start[k] = get_cycles();
- ret = crypto_ahash_digest(req[k]);
- if (ret == -EBUSY || ret == -EINPROGRESS)
+ start = get_cycles();
+
+ for (k = 0; k < 8; k++) {
+ ret = crypto_ahash_digest(data[k].req);
+ if (ret == -EINPROGRESS)
continue;
- if (ret) {
- pr_err("alg (%s) something wrong, ret = %d ...\n",
- algo, ret);
- goto out;
- }
+
+ if (ret)
+ break;
+
+ complete(&data[k].tresult.completion);
}
- mid = get_cycles();
- for (k = 0; k < 8; ++k) {
- struct tcrypt_result *tr = &tresult[k];
+ for (j = 0; j < k; j++) {
+ struct tcrypt_result *tr = &data[k].tresult;
- ret = wait_for_completion_interruptible(&tr->completion);
- if (ret)
- pr_err("alg(%s): hash: digest failed\n", algo);
- end[k] = get_cycles();
+ wait_for_completion(&tr->completion);
+ if (tr->err)
+ ret = tr->err;
}
- cycles = end[7] - start[0];
- printk("\nBlock: %6lu cycles (%4lu cycles/byte)\n",
- cycles, cycles / (8 * speed[i].blen));
+ end = get_cycles();
+ cycles = end - start;
+ pr_cont("%6lu cycles/operation, %4lu cycles/byte\n",
+ cycles, cycles / (8 * speed[i].blen));
+
+ if (ret) {
+ pr_err("At least one hashing failed ret=%d\n", ret);
+ break;
+ }
}
- ret = 0;
out:
for (k = 0; k < 8; ++k)
- ahash_request_free(req[k]);
-out_noreq:
+ ahash_request_free(data[k].req);
+
for (k = 0; k < 8; ++k)
- testmgr_free_buf(xbuf[k]);
-out_nobuf:
- return;
+ testmgr_free_buf(data[k].xbuf);
+
+ crypto_free_ahash(tfm);
+
+free_data:
+ kfree(data);
}
static int test_ahash_jiffies_digest(struct ahash_request *req, int blen,
--
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: crypto: tcrypt - Fix memory leaks/crashes in multibuffer hash speed test
From: Krzysztof Kozlowski @ 2016-06-28 10:15 UTC (permalink / raw)
To: Herbert Xu
Cc: David S. Miller, linux-crypto, linux-kernel,
Bartlomiej Zolnierkiewicz, Megha Dey, Fenghua Yu, Tim Chen
In-Reply-To: <20160628095511.GA16644@gondor.apana.org.au>
On Tue, Jun 28, 2016 at 11:55 AM, Herbert Xu
<herbert@gondor.apana.org.au> wrote:
> This patch resolves a number of issues with the mb speed test
> function:
>
> * The tfm is never freed.
> * Memory is allocated even when we're not using mb.
> * When an error occurs we don't wait for completion for other requests.
> * When an error occurs during allocation we may leak memory.
> * The test function ignores plen but still runs for plen != blen.
> * The backlog flag is incorrectly used (may crash).
>
> This patch tries to resolve all these issues as well as making
> the code consistent with the existing hash speed testing function.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
Oops:
[root@odroidxu3 ~]# modprobe tcrypt mode=422
[ 23.466899]
[ 23.466899] testing speed of multibuffer sha1 (sha1-neon)
[ 23.472517] test 0 ( 16 byte blocks, 16 bytes per update, 1 updates):
[ 23.480312] BUG: spinlock bad magic on CPU#5, modprobe/285
[ 23.484918] lock: 0xecab06a8, .magic: 00000000, .owner: <none>/-1,
.owner_cpu: 0
[ 23.492374] CPU: 5 PID: 285 Comm: modprobe Not tainted
4.7.0-rc5-next-20160628-00003-g28618d2d8c0b #862
[ 23.501724] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 23.507828] [<c010d738>] (unwind_backtrace) from [<c010a4b0>]
(show_stack+0x10/0x14)
[ 23.515526] [<c010a4b0>] (show_stack) from [<c031f06c>]
(dump_stack+0x74/0x94)
[ 23.522711] [<c031f06c>] (dump_stack) from [<c0156f78>]
(do_raw_spin_lock+0x160/0x1a8)
[ 23.530605] [<c0156f78>] (do_raw_spin_lock) from [<c06bd34c>]
(wait_for_common+0x20/0x144)
[ 23.538853] [<c06bd34c>] (wait_for_common) from [<bf157898>]
(test_mb_ahash_speed.constprop.2+0x20c/0x354 [tcrypt])
[ 23.549196] [<bf157898>] (test_mb_ahash_speed.constprop.2 [tcrypt])
from [<bf159284>] (do_test+0x12c4/0x32ec [tcrypt])
[ 23.559848] [<bf159284>] (do_test [tcrypt]) from [<bf15f048>]
(tcrypt_mod_init+0x48/0xa4 [tcrypt])
[ 23.568776] [<bf15f048>] (tcrypt_mod_init [tcrypt]) from
[<c010178c>] (do_one_initcall+0x3c/0x16c)
[ 23.577703] [<c010178c>] (do_one_initcall) from [<c0193370>]
(do_init_module+0x5c/0x1ac)
[ 23.585764] [<c0193370>] (do_init_module) from [<c018659c>]
(load_module+0x1a30/0x1d08)
[ 23.593731] [<c018659c>] (load_module) from [<c0186a3c>]
(SyS_finit_module+0x8c/0x98)
[ 23.601530] [<c0186a3c>] (SyS_finit_module) from [<c0107900>]
(ret_fast_syscall+0x0/0x3c)
[ 23.609677] Unable to handle kernel NULL pointer dereference at
virtual address 00000000
[ 23.617728] pgd = ec888000
[ 23.620406] [00000000] *pgd=6ddc7835, *pte=00000000, *ppte=00000000
[ 23.626646] Internal error: Oops: 817 [#1] PREEMPT SMP ARM
[ 23.632106] Modules linked in: sha1_generic sha1_arm_neon sha1_arm
tcrypt(+) s5p_jpeg s5p_mfc v4l2_mem2mem videobuf2_dma_contig
v4l2_common videobuf2_memops videobuf2_v4l2 videobuf2_core videodev
media
[ 23.649956] CPU: 5 PID: 285 Comm: modprobe Not tainted
4.7.0-rc5-next-20160628-00003-g28618d2d8c0b #862
[ 23.659323] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 23.665383] task: eddcf800 ti: ec974000 task.ti: ec974000
[ 23.670757] PC is at wait_for_common+0x6c/0x144
[ 23.675262] LR is at unwind_frame+0x68/0x564
[ 23.679507] pc : [<c06bd398>] lr : [<c010d23c>] psr: 60070093
[ 23.679507] sp : ec975d30 ip : eddcf800 fp : 00000000
[ 23.690953] r10: 00000000 r9 : 00000001 r8 : 00000000
[ 23.696144] r7 : 00000002 r6 : ecab06a4 r5 : ecab06a8 r4 : 7fffffff
[ 23.702644] r3 : 00000000 r2 : ec975d40 r1 : ecab06b8 r0 : 00000001
[ 23.709141] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM
Segment none
[ 23.716335] Control: 10c5387d Table: 6c88806a DAC: 00000051
[ 23.722050] Process modprobe (pid: 285, stack limit = 0xec974210)
[ 23.728116] Stack: (0xec975d30 to 0xec976000)
[ 23.732450] 5d20: bf15c0c0
00000001 eddcf800 c013aa58
[ 23.740601] 5d40: ecab06b8 00000000 00000000 00000008 bf15c0b4
ecab0620 bf15c0c0 ecab06a4
[ 23.748746] 5d60: 00000000 bf157898 00000001 ee3f6110 ecab0000
2196bd9e ec8af480 ec8af4c0
[ 23.756891] 5d80: 00000001 2196bd9e 000000c3 bf15c640 bf15c640
edee7940 bf15c4c0 bf15f000
[ 23.765037] 5da0: 00000124 00000000 c0183918 bf159284 bf15c640
edee7940 00000010 bf15c640
[ 23.773182] 5dc0: edee7940 bf15c4c0 bf15f000 00000124 00000000
bf15f048 bf15c4c0 00000001
[ 23.781328] 5de0: edee7940 c010178c ffffff04 ecac8000 00000000
8040003e ee801b80 8040003f
[ 23.789473] 5e00: c0b13154 c0a67b60 ef5e5000 ee801f00 c0183918
8040003e 00000080 ef5e5000
[ 23.797618] 5e20: ee801f00 ee801f00 c0a67b60 c0b025ac c0183918
c01d37dc bf15c4c0 00000001
[ 23.805764] 5e40: edee7940 bf15c4c0 00000001 00000124 00000003
c0193370 ec975f4c 00000001
[ 23.813909] 5e60: edee7880 ec975f4c 00000001 edee7880 bf15c4c0
c018659c bf15c4cc 00007fff
[ 23.822055] 5e80: bf15c4c0 c0184168 ed6f0300 00000000 c018398c
bf15c508 bf15c664 00000000
[ 23.830201] 5ea0: bf15c4cc bf15c628 00000000 0000b320 00000000
ec975f3c ec975f40 c01e17fc
[ 23.838346] 5ec0: 0000b320 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 23.846491] 5ee0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 23.854637] 5f00: 7fffffff 7fffffff 00000000 00000000 0003ff70
c0107ac4 ec974000 00000000
[ 23.862782] 5f20: 0003fe38 c0186a3c 7fffffff 00000000 00000003
0000b320 ed6f0300 f2781000
[ 23.870928] 5f40: 0000b320 00000000 bedfb924 f2781000 0000b320
f2787a88 f278792b f2788b00
[ 23.879074] 5f60: 00006664 00006c84 00000000 00000000 00000000
000012bc 00000021 00000022
[ 23.887219] 5f80: 00000019 00000000 00000015 00000000 00000000
0003fe08 00000008 0003e240
[ 23.895364] 5fa0: 0000017b c0107900 0003fe08 00000008 00000003
0003ff70 00000000 00000000
[ 23.903510] 5fc0: 0003fe08 00000008 0003e240 0000017b 0003ff70
00000008 0003ff70 0003fe38
[ 23.911655] 5fe0: bedfb928 bedfb918 00020b08 b6e206e0 60070010
00000003 00000000 00000000
[ 23.919806] [<c06bd398>] (wait_for_common) from [<bf157898>]
(test_mb_ahash_speed.constprop.2+0x20c/0x354 [tcrypt])
[ 23.930206] [<bf157898>] (test_mb_ahash_speed.constprop.2 [tcrypt])
from [<bf159284>] (do_test+0x12c4/0x32ec [tcrypt])
[ 23.940865] [<bf159284>] (do_test [tcrypt]) from [<bf15f048>]
(tcrypt_mod_init+0x48/0xa4 [tcrypt])
[ 23.949787] [<bf15f048>] (tcrypt_mod_init [tcrypt]) from
[<c010178c>] (do_one_initcall+0x3c/0x16c)
[ 23.958710] [<c010178c>] (do_one_initcall) from [<c0193370>]
(do_init_module+0x5c/0x1ac)
[ 23.966769] [<c0193370>] (do_init_module) from [<c018659c>]
(load_module+0x1a30/0x1d08)
[ 23.974741] [<c018659c>] (load_module) from [<c0186a3c>]
(SyS_finit_module+0x8c/0x98)
[ 23.982539] [<c0186a3c>] (SyS_finit_module) from [<c0107900>]
(ret_fast_syscall+0x0/0x3c)
[ 23.990685] Code: e28d2010 e98d1001 e5862018 e58d1010 (e5832000)
[ 23.996749] ---[ end trace ba7506845ca39b55 ]---
[ 24.001338] note: modprobe[285] exited with preempt_count 1
Segmentation fault
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v2] crypto: tcrypt - Fix memory leaks/crashes in multibuffer hash speed test
From: Herbert Xu @ 2016-06-28 12:33 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David S. Miller, linux-crypto, linux-kernel,
Bartlomiej Zolnierkiewicz, Megha Dey, Fenghua Yu, Tim Chen
In-Reply-To: <CAJKOXPexSTvfPtEhaSFQ5-igtE0DbgnVeOPzFPkXaSv5r+vZHw@mail.gmail.com>
On Tue, Jun 28, 2016 at 12:15:43PM +0200, Krzysztof Kozlowski wrote:
> Oops:
Thanks, there was a typo where it said k instead of j in the second
loop.
---8<---
This patch resolves a number of issues with the mb speed test
function:
* The tfm is never freed.
* Memory is allocated even when we're not using mb.
* When an error occurs we don't wait for completion for other requests.
* When an error occurs during allocation we may leak memory.
* The test function ignores plen but still runs for plen != blen.
* The backlog flag is incorrectly used (may crash).
This patch tries to resolve all these issues as well as making
the code consistent with the existing hash speed testing function.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1537a1c..68064fc 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -409,54 +409,61 @@ static inline int do_one_ahash_op(struct ahash_request *req, int ret)
return ret;
}
-char ptext[4096];
-struct scatterlist sg[8][8];
-char result[8][64];
-struct ahash_request *req[8];
-struct tcrypt_result tresult[8];
-char *xbuf[8][XBUFSIZE];
-unsigned long start[8], end[8], mid;
+struct test_mb_ahash_data {
+ struct scatterlist sg[TVMEMSIZE];
+ char result[64];
+ struct ahash_request *req;
+ struct tcrypt_result tresult;
+ char *xbuf[XBUFSIZE];
+};
static void test_mb_ahash_speed(const char *algo, unsigned int sec,
- struct hash_speed *speed)
+ struct hash_speed *speed)
{
- unsigned int i, j, k;
- void *hash_buff;
- int ret = -ENOMEM;
+ struct test_mb_ahash_data *data;
struct crypto_ahash *tfm;
+ unsigned long start, end;
unsigned long cycles;
+ unsigned int i, j, k;
+ int ret;
+
+ data = kzalloc(sizeof(*data) * 8, GFP_KERNEL);
+ if (!data)
+ return;
tfm = crypto_alloc_ahash(algo, 0, 0);
if (IS_ERR(tfm)) {
pr_err("failed to load transform for %s: %ld\n",
algo, PTR_ERR(tfm));
- return;
+ goto free_data;
}
+
for (i = 0; i < 8; ++i) {
- if (testmgr_alloc_buf(xbuf[i]))
- goto out_nobuf;
+ if (testmgr_alloc_buf(data[i].xbuf))
+ goto out;
- init_completion(&tresult[i].completion);
+ init_completion(&data[i].tresult.completion);
- req[i] = ahash_request_alloc(tfm, GFP_KERNEL);
- if (!req[i]) {
+ data[i].req = ahash_request_alloc(tfm, GFP_KERNEL);
+ if (!data[i].req) {
pr_err("alg: hash: Failed to allocate request for %s\n",
algo);
- goto out_noreq;
+ goto out;
}
- ahash_request_set_callback(req[i], CRYPTO_TFM_REQ_MAY_BACKLOG,
- tcrypt_complete, &tresult[i]);
- hash_buff = xbuf[i][0];
- memcpy(hash_buff, ptext, 4096);
+ ahash_request_set_callback(data[i].req, 0,
+ tcrypt_complete, &data[i].tresult);
+ test_hash_sg_init(data[i].sg);
}
- j = 0;
-
- pr_err("\ntesting speed of %s (%s)\n", algo,
- get_driver_name(crypto_ahash, tfm));
+ pr_info("\ntesting speed of multibuffer %s (%s)\n", algo,
+ get_driver_name(crypto_ahash, tfm));
for (i = 0; speed[i].blen != 0; i++) {
+ /* For some reason this only tests digests. */
+ if (speed[i].blen != speed[i].plen)
+ continue;
+
if (speed[i].blen > TVMEMSIZE * PAGE_SIZE) {
pr_err("template (%u) too big for tvmem (%lu)\n",
speed[i].blen, TVMEMSIZE * PAGE_SIZE);
@@ -466,53 +473,59 @@ static void test_mb_ahash_speed(const char *algo, unsigned int sec,
if (speed[i].klen)
crypto_ahash_setkey(tfm, tvmem[0], speed[i].klen);
- for (k = 0; k < 8; ++k) {
- sg_init_one(&sg[k][0], (void *) xbuf[k][0],
- speed[i].blen);
- ahash_request_set_crypt(req[k], sg[k],
- result[k], speed[i].blen);
- }
+ for (k = 0; k < 8; k++)
+ ahash_request_set_crypt(data[k].req, data[k].sg,
+ data[k].result, speed[i].blen);
- pr_err("test%3u (%5u byte blocks,%5u bytes per update,%4u updates): ",
+ pr_info("test%3u "
+ "(%5u byte blocks,%5u bytes per update,%4u updates): ",
i, speed[i].blen, speed[i].plen,
speed[i].blen / speed[i].plen);
- for (k = 0; k < 8; ++k) {
- start[k] = get_cycles();
- ret = crypto_ahash_digest(req[k]);
- if (ret == -EBUSY || ret == -EINPROGRESS)
+ start = get_cycles();
+
+ for (k = 0; k < 8; k++) {
+ ret = crypto_ahash_digest(data[k].req);
+ if (ret == -EINPROGRESS)
continue;
- if (ret) {
- pr_err("alg (%s) something wrong, ret = %d ...\n",
- algo, ret);
- goto out;
- }
+
+ if (ret)
+ break;
+
+ complete(&data[k].tresult.completion);
+ data[k].tresult.err = 0;
}
- mid = get_cycles();
- for (k = 0; k < 8; ++k) {
- struct tcrypt_result *tr = &tresult[k];
+ for (j = 0; j < k; j++) {
+ struct tcrypt_result *tr = &data[j].tresult;
- ret = wait_for_completion_interruptible(&tr->completion);
- if (ret)
- pr_err("alg(%s): hash: digest failed\n", algo);
- end[k] = get_cycles();
+ wait_for_completion(&tr->completion);
+ if (tr->err)
+ ret = tr->err;
}
- cycles = end[7] - start[0];
- printk("\nBlock: %6lu cycles (%4lu cycles/byte)\n",
- cycles, cycles / (8 * speed[i].blen));
+ end = get_cycles();
+ cycles = end - start;
+ pr_cont("%6lu cycles/operation, %4lu cycles/byte\n",
+ cycles, cycles / (8 * speed[i].blen));
+
+ if (ret) {
+ pr_err("At least one hashing failed ret=%d\n", ret);
+ break;
+ }
}
- ret = 0;
out:
for (k = 0; k < 8; ++k)
- ahash_request_free(req[k]);
-out_noreq:
+ ahash_request_free(data[k].req);
+
for (k = 0; k < 8; ++k)
- testmgr_free_buf(xbuf[k]);
-out_nobuf:
- return;
+ testmgr_free_buf(data[k].xbuf);
+
+ crypto_free_ahash(tfm);
+
+free_data:
+ kfree(data);
}
static int test_ahash_jiffies_digest(struct ahash_request *req, int blen,
--
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: Doing crypto in small stack buffers (bluetooth vs vmalloc-stack crash, etc)
From: George Spelvin @ 2016-06-28 12:37 UTC (permalink / raw)
To: herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q,
luto-kltTT9wpgjJwATOyAt5JVQ
Cc: linux-+7tBnqSOmQ59SlIZoIAWRdHuzzzSOjJt,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
Just a note, crypto/cts.c also does a lot of sg_set_buf() in stack buffers.
I have a local patch (appended, if anyone wants) to reduce the wasteful
amount of buffer space it uses (from 7 to 3 blocks on encrypt, from
6 to 3 blocks on decrypt), but it would take some rework to convert to
crypto_cipher_encrypt_one() or avoid stack buffers entirely.
commit c0aa0ae38dc6115b378939c5483ba6c7eb65d92a
Author: George Spelvin <linux-+7tBnqSOmQ59SlIZoIAWRdHuzzzSOjJt@public.gmane.org>
Date: Sat Oct 10 17:26:08 2015 -0400
crypto: cts - Reduce internal buffer usage
It only takes a 3-block temporary buffer to handle all the tricky
CTS cases. Encryption could in theory be done with two, but at a cost
in complexity.
But it's still a saving from the previous six blocks on the stack.
One issue I'm uncertain of and I'd like clarification on: to simplify
the cts_cbc_{en,de}crypt calls, I pass in the lcldesc structure which
contains the ctx->child transform rather than the parent one. I'm
assuming the block sizes are guaranteed to be the same (they're set up
in crypto_cts_alloc by copying), but I haven't been able to prove it to
my satisfaction.
Signed-off-by: George Spelvin <linux-+7tBnqSOmQ59SlIZoIAWRdHuzzzSOjJt@public.gmane.org>
diff --git a/crypto/cts.c b/crypto/cts.c
index e467ec0ac..e24d2e15 100644
--- a/crypto/cts.c
+++ b/crypto/cts.c
@@ -70,54 +70,44 @@ static int crypto_cts_setkey(struct crypto_tfm *parent, const u8 *key,
return err;
}
-static int cts_cbc_encrypt(struct crypto_cts_ctx *ctx,
- struct blkcipher_desc *desc,
+/*
+ * The final CTS encryption is just like CBC encryption except that:
+ * - the last plaintext block is zero-padded,
+ * - the second-last ciphertext block is trimmed, and
+ * - the last (complete) block of ciphertext is output before the
+ * (truncated) second-last one.
+ */
+static int cts_cbc_encrypt(struct blkcipher_desc *lcldesc,
struct scatterlist *dst,
struct scatterlist *src,
unsigned int offset,
unsigned int nbytes)
{
- int bsize = crypto_blkcipher_blocksize(desc->tfm);
- u8 tmp[bsize], tmp2[bsize];
- struct blkcipher_desc lcldesc;
- struct scatterlist sgsrc[1], sgdst[1];
+ int bsize = crypto_blkcipher_blocksize(lcldesc->tfm);
+ u8 tmp[3*bsize] __aligned(8);
+ struct scatterlist sgsrc[1], sgdst[2];
int lastn = nbytes - bsize;
- u8 iv[bsize];
- u8 s[bsize * 2], d[bsize * 2];
int err;
- if (lastn < 0)
+ if (lastn <= 0)
return -EINVAL;
- sg_init_table(sgsrc, 1);
- sg_init_table(sgdst, 1);
-
- memset(s, 0, sizeof(s));
- scatterwalk_map_and_copy(s, src, offset, nbytes, 0);
-
- memcpy(iv, desc->info, bsize);
-
- lcldesc.tfm = ctx->child;
- lcldesc.info = iv;
- lcldesc.flags = desc->flags;
-
- sg_set_buf(&sgsrc[0], s, bsize);
- sg_set_buf(&sgdst[0], tmp, bsize);
- err = crypto_blkcipher_encrypt_iv(&lcldesc, sgdst, sgsrc, bsize);
-
- memcpy(d + bsize, tmp, lastn);
-
- lcldesc.info = tmp;
-
- sg_set_buf(&sgsrc[0], s + bsize, bsize);
- sg_set_buf(&sgdst[0], tmp2, bsize);
- err = crypto_blkcipher_encrypt_iv(&lcldesc, sgdst, sgsrc, bsize);
-
- memcpy(d, tmp2, bsize);
-
- scatterwalk_map_and_copy(d, dst, offset, nbytes, 1);
-
- memcpy(desc->info, tmp2, bsize);
+ /* Copy the input to a temporary buffer; tmp = xxx, P[n-1], P[n] */
+ memset(tmp+2*bsize, 0, bsize);
+ scatterwalk_map_and_copy(tmp+bsize, src, offset, nbytes, 0);
+
+ sg_init_one(sgsrc, tmp+bsize, 2*bsize);
+ /* Initialize dst specially to do the rearrangement for us */
+ sg_init_table(sgdst, 2);
+ sg_set_buf(sgdst+0, tmp+bsize, bsize);
+ sg_set_buf(sgdst+1, tmp, bsize);
+
+ /* CBC-encrypt in place the two blocks; tmp = C[n], C[n-1], P[n] */
+ err = crypto_blkcipher_encrypt_iv(lcldesc, sgdst, sgsrc, 2*bsize);
+
+ /* Copy beginning of tmp to the output */
+ scatterwalk_map_and_copy(tmp, dst, offset, nbytes, 1);
+ memzero_explicit(tmp, sizeof(tmp));
return err;
}
@@ -126,8 +116,8 @@ static int crypto_cts_encrypt(struct blkcipher_desc *desc,
struct scatterlist *dst, struct scatterlist *src,
unsigned int nbytes)
{
- struct crypto_cts_ctx *ctx = crypto_blkcipher_ctx(desc->tfm);
int bsize = crypto_blkcipher_blocksize(desc->tfm);
+ struct crypto_cts_ctx *ctx = crypto_blkcipher_ctx(desc->tfm);
int tot_blocks = (nbytes + bsize - 1) / bsize;
int cbc_blocks = tot_blocks > 2 ? tot_blocks - 2 : 0;
struct blkcipher_desc lcldesc;
@@ -140,14 +130,14 @@ static int crypto_cts_encrypt(struct blkcipher_desc *desc,
if (tot_blocks == 1) {
err = crypto_blkcipher_encrypt_iv(&lcldesc, dst, src, bsize);
} else if (nbytes <= bsize * 2) {
- err = cts_cbc_encrypt(ctx, desc, dst, src, 0, nbytes);
+ err = cts_cbc_encrypt(&lcldesc, dst, src, 0, nbytes);
} else {
/* do normal function for tot_blocks - 2 */
err = crypto_blkcipher_encrypt_iv(&lcldesc, dst, src,
cbc_blocks * bsize);
if (err == 0) {
/* do cts for final two blocks */
- err = cts_cbc_encrypt(ctx, desc, dst, src,
+ err = cts_cbc_encrypt(&lcldesc, dst, src,
cbc_blocks * bsize,
nbytes - (cbc_blocks * bsize));
}
@@ -156,64 +146,68 @@ static int crypto_cts_encrypt(struct blkcipher_desc *desc,
return err;
}
-static int cts_cbc_decrypt(struct crypto_cts_ctx *ctx,
- struct blkcipher_desc *desc,
+/*
+ * Decrypting the final two blocks in CTS is a bit trickier;
+ * it has to be done in two separate steps.
+ *
+ * The last two blocks of the CTS ciphertext are (first) the
+ * last block C[n] of the equivalent zero-padded CBC encryption,
+ * followed by a truncated version of the second-last block C[n-1].
+ *
+ * Expressed in terms of CBC decryption (P[i] = decrypt(C[i]) ^ IV),
+ * CTS decryption can be expressed as:
+ * - Pad C[n-1] with zeros to get an IV for C[n].
+ * - CBC-decrypt C[n] to get an intermediate plaintext buffer P.
+ * - P[n] is the prefix of P (1..bsize bytes).
+ * - The suffix of P (0..bzize-1 bytes) is the missing part of C[n-1].
+ * - CBC-decrypt that C[n-1], with the incoming IV, to recover P[n-1].
+ */
+static int cts_cbc_decrypt(struct blkcipher_desc *lcldesc,
struct scatterlist *dst,
struct scatterlist *src,
unsigned int offset,
unsigned int nbytes)
{
- int bsize = crypto_blkcipher_blocksize(desc->tfm);
- u8 tmp[bsize];
- struct blkcipher_desc lcldesc;
+ int bsize = crypto_blkcipher_blocksize(lcldesc->tfm);
+ u8 tmp[3*bsize] __aligned(8);
struct scatterlist sgsrc[1], sgdst[1];
int lastn = nbytes - bsize;
- u8 iv[bsize];
- u8 s[bsize * 2], d[bsize * 2];
+ u8 *orig_iv;
int err;
- if (lastn < 0)
+ if (lastn <= 0)
return -EINVAL;
- sg_init_table(sgsrc, 1);
- sg_init_table(sgdst, 1);
+ /* 1. Copy source into tmp, zero-padded; tmp = C[n], C[n-1]+0, xxx */
+ memset(tmp+bsize, 0, bsize);
+ scatterwalk_map_and_copy(tmp, src, offset, nbytes, 0);
- scatterwalk_map_and_copy(s, src, offset, nbytes, 0);
-
- lcldesc.tfm = ctx->child;
- lcldesc.info = iv;
- lcldesc.flags = desc->flags;
-
- /* 1. Decrypt Cn-1 (s) to create Dn (tmp)*/
- memset(iv, 0, sizeof(iv));
- sg_set_buf(&sgsrc[0], s, bsize);
- sg_set_buf(&sgdst[0], tmp, bsize);
- err = crypto_blkcipher_decrypt_iv(&lcldesc, sgdst, sgsrc, bsize);
+ /* 2. Decrypt C[n] into P; tmp = C[n], C[n-1]+0, P */
+ sg_init_one(sgsrc, tmp, bsize);
+ sg_init_one(sgdst, tmp+2*bsize, bsize);
+ orig_iv = lcldesc->info;
+ lcldesc->info = tmp+bsize; /* IV for decryption: padded C[n-1] */
+ err = crypto_blkcipher_decrypt_iv(lcldesc, sgdst, sgsrc, bsize);
if (err)
- return err;
- /* 2. Pad Cn with zeros at the end to create C of length BB */
- memset(iv, 0, sizeof(iv));
- memcpy(iv, s + bsize, lastn);
- /* 3. Exclusive-or Dn (tmp) with C (iv) to create Xn (tmp) */
- crypto_xor(tmp, iv, bsize);
- /* 4. Select the first Ln bytes of Xn (tmp) to create Pn */
- memcpy(d + bsize, tmp, lastn);
+ goto cleanup;
- /* 5. Append the tail (BB - Ln) bytes of Xn (tmp) to Cn to create En */
- memcpy(s + bsize + lastn, tmp + lastn, bsize - lastn);
- /* 6. Decrypt En to create Pn-1 */
- memzero_explicit(iv, sizeof(iv));
+ /* 3. Copy tail of P to C[n-1]; tmp = C[n], C[n-1], P */
+ memcpy(tmp+bsize + lastn, tmp+2*bsize + lastn, bsize - lastn);
- sg_set_buf(&sgsrc[0], s + bsize, bsize);
- sg_set_buf(&sgdst[0], d, bsize);
- err = crypto_blkcipher_decrypt_iv(&lcldesc, sgdst, sgsrc, bsize);
+ /* 4. Decrypt C[n-1] in place; tmp = C[n], P[n-1], P */
+ sg_set_buf(sgsrc, tmp + bsize, bsize);
+ sg_set_buf(sgdst, tmp + bsize, bsize);
+ lcldesc->info = orig_iv;
+ err = crypto_blkcipher_decrypt_iv(lcldesc, sgdst, sgsrc, bsize);
- /* XOR with previous block */
- crypto_xor(d, desc->info, bsize);
+ /* 5. Copy P[n-1] and head of P to output */
+ scatterwalk_map_and_copy(tmp+bsize, dst, offset, nbytes, 1);
- scatterwalk_map_and_copy(d, dst, offset, nbytes, 1);
+ /* C[n] is the continuing IV (if anyone cares) */
+ memcpy(lcldesc->info, tmp, bsize);
- memcpy(desc->info, s, bsize);
+cleanup:
+ memzero_explicit(tmp, sizeof(tmp));
return err;
}
@@ -235,14 +229,14 @@ static int crypto_cts_decrypt(struct blkcipher_desc *desc,
if (tot_blocks == 1) {
err = crypto_blkcipher_decrypt_iv(&lcldesc, dst, src, bsize);
} else if (nbytes <= bsize * 2) {
- err = cts_cbc_decrypt(ctx, desc, dst, src, 0, nbytes);
+ err = cts_cbc_decrypt(&lcldesc, dst, src, 0, nbytes);
} else {
/* do normal function for tot_blocks - 2 */
err = crypto_blkcipher_decrypt_iv(&lcldesc, dst, src,
cbc_blocks * bsize);
if (err == 0) {
/* do cts for final two blocks */
- err = cts_cbc_decrypt(ctx, desc, dst, src,
+ err = cts_cbc_decrypt(&lcldesc, dst, src,
cbc_blocks * bsize,
nbytes - (cbc_blocks * bsize));
}
^ permalink raw reply related
* Re: Doing crypto in small stack buffers (bluetooth vs vmalloc-stack crash, etc)
From: Herbert Xu @ 2016-06-28 12:42 UTC (permalink / raw)
To: George Spelvin; +Cc: luto, linux-bluetooth, linux-crypto, netdev
In-Reply-To: <20160628123743.7960.qmail@ns.sciencehorizons.net>
On Tue, Jun 28, 2016 at 08:37:43AM -0400, George Spelvin wrote:
> Just a note, crypto/cts.c also does a lot of sg_set_buf() in stack buffers.
>
> I have a local patch (appended, if anyone wants) to reduce the wasteful
> amount of buffer space it uses (from 7 to 3 blocks on encrypt, from
> 6 to 3 blocks on decrypt), but it would take some rework to convert to
> crypto_cipher_encrypt_one() or avoid stack buffers entirely.
I'm currently working on cts and I'm removing the stack usage
altogether by having it operate on the src/dst SG lists only.
It's part of the skcipher conversion though so it'll have to go
through the crypto tree.
BTW, the only cts user in our tree appears to be implementing
CTS all over again and is only calling the crypto API cts for
the last two blocks. Someone should fix that.
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: Doing crypto in small stack buffers (bluetooth vs vmalloc-stack crash, etc)
From: George Spelvin @ 2016-06-28 13:23 UTC (permalink / raw)
To: herbert, linux; +Cc: linux-bluetooth, linux-crypto, luto, netdev
In-Reply-To: <20160628124241.GA17940@gondor.apana.org.au>
Herbert Xu wrote:
> I'm currently working on cts and I'm removing the stack usage
> altogether by having it operate on the src/dst SG lists only.
Wow, I should see how you do that. I couldn't get it below 3
blocks of temporary, and the dst SG list only gives you
one and a half.
> BTW, the only cts user in our tree appears to be implementing
> CTS all over again and is only calling the crypto API cts for
> the last two blocks. Someone should fix that.
Hint taken. Although I'm having a hard time finding that only user
amidst all the drivers thinking it means Clear To Send or (for HDMI)
Cycle Time Stamp.
Um...the uses in fs/crypto/keyinfo.c and fs/ext4/crypto_key.c
don't seem to do anything untoward.
Is net/sunrpc/auth_gss/gss_krb5_mech.c doing something odd?
I have a request of you: like Andy, I find the crypto layer an
impenetrable thicket of wrapper structures. I'm not suggesting there
aren't reasons for it, but it's extremely hard to infer those reasons by
looking at the code. If I were to draft a (hilariously wrong) overview
document, would you be willing to edit it into correctness?
^ permalink raw reply
* Re: Doing crypto in small stack buffers (bluetooth vs vmalloc-stack crash, etc)
From: Herbert Xu @ 2016-06-28 13:30 UTC (permalink / raw)
To: George Spelvin; +Cc: linux-bluetooth, linux-crypto, luto, netdev
In-Reply-To: <20160628132301.11479.qmail@ns.sciencehorizons.net>
On Tue, Jun 28, 2016 at 09:23:01AM -0400, George Spelvin wrote:
>
> Wow, I should see how you do that. I couldn't get it below 3
> blocks of temporary, and the dst SG list only gives you
> one and a half.
I don't mean that I'm using no temporary buffers at all, just
that the actual crypto only operates on the SG lists. I'm still
doing the xoring and stitching in temp buffers. I just counted
and I'm using three blocks like you.
> Is net/sunrpc/auth_gss/gss_krb5_mech.c doing something odd?
Yes gss_krb5_crypto.c is the one.
> I have a request of you: like Andy, I find the crypto layer an
> impenetrable thicket of wrapper structures. I'm not suggesting there
> aren't reasons for it, but it's extremely hard to infer those reasons by
> looking at the code. If I were to draft a (hilariously wrong) overview
> document, would you be willing to edit it into correctness?
We have actually gained quite a bit of documentation recently.
Have you looked at Documentation/DocBook/crypto-API.tmpl?
More is always welcome of course.
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
* Re: Doing crypto in small stack buffers (bluetooth vs vmalloc-stack crash, etc)
From: George Spelvin @ 2016-06-28 14:32 UTC (permalink / raw)
To: herbert, linux; +Cc: linux-bluetooth, linux-crypto, luto, netdev
In-Reply-To: <20160628133050.GA18468@gondor.apana.org.au>
> We have actually gained quite a bit of documentation recently.
> Have you looked at Documentation/DocBook/crypto-API.tmpl?
>
> More is always welcome of course.
It's improved since I last looked at it, but there are still many structures
that aren't described:
- struct crypto_instance
- struct crypto_spawn
- struct crypto_blkcipher
- struct blkcipher_desc
- More on the context structures returned by crypto_tfm_ctx
Also not mentioned in the documentation is that some algorithms *do*
have different implementations depending on key size. SHA-2 is the
classic example.
^ permalink raw reply
* [RFC] WireGuard: next generation secure network tunnel
From: Jason A. Donenfeld @ 2016-06-28 14:49 UTC (permalink / raw)
To: David Miller, Netdev, linux-crypto, LKML
Hi Dave & Folks,
Today I'm releasing WireGuard, an encrypted and authenticated
tunneling virtual interface for the kernel. It uses next-generation
cryptography and is designed to be both easy to use and simple to
implement (only ~4000 LoC, which compared to xfrm or openvpn is
spectacular), avoiding the enormous complexities of all other secure
tunneling tools. It's been a long road, but after considerable
research, experiments, cryptographic review, and implementing, I think
I'm at a point where I feel comfortable releasing this and asking for
your feedback. This isn't yet a patch series, however. There's still
some work to be done, I anticipate, before this is mergeable. But what
we have now is a good basis for discussion and talking about what
needs to be done for this to be a proper patch series.
You may visit the main info site about WireGuard at
https://www.wireguard.io and you can read the whitepaper and full
technical description and argumentation at
https://www.wireguard.io/papers/wireguard.pdf . The source code lives
at https://git.zx2c4.com/WireGuard/tree/src/ and you can read
instructions on building it in the install and quickstart sections of
the website. I'm not going to recapitulate all of the paper here, but
I will discuss the things that are most relevant to kernel
development.
WireGuard acts as a virtual interface, doing layer 3 IP tunneling,
addable with "ip link add dev wg0 type wireguard". You can set the
interface's local IP and routes using the usual ip-address and
ip-route tools. The WireGuard-specific elements are in a new tool
called `wg`, which will at some point be merged into the usual ip
tools. With `wg` you can set the device's private key, and give it a
list of associations between peers' public keys, their allowed IP
addresses, and their remote UDP endpoints. When a locally generated
packet hits the device, it looks at the dst IP, looks up this dst IP
in the aforementioned association table, and then encrypts it using
the proper public key's session. Conversely, when an encrypted packet
arrives on the interface, after it's been decrypted, the inner src IP
is looked up in this association table to see if it matches the public
key from which it originated. This is the "cryptokey routing table",
and many more details and explanations are found on the site and paper
above. But that's the basic gist; you add a device with ip-link, give
it keys with `wg`, and then you can start sending and receiving
packets on the interface that are secure.
In order to make this so seamless, WireGuard does away with a lot of
the _theoretically pure_ layering abstractions typically seen. First
of all, WireGuard is an interface, where crypto is done, which is a
considerable departure from the (hugely complex) xfrm-approach. It is
not unprecedented, however; the mac80211 infrastructure also does
crypto at this same layer. The massive gain is not only greater
simplicity in the codebase, but huge simplicity earnings and
ease-of-security for administrators. If a packet comes from a
WireGuard interface, it can be trusted as authentic and confidential.
If you want outgoing packets to be tunneled, point your routing table
at the WireGuard interface. It's basically that simple, removing years
and years of headaches (and catastrophically insecure
misconfigurations) people often have with the xfrm layer.
Second, WireGuard uses something based on the Noise Protocol Framework
(in Noise_IK) for key agreement and handshake, rather than, say,
relegating to a userspace daemon. The reason, again, is massive
simplicity and security savings. The Noise_IK handshake is extremely
simple, and tight integration between the handshake and the transport
layer allows WireGuard itself to handle all session-state and
connection-state and so-forth, making the whole process appear
"stateless" to the administrator (you set it up with `wg`, and then it
_just works_). There is no x509, no ASN.1, no huge complexity; the
user configures the public keys, and then the rest is taken care of.
Other configuration frameworks (based on x509 or SSL or LDAP or
whatever you want) can then build on top of this in userspace, if that
sort of thing is desired. But the basic handshake fundamentals are
left to WireGuard. This is more or less similar to SSH, which cares
about the authorized_keys file.
These two design choices are fundamental to WireGuard, and I believe
they confer significant benefits, which are discussed extensively in
the paper. There are two incidental implementation choices, however,
that I think will be more controversial from a kernel perspective, and
depending on the result of this discussion, maybe things will change,
or maybe they wont.
First, WireGuard doesn't use the kernel's crypto API. The overhead of
memory allocation and abstraction/indirection behind each
encryption/hashing/ec-multiplication operation not only adds
unfortunate performance overhead, but also bloats the code, impacting
ease of auditing and verification. Furthermore, the flexible design of
the kernel's crypto API isn't needed, or even desired, because, as
discussed in the paper, WireGuard uses a fixed set of cryptographic
primitives (ChaCha20, Poly1305, Blake2s, Curve25519). Instead
WireGuard ships its own primitives, with the ChaPoly ones being based
directly on Martin's existing kernel implementations. It does use some
nice aspects of the kernel's crypto layer, though. It makes use of
scatterwalk, blkcipher_walk, crypto_memneq, and padata, with padata in
particular being very nice.
Second, WireGuard initially used Netlink for configuration, but big
limitations and complexities lead to reimplementing it with ioctl
instead. It was really so much cleaner and simpler in the end to do it
that way. Probably upon reading that you're having a panic attack or
an embolism. If, after reviewing the current configuration code
(src/config.c), you have some ideas for a Netlink implementation that
is just as clean and isn't horrible, I'd be happy to return to
Netlink. With considerable hubris, though, I sort of suspect you'll
find the ioctl interface the most clean way. But who knows? I guess
you do.
There are a few code style issues that I'll need to clean up for you
as well. I happen to like long lines, I should probably prefix
non-static function names with "wg_", and I shouldn't make inline
functions outside of headers. But these are silly trivial things that
will get fixed up before it's git-send-email time.
Beyond those issues, I think you'll be rather pleased with WireGuard.
It makes use of a few tricks that are worth noting. I found that the
pattern of "encrypt(packet1), send(packet1), encrypt(packet2),
send(packet2), encrypt(packet3), send(packet3)" was much slower than
"encrypt(packet1), encrypt(packet2), encrypt(packet3), send(packet1),
send(packet2), send(packet3)", because (I suspect) cache misses along
the UDP xmit path. Using the faster pattern, the question is, "how
many packets should we encrypt before sending the list of them?" It
turns out there's no magic number, but rather we can learn this
dynamically due to GSO. The WireGuard driver claims that it can handle
un-segmented GSO "super-packets". When it receives one of these, it
splits it into N packets (using the usual skb_gso_segment() function),
and then encrypts each of these before sending each of them. That way,
the number is related to the way in which userspace is sending
packets. In practice this works very well, in case others would like
to use this technique too.
By the way, the design allows for easy namespace separation, where the
UDP sending/receiving socket can be in one namespace and the virtual
interface itself in another, so that you could, for example, give a
Docker container as its only interface a WireGuard tunnel, ensuring
that the Docker container's only way to get packets out is through the
secure tunnel.
Another neat thing I do is make use of SipHash24. WireGuard has a
hashtable of public keys. These public keys are supplied by userspace,
and thus could be maliciously crafted to create hash collisions. To
prevent against this attack, I use SipHash24, which is
cryptographically secure. This is nothing new; OpenBSD and Python and
a bunch of other projects use SipHash24 exactly the same way. But
aside from WireGuard, I haven't seen it used in the Linux kernel yet.
I'd be happy to put my implementation someplace where it belongs and
convert some other prone-to-poisoning code to use it, if you're
interested.
Generally speaking, though, I try to integrate and re-use as much as
possible. The driver itself is rtnl_link_register()-based. Packets are
sent and received using Tom's udp_tunnel_*() family of functions. ICMP
is handled by the usual icmp_send() functions. Stats utilize the newer
->tstats member. Hashlimit is used via xt_request_find_match() in the
proper way. skb_to_sgvec() is used for avoiding linearization. There's
lots of nice code-reuse, so you'll probably find your favorite goody
from the networking subsystem in there. Sparse is generally happy,
even when checking for endianness. Coverity Scan is happy too. I've
been working with Greg KH (CC'd) to ensure that for kernel code, in
general, WireGuard is up to snuff.
The best thing to do now would be to peruse the documentation, try
making some secure tunnels, and then take a look at the code. I'm
open to any and all feedback, and remain available for questions and
fixes and so forth, via email, on the mailing list, on IRC (#wireguard
on freenode), or I guess by telephone if you hate typing. In other
words, I'm committed to working with you any which way to get this in
shape for upstream. Right now it builds as a module for Linux >=4.1,
but as we get closer to [PATCH] posting time, I'll likely change
things into a full kernel tree and ditch the backwards-compatibility
#ifdefs. Importantly, though, WireGuard doesn't require any
modifications in other parts of the kernel, making it nicely
standalone. And most of all, the codebase is pretty short; I hope you
find it enjoyable to read.
I look forward to your feedback and comments.
Thank you,
Jason Donenfeld
^ permalink raw reply
* [PATCH] Remove redundant sg_init_table call.
From: Harsh Jain @ 2016-06-28 18:54 UTC (permalink / raw)
To: linux-crypto, herbert; +Cc: harshjain32, Harsh Jain
Remove redundant sg_init_table call. scatterwalk_ffwd doing the same.
Signed-off-by: Harsh Jain <harshjain.prof@gmail.com>
---
crypto/authenc.c | 7 +------
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/crypto/authenc.c b/crypto/authenc.c
index 55a354d..c7cc11d 100644
--- a/crypto/authenc.c
+++ b/crypto/authenc.c
@@ -206,7 +206,6 @@ static int crypto_authenc_encrypt(struct aead_request *req)
struct scatterlist *src, *dst;
int err;
- sg_init_table(areq_ctx->src, 2);
src = scatterwalk_ffwd(areq_ctx->src, req->src, req->assoclen);
dst = src;
@@ -215,7 +214,6 @@ static int crypto_authenc_encrypt(struct aead_request *req)
if (err)
return err;
- sg_init_table(areq_ctx->dst, 2);
dst = scatterwalk_ffwd(areq_ctx->dst, req->dst, req->assoclen);
}
@@ -251,14 +249,11 @@ static int crypto_authenc_decrypt_tail(struct aead_request *req,
if (crypto_memneq(ihash, ahreq->result, authsize))
return -EBADMSG;
- sg_init_table(areq_ctx->src, 2);
src = scatterwalk_ffwd(areq_ctx->src, req->src, req->assoclen);
dst = src;
- if (req->src != req->dst) {
- sg_init_table(areq_ctx->dst, 2);
+ if (req->src != req->dst)
dst = scatterwalk_ffwd(areq_ctx->dst, req->dst, req->assoclen);
- }
ablkcipher_request_set_tfm(abreq, ctx->enc);
ablkcipher_request_set_callback(abreq, aead_request_flags(req),
--
1.7.1
^ permalink raw reply related
* Re: Doing crypto in small stack buffers (bluetooth vs vmalloc-stack crash, etc)
From: Herbert Xu @ 2016-06-29 2:20 UTC (permalink / raw)
To: George Spelvin; +Cc: linux-bluetooth, linux-crypto, luto, netdev
In-Reply-To: <20160628143212.17215.qmail@ns.sciencehorizons.net>
On Tue, Jun 28, 2016 at 10:32:12AM -0400, George Spelvin wrote:
>
> - struct crypto_instance
> - struct crypto_spawn
> - struct crypto_blkcipher
> - struct blkcipher_desc
> - More on the context structures returned by crypto_tfm_ctx
blkcipher is obsolete and will be removed soon. So if you are
going to write this then please document skcipher instead.
> Also not mentioned in the documentation is that some algorithms *do*
> have different implementations depending on key size. SHA-2 is the
> classic example.
What do you mean by that? SHA has no keying at all.
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
* Re: [PATCH v2] crypto: tcrypt - Fix memory leaks/crashes in multibuffer hash speed test
From: Krzysztof Kozlowski @ 2016-06-29 8:16 UTC (permalink / raw)
To: Herbert Xu
Cc: David S. Miller, linux-crypto, linux-kernel,
Bartlomiej Zolnierkiewicz, Megha Dey, Fenghua Yu, Tim Chen
In-Reply-To: <20160628123352.GA17844@gondor.apana.org.au>
On 06/28/2016 02:33 PM, Herbert Xu wrote:
> On Tue, Jun 28, 2016 at 12:15:43PM +0200, Krzysztof Kozlowski wrote:
>> Oops:
>
> Thanks, there was a typo where it said k instead of j in the second
> loop.
>
> ---8<---
> This patch resolves a number of issues with the mb speed test
> function:
>
> * The tfm is never freed.
> * Memory is allocated even when we're not using mb.
> * When an error occurs we don't wait for completion for other requests.
> * When an error occurs during allocation we may leak memory.
> * The test function ignores plen but still runs for plen != blen.
> * The backlog flag is incorrectly used (may crash).
>
> This patch tries to resolve all these issues as well as making
> the code consistent with the existing hash speed testing function.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Seems to work fine except:
1. The updates are always 1.
2. For bigger blocks it reports always 1 or 3 cycles per byte:
[root@odroidxu3 ~]# modprobe tcrypt mode=423
[ 70.129206]
[ 70.129206] testing speed of multibuffer sha256 (sha256-neon)
[ 70.135511] test 0 ( 16 byte blocks, 16 bytes per update, 1 updates): 4597 cycles/operation, 35 cycles/byte
[ 70.145826] test 2 ( 64 byte blocks, 64 bytes per update, 1 updates): 5762 cycles/operation, 11 cycles/byte
[ 70.156469] test 5 ( 256 byte blocks, 256 bytes per update, 1 updates): 10687 cycles/operation, 5 cycles/byte
[ 70.167125] test 8 ( 1024 byte blocks, 1024 bytes per update, 1 updates): 31485 cycles/operation, 3 cycles/byte
[ 70.177919] test 12 ( 2048 byte blocks, 2048 bytes per update, 1 updates): 59281 cycles/operation, 3 cycles/byte
[ 70.189780] test 16 ( 4096 byte blocks, 4096 bytes per update, 1 updates): 118097 cycles/operation, 3 cycles/byte
[ 70.204117] test 21 ( 8192 byte blocks, 8192 bytes per update, 1 updates): 232309 cycles/operation, 3 cycles/byte
[root@odroidxu3 ~]# modprobe tcrypt mode=422
[ 71.988248]
[ 71.988248] testing speed of multibuffer sha1 (sha1-neon)
[ 71.994097] test 0 ( 16 byte blocks, 16 bytes per update, 1 updates): 2506 cycles/operation, 19 cycles/byte
[ 72.004547] test 2 ( 64 byte blocks, 64 bytes per update, 1 updates): 2299 cycles/operation, 4 cycles/byte
[ 72.015152] test 5 ( 256 byte blocks, 256 bytes per update, 1 updates): 3535 cycles/operation, 1 cycles/byte
[ 72.025807] test 8 ( 1024 byte blocks, 1024 bytes per update, 1 updates): 9403 cycles/operation, 1 cycles/byte
[ 72.036401] test 12 ( 2048 byte blocks, 2048 bytes per update, 1 updates): 17142 cycles/operation, 1 cycles/byte
[ 72.047058] test 16 ( 4096 byte blocks, 4096 bytes per update, 1 updates): 33109 cycles/operation, 1 cycles/byte
[ 72.057821] test 21 ( 8192 byte blocks, 8192 bytes per update, 1 updates): 67750 cycles/operation, 1 cycles/byte
modprobe: ERROR: could not insert 'tcrypt': Resource temporarily unavailable
Is it expected?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2] crypto: tcrypt - Fix memory leaks/crashes in multibuffer hash speed test
From: Herbert Xu @ 2016-06-29 8:19 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David S. Miller, linux-crypto, linux-kernel,
Bartlomiej Zolnierkiewicz, Megha Dey, Fenghua Yu, Tim Chen
In-Reply-To: <577383CA.1040204@samsung.com>
On Wed, Jun 29, 2016 at 10:16:10AM +0200, Krzysztof Kozlowski wrote:
>
> Seems to work fine except:
> 1. The updates are always 1.
Yes the test function only does digest so it's always one update.
> 2. For bigger blocks it reports always 1 or 3 cycles per byte:
Yes the average cycles per-byte should reach an asymptotic value.
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v2] crypto: tcrypt - Fix memory leaks/crashes in multibuffer hash speed test
From: Krzysztof Kozlowski @ 2016-06-29 8:28 UTC (permalink / raw)
To: Herbert Xu
Cc: David S. Miller, linux-crypto, linux-kernel,
Bartlomiej Zolnierkiewicz, Megha Dey, Fenghua Yu, Tim Chen
In-Reply-To: <20160629081905.GA25215@gondor.apana.org.au>
On 06/29/2016 10:19 AM, Herbert Xu wrote:
> On Wed, Jun 29, 2016 at 10:16:10AM +0200, Krzysztof Kozlowski wrote:
>>
>> Seems to work fine except:
>> 1. The updates are always 1.
>
> Yes the test function only does digest so it's always one update.
>
>> 2. For bigger blocks it reports always 1 or 3 cycles per byte:
>
> Yes the average cycles per-byte should reach an asymptotic value.
Then:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Best regards,
Krzysztof
^ permalink raw reply
* [v2 PATCH 0/7] crypto: rsa - Do not gratuitously drop leading zeroes
From: Herbert Xu @ 2016-06-29 9:56 UTC (permalink / raw)
To: Andrzej Zaborowski, Tadeusz Struk, Linux Crypto Mailing List
Cc: Tudor Ambarus, Stephan Mueller, Mat Martineau, Denis Kenzior,
Salvatore Benedetto
In-Reply-To: <20160622101432.GA30454@gondor.apana.org.au>
Hi:
This was prompted by the caam RSA submission where a lot of work
was done just to strip the RSA output of leading zeroes. This is
in fact completely pointless because the only user of RSA in the
kernel then promptly puts them back.
This patch series resolves this madness by simply leaving any
leading zeroes in place. Note that we're not requiring authors
to add leading zeroes, even though that is encouraged if it is
easy to do. In practice you'd only run into this every 2^32 or
2^64 operations so please don't overdo it.
I've also taken the opportunity to cleanup the pkcs1pad code.
v2 fixes the newly added dh to use the new MPI SG interface.
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
* [v2 PATCH 4/7] crypto: rsa-pkcs1pad - Remove bogus page splitting
From: Herbert Xu @ 2016-06-29 9:58 UTC (permalink / raw)
To: Andrzej Zaborowski, Tadeusz Struk, Linux Crypto Mailing List,
Tudor Ambarus, Stephan Mueller, Mat Martineau, Denis Kenzior,
Salvatore Benedetto
In-Reply-To: <20160629095655.GA26056@gondor.apana.org.au>
The helper pkcs1pad_sg_set_buf tries to split a buffer that crosses
a page boundary into two SG entries. This is unnecessary. This
patch removes that.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
crypto/rsa-pkcs1pad.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index 5c1c78e..d9baefb 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -103,7 +103,7 @@ struct pkcs1pad_inst_ctx {
struct pkcs1pad_request {
struct akcipher_request child_req;
- struct scatterlist in_sg[3], out_sg[2];
+ struct scatterlist in_sg[2], out_sg[1];
uint8_t *in_buf, *out_buf;
};
@@ -163,19 +163,10 @@ static int pkcs1pad_get_max_size(struct crypto_akcipher *tfm)
static void pkcs1pad_sg_set_buf(struct scatterlist *sg, void *buf, size_t len,
struct scatterlist *next)
{
- int nsegs = next ? 1 : 0;
-
- if (offset_in_page(buf) + len <= PAGE_SIZE) {
- nsegs += 1;
- sg_init_table(sg, nsegs);
- sg_set_buf(sg, buf, len);
- } else {
- nsegs += 2;
- sg_init_table(sg, nsegs);
- sg_set_buf(sg + 0, buf, PAGE_SIZE - offset_in_page(buf));
- sg_set_buf(sg + 1, buf + PAGE_SIZE - offset_in_page(buf),
- offset_in_page(buf) + len - PAGE_SIZE);
- }
+ int nsegs = next ? 2 : 1;
+
+ sg_init_table(sg, nsegs);
+ sg_set_buf(sg, buf, len);
if (next)
sg_chain(sg, nsegs, next);
^ permalink raw reply related
* [v2 PATCH 7/7] crypto: rsa-pkcs1pad - Avoid copying output when possible
From: Herbert Xu @ 2016-06-29 9:58 UTC (permalink / raw)
To: Andrzej Zaborowski, Tadeusz Struk, Linux Crypto Mailing List,
Tudor Ambarus, Stephan Mueller, Mat Martineau, Denis Kenzior,
Salvatore Benedetto
In-Reply-To: <20160629095655.GA26056@gondor.apana.org.au>
In the vast majority of cases (2^-32 on 32-bit and 2^-64 on 64-bit)
cases, the result from encryption/signing will require no padding.
This patch makes these two operations write their output directly
to the final destination. Only in the exceedingly rare cases where
fixup is needed to we copy it out and back to add the leading zeroes.
This patch also makes use of the crypto_akcipher_set_crypt API
instead of writing the akcipher request directly.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
crypto/rsa-pkcs1pad.c | 112 ++++++++++++++++++++------------------------------
1 file changed, 45 insertions(+), 67 deletions(-)
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index ebd8514..8ccfdd7 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -185,37 +185,36 @@ static int pkcs1pad_encrypt_sign_complete(struct akcipher_request *req, int err)
struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
struct pkcs1pad_ctx *ctx = akcipher_tfm_ctx(tfm);
struct pkcs1pad_request *req_ctx = akcipher_request_ctx(req);
- size_t pad_len = ctx->key_size - req_ctx->child_req.dst_len;
- size_t chunk_len, pad_left;
- struct sg_mapping_iter miter;
-
- if (!err) {
- if (pad_len) {
- sg_miter_start(&miter, req->dst,
- sg_nents_for_len(req->dst, pad_len),
- SG_MITER_ATOMIC | SG_MITER_TO_SG);
-
- pad_left = pad_len;
- while (pad_left) {
- sg_miter_next(&miter);
-
- chunk_len = min(miter.length, pad_left);
- memset(miter.addr, 0, chunk_len);
- pad_left -= chunk_len;
- }
-
- sg_miter_stop(&miter);
- }
-
- sg_pcopy_from_buffer(req->dst,
- sg_nents_for_len(req->dst, ctx->key_size),
- req_ctx->out_buf, req_ctx->child_req.dst_len,
- pad_len);
- }
+ unsigned int pad_len;
+ unsigned int len;
+ u8 *out_buf;
+
+ if (err)
+ goto out;
+
+ len = req_ctx->child_req.dst_len;
+ pad_len = ctx->key_size - len;
+
+ /* Four billion to one */
+ if (likely(!pad_len))
+ goto out;
+
+ out_buf = kzalloc(ctx->key_size, GFP_ATOMIC);
+ err = -ENOMEM;
+ if (!out_buf)
+ goto out;
+
+ sg_copy_to_buffer(req->dst, sg_nents_for_len(req->dst, len),
+ out_buf + pad_len, len);
+ sg_copy_from_buffer(req->dst,
+ sg_nents_for_len(req->dst, ctx->key_size),
+ out_buf, ctx->key_size);
+ kzfree(out_buf);
+
+out:
req->dst_len = ctx->key_size;
kfree(req_ctx->in_buf);
- kzfree(req_ctx->out_buf);
return err;
}
@@ -255,15 +254,6 @@ static int pkcs1pad_encrypt(struct akcipher_request *req)
return -EOVERFLOW;
}
- /*
- * Replace both input and output to add the padding in the input and
- * the potential missing leading zeros in the output.
- */
- req_ctx->child_req.src = req_ctx->in_sg;
- req_ctx->child_req.src_len = ctx->key_size - 1;
- req_ctx->child_req.dst = req_ctx->out_sg;
- req_ctx->child_req.dst_len = ctx->key_size;
-
req_ctx->in_buf = kmalloc(ctx->key_size - 1 - req->src_len,
GFP_KERNEL);
if (!req_ctx->in_buf)
@@ -291,6 +281,10 @@ static int pkcs1pad_encrypt(struct akcipher_request *req)
akcipher_request_set_callback(&req_ctx->child_req, req->base.flags,
pkcs1pad_encrypt_sign_complete_cb, req);
+ /* Reuse output buffer */
+ akcipher_request_set_crypt(&req_ctx->child_req, req_ctx->in_sg,
+ req->dst, ctx->key_size - 1, req->dst_len);
+
err = crypto_akcipher_encrypt(&req_ctx->child_req);
if (err != -EINPROGRESS &&
(err != -EBUSY ||
@@ -372,12 +366,6 @@ static int pkcs1pad_decrypt(struct akcipher_request *req)
if (!ctx->key_size || req->src_len != ctx->key_size)
return -EINVAL;
- /* Reuse input buffer, output to a new buffer */
- req_ctx->child_req.src = req->src;
- req_ctx->child_req.src_len = req->src_len;
- req_ctx->child_req.dst = req_ctx->out_sg;
- req_ctx->child_req.dst_len = ctx->key_size ;
-
req_ctx->out_buf = kmalloc(ctx->key_size, GFP_KERNEL);
if (!req_ctx->out_buf)
return -ENOMEM;
@@ -389,6 +377,11 @@ static int pkcs1pad_decrypt(struct akcipher_request *req)
akcipher_request_set_callback(&req_ctx->child_req, req->base.flags,
pkcs1pad_decrypt_complete_cb, req);
+ /* Reuse input buffer, output to a new buffer */
+ akcipher_request_set_crypt(&req_ctx->child_req, req->src,
+ req_ctx->out_sg, req->src_len,
+ ctx->key_size);
+
err = crypto_akcipher_decrypt(&req_ctx->child_req);
if (err != -EINPROGRESS &&
(err != -EBUSY ||
@@ -422,15 +415,6 @@ static int pkcs1pad_sign(struct akcipher_request *req)
return -EOVERFLOW;
}
- /*
- * Replace both input and output to add the padding in the input and
- * the potential missing leading zeros in the output.
- */
- req_ctx->child_req.src = req_ctx->in_sg;
- req_ctx->child_req.src_len = ctx->key_size - 1;
- req_ctx->child_req.dst = req_ctx->out_sg;
- req_ctx->child_req.dst_len = ctx->key_size;
-
req_ctx->in_buf = kmalloc(ctx->key_size - 1 - req->src_len,
GFP_KERNEL);
if (!req_ctx->in_buf)
@@ -447,19 +431,14 @@ static int pkcs1pad_sign(struct akcipher_request *req)
pkcs1pad_sg_set_buf(req_ctx->in_sg, req_ctx->in_buf,
ctx->key_size - 1 - req->src_len, req->src);
- req_ctx->out_buf = kmalloc(ctx->key_size, GFP_KERNEL);
- if (!req_ctx->out_buf) {
- kfree(req_ctx->in_buf);
- return -ENOMEM;
- }
-
- pkcs1pad_sg_set_buf(req_ctx->out_sg, req_ctx->out_buf,
- ctx->key_size, NULL);
-
akcipher_request_set_tfm(&req_ctx->child_req, ctx->child);
akcipher_request_set_callback(&req_ctx->child_req, req->base.flags,
pkcs1pad_encrypt_sign_complete_cb, req);
+ /* Reuse output buffer */
+ akcipher_request_set_crypt(&req_ctx->child_req, req_ctx->in_sg,
+ req->dst, ctx->key_size - 1, req->dst_len);
+
err = crypto_akcipher_sign(&req_ctx->child_req);
if (err != -EINPROGRESS &&
(err != -EBUSY ||
@@ -559,12 +538,6 @@ static int pkcs1pad_verify(struct akcipher_request *req)
if (!ctx->key_size || req->src_len < ctx->key_size)
return -EINVAL;
- /* Reuse input buffer, output to a new buffer */
- req_ctx->child_req.src = req->src;
- req_ctx->child_req.src_len = req->src_len;
- req_ctx->child_req.dst = req_ctx->out_sg;
- req_ctx->child_req.dst_len = ctx->key_size;
-
req_ctx->out_buf = kmalloc(ctx->key_size, GFP_KERNEL);
if (!req_ctx->out_buf)
return -ENOMEM;
@@ -576,6 +549,11 @@ static int pkcs1pad_verify(struct akcipher_request *req)
akcipher_request_set_callback(&req_ctx->child_req, req->base.flags,
pkcs1pad_verify_complete_cb, req);
+ /* Reuse input buffer, output to a new buffer */
+ akcipher_request_set_crypt(&req_ctx->child_req, req->src,
+ req_ctx->out_sg, req->src_len,
+ ctx->key_size);
+
err = crypto_akcipher_verify(&req_ctx->child_req);
if (err != -EINPROGRESS &&
(err != -EBUSY ||
^ permalink raw reply related
* [v2 PATCH 2/7] lib/mpi: Do not do sg_virt
From: Herbert Xu @ 2016-06-29 9:58 UTC (permalink / raw)
To: Andrzej Zaborowski, Tadeusz Struk, Linux Crypto Mailing List,
Tudor Ambarus, Stephan Mueller, Mat Martineau, Denis Kenzior,
Salvatore Benedetto
In-Reply-To: <20160629095655.GA26056@gondor.apana.org.au>
Currently the mpi SG helpers use sg_virt which is completely
broken. It happens to work with normal kernel memory but will
fail with anything that is not linearly mapped.
This patch fixes this by using the SG iterator helpers.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
lib/mpi/mpicoder.c | 86 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 50 insertions(+), 36 deletions(-)
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 7150e5c..c6272ae 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -21,6 +21,7 @@
#include <linux/bitops.h>
#include <linux/count_zeros.h>
#include <linux/byteorder/generic.h>
+#include <linux/scatterlist.h>
#include <linux/string.h>
#include "mpi-internal.h"
@@ -255,7 +256,9 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl, unsigned nbytes,
#error please implement for this limb size.
#endif
unsigned int n = mpi_get_size(a);
+ struct sg_mapping_iter miter;
int i, x, buf_len;
+ int nents;
if (sign)
*sign = a->sign;
@@ -263,23 +266,27 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl, unsigned nbytes,
if (nbytes < n)
return -EOVERFLOW;
- buf_len = sgl->length;
- p2 = sg_virt(sgl);
+ nents = sg_nents_for_len(sgl, nbytes);
+ if (nents < 0)
+ return -EINVAL;
- while (nbytes > n) {
- if (!buf_len) {
- sgl = sg_next(sgl);
- if (!sgl)
- return -EINVAL;
- buf_len = sgl->length;
- p2 = sg_virt(sgl);
- }
+ sg_miter_start(&miter, sgl, nents, SG_MITER_ATOMIC | SG_MITER_TO_SG);
+ sg_miter_next(&miter);
+ buf_len = miter.length;
+ p2 = miter.addr;
+ while (nbytes > n) {
i = min_t(unsigned, nbytes - n, buf_len);
memset(p2, 0, i);
p2 += i;
- buf_len -= i;
nbytes -= i;
+
+ buf_len -= i;
+ if (!buf_len) {
+ sg_miter_next(&miter);
+ buf_len = miter.length;
+ p2 = miter.addr;
+ }
}
for (i = a->nlimbs - 1; i >= 0; i--) {
@@ -293,17 +300,16 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl, unsigned nbytes,
p = (u8 *)&alimb;
for (x = 0; x < sizeof(alimb); x++) {
- if (!buf_len) {
- sgl = sg_next(sgl);
- if (!sgl)
- return -EINVAL;
- buf_len = sgl->length;
- p2 = sg_virt(sgl);
- }
*p2++ = *p++;
- buf_len--;
+ if (!--buf_len) {
+ sg_miter_next(&miter);
+ buf_len = miter.length;
+ p2 = miter.addr;
+ }
}
}
+
+ sg_miter_stop(&miter);
return 0;
}
EXPORT_SYMBOL_GPL(mpi_write_to_sgl);
@@ -323,19 +329,23 @@ EXPORT_SYMBOL_GPL(mpi_write_to_sgl);
*/
MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
{
- struct scatterlist *sg;
- int x, i, j, z, lzeros, ents;
+ struct sg_mapping_iter miter;
unsigned int nbits, nlimbs;
+ int x, j, z, lzeros, ents;
+ unsigned int len;
+ const u8 *buff;
mpi_limb_t a;
MPI val = NULL;
- lzeros = 0;
- ents = sg_nents(sgl);
+ ents = sg_nents_for_len(sgl, nbytes);
+ if (ents < 0)
+ return NULL;
- for_each_sg(sgl, sg, ents, i) {
- const u8 *buff = sg_virt(sg);
- int len = sg->length;
+ sg_miter_start(&miter, sgl, ents, SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+ lzeros = 0;
+ len = 0;
+ while (nbytes > 0) {
while (len && !*buff) {
lzeros++;
len--;
@@ -345,12 +355,14 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
if (len && *buff)
break;
- ents--;
+ sg_miter_next(&miter);
+ buff = miter.addr;
+ len = miter.length;
+
nbytes -= lzeros;
lzeros = 0;
}
- sgl = sg;
nbytes -= lzeros;
nbits = nbytes * 8;
if (nbits > MAX_EXTERN_MPI_BITS) {
@@ -359,8 +371,7 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
}
if (nbytes > 0)
- nbits -= count_leading_zeros(*(u8 *)(sg_virt(sgl) + lzeros)) -
- (BITS_PER_LONG - 8);
+ nbits -= count_leading_zeros(*buff) - (BITS_PER_LONG - 8);
nlimbs = DIV_ROUND_UP(nbytes, BYTES_PER_MPI_LIMB);
val = mpi_alloc(nlimbs);
@@ -379,21 +390,24 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
z = BYTES_PER_MPI_LIMB - nbytes % BYTES_PER_MPI_LIMB;
z %= BYTES_PER_MPI_LIMB;
- for_each_sg(sgl, sg, ents, i) {
- const u8 *buffer = sg_virt(sg) + lzeros;
- int len = sg->length - lzeros;
-
+ for (;;) {
for (x = 0; x < len; x++) {
a <<= 8;
- a |= *buffer++;
+ a |= *buff++;
if (((z + x + 1) % BYTES_PER_MPI_LIMB) == 0) {
val->d[j--] = a;
a = 0;
}
}
z += x;
- lzeros = 0;
+
+ if (!sg_miter_next(&miter))
+ break;
+
+ buff = miter.addr;
+ len = miter.length;
}
+
return val;
}
EXPORT_SYMBOL_GPL(mpi_read_raw_from_sgl);
^ permalink raw reply related
* [v2 PATCH 3/7] crypto: rsa-pkcs1pad - Require hash to be present
From: Herbert Xu @ 2016-06-29 9:58 UTC (permalink / raw)
To: Andrzej Zaborowski, Tadeusz Struk, Linux Crypto Mailing List,
Tudor Ambarus, Stephan Mueller, Mat Martineau, Denis Kenzior,
Salvatore Benedetto
In-Reply-To: <20160629095655.GA26056@gondor.apana.org.au>
The only user of rsa-pkcs1pad always uses the hash so there is
no reason to support the case of not having a hash.
This patch also changes the digest info lookup so that it is
only done once during template instantiation rather than on each
operation.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
crypto/rsa-pkcs1pad.c | 83 ++++++++++++++++++--------------------------------
1 file changed, 30 insertions(+), 53 deletions(-)
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index ead8dc0..5c1c78e 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -92,13 +92,12 @@ static const struct rsa_asn1_template *rsa_lookup_asn1(const char *name)
struct pkcs1pad_ctx {
struct crypto_akcipher *child;
- const char *hash_name;
unsigned int key_size;
};
struct pkcs1pad_inst_ctx {
struct crypto_akcipher_spawn spawn;
- const char *hash_name;
+ const struct rsa_asn1_template *digest_info;
};
struct pkcs1pad_request {
@@ -416,20 +415,16 @@ static int pkcs1pad_sign(struct akcipher_request *req)
struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
struct pkcs1pad_ctx *ctx = akcipher_tfm_ctx(tfm);
struct pkcs1pad_request *req_ctx = akcipher_request_ctx(req);
- const struct rsa_asn1_template *digest_info = NULL;
+ struct akcipher_instance *inst = akcipher_alg_instance(tfm);
+ struct pkcs1pad_inst_ctx *ictx = akcipher_instance_ctx(inst);
+ const struct rsa_asn1_template *digest_info = ictx->digest_info;
int err;
unsigned int ps_end, digest_size = 0;
if (!ctx->key_size)
return -EINVAL;
- if (ctx->hash_name) {
- digest_info = rsa_lookup_asn1(ctx->hash_name);
- if (!digest_info)
- return -EINVAL;
-
- digest_size = digest_info->size;
- }
+ digest_size = digest_info->size;
if (req->src_len + digest_size > ctx->key_size - 11)
return -EOVERFLOW;
@@ -462,10 +457,8 @@ static int pkcs1pad_sign(struct akcipher_request *req)
memset(req_ctx->in_buf + 1, 0xff, ps_end - 1);
req_ctx->in_buf[ps_end] = 0x00;
- if (digest_info) {
- memcpy(req_ctx->in_buf + ps_end + 1, digest_info->data,
- digest_info->size);
- }
+ memcpy(req_ctx->in_buf + ps_end + 1, digest_info->data,
+ digest_info->size);
pkcs1pad_sg_set_buf(req_ctx->in_sg, req_ctx->in_buf,
ctx->key_size - 1 - req->src_len, req->src);
@@ -499,7 +492,9 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err)
struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
struct pkcs1pad_ctx *ctx = akcipher_tfm_ctx(tfm);
struct pkcs1pad_request *req_ctx = akcipher_request_ctx(req);
- const struct rsa_asn1_template *digest_info;
+ struct akcipher_instance *inst = akcipher_alg_instance(tfm);
+ struct pkcs1pad_inst_ctx *ictx = akcipher_instance_ctx(inst);
+ const struct rsa_asn1_template *digest_info = ictx->digest_info;
unsigned int pos;
if (err == -EOVERFLOW)
@@ -527,17 +522,11 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err)
goto done;
pos++;
- if (ctx->hash_name) {
- digest_info = rsa_lookup_asn1(ctx->hash_name);
- if (!digest_info)
- goto done;
+ if (memcmp(req_ctx->out_buf + pos, digest_info->data,
+ digest_info->size))
+ goto done;
- if (memcmp(req_ctx->out_buf + pos, digest_info->data,
- digest_info->size))
- goto done;
-
- pos += digest_info->size;
- }
+ pos += digest_info->size;
err = 0;
@@ -626,12 +615,11 @@ static int pkcs1pad_init_tfm(struct crypto_akcipher *tfm)
struct pkcs1pad_ctx *ctx = akcipher_tfm_ctx(tfm);
struct crypto_akcipher *child_tfm;
- child_tfm = crypto_spawn_akcipher(akcipher_instance_ctx(inst));
+ child_tfm = crypto_spawn_akcipher(&ictx->spawn);
if (IS_ERR(child_tfm))
return PTR_ERR(child_tfm);
ctx->child = child_tfm;
- ctx->hash_name = ictx->hash_name;
return 0;
}
@@ -648,12 +636,12 @@ static void pkcs1pad_free(struct akcipher_instance *inst)
struct crypto_akcipher_spawn *spawn = &ctx->spawn;
crypto_drop_akcipher(spawn);
- kfree(ctx->hash_name);
kfree(inst);
}
static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb)
{
+ const struct rsa_asn1_template *digest_info;
struct crypto_attr_type *algt;
struct akcipher_instance *inst;
struct pkcs1pad_inst_ctx *ctx;
@@ -676,7 +664,11 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb)
hash_name = crypto_attr_alg_name(tb[2]);
if (IS_ERR(hash_name))
- hash_name = NULL;
+ return PTR_ERR(hash_name);
+
+ digest_info = rsa_lookup_asn1(hash_name);
+ if (!digest_info)
+ return -EINVAL;
inst = kzalloc(sizeof(*inst) + sizeof(*ctx), GFP_KERNEL);
if (!inst)
@@ -684,7 +676,7 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb)
ctx = akcipher_instance_ctx(inst);
spawn = &ctx->spawn;
- ctx->hash_name = hash_name ? kstrdup(hash_name, GFP_KERNEL) : NULL;
+ ctx->digest_info = digest_info;
crypto_set_spawn(&spawn->base, akcipher_crypto_instance(inst));
err = crypto_grab_akcipher(spawn, rsa_alg_name, 0,
@@ -696,27 +688,14 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb)
err = -ENAMETOOLONG;
- if (!hash_name) {
- if (snprintf(inst->alg.base.cra_name,
- CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s)",
- rsa_alg->base.cra_name) >=
- CRYPTO_MAX_ALG_NAME ||
- snprintf(inst->alg.base.cra_driver_name,
- CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s)",
- rsa_alg->base.cra_driver_name) >=
- CRYPTO_MAX_ALG_NAME)
+ if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
+ "pkcs1pad(%s,%s)", rsa_alg->base.cra_name, hash_name) >=
+ CRYPTO_MAX_ALG_NAME ||
+ snprintf(inst->alg.base.cra_driver_name, CRYPTO_MAX_ALG_NAME,
+ "pkcs1pad(%s,%s)",
+ rsa_alg->base.cra_driver_name, hash_name) >=
+ CRYPTO_MAX_ALG_NAME)
goto out_drop_alg;
- } else {
- if (snprintf(inst->alg.base.cra_name,
- CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s,%s)",
- rsa_alg->base.cra_name, hash_name) >=
- CRYPTO_MAX_ALG_NAME ||
- snprintf(inst->alg.base.cra_driver_name,
- CRYPTO_MAX_ALG_NAME, "pkcs1pad(%s,%s)",
- rsa_alg->base.cra_driver_name, hash_name) >=
- CRYPTO_MAX_ALG_NAME)
- goto out_free_hash;
- }
inst->alg.base.cra_flags = rsa_alg->base.cra_flags & CRYPTO_ALG_ASYNC;
inst->alg.base.cra_priority = rsa_alg->base.cra_priority;
@@ -738,12 +717,10 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb)
err = akcipher_register_instance(tmpl, inst);
if (err)
- goto out_free_hash;
+ goto out_drop_alg;
return 0;
-out_free_hash:
- kfree(ctx->hash_name);
out_drop_alg:
crypto_drop_akcipher(spawn);
out_free_inst:
^ permalink raw reply related
* [v2 PATCH 5/7] crypto: rsa-pkcs1pad - Always use GFP_KERNEL
From: Herbert Xu @ 2016-06-29 9:58 UTC (permalink / raw)
To: Andrzej Zaborowski, Tadeusz Struk, Linux Crypto Mailing List,
Tudor Ambarus, Stephan Mueller, Mat Martineau, Denis Kenzior,
Salvatore Benedetto
In-Reply-To: <20160629095655.GA26056@gondor.apana.org.au>
We don't currently support using akcipher in atomic contexts,
so GFP_KERNEL should always be used.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
crypto/rsa-pkcs1pad.c | 22 ++++++----------------
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index d9baefb..db19284 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -260,8 +260,7 @@ static int pkcs1pad_encrypt(struct akcipher_request *req)
req_ctx->child_req.dst_len = ctx->key_size;
req_ctx->in_buf = kmalloc(ctx->key_size - 1 - req->src_len,
- (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
- GFP_KERNEL : GFP_ATOMIC);
+ GFP_KERNEL);
if (!req_ctx->in_buf)
return -ENOMEM;
@@ -274,9 +273,7 @@ static int pkcs1pad_encrypt(struct akcipher_request *req)
pkcs1pad_sg_set_buf(req_ctx->in_sg, req_ctx->in_buf,
ctx->key_size - 1 - req->src_len, req->src);
- req_ctx->out_buf = kmalloc(ctx->key_size,
- (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
- GFP_KERNEL : GFP_ATOMIC);
+ req_ctx->out_buf = kmalloc(ctx->key_size, GFP_KERNEL);
if (!req_ctx->out_buf) {
kfree(req_ctx->in_buf);
return -ENOMEM;
@@ -379,9 +376,7 @@ static int pkcs1pad_decrypt(struct akcipher_request *req)
req_ctx->child_req.dst = req_ctx->out_sg;
req_ctx->child_req.dst_len = ctx->key_size ;
- req_ctx->out_buf = kmalloc(ctx->key_size,
- (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
- GFP_KERNEL : GFP_ATOMIC);
+ req_ctx->out_buf = kmalloc(ctx->key_size, GFP_KERNEL);
if (!req_ctx->out_buf)
return -ENOMEM;
@@ -438,8 +433,7 @@ static int pkcs1pad_sign(struct akcipher_request *req)
req_ctx->child_req.dst_len = ctx->key_size;
req_ctx->in_buf = kmalloc(ctx->key_size - 1 - req->src_len,
- (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
- GFP_KERNEL : GFP_ATOMIC);
+ GFP_KERNEL);
if (!req_ctx->in_buf)
return -ENOMEM;
@@ -454,9 +448,7 @@ static int pkcs1pad_sign(struct akcipher_request *req)
pkcs1pad_sg_set_buf(req_ctx->in_sg, req_ctx->in_buf,
ctx->key_size - 1 - req->src_len, req->src);
- req_ctx->out_buf = kmalloc(ctx->key_size,
- (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
- GFP_KERNEL : GFP_ATOMIC);
+ req_ctx->out_buf = kmalloc(ctx->key_size, GFP_KERNEL);
if (!req_ctx->out_buf) {
kfree(req_ctx->in_buf);
return -ENOMEM;
@@ -577,9 +569,7 @@ static int pkcs1pad_verify(struct akcipher_request *req)
req_ctx->child_req.dst = req_ctx->out_sg;
req_ctx->child_req.dst_len = ctx->key_size;
- req_ctx->out_buf = kmalloc(ctx->key_size,
- (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
- GFP_KERNEL : GFP_ATOMIC);
+ req_ctx->out_buf = kmalloc(ctx->key_size, GFP_KERNEL);
if (!req_ctx->out_buf)
return -ENOMEM;
^ permalink raw reply related
* [v2 PATCH 6/7] crypto: rsa-pkcs1pad - Move key size check to setkey
From: Herbert Xu @ 2016-06-29 9:58 UTC (permalink / raw)
To: Andrzej Zaborowski, Tadeusz Struk, Linux Crypto Mailing List,
Tudor Ambarus, Stephan Mueller, Mat Martineau, Denis Kenzior,
Salvatore Benedetto
In-Reply-To: <20160629095655.GA26056@gondor.apana.org.au>
Rather than repeatedly checking the key size on each operation,
we should be checking it once when the key is set.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
crypto/rsa-pkcs1pad.c | 56 +++++++++++++++++++++++---------------------------
1 file changed, 26 insertions(+), 30 deletions(-)
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index db19284..ebd8514 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -111,40 +111,48 @@ static int pkcs1pad_set_pub_key(struct crypto_akcipher *tfm, const void *key,
unsigned int keylen)
{
struct pkcs1pad_ctx *ctx = akcipher_tfm_ctx(tfm);
- int err, size;
+ int err;
+
+ ctx->key_size = 0;
err = crypto_akcipher_set_pub_key(ctx->child, key, keylen);
+ if (err)
+ return err;
- if (!err) {
- /* Find out new modulus size from rsa implementation */
- size = crypto_akcipher_maxsize(ctx->child);
+ /* Find out new modulus size from rsa implementation */
+ err = crypto_akcipher_maxsize(ctx->child);
+ if (err < 0)
+ return err;
- ctx->key_size = size > 0 ? size : 0;
- if (size <= 0)
- err = size;
- }
+ if (err > PAGE_SIZE)
+ return -ENOTSUPP;
- return err;
+ ctx->key_size = err;
+ return 0;
}
static int pkcs1pad_set_priv_key(struct crypto_akcipher *tfm, const void *key,
unsigned int keylen)
{
struct pkcs1pad_ctx *ctx = akcipher_tfm_ctx(tfm);
- int err, size;
+ int err;
+
+ ctx->key_size = 0;
err = crypto_akcipher_set_priv_key(ctx->child, key, keylen);
+ if (err)
+ return err;
- if (!err) {
- /* Find out new modulus size from rsa implementation */
- size = crypto_akcipher_maxsize(ctx->child);
+ /* Find out new modulus size from rsa implementation */
+ err = crypto_akcipher_maxsize(ctx->child);
+ if (err < 0)
+ return err;
- ctx->key_size = size > 0 ? size : 0;
- if (size <= 0)
- err = size;
- }
+ if (err > PAGE_SIZE)
+ return -ENOTSUPP;
- return err;
+ ctx->key_size = err;
+ return 0;
}
static int pkcs1pad_get_max_size(struct crypto_akcipher *tfm)
@@ -247,9 +255,6 @@ static int pkcs1pad_encrypt(struct akcipher_request *req)
return -EOVERFLOW;
}
- if (ctx->key_size > PAGE_SIZE)
- return -ENOTSUPP;
-
/*
* Replace both input and output to add the padding in the input and
* the potential missing leading zeros in the output.
@@ -367,9 +372,6 @@ static int pkcs1pad_decrypt(struct akcipher_request *req)
if (!ctx->key_size || req->src_len != ctx->key_size)
return -EINVAL;
- if (ctx->key_size > PAGE_SIZE)
- return -ENOTSUPP;
-
/* Reuse input buffer, output to a new buffer */
req_ctx->child_req.src = req->src;
req_ctx->child_req.src_len = req->src_len;
@@ -420,9 +422,6 @@ static int pkcs1pad_sign(struct akcipher_request *req)
return -EOVERFLOW;
}
- if (ctx->key_size > PAGE_SIZE)
- return -ENOTSUPP;
-
/*
* Replace both input and output to add the padding in the input and
* the potential missing leading zeros in the output.
@@ -560,9 +559,6 @@ static int pkcs1pad_verify(struct akcipher_request *req)
if (!ctx->key_size || req->src_len < ctx->key_size)
return -EINVAL;
- if (ctx->key_size > PAGE_SIZE)
- return -ENOTSUPP;
-
/* Reuse input buffer, output to a new buffer */
req_ctx->child_req.src = req->src;
req_ctx->child_req.src_len = req->src_len;
^ permalink raw reply related
* [v2 PATCH 1/7] crypto: rsa - Generate fixed-length output
From: Herbert Xu @ 2016-06-29 9:58 UTC (permalink / raw)
To: Andrzej Zaborowski, Tadeusz Struk, Linux Crypto Mailing List,
Tudor Ambarus, Stephan Mueller, Mat Martineau, Denis Kenzior,
Salvatore Benedetto
In-Reply-To: <20160629095655.GA26056@gondor.apana.org.au>
Every implementation of RSA that we have naturally generates
output with leading zeroes. The one and only user of RSA,
pkcs1pad wants to have those leading zeroes in place, in fact
because they are currently absent it has to write those zeroes
itself.
So we shouldn't be stripping leading zeroes in the first place.
In fact this patch makes rsa-generic produce output with fixed
length so that pkcs1pad does not need to do any extra work.
This patch also changes DH to use the new interface.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
crypto/rsa.c | 8 +++----
include/linux/mpi.h | 2 -
lib/mpi/mpicoder.c | 55 ++++++++++++++++++++++++----------------------------
3 files changed, 31 insertions(+), 34 deletions(-)
diff --git a/crypto/rsa.c b/crypto/rsa.c
index dc692d4..4c280b6 100644
--- a/crypto/rsa.c
+++ b/crypto/rsa.c
@@ -108,7 +108,7 @@ static int rsa_enc(struct akcipher_request *req)
if (ret)
goto err_free_m;
- ret = mpi_write_to_sgl(c, req->dst, &req->dst_len, &sign);
+ ret = mpi_write_to_sgl(c, req->dst, req->dst_len, &sign);
if (ret)
goto err_free_m;
@@ -147,7 +147,7 @@ static int rsa_dec(struct akcipher_request *req)
if (ret)
goto err_free_c;
- ret = mpi_write_to_sgl(m, req->dst, &req->dst_len, &sign);
+ ret = mpi_write_to_sgl(m, req->dst, req->dst_len, &sign);
if (ret)
goto err_free_c;
@@ -185,7 +185,7 @@ static int rsa_sign(struct akcipher_request *req)
if (ret)
goto err_free_m;
- ret = mpi_write_to_sgl(s, req->dst, &req->dst_len, &sign);
+ ret = mpi_write_to_sgl(s, req->dst, req->dst_len, &sign);
if (ret)
goto err_free_m;
@@ -226,7 +226,7 @@ static int rsa_verify(struct akcipher_request *req)
if (ret)
goto err_free_s;
- ret = mpi_write_to_sgl(m, req->dst, &req->dst_len, &sign);
+ ret = mpi_write_to_sgl(m, req->dst, req->dst_len, &sign);
if (ret)
goto err_free_s;
diff --git a/include/linux/mpi.h b/include/linux/mpi.h
index f219559..1cc5ffb 100644
--- a/include/linux/mpi.h
+++ b/include/linux/mpi.h
@@ -80,7 +80,7 @@ void *mpi_get_buffer(MPI a, unsigned *nbytes, int *sign);
int mpi_read_buffer(MPI a, uint8_t *buf, unsigned buf_len, unsigned *nbytes,
int *sign);
void *mpi_get_secure_buffer(MPI a, unsigned *nbytes, int *sign);
-int mpi_write_to_sgl(MPI a, struct scatterlist *sg, unsigned *nbytes,
+int mpi_write_to_sgl(MPI a, struct scatterlist *sg, unsigned nbytes,
int *sign);
#define log_mpidump g10_log_mpidump
diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 823cf5f..7150e5c 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -237,16 +237,13 @@ EXPORT_SYMBOL_GPL(mpi_get_buffer);
* @a: a multi precision integer
* @sgl: scatterlist to write to. Needs to be at least
* mpi_get_size(a) long.
- * @nbytes: in/out param - it has the be set to the maximum number of
- * bytes that can be written to sgl. This has to be at least
- * the size of the integer a. On return it receives the actual
- * length of the data written on success or the data that would
- * be written if buffer was too small.
+ * @nbytes: the number of bytes to write. Leading bytes will be
+ * filled with zero.
* @sign: if not NULL, it will be set to the sign of a.
*
* Return: 0 on success or error code in case of error
*/
-int mpi_write_to_sgl(MPI a, struct scatterlist *sgl, unsigned *nbytes,
+int mpi_write_to_sgl(MPI a, struct scatterlist *sgl, unsigned nbytes,
int *sign)
{
u8 *p, *p2;
@@ -258,43 +255,44 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl, unsigned *nbytes,
#error please implement for this limb size.
#endif
unsigned int n = mpi_get_size(a);
- int i, x, y = 0, lzeros, buf_len;
-
- if (!nbytes)
- return -EINVAL;
+ int i, x, buf_len;
if (sign)
*sign = a->sign;
- lzeros = count_lzeros(a);
-
- if (*nbytes < n - lzeros) {
- *nbytes = n - lzeros;
+ if (nbytes < n)
return -EOVERFLOW;
- }
- *nbytes = n - lzeros;
buf_len = sgl->length;
p2 = sg_virt(sgl);
- for (i = a->nlimbs - 1 - lzeros / BYTES_PER_MPI_LIMB,
- lzeros %= BYTES_PER_MPI_LIMB;
- i >= 0; i--) {
+ while (nbytes > n) {
+ if (!buf_len) {
+ sgl = sg_next(sgl);
+ if (!sgl)
+ return -EINVAL;
+ buf_len = sgl->length;
+ p2 = sg_virt(sgl);
+ }
+
+ i = min_t(unsigned, nbytes - n, buf_len);
+ memset(p2, 0, i);
+ p2 += i;
+ buf_len -= i;
+ nbytes -= i;
+ }
+
+ for (i = a->nlimbs - 1; i >= 0; i--) {
#if BYTES_PER_MPI_LIMB == 4
- alimb = cpu_to_be32(a->d[i]);
+ alimb = a->d[i] ? cpu_to_be32(a->d[i]) : 0;
#elif BYTES_PER_MPI_LIMB == 8
- alimb = cpu_to_be64(a->d[i]);
+ alimb = a->d[i] ? cpu_to_be64(a->d[i]) : 0;
#else
#error please implement for this limb size.
#endif
- if (lzeros) {
- y = lzeros;
- lzeros = 0;
- }
-
- p = (u8 *)&alimb + y;
+ p = (u8 *)&alimb;
- for (x = 0; x < sizeof(alimb) - y; x++) {
+ for (x = 0; x < sizeof(alimb); x++) {
if (!buf_len) {
sgl = sg_next(sgl);
if (!sgl)
@@ -305,7 +303,6 @@ int mpi_write_to_sgl(MPI a, struct scatterlist *sgl, unsigned *nbytes,
*p2++ = *p++;
buf_len--;
}
- y = 0;
}
return 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