* Re: [PATCH] crypto: move ablk_helper out of arch/x86
From: Jussi Kivilinna @ 2013-09-16 6:53 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-crypto, x86, linux-arch, herbert, mingo, arnd
In-Reply-To: <1379154250-14803-1-git-send-email-ard.biesheuvel@linaro.org>
On 14.09.2013 13:24, Ard Biesheuvel wrote:
> Move the ablk_helper code out of arch/x86 so it can be reused
> by other architectures. The only x86 specific dependency is
> a call to irq_fpu_usable(), in the generic case we use
> !in_interrupt() instead.
>
> Cc: jussi.kivilinna@iki.fi
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> Any need to split this up between generic/crypto and x86?
>
>
> arch/x86/crypto/Makefile | 1 -
> arch/x86/crypto/ablk_helper.c | 149 ----------------------------
> arch/x86/crypto/aesni-intel_glue.c | 2 +-
> arch/x86/crypto/camellia_aesni_avx2_glue.c | 2 +-
> arch/x86/crypto/camellia_aesni_avx_glue.c | 2 +-
> arch/x86/crypto/cast5_avx_glue.c | 2 +-
> arch/x86/crypto/cast6_avx_glue.c | 2 +-
> arch/x86/crypto/serpent_avx2_glue.c | 2 +-
> arch/x86/crypto/serpent_avx_glue.c | 2 +-
> arch/x86/crypto/serpent_sse2_glue.c | 2 +-
> arch/x86/crypto/twofish_avx_glue.c | 2 +-
> arch/x86/include/asm/crypto/ablk_helper.h | 38 ++------
> crypto/Kconfig | 23 +++--
> crypto/Makefile | 1 +
> crypto/ablk_helper.c | 150 +++++++++++++++++++++++++++++
> include/asm-generic/crypto/ablk_helper.h | 13 +++
> include/crypto/ablk_helper.h | 31 ++++++
> 17 files changed, 224 insertions(+), 200 deletions(-)
> delete mode 100644 arch/x86/crypto/ablk_helper.c
> create mode 100644 crypto/ablk_helper.c
> create mode 100644 include/asm-generic/crypto/ablk_helper.h
> create mode 100644 include/crypto/ablk_helper.h
>
> diff --git a/arch/x86/crypto/Makefile b/arch/x86/crypto/Makefile
> index 7d6ba9d..18fda50 100644
> --- a/arch/x86/crypto/Makefile
> +++ b/arch/x86/crypto/Makefile
> @@ -4,7 +4,6 @@
>
> avx_supported := $(call as-instr,vpxor %xmm0$(comma)%xmm0$(comma)%xmm0,yes,no)
>
> -obj-$(CONFIG_CRYPTO_ABLK_HELPER_X86) += ablk_helper.o
> obj-$(CONFIG_CRYPTO_GLUE_HELPER_X86) += glue_helper.o
This part does not apply cleanly to cryptodev tree (git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git).
>
[snip]
> diff --git a/include/asm-generic/crypto/ablk_helper.h b/include/asm-generic/crypto/ablk_helper.h
> new file mode 100644
> index 0000000..ede807f
> --- /dev/null
> +++ b/include/asm-generic/crypto/ablk_helper.h
> @@ -0,0 +1,13 @@
> +
> +#include <linux/hardirq.h>
> +
> +/*
> + * ablk_can_run_sync - used by crypto/ablk_helper to decide whether a request
> + * can be handled synchronously or needs to be queued up.
> + *
> + * Choose in_interrupt() as a reasonable default
> + */
Trailing whitespace in above comment block.
ERROR: trailing whitespace
#702: FILE: include/asm-generic/crypto/ablk_helper.h:7:
+ * $
Otherwise,
Acked-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
^ permalink raw reply
* Re: [PATCH] crypto_mem_not_equal: add constant-time equality testing of memory regions
From: Daniel Borkmann @ 2013-09-16 7:56 UTC (permalink / raw)
To: James Yonan; +Cc: Florian Weimer, Marcelo Cerri, linux-crypto, herbert
In-Reply-To: <5235E77F.1050807@openvpn.net>
On 09/15/2013 06:59 PM, James Yonan wrote:
> On 15/09/2013 09:45, Florian Weimer wrote:
>> * James Yonan:
>>
>>> + * Constant-time equality testing of memory regions.
>>> + * Returns 0 when data is equal, non-zero otherwise.
>>> + * Fast path if size == 16.
>>> + */
>>> +noinline unsigned long crypto_mem_not_equal(const void *a, const void *b, size_t size)
>>
>> I think this should really return unsigned or int, to reduce the risk
>> that the upper bytes are truncated because the caller uses an
>> inappropriate type, resulting in a bogus zero result. Reducing the
>> value to 0/1 probably doesn't hurt performance too much. It also
>> doesn't encode any information about the location of the difference in
>> the result value, which helps if that ever leaks.
>
> The problem with returning 0/1 within the function body of crypto_mem_not_equal is that it makes it easier for the compiler to introduce a short-circuit optimization.
>
> It might be better to move the test where the result is compared against 0 into an inline function:
>
> noinline unsigned long __crypto_mem_not_equal(const void *a, const void *b, size_t size);
>
> static inline int crypto_mem_not_equal(const void *a, const void *b, size_t size) {
> return __crypto_mem_not_equal(a, b, size) != 0UL ? 1 : 0;
> }
>
> This hides the fact that we are only interested in a boolean result from the compiler when it's compiling crypto_mem_not_equal.c, but also ensures type safety when users test the return value. It's also likely to have little or no performance impact.
Well, the code snippet I've provided from NaCl [1] is not really "fast-path"
as you say, but rather to prevent the compiler from doing such optimizations
by having a transformation of the "accumulated" bits into 0 and 1 as an end
result (likely to prevent a short circuit), plus it has static size, so no
loops applied here that could screw up.
Variable size could be done under arch/ in asm, and if not available, that
just falls back to normal memcmp that is being transformed into a same return
value. By that, all other archs could easily migrate afterwards. What do you
think?
[1] http://www.spinics.net/lists/linux-crypto/msg09558.html
^ permalink raw reply
* crypto: GCM API usage
From: dominik.d.paulus @ 2013-09-16 10:58 UTC (permalink / raw)
To: herbert, davem, linux-crypto; +Cc: linux-kernel, tobias.polzer
Hi,
We are currently trying to add encryption support to the usbip kernel
driver. Unfortunately, there is almost no documentation for the kernel
crypto API. So far, we couldn't figure out how to use the GCM encryption
mode in the kernel. There seems to be infrastructure for IV generation
in place (e.g. seqiv.c, the geniv stuff and the RFC 4106 implementation),
but no code directly using it.
What's the recommended way to use the IV generators with a "high-level"
API?
Regards,
Tobias Polzer and Dominik Paulus
^ permalink raw reply
* Re: [PATCH] crypto: tegra: use kernel entropy instead of ad-hoc
From: Stephen Warren @ 2013-09-16 15:28 UTC (permalink / raw)
To: Herbert Xu
Cc: Linus Walleij, Varun Wadekar, linux-crypto-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Neil Horman,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20130913233912.GA16389-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
On 09/13/2013 05:39 PM, Herbert Xu wrote:
> On Fri, Sep 13, 2013 at 10:12:36AM -0600, Stephen Warren wrote:
>>
>> I'm curious which kernel version it was merged for; it'd be nice to
>> remove tegra_chip_uid() from the Tegra tree now since it's unused, but
>> that obviously requires this patch in the history.
>
> This is in cryptodev which will be pushed upstream for 3.13.
OK, I guess I'll send you the follow-on cleanup patch then. While it
does touch arch/arm/mach-tegra code, I hope it won't conflict with
anything else going through the Tegra tree, so should be safe.
^ permalink raw reply
* [PATCH] ARM: tegra: remove tegra_chip_uid()
From: Stephen Warren @ 2013-09-16 15:34 UTC (permalink / raw)
To: Herbert Xu, David S. Miller
Cc: linux-crypto-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Linus Walleij,
Stephen Warren
From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Following commit f5b38c5 "crypto: tegra - use kernel entropy instead
of ad-hoc", this function is no longer used. It's also only accurate
for Tegra20 and not later SoCs. So, remove it.
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Herbert, this is a patch for the crypto tree, since it depends on a
patch to the tegra-aes crypto driver that's in your tree.
arch/arm/mach-tegra/fuse.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/arch/arm/mach-tegra/fuse.c b/arch/arm/mach-tegra/fuse.c
index e035cd2..64652b3 100644
--- a/arch/arm/mach-tegra/fuse.c
+++ b/arch/arm/mach-tegra/fuse.c
@@ -155,13 +155,3 @@ void tegra_init_fuse(void)
tegra_sku_id, tegra_cpu_process_id,
tegra_core_process_id);
}
-
-unsigned long long tegra_chip_uid(void)
-{
- unsigned long long lo, hi;
-
- lo = tegra_fuse_readl(FUSE_UID_LOW);
- hi = tegra_fuse_readl(FUSE_UID_HIGH);
- return (hi << 32ull) | lo;
-}
-EXPORT_SYMBOL(tegra_chip_uid);
--
1.8.1.5
^ permalink raw reply related
* Re: [PATCH] crypto_mem_not_equal: add constant-time equality testing of memory regions
From: James Yonan @ 2013-09-16 17:10 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Florian Weimer, Marcelo Cerri, linux-crypto, herbert
In-Reply-To: <5236B9A7.3090001@redhat.com>
On 16/09/2013 01:56, Daniel Borkmann wrote:
> On 09/15/2013 06:59 PM, James Yonan wrote:
>> On 15/09/2013 09:45, Florian Weimer wrote:
>>> * James Yonan:
>>>
>>>> + * Constant-time equality testing of memory regions.
>>>> + * Returns 0 when data is equal, non-zero otherwise.
>>>> + * Fast path if size == 16.
>>>> + */
>>>> +noinline unsigned long crypto_mem_not_equal(const void *a, const
>>>> void *b, size_t size)
>>>
>>> I think this should really return unsigned or int, to reduce the risk
>>> that the upper bytes are truncated because the caller uses an
>>> inappropriate type, resulting in a bogus zero result. Reducing the
>>> value to 0/1 probably doesn't hurt performance too much. It also
>>> doesn't encode any information about the location of the difference in
>>> the result value, which helps if that ever leaks.
>>
>> The problem with returning 0/1 within the function body of
>> crypto_mem_not_equal is that it makes it easier for the compiler to
>> introduce a short-circuit optimization.
>>
>> It might be better to move the test where the result is compared
>> against 0 into an inline function:
>>
>> noinline unsigned long __crypto_mem_not_equal(const void *a, const
>> void *b, size_t size);
>>
>> static inline int crypto_mem_not_equal(const void *a, const void *b,
>> size_t size) {
>> return __crypto_mem_not_equal(a, b, size) != 0UL ? 1 : 0;
>> }
>>
>> This hides the fact that we are only interested in a boolean result
>> from the compiler when it's compiling crypto_mem_not_equal.c, but also
>> ensures type safety when users test the return value. It's also
>> likely to have little or no performance impact.
>
> Well, the code snippet I've provided from NaCl [1] is not really
> "fast-path"
> as you say, but rather to prevent the compiler from doing such
> optimizations
> by having a transformation of the "accumulated" bits into 0 and 1 as an end
> result (likely to prevent a short circuit), plus it has static size, so no
> loops applied here that could screw up.
>
> Variable size could be done under arch/ in asm, and if not available, that
> just falls back to normal memcmp that is being transformed into a same
> return
> value. By that, all other archs could easily migrate afterwards. What do
> you
> think?
>
> [1] http://www.spinics.net/lists/linux-crypto/msg09558.html
I'm not sure that the differentbits -> 0/-1 transform in NaCl really
buys us anything because we don't care very much about making the final
test of differentbits != 0 constant-time. An attacker already knows
whether the test succeeded or failed -- we care more about making the
failure cases constant-time.
To do this, we need to make sure that the compiler doesn't insert one or
more early instructions to compare differentbits with 0xFF and then
bypass the rest of the F(n) lines because it knows then that the value
of differentbits cannot be changed by subsequent F(n) lines. It seems
that all of the approaches that use |= to build up the differentbits
value suffer from this problem.
My proposed solution is rather than trying to outsmart the compiler with
code that resists optimization, why not just turn optimization off
directly with #pragma GCC optimize. Or better yet, use an optimization
setting that provides reasonable speed without introducing potential
short-circuit optimizations.
By optimizing for size ("Os"), the compiler will need to turn off
optimizations that add code for a possible speed benefit, and the kind
of short-circuit optimizations that we are trying to avoid fall
precisely into this class -- they add an instruction to check if the OR
accumulator has all of its bits set, then if so, do an early exit. So
by using Os, we still benefit from optimizations that increase speed but
don't increase code size.
Once we eliminate the possibility of short-circuit optimizations, then
it's much more straightforward to make the function fast (e.g. by
comparing 64-bits at a time) and flexible (by handling arbitrary size).
My current implementation of crypto_mem_not_equal does both.
James
^ permalink raw reply
* Re: [PATCH] crypto_mem_not_equal: add constant-time equality testing of memory regions
From: Florian Weimer @ 2013-09-16 17:25 UTC (permalink / raw)
To: James Yonan; +Cc: Daniel Borkmann, Marcelo Cerri, linux-crypto, herbert
In-Reply-To: <5235E77F.1050807@openvpn.net>
* James Yonan:
> noinline unsigned long __crypto_mem_not_equal(const void *a, const
> void *b, size_t size);
>
> static inline int crypto_mem_not_equal(const void *a, const void *b,
> size_t size) {
> return __crypto_mem_not_equal(a, b, size) != 0UL ? 1 : 0;
> }
>
> This hides the fact that we are only interested in a boolean result
> from the compiler when it's compiling crypto_mem_not_equal.c, but also
> ensures type safety when users test the return value. It's also
> likely to have little or no performance impact.
Yes, that looks good to me.
(I haven't checked the other parts of your patch, though, especially
those where you adjust all callers.)
^ permalink raw reply
* Re: crypto: GCM API usage
From: Dominik Paulus @ 2013-09-16 18:34 UTC (permalink / raw)
To: linux-crypto, herbert, davem; +Cc: tobias.polzer, linux-kernel
In-Reply-To: <65abe254e199230984fd6a15dec39a76.squirrel@faumail.uni-erlangen.de>
Hi,
On Mon, Sep 16, 2013 at 12:58:40PM +0200, dominik.d.paulus@studium.uni-erlangen.de wrote:
> We are currently trying to add encryption support to the usbip kernel
> driver. Unfortunately, there is almost no documentation for the kernel
> crypto API. So far, we couldn't figure out how to use the GCM encryption
> mode in the kernel. There seems to be infrastructure for IV generation
> in place (e.g. seqiv.c, the geniv stuff and the RFC 4106 implementation),
> but no code directly using it.
>
> What's the recommended way to use the IV generators with a "high-level"
> API?
Sorry, that mail probably got a bit too short. To explain our problem a bit
more: We are currently using a 64-bit counter to generate IVs. As the
keys are randomly generated for each session and thus never reused,
that's probably a not too bad idea (if it is, please tell us why ;)),
assuming this counter is never going to overflow. We pass the IVs
directly to aead_request_set_crypt for each message. This currently
works quite fine.
However, we would expect that IV generation is at least partially handled
by the crypto API. As I said, there seems to be infrastructure for that,
that abstracts the sequence number quite nicely. The seqiv generator
seems to provide a high-level interface to the AEAD crypto, including an
abstraction for the sequence number generation. However, due to the lack
of documentation and/or reference code using the API, we couldn't find
out how to use it yet.
Any help on this would be appreciated. If we feel competent enough to do
so after finishing this project, we would also volunteer to extend the
introduction in Documentation/crypto/api-intro.txt a bit.
Regards,
Tobias Polzer and Dominik Paulus
^ permalink raw reply
* [PATCH v2 0/2] crypto: move ablk_helper out of arch/x86
From: Ard Biesheuvel @ 2013-09-17 7:49 UTC (permalink / raw)
To: linux-crypto; +Cc: jussi.kivilinna, herbert, patches, Ard Biesheuvel
v2:
- whitespace fix
- split into two patches so that the first one applies cleanly to the ARM/ARM64
trees as well
- rebased onto cryptodev/master
Ard Biesheuvel (2):
crypto: create generic version of ablk_helper
crypto: move x86 to the generic version of ablk_helper
arch/x86/crypto/Makefile | 1 -
arch/x86/crypto/ablk_helper.c | 149 ----------------------------
arch/x86/crypto/aesni-intel_glue.c | 2 +-
arch/x86/crypto/camellia_aesni_avx2_glue.c | 2 +-
arch/x86/crypto/camellia_aesni_avx_glue.c | 2 +-
arch/x86/crypto/cast5_avx_glue.c | 2 +-
arch/x86/crypto/cast6_avx_glue.c | 2 +-
arch/x86/crypto/serpent_avx2_glue.c | 2 +-
arch/x86/crypto/serpent_avx_glue.c | 2 +-
arch/x86/crypto/serpent_sse2_glue.c | 2 +-
arch/x86/crypto/twofish_avx_glue.c | 2 +-
arch/x86/include/asm/crypto/ablk_helper.h | 38 ++------
crypto/Kconfig | 23 +++--
crypto/Makefile | 1 +
crypto/ablk_helper.c | 150 +++++++++++++++++++++++++++++
include/asm-generic/crypto/ablk_helper.h | 13 +++
include/crypto/ablk_helper.h | 31 ++++++
17 files changed, 224 insertions(+), 200 deletions(-)
delete mode 100644 arch/x86/crypto/ablk_helper.c
create mode 100644 crypto/ablk_helper.c
create mode 100644 include/asm-generic/crypto/ablk_helper.h
create mode 100644 include/crypto/ablk_helper.h
--
1.8.1.2
^ permalink raw reply
* [PATCH v2 1/2] crypto: create generic version of ablk_helper
From: Ard Biesheuvel @ 2013-09-17 7:49 UTC (permalink / raw)
To: linux-crypto; +Cc: jussi.kivilinna, herbert, patches, Ard Biesheuvel
In-Reply-To: <1379404154-31537-1-git-send-email-ard.biesheuvel@linaro.org>
Create a generic version of ablk_helper so it can be reused
by other architectures. The only x86 specific dependency is
a call to irq_fpu_usable(), in the generic case we use
!in_interrupt() instead.
Acked-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
crypto/Kconfig | 4 +
crypto/Makefile | 1 +
crypto/ablk_helper.c | 150 +++++++++++++++++++++++++++++++
include/asm-generic/crypto/ablk_helper.h | 13 +++
include/crypto/ablk_helper.h | 31 +++++++
5 files changed, 199 insertions(+)
create mode 100644 crypto/ablk_helper.c
create mode 100644 include/asm-generic/crypto/ablk_helper.h
create mode 100644 include/crypto/ablk_helper.h
diff --git a/crypto/Kconfig b/crypto/Kconfig
index 69ce573..8179ae6 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -179,6 +179,10 @@ config CRYPTO_ABLK_HELPER_X86
depends on X86
select CRYPTO_CRYPTD
+config CRYPTO_ABLK_HELPER
+ tristate
+ select CRYPTO_CRYPTD
+
config CRYPTO_GLUE_HELPER_X86
tristate
depends on X86
diff --git a/crypto/Makefile b/crypto/Makefile
index 2d5ed08..580af97 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -104,3 +104,4 @@ obj-$(CONFIG_CRYPTO_USER_API_SKCIPHER) += algif_skcipher.o
obj-$(CONFIG_XOR_BLOCKS) += xor.o
obj-$(CONFIG_ASYNC_CORE) += async_tx/
obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += asymmetric_keys/
+obj-$(CONFIG_CRYPTO_ABLK_HELPER) += ablk_helper.o
diff --git a/crypto/ablk_helper.c b/crypto/ablk_helper.c
new file mode 100644
index 0000000..63ee760
--- /dev/null
+++ b/crypto/ablk_helper.c
@@ -0,0 +1,150 @@
+/*
+ * Shared async block cipher helpers
+ *
+ * Copyright (c) 2012 Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
+ *
+ * Based on aesni-intel_glue.c by:
+ * Copyright (C) 2008, Intel Corp.
+ * Author: Huang Ying <ying.huang@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
+ * USA
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/crypto.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/hardirq.h>
+#include <crypto/algapi.h>
+#include <crypto/cryptd.h>
+#include <crypto/ablk_helper.h>
+#include <asm/crypto/ablk_helper.h>
+
+int ablk_set_key(struct crypto_ablkcipher *tfm, const u8 *key,
+ unsigned int key_len)
+{
+ struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+ struct crypto_ablkcipher *child = &ctx->cryptd_tfm->base;
+ int err;
+
+ crypto_ablkcipher_clear_flags(child, CRYPTO_TFM_REQ_MASK);
+ crypto_ablkcipher_set_flags(child, crypto_ablkcipher_get_flags(tfm)
+ & CRYPTO_TFM_REQ_MASK);
+ err = crypto_ablkcipher_setkey(child, key, key_len);
+ crypto_ablkcipher_set_flags(tfm, crypto_ablkcipher_get_flags(child)
+ & CRYPTO_TFM_RES_MASK);
+ return err;
+}
+EXPORT_SYMBOL_GPL(ablk_set_key);
+
+int __ablk_encrypt(struct ablkcipher_request *req)
+{
+ struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
+ struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+ struct blkcipher_desc desc;
+
+ desc.tfm = cryptd_ablkcipher_child(ctx->cryptd_tfm);
+ desc.info = req->info;
+ desc.flags = 0;
+
+ return crypto_blkcipher_crt(desc.tfm)->encrypt(
+ &desc, req->dst, req->src, req->nbytes);
+}
+EXPORT_SYMBOL_GPL(__ablk_encrypt);
+
+int ablk_encrypt(struct ablkcipher_request *req)
+{
+ struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
+ struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+
+ if (!ablk_can_run_sync()) {
+ struct ablkcipher_request *cryptd_req =
+ ablkcipher_request_ctx(req);
+
+ memcpy(cryptd_req, req, sizeof(*req));
+ ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
+
+ return crypto_ablkcipher_encrypt(cryptd_req);
+ } else {
+ return __ablk_encrypt(req);
+ }
+}
+EXPORT_SYMBOL_GPL(ablk_encrypt);
+
+int ablk_decrypt(struct ablkcipher_request *req)
+{
+ struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
+ struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
+
+ if (!ablk_can_run_sync()) {
+ struct ablkcipher_request *cryptd_req =
+ ablkcipher_request_ctx(req);
+
+ memcpy(cryptd_req, req, sizeof(*req));
+ ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
+
+ return crypto_ablkcipher_decrypt(cryptd_req);
+ } else {
+ struct blkcipher_desc desc;
+
+ desc.tfm = cryptd_ablkcipher_child(ctx->cryptd_tfm);
+ desc.info = req->info;
+ desc.flags = 0;
+
+ return crypto_blkcipher_crt(desc.tfm)->decrypt(
+ &desc, req->dst, req->src, req->nbytes);
+ }
+}
+EXPORT_SYMBOL_GPL(ablk_decrypt);
+
+void ablk_exit(struct crypto_tfm *tfm)
+{
+ struct async_helper_ctx *ctx = crypto_tfm_ctx(tfm);
+
+ cryptd_free_ablkcipher(ctx->cryptd_tfm);
+}
+EXPORT_SYMBOL_GPL(ablk_exit);
+
+int ablk_init_common(struct crypto_tfm *tfm, const char *drv_name)
+{
+ struct async_helper_ctx *ctx = crypto_tfm_ctx(tfm);
+ struct cryptd_ablkcipher *cryptd_tfm;
+
+ cryptd_tfm = cryptd_alloc_ablkcipher(drv_name, 0, 0);
+ if (IS_ERR(cryptd_tfm))
+ return PTR_ERR(cryptd_tfm);
+
+ ctx->cryptd_tfm = cryptd_tfm;
+ tfm->crt_ablkcipher.reqsize = sizeof(struct ablkcipher_request) +
+ crypto_ablkcipher_reqsize(&cryptd_tfm->base);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ablk_init_common);
+
+int ablk_init(struct crypto_tfm *tfm)
+{
+ char drv_name[CRYPTO_MAX_ALG_NAME];
+
+ snprintf(drv_name, sizeof(drv_name), "__driver-%s",
+ crypto_tfm_alg_driver_name(tfm));
+
+ return ablk_init_common(tfm, drv_name);
+}
+EXPORT_SYMBOL_GPL(ablk_init);
+
+MODULE_LICENSE("GPL");
diff --git a/include/asm-generic/crypto/ablk_helper.h b/include/asm-generic/crypto/ablk_helper.h
new file mode 100644
index 0000000..156a1d4
--- /dev/null
+++ b/include/asm-generic/crypto/ablk_helper.h
@@ -0,0 +1,13 @@
+
+#include <linux/hardirq.h>
+
+/*
+ * ablk_can_run_sync - used by crypto/ablk_helper to decide whether a request
+ * can be handled synchronously or needs to be queued up.
+ *
+ * Choose in_interrupt() as a reasonable default
+ */
+static inline int ablk_can_run_sync(void)
+{
+ return !in_interrupt();
+}
diff --git a/include/crypto/ablk_helper.h b/include/crypto/ablk_helper.h
new file mode 100644
index 0000000..4f93df5
--- /dev/null
+++ b/include/crypto/ablk_helper.h
@@ -0,0 +1,31 @@
+/*
+ * Shared async block cipher helpers
+ */
+
+#ifndef _CRYPTO_ABLK_HELPER_H
+#define _CRYPTO_ABLK_HELPER_H
+
+#include <linux/crypto.h>
+#include <linux/kernel.h>
+#include <crypto/cryptd.h>
+
+struct async_helper_ctx {
+ struct cryptd_ablkcipher *cryptd_tfm;
+};
+
+extern int ablk_set_key(struct crypto_ablkcipher *tfm, const u8 *key,
+ unsigned int key_len);
+
+extern int __ablk_encrypt(struct ablkcipher_request *req);
+
+extern int ablk_encrypt(struct ablkcipher_request *req);
+
+extern int ablk_decrypt(struct ablkcipher_request *req);
+
+extern void ablk_exit(struct crypto_tfm *tfm);
+
+extern int ablk_init_common(struct crypto_tfm *tfm, const char *drv_name);
+
+extern int ablk_init(struct crypto_tfm *tfm);
+
+#endif /* _CRYPTO_ABLK_HELPER_H */
--
1.8.1.2
^ permalink raw reply related
* [PATCH v2 2/2] crypto: move x86 to the generic version of ablk_helper
From: Ard Biesheuvel @ 2013-09-17 7:49 UTC (permalink / raw)
To: linux-crypto; +Cc: jussi.kivilinna, herbert, patches, Ard Biesheuvel
In-Reply-To: <1379404154-31537-1-git-send-email-ard.biesheuvel@linaro.org>
Move all users of ablk_helper under x86/ to the generic version
and delete the x86 specific version.
Acked-by: Jussi Kivilinna <jussi.kivilinna@iki.fi>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
arch/x86/crypto/Makefile | 1 -
arch/x86/crypto/ablk_helper.c | 149 -----------------------------
arch/x86/crypto/aesni-intel_glue.c | 2 +-
arch/x86/crypto/camellia_aesni_avx2_glue.c | 2 +-
arch/x86/crypto/camellia_aesni_avx_glue.c | 2 +-
arch/x86/crypto/cast5_avx_glue.c | 2 +-
arch/x86/crypto/cast6_avx_glue.c | 2 +-
arch/x86/crypto/serpent_avx2_glue.c | 2 +-
arch/x86/crypto/serpent_avx_glue.c | 2 +-
arch/x86/crypto/serpent_sse2_glue.c | 2 +-
arch/x86/crypto/twofish_avx_glue.c | 2 +-
arch/x86/include/asm/crypto/ablk_helper.h | 38 ++------
crypto/Kconfig | 25 ++---
13 files changed, 28 insertions(+), 203 deletions(-)
delete mode 100644 arch/x86/crypto/ablk_helper.c
diff --git a/arch/x86/crypto/Makefile b/arch/x86/crypto/Makefile
index 75b08e1e..e0fc24d 100644
--- a/arch/x86/crypto/Makefile
+++ b/arch/x86/crypto/Makefile
@@ -6,7 +6,6 @@ avx_supported := $(call as-instr,vpxor %xmm0$(comma)%xmm0$(comma)%xmm0,yes,no)
avx2_supported := $(call as-instr,vpgatherdd %ymm0$(comma)(%eax$(comma)%ymm1\
$(comma)4)$(comma)%ymm2,yes,no)
-obj-$(CONFIG_CRYPTO_ABLK_HELPER_X86) += ablk_helper.o
obj-$(CONFIG_CRYPTO_GLUE_HELPER_X86) += glue_helper.o
obj-$(CONFIG_CRYPTO_AES_586) += aes-i586.o
diff --git a/arch/x86/crypto/ablk_helper.c b/arch/x86/crypto/ablk_helper.c
deleted file mode 100644
index 43282fe..0000000
--- a/arch/x86/crypto/ablk_helper.c
+++ /dev/null
@@ -1,149 +0,0 @@
-/*
- * Shared async block cipher helpers
- *
- * Copyright (c) 2012 Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
- *
- * Based on aesni-intel_glue.c by:
- * Copyright (C) 2008, Intel Corp.
- * Author: Huang Ying <ying.huang@intel.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
- * USA
- *
- */
-
-#include <linux/kernel.h>
-#include <linux/crypto.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <crypto/algapi.h>
-#include <crypto/cryptd.h>
-#include <asm/i387.h>
-#include <asm/crypto/ablk_helper.h>
-
-int ablk_set_key(struct crypto_ablkcipher *tfm, const u8 *key,
- unsigned int key_len)
-{
- struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
- struct crypto_ablkcipher *child = &ctx->cryptd_tfm->base;
- int err;
-
- crypto_ablkcipher_clear_flags(child, CRYPTO_TFM_REQ_MASK);
- crypto_ablkcipher_set_flags(child, crypto_ablkcipher_get_flags(tfm)
- & CRYPTO_TFM_REQ_MASK);
- err = crypto_ablkcipher_setkey(child, key, key_len);
- crypto_ablkcipher_set_flags(tfm, crypto_ablkcipher_get_flags(child)
- & CRYPTO_TFM_RES_MASK);
- return err;
-}
-EXPORT_SYMBOL_GPL(ablk_set_key);
-
-int __ablk_encrypt(struct ablkcipher_request *req)
-{
- struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
- struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
- struct blkcipher_desc desc;
-
- desc.tfm = cryptd_ablkcipher_child(ctx->cryptd_tfm);
- desc.info = req->info;
- desc.flags = 0;
-
- return crypto_blkcipher_crt(desc.tfm)->encrypt(
- &desc, req->dst, req->src, req->nbytes);
-}
-EXPORT_SYMBOL_GPL(__ablk_encrypt);
-
-int ablk_encrypt(struct ablkcipher_request *req)
-{
- struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
- struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
-
- if (!irq_fpu_usable()) {
- struct ablkcipher_request *cryptd_req =
- ablkcipher_request_ctx(req);
-
- memcpy(cryptd_req, req, sizeof(*req));
- ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
-
- return crypto_ablkcipher_encrypt(cryptd_req);
- } else {
- return __ablk_encrypt(req);
- }
-}
-EXPORT_SYMBOL_GPL(ablk_encrypt);
-
-int ablk_decrypt(struct ablkcipher_request *req)
-{
- struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
- struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
-
- if (!irq_fpu_usable()) {
- struct ablkcipher_request *cryptd_req =
- ablkcipher_request_ctx(req);
-
- memcpy(cryptd_req, req, sizeof(*req));
- ablkcipher_request_set_tfm(cryptd_req, &ctx->cryptd_tfm->base);
-
- return crypto_ablkcipher_decrypt(cryptd_req);
- } else {
- struct blkcipher_desc desc;
-
- desc.tfm = cryptd_ablkcipher_child(ctx->cryptd_tfm);
- desc.info = req->info;
- desc.flags = 0;
-
- return crypto_blkcipher_crt(desc.tfm)->decrypt(
- &desc, req->dst, req->src, req->nbytes);
- }
-}
-EXPORT_SYMBOL_GPL(ablk_decrypt);
-
-void ablk_exit(struct crypto_tfm *tfm)
-{
- struct async_helper_ctx *ctx = crypto_tfm_ctx(tfm);
-
- cryptd_free_ablkcipher(ctx->cryptd_tfm);
-}
-EXPORT_SYMBOL_GPL(ablk_exit);
-
-int ablk_init_common(struct crypto_tfm *tfm, const char *drv_name)
-{
- struct async_helper_ctx *ctx = crypto_tfm_ctx(tfm);
- struct cryptd_ablkcipher *cryptd_tfm;
-
- cryptd_tfm = cryptd_alloc_ablkcipher(drv_name, 0, 0);
- if (IS_ERR(cryptd_tfm))
- return PTR_ERR(cryptd_tfm);
-
- ctx->cryptd_tfm = cryptd_tfm;
- tfm->crt_ablkcipher.reqsize = sizeof(struct ablkcipher_request) +
- crypto_ablkcipher_reqsize(&cryptd_tfm->base);
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(ablk_init_common);
-
-int ablk_init(struct crypto_tfm *tfm)
-{
- char drv_name[CRYPTO_MAX_ALG_NAME];
-
- snprintf(drv_name, sizeof(drv_name), "__driver-%s",
- crypto_tfm_alg_driver_name(tfm));
-
- return ablk_init_common(tfm, drv_name);
-}
-EXPORT_SYMBOL_GPL(ablk_init);
-
-MODULE_LICENSE("GPL");
diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index f80e668..835488b 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -34,7 +34,7 @@
#include <asm/cpu_device_id.h>
#include <asm/i387.h>
#include <asm/crypto/aes.h>
-#include <asm/crypto/ablk_helper.h>
+#include <crypto/ablk_helper.h>
#include <crypto/scatterwalk.h>
#include <crypto/internal/aead.h>
#include <linux/workqueue.h>
diff --git a/arch/x86/crypto/camellia_aesni_avx2_glue.c b/arch/x86/crypto/camellia_aesni_avx2_glue.c
index 414fe5d..4209a76 100644
--- a/arch/x86/crypto/camellia_aesni_avx2_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx2_glue.c
@@ -14,6 +14,7 @@
#include <linux/types.h>
#include <linux/crypto.h>
#include <linux/err.h>
+#include <crypto/ablk_helper.h>
#include <crypto/algapi.h>
#include <crypto/ctr.h>
#include <crypto/lrw.h>
@@ -21,7 +22,6 @@
#include <asm/xcr.h>
#include <asm/xsave.h>
#include <asm/crypto/camellia.h>
-#include <asm/crypto/ablk_helper.h>
#include <asm/crypto/glue_helper.h>
#define CAMELLIA_AESNI_PARALLEL_BLOCKS 16
diff --git a/arch/x86/crypto/camellia_aesni_avx_glue.c b/arch/x86/crypto/camellia_aesni_avx_glue.c
index 37fd0c0..87a041a 100644
--- a/arch/x86/crypto/camellia_aesni_avx_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx_glue.c
@@ -14,6 +14,7 @@
#include <linux/types.h>
#include <linux/crypto.h>
#include <linux/err.h>
+#include <crypto/ablk_helper.h>
#include <crypto/algapi.h>
#include <crypto/ctr.h>
#include <crypto/lrw.h>
@@ -21,7 +22,6 @@
#include <asm/xcr.h>
#include <asm/xsave.h>
#include <asm/crypto/camellia.h>
-#include <asm/crypto/ablk_helper.h>
#include <asm/crypto/glue_helper.h>
#define CAMELLIA_AESNI_PARALLEL_BLOCKS 16
diff --git a/arch/x86/crypto/cast5_avx_glue.c b/arch/x86/crypto/cast5_avx_glue.c
index c663181..e6a3700 100644
--- a/arch/x86/crypto/cast5_avx_glue.c
+++ b/arch/x86/crypto/cast5_avx_glue.c
@@ -26,13 +26,13 @@
#include <linux/types.h>
#include <linux/crypto.h>
#include <linux/err.h>
+#include <crypto/ablk_helper.h>
#include <crypto/algapi.h>
#include <crypto/cast5.h>
#include <crypto/cryptd.h>
#include <crypto/ctr.h>
#include <asm/xcr.h>
#include <asm/xsave.h>
-#include <asm/crypto/ablk_helper.h>
#include <asm/crypto/glue_helper.h>
#define CAST5_PARALLEL_BLOCKS 16
diff --git a/arch/x86/crypto/cast6_avx_glue.c b/arch/x86/crypto/cast6_avx_glue.c
index 8d0dfb8..09f3677 100644
--- a/arch/x86/crypto/cast6_avx_glue.c
+++ b/arch/x86/crypto/cast6_avx_glue.c
@@ -28,6 +28,7 @@
#include <linux/types.h>
#include <linux/crypto.h>
#include <linux/err.h>
+#include <crypto/ablk_helper.h>
#include <crypto/algapi.h>
#include <crypto/cast6.h>
#include <crypto/cryptd.h>
@@ -37,7 +38,6 @@
#include <crypto/xts.h>
#include <asm/xcr.h>
#include <asm/xsave.h>
-#include <asm/crypto/ablk_helper.h>
#include <asm/crypto/glue_helper.h>
#define CAST6_PARALLEL_BLOCKS 8
diff --git a/arch/x86/crypto/serpent_avx2_glue.c b/arch/x86/crypto/serpent_avx2_glue.c
index 23aabc6..2fae489 100644
--- a/arch/x86/crypto/serpent_avx2_glue.c
+++ b/arch/x86/crypto/serpent_avx2_glue.c
@@ -14,6 +14,7 @@
#include <linux/types.h>
#include <linux/crypto.h>
#include <linux/err.h>
+#include <crypto/ablk_helper.h>
#include <crypto/algapi.h>
#include <crypto/ctr.h>
#include <crypto/lrw.h>
@@ -22,7 +23,6 @@
#include <asm/xcr.h>
#include <asm/xsave.h>
#include <asm/crypto/serpent-avx.h>
-#include <asm/crypto/ablk_helper.h>
#include <asm/crypto/glue_helper.h>
#define SERPENT_AVX2_PARALLEL_BLOCKS 16
diff --git a/arch/x86/crypto/serpent_avx_glue.c b/arch/x86/crypto/serpent_avx_glue.c
index 9ae83cf..ff48708 100644
--- a/arch/x86/crypto/serpent_avx_glue.c
+++ b/arch/x86/crypto/serpent_avx_glue.c
@@ -28,6 +28,7 @@
#include <linux/types.h>
#include <linux/crypto.h>
#include <linux/err.h>
+#include <crypto/ablk_helper.h>
#include <crypto/algapi.h>
#include <crypto/serpent.h>
#include <crypto/cryptd.h>
@@ -38,7 +39,6 @@
#include <asm/xcr.h>
#include <asm/xsave.h>
#include <asm/crypto/serpent-avx.h>
-#include <asm/crypto/ablk_helper.h>
#include <asm/crypto/glue_helper.h>
/* 8-way parallel cipher functions */
diff --git a/arch/x86/crypto/serpent_sse2_glue.c b/arch/x86/crypto/serpent_sse2_glue.c
index 97a356e..8c95f86 100644
--- a/arch/x86/crypto/serpent_sse2_glue.c
+++ b/arch/x86/crypto/serpent_sse2_glue.c
@@ -34,6 +34,7 @@
#include <linux/types.h>
#include <linux/crypto.h>
#include <linux/err.h>
+#include <crypto/ablk_helper.h>
#include <crypto/algapi.h>
#include <crypto/serpent.h>
#include <crypto/cryptd.h>
@@ -42,7 +43,6 @@
#include <crypto/lrw.h>
#include <crypto/xts.h>
#include <asm/crypto/serpent-sse2.h>
-#include <asm/crypto/ablk_helper.h>
#include <asm/crypto/glue_helper.h>
static void serpent_decrypt_cbc_xway(void *ctx, u128 *dst, const u128 *src)
diff --git a/arch/x86/crypto/twofish_avx_glue.c b/arch/x86/crypto/twofish_avx_glue.c
index a62ba54..4e3c665 100644
--- a/arch/x86/crypto/twofish_avx_glue.c
+++ b/arch/x86/crypto/twofish_avx_glue.c
@@ -28,6 +28,7 @@
#include <linux/types.h>
#include <linux/crypto.h>
#include <linux/err.h>
+#include <crypto/ablk_helper.h>
#include <crypto/algapi.h>
#include <crypto/twofish.h>
#include <crypto/cryptd.h>
@@ -39,7 +40,6 @@
#include <asm/xcr.h>
#include <asm/xsave.h>
#include <asm/crypto/twofish.h>
-#include <asm/crypto/ablk_helper.h>
#include <asm/crypto/glue_helper.h>
#include <crypto/scatterwalk.h>
#include <linux/workqueue.h>
diff --git a/arch/x86/include/asm/crypto/ablk_helper.h b/arch/x86/include/asm/crypto/ablk_helper.h
index 4f93df5..0f68111 100644
--- a/arch/x86/include/asm/crypto/ablk_helper.h
+++ b/arch/x86/include/asm/crypto/ablk_helper.h
@@ -1,31 +1,11 @@
-/*
- * Shared async block cipher helpers
- */
-
-#ifndef _CRYPTO_ABLK_HELPER_H
-#define _CRYPTO_ABLK_HELPER_H
-
-#include <linux/crypto.h>
-#include <linux/kernel.h>
-#include <crypto/cryptd.h>
-
-struct async_helper_ctx {
- struct cryptd_ablkcipher *cryptd_tfm;
-};
-
-extern int ablk_set_key(struct crypto_ablkcipher *tfm, const u8 *key,
- unsigned int key_len);
-
-extern int __ablk_encrypt(struct ablkcipher_request *req);
-extern int ablk_encrypt(struct ablkcipher_request *req);
+#include <asm/i387.h>
-extern int ablk_decrypt(struct ablkcipher_request *req);
-
-extern void ablk_exit(struct crypto_tfm *tfm);
-
-extern int ablk_init_common(struct crypto_tfm *tfm, const char *drv_name);
-
-extern int ablk_init(struct crypto_tfm *tfm);
-
-#endif /* _CRYPTO_ABLK_HELPER_H */
+/*
+ * ablk_can_run_sync - used by crypto/ablk_helper to decide whether a request
+ * can be handled synchronously or needs to be queued up
+ */
+static inline int ablk_can_run_sync(void)
+{
+ return irq_fpu_usable();
+}
diff --git a/crypto/Kconfig b/crypto/Kconfig
index 8179ae6..7c0a4c5 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -174,11 +174,6 @@ config CRYPTO_TEST
help
Quick & dirty crypto test module.
-config CRYPTO_ABLK_HELPER_X86
- tristate
- depends on X86
- select CRYPTO_CRYPTD
-
config CRYPTO_ABLK_HELPER
tristate
select CRYPTO_CRYPTD
@@ -699,7 +694,7 @@ config CRYPTO_AES_NI_INTEL
select CRYPTO_AES_X86_64 if 64BIT
select CRYPTO_AES_586 if !64BIT
select CRYPTO_CRYPTD
- select CRYPTO_ABLK_HELPER_X86
+ select CRYPTO_ABLK_HELPER
select CRYPTO_ALGAPI
select CRYPTO_GLUE_HELPER_X86 if 64BIT
select CRYPTO_LRW
@@ -883,7 +878,7 @@ config CRYPTO_CAMELLIA_AESNI_AVX_X86_64
depends on CRYPTO
select CRYPTO_ALGAPI
select CRYPTO_CRYPTD
- select CRYPTO_ABLK_HELPER_X86
+ select CRYPTO_ABLK_HELPER
select CRYPTO_GLUE_HELPER_X86
select CRYPTO_CAMELLIA_X86_64
select CRYPTO_LRW
@@ -905,7 +900,7 @@ config CRYPTO_CAMELLIA_AESNI_AVX2_X86_64
depends on CRYPTO
select CRYPTO_ALGAPI
select CRYPTO_CRYPTD
- select CRYPTO_ABLK_HELPER_X86
+ select CRYPTO_ABLK_HELPER
select CRYPTO_GLUE_HELPER_X86
select CRYPTO_CAMELLIA_X86_64
select CRYPTO_CAMELLIA_AESNI_AVX_X86_64
@@ -957,7 +952,7 @@ config CRYPTO_CAST5_AVX_X86_64
depends on X86 && 64BIT
select CRYPTO_ALGAPI
select CRYPTO_CRYPTD
- select CRYPTO_ABLK_HELPER_X86
+ select CRYPTO_ABLK_HELPER
select CRYPTO_CAST_COMMON
select CRYPTO_CAST5
help
@@ -980,7 +975,7 @@ config CRYPTO_CAST6_AVX_X86_64
depends on X86 && 64BIT
select CRYPTO_ALGAPI
select CRYPTO_CRYPTD
- select CRYPTO_ABLK_HELPER_X86
+ select CRYPTO_ABLK_HELPER
select CRYPTO_GLUE_HELPER_X86
select CRYPTO_CAST_COMMON
select CRYPTO_CAST6
@@ -1098,7 +1093,7 @@ config CRYPTO_SERPENT_SSE2_X86_64
depends on X86 && 64BIT
select CRYPTO_ALGAPI
select CRYPTO_CRYPTD
- select CRYPTO_ABLK_HELPER_X86
+ select CRYPTO_ABLK_HELPER
select CRYPTO_GLUE_HELPER_X86
select CRYPTO_SERPENT
select CRYPTO_LRW
@@ -1120,7 +1115,7 @@ config CRYPTO_SERPENT_SSE2_586
depends on X86 && !64BIT
select CRYPTO_ALGAPI
select CRYPTO_CRYPTD
- select CRYPTO_ABLK_HELPER_X86
+ select CRYPTO_ABLK_HELPER
select CRYPTO_GLUE_HELPER_X86
select CRYPTO_SERPENT
select CRYPTO_LRW
@@ -1142,7 +1137,7 @@ config CRYPTO_SERPENT_AVX_X86_64
depends on X86 && 64BIT
select CRYPTO_ALGAPI
select CRYPTO_CRYPTD
- select CRYPTO_ABLK_HELPER_X86
+ select CRYPTO_ABLK_HELPER
select CRYPTO_GLUE_HELPER_X86
select CRYPTO_SERPENT
select CRYPTO_LRW
@@ -1164,7 +1159,7 @@ config CRYPTO_SERPENT_AVX2_X86_64
depends on X86 && 64BIT
select CRYPTO_ALGAPI
select CRYPTO_CRYPTD
- select CRYPTO_ABLK_HELPER_X86
+ select CRYPTO_ABLK_HELPER
select CRYPTO_GLUE_HELPER_X86
select CRYPTO_SERPENT
select CRYPTO_SERPENT_AVX_X86_64
@@ -1280,7 +1275,7 @@ config CRYPTO_TWOFISH_AVX_X86_64
depends on X86 && 64BIT
select CRYPTO_ALGAPI
select CRYPTO_CRYPTD
- select CRYPTO_ABLK_HELPER_X86
+ select CRYPTO_ABLK_HELPER
select CRYPTO_GLUE_HELPER_X86
select CRYPTO_TWOFISH_COMMON
select CRYPTO_TWOFISH_X86_64
--
1.8.1.2
^ permalink raw reply related
* [PATCH] ansi_cprng: Fix off by one error in non-block size request
From: Neil Horman @ 2013-09-17 12:33 UTC (permalink / raw)
To: linux-crypto
Cc: Neil Horman, Stephan Mueller, Petr Matousek, Herbert Xu,
David S. Miller
Stephan Mueller reported to me recently a error in random number generation in
the ansi cprng. If several small requests are made that are less than the
instances block size, the remainder for loop code doesn't increment
rand_data_valid in the last iteration, meaning that the last bytes in the
rand_data buffer gets reused on the subsequent smaller-than-a-block request for
random data.
The fix is pretty easy, just re-code the for loop to make sure that
rand_data_valid gets incremented appropriately
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Stephan Mueller <stephan.mueller@atsec.com>
CC: Stephan Mueller <stephan.mueller@atsec.com>
CC: Petr Matousek <pmatouse@redhat.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: "David S. Miller" <davem@davemloft.net>
---
crypto/ansi_cprng.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/crypto/ansi_cprng.c b/crypto/ansi_cprng.c
index c0bb377..666f196 100644
--- a/crypto/ansi_cprng.c
+++ b/crypto/ansi_cprng.c
@@ -230,11 +230,11 @@ remainder:
*/
if (byte_count < DEFAULT_BLK_SZ) {
empty_rbuf:
- for (; ctx->rand_data_valid < DEFAULT_BLK_SZ;
- ctx->rand_data_valid++) {
+ while (ctx->rand_data_valid < DEFAULT_BLK_SZ) {
*ptr = ctx->rand_data[ctx->rand_data_valid];
ptr++;
byte_count--;
+ ctx->rand_data_valid++;
if (byte_count == 0)
goto done;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] crypto_mem_not_equal: add constant-time equality testing of memory regions
From: Daniel Borkmann @ 2013-09-17 19:07 UTC (permalink / raw)
To: James Yonan; +Cc: Florian Weimer, Marcelo Cerri, linux-crypto, herbert
In-Reply-To: <52373BA3.9090709@openvpn.net>
On 09/16/2013 07:10 PM, James Yonan wrote:
> On 16/09/2013 01:56, Daniel Borkmann wrote:
>> On 09/15/2013 06:59 PM, James Yonan wrote:
>>> On 15/09/2013 09:45, Florian Weimer wrote:
>>>> * James Yonan:
>>>>
>>>>> + * Constant-time equality testing of memory regions.
>>>>> + * Returns 0 when data is equal, non-zero otherwise.
>>>>> + * Fast path if size == 16.
>>>>> + */
>>>>> +noinline unsigned long crypto_mem_not_equal(const void *a, const
>>>>> void *b, size_t size)
>>>>
>>>> I think this should really return unsigned or int, to reduce the risk
>>>> that the upper bytes are truncated because the caller uses an
>>>> inappropriate type, resulting in a bogus zero result. Reducing the
>>>> value to 0/1 probably doesn't hurt performance too much. It also
>>>> doesn't encode any information about the location of the difference in
>>>> the result value, which helps if that ever leaks.
>>>
>>> The problem with returning 0/1 within the function body of
>>> crypto_mem_not_equal is that it makes it easier for the compiler to
>>> introduce a short-circuit optimization.
>>>
>>> It might be better to move the test where the result is compared
>>> against 0 into an inline function:
>>>
>>> noinline unsigned long __crypto_mem_not_equal(const void *a, const
>>> void *b, size_t size);
>>>
>>> static inline int crypto_mem_not_equal(const void *a, const void *b,
>>> size_t size) {
>>> return __crypto_mem_not_equal(a, b, size) != 0UL ? 1 : 0;
>>> }
>>>
>>> This hides the fact that we are only interested in a boolean result
>>> from the compiler when it's compiling crypto_mem_not_equal.c, but also
>>> ensures type safety when users test the return value. It's also
>>> likely to have little or no performance impact.
>>
>> Well, the code snippet I've provided from NaCl [1] is not really
>> "fast-path"
>> as you say, but rather to prevent the compiler from doing such
>> optimizations
>> by having a transformation of the "accumulated" bits into 0 and 1 as an end
>> result (likely to prevent a short circuit), plus it has static size, so no
>> loops applied here that could screw up.
>>
>> Variable size could be done under arch/ in asm, and if not available, that
>> just falls back to normal memcmp that is being transformed into a same
>> return
>> value. By that, all other archs could easily migrate afterwards. What do
>> you
>> think?
>>
>> [1] http://www.spinics.net/lists/linux-crypto/msg09558.html
>
> I'm not sure that the differentbits -> 0/-1 transform in NaCl really buys us anything because
> we don't care very much about making the final test of differentbits != 0 constant-time. An
> attacker already knows whether the test succeeded or failed -- we care more about making the
> failure cases constant-time.
>
> To do this, we need to make sure that the compiler doesn't insert one or more early instructions
> to compare differentbits with 0xFF and then bypass the rest of the F(n) lines because it knows
> then that the value of differentbits cannot be changed by subsequent F(n) lines. It seems that
> all of the approaches that use |= to build up the differentbits value suffer from this problem.
>
> My proposed solution is rather than trying to outsmart the compiler with code that resists
> optimization, why not just turn optimization off directly with #pragma GCC optimize. Or better
> yet, use an optimization setting that provides reasonable speed without introducing potential
> short-circuit optimizations.
>
> By optimizing for size ("Os"), the compiler will need to turn off optimizations that add code
> for a possible speed benefit, and the kind of short-circuit optimizations that we are trying to
> avoid fall precisely into this class -- they add an instruction to check if the OR accumulator has
> all of its bits set, then if so, do an early exit. So by using Os, we still benefit from
> optimizations that increase speed but don't increase code size.
While I was looking a bit further into this, I thought using an attribute like this might be a
more clean variant ...
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 92669cd..2505b1b 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -351,6 +351,11 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
*/
#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
+/* Tell GCC to turn off optimization for a particular function. */
+#ifndef __do_not_optimize
+#define __do_not_optimize __attribute__((optimize("O0")))
+#endif
+
/* Ignore/forbid kprobes attach on very low level functions marked by this attribute: */
#ifdef CONFIG_KPROBES
# define __kprobes __attribute__((__section__(".kprobes.text")))
... however, then I found on the GCC developer mailing list [1]: That said, I consider the
optimize attribute code seriously broken and unmaintained (but sometimes useful for debugging -
and only that). [...] Does "#pragma Gcc optimize" work more reliably? No, it uses the same
mechanism internally. [...] And if these options are so broken, should they be marked as such
in the manual? [...] Probably yes.
Therefore, I still need to ask ... what about the option of an arch/*/crypto/... asm
implementation? This still seems safer for something that crucial, imho.
> Once we eliminate the possibility of short-circuit optimizations, then it's much more straightforward
> to make the function fast (e.g. by comparing 64-bits at a time) and flexible (by handling arbitrary
> size). My current implementation of crypto_mem_not_equal does both.
[1] http://gcc.gnu.org/ml/gcc/2012-07/msg00211.html
^ permalink raw reply related
* Re: [PATCH V4 02/15] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
From: Dmitry Kasatkin @ 2013-09-17 21:51 UTC (permalink / raw)
To: Lee, Chun-Yi
Cc: linux-kernel@vger.kernel.org, linux-security-module, linux-efi,
linux-pm, linux-crypto, opensuse-kernel, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Pavel Machek,
Josh Boyer, Vojtech Pavlik, Matt Fleming, James Bottomley,
Greg KH, JKosina, Rusty Russell, Herbert Xu, David S. Miller,
H. Peter Anvin, Michal Marek, Gary Lin, Vivek Goyal, Lee, Chun-Yi
In-Reply-To: <1379206621-18639-3-git-send-email-jlee@suse.com>
Hello,
On Sat, Sep 14, 2013 at 7:56 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> first step of signature generation operation (RSASSA-PKCS1-v1_5-SIGN).
>
> This patch is temporary set emLen to pks->k, and temporary set EM to
> pks->S for debugging. We will replace the above values to real signature
> after implement RSASP1.
>
> The naming of EMSA_PKCS1_v1_5_ENCODE and the variables used in this function
> accord PKCS#1 spec but not follow kernel naming convention, it useful when look
> at them with spec.
>
> Reference: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1v2/pkcs1ietffinal.txt
> Reference: http://www.emc.com/collateral/white-papers/h11300-pkcs-1v2-2-rsa-cryptography-standard-wp.pdf
>
> V2:
> - Clean up naming of variable: replace _EM by EM, replace EM by EM_tmp.
> - Add comment to EMSA_PKCS1-v1_5-ENCODE function.
>
> Cc: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Jiri Kosina <jkosina@suse.cz>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
> crypto/asymmetric_keys/rsa.c | 163 +++++++++++++++++++++++++++++++++++++++++-
> include/crypto/public_key.h | 2 +
> 2 files changed, 164 insertions(+), 1 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c
> index 47f3be4..352ba45 100644
> --- a/crypto/asymmetric_keys/rsa.c
> +++ b/crypto/asymmetric_keys/rsa.c
> @@ -13,6 +13,7 @@
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> +#include <crypto/hash.h>
> #include "public_key.h"
> #include "private_key.h"
>
> @@ -152,6 +153,132 @@ static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> }
>
> /*
> + * EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2]
> + * @M: message to be signed, an octet string
> + * @emLen: intended length in octets of the encoded message
> + * @hash_algo: hash function (option)
> + * @hash: true means hash M, otherwise M is already a digest
> + * @EM: encoded message, an octet string of length emLen
> + *
> + * This function is a implementation of the EMSA-PKCS1-v1_5 encoding operation
> + * in RSA PKCS#1 spec. It used by the signautre generation operation of
> + * RSASSA-PKCS1-v1_5 to encode message M to encoded message EM.
> + *
> + * The variables used in this function accord PKCS#1 spec but not follow kernel
> + * naming convention, it useful when look at them with spec.
> + */
> +static int EMSA_PKCS1_v1_5_ENCODE(const u8 *M, size_t emLen,
> + enum pkey_hash_algo hash_algo, const bool hash,
> + u8 **EM, struct public_key_signature *pks)
> +{
> + u8 *digest;
> + struct crypto_shash *tfm;
> + struct shash_desc *desc;
> + size_t digest_size, desc_size;
> + size_t tLen;
> + u8 *T, *PS, *EM_tmp;
> + int i, ret;
> +
> + pr_info("EMSA_PKCS1_v1_5_ENCODE start\n");
> +
> + if (!RSA_ASN1_templates[hash_algo].data)
What about checking hash_algo against PKEY_HASH__LAST, or it relies on
the caller?
> + ret = -ENOTSUPP;
> + else
> + pks->pkey_hash_algo = hash_algo;
> +
> + /* 1) Apply the hash function to the message M to produce a hash value H */
> + tfm = crypto_alloc_shash(pkey_hash_algo[hash_algo], 0, 0);
> + if (IS_ERR(tfm))
> + return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> +
> + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> + digest_size = crypto_shash_digestsize(tfm);
> +
> + ret = -ENOMEM;
> +
> + digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
> + if (!digest)
> + goto error_digest;
> + pks->digest = digest;
> + pks->digest_size = digest_size;
> +
Ok. You allocated tfm to get hash size, right?
But why do you allocate descriptor even it might not be needed?
> + if (hash) {
> + desc = (void *) digest + digest_size;
> + desc->tfm = tfm;
> + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> +
> + ret = crypto_shash_init(desc);
> + if (ret < 0)
> + goto error_shash;
> + ret = crypto_shash_finup(desc, M, sizeof(M), pks->digest);
This is I completely fail to understand... You expect sizeof(M) to be
the message length?????
Have you ever tested it?
> + if (ret < 0)
> + goto error_shash;
> + } else {
> + memcpy(pks->digest, M, pks->digest_size);
> + pks->digest_size = digest_size;
> + }
Does caller use pks->digest and pks->digest_size after return?
I think it needs encoded value, not the hash...
So why do you pass pks?
> + crypto_free_shash(tfm);
> +
> + /* 2) Encode the algorithm ID for the hash function and the hash value into
> + * an ASN.1 value of type DigestInfo with the DER. Let T be the DER encoding of
> + * the DigestInfo value and let tLen be the length in octets of T.
> + */
> + tLen = RSA_ASN1_templates[hash_algo].size + pks->digest_size;
> + T = kmalloc(tLen, GFP_KERNEL);
> + if (!T)
> + goto error_T;
> +
Why do you need T and PS memory allocations at all?
You need only EM_tmp allocation and copy directly to the destination...
> + memcpy(T, RSA_ASN1_templates[hash_algo].data, RSA_ASN1_templates[hash_algo].size);
> + memcpy(T + RSA_ASN1_templates[hash_algo].size, pks->digest, pks->digest_size);
> +
> + /* 3) check If emLen < tLen + 11, output "intended encoded message length too short" */
> + if (emLen < tLen + 11) {
> + ret = -EINVAL;
> + goto error_emLen;
> + }
> +
> + /* 4) Generate an octet string PS consisting of emLen - tLen - 3 octets with 0xff. */
> + PS = kmalloc(emLen - tLen - 3, GFP_KERNEL);
> + if (!PS)
> + goto error_P;
> +
ditto
> + for (i = 0; i < (emLen - tLen - 3); i++)
> + PS[i] = 0xff;
> +
> + /* 5) Concatenate PS, the DER encoding T, and other padding to form the encoded
> + * message EM as EM = 0x00 || 0x01 || PS || 0x00 || T
> + */
> + EM_tmp = kmalloc(3 + emLen - tLen - 3 + tLen, GFP_KERNEL);
> + if (!EM_tmp)
> + goto error_EM;
> +
> + EM_tmp[0] = 0x00;
> + EM_tmp[1] = 0x01;
> + memcpy(EM_tmp + 2, PS, emLen - tLen - 3);
> + EM_tmp[2 + emLen - tLen - 3] = 0x00;
> + memcpy(EM_tmp + 2 + emLen - tLen - 3 + 1, T, tLen);
> +
> + *EM = EM_tmp;
> +
> + kfree(PS);
> + kfree(T);
get rid of it...
- Dmitry
> +
> + return 0;
> +
> +error_EM:
> + kfree(PS);
> +error_P:
> +error_emLen:
> + kfree(T);
> +error_T:
> +error_shash:
> + kfree(digest);
> +error_digest:
> + crypto_free_shash(tfm);
> + return ret;
> +}
> +
> +/*
> * Perform the RSA signature verification.
> * @H: Value of hash of data and metadata
> * @EM: The computed signature value
> @@ -275,9 +402,43 @@ static struct public_key_signature *RSA_generate_signature(
> const struct private_key *key, u8 *M,
> enum pkey_hash_algo hash_algo, const bool hash)
> {
> + struct public_key_signature *pks;
> + u8 *EM = NULL;
> + size_t emLen;
> + int ret;
> +
> pr_info("RSA_generate_signature start\n");
>
> - return 0;
> + ret = -ENOMEM;
> + pks = kzalloc(sizeof(*pks), GFP_KERNEL);
> + if (!pks)
> + goto error_no_pks;
> +
> + /* 1): EMSA-PKCS1-v1_5 encoding: */
> + /* Use the private key modulus size to be EM length */
> + emLen = mpi_get_nbits(key->rsa.n);
> + emLen = (emLen + 7) / 8;
> +
> + ret = EMSA_PKCS1_v1_5_ENCODE(M, emLen, hash_algo, hash, &EM, pks);
> + if (ret < 0)
> + goto error_v1_5_encode;
> +
> + /* TODO 2): m = OS2IP (EM) */
> +
> + /* TODO 3): s = RSASP1 (K, m) */
> +
> + /* TODO 4): S = I2OSP (s, k) */
> +
> + /* TODO: signature S to a u8* S or set to sig->rsa.s? */
> + pks->S = EM; /* TODO: temporary set S to EM */
> +
> + return pks;
> +
> +error_v1_5_encode:
> + kfree(pks);
> +error_no_pks:
> + pr_info("<==%s() = %d\n", __func__, ret);
> + return ERR_PTR(ret);
> }
>
> const struct public_key_algorithm RSA_public_key_algorithm = {
> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> index d44b29f..1cdf457 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -110,6 +110,8 @@ extern void public_key_destroy(void *payload);
> struct public_key_signature {
> u8 *digest;
> u8 digest_size; /* Number of bytes in digest */
> + u8 *S; /* signature S of length k octets */
> + size_t k; /* length k of signature S */
> u8 nr_mpi; /* Occupancy of mpi[] */
> enum pkey_hash_algo pkey_hash_algo : 8;
> union {
> --
> 1.6.0.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
^ permalink raw reply
* Re: [PATCH] modules: add support for soft module dependencies
From: Lucas De Marchi @ 2013-09-17 22:27 UTC (permalink / raw)
To: Rusty Russell
Cc: Herbert Xu, Andreas Robinson, Linux Kernel Mailing List,
Zhao Hongjiang, David Miller, tim.c.chen, Andrew Morton,
gregkh@linuxfoundation.org, linux-crypto, linux-modules,
Tom Gundersen
In-Reply-To: <87ob7x37p6.fsf@rustcorp.com.au>
On Thu, Sep 12, 2013 at 9:07 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>> On Wed, Jul 24, 2013 at 11:03 PM, Herbert Xu
>> <herbert@gondor.apana.org.au> wrote:
>>> On Thu, Jul 25, 2013 at 09:32:02AM +0930, Rusty Russell wrote:
>>>> Herbert Xu <herbert@gondor.apana.org.au> writes:
>>>> > Hi Rusty:
>>>> >
>>>> > I don't know why this patch never went into the kernel, even
>>>> > though the corresponding features have been added to modprobe
>>>> > in most if not all distros.
>>>>
>>>> Because Andreas never sent me the patch? This is the first I've *heard*
>>>> of this feature. Looks like it didn't hit lkml either. And what was
>>>> 2/2?
>>>
>>> 2/2 was the patch to actually use this in crc32c.
>>>
>>>> It's not how I would have done this: post-deps are more flexibly done at
>>>> runtime, because the module may have to do work to figure out what to
>>>> pull in. But since it already exists, I'll apply this patch: it doesn't
>>>> cost the kernel anything.
>>
>> But it did cause boot failures. The file modules.softdep file was
>> supposed to be informational until now. That's why depmod put a
>> comment saying to "copy on user's discretion to /etc/modules.d"
>> instead of parsing it directly.
>
> I'm happy to change this macro to create a modinfo line like
> "softdep:<modname>"
how is that solving the issue that this macro can be used to designate
a mandatory or optional dependency
(https://lkml.org/lkml/2013/9/10/371)? If we decide the dependency is
mandatory we can very well let modprobe use that dependency during
module load
>
> ie. tools like mkinitrd could pick it up and try to find a matching
> module, but depmod would ignore it.
Some mkinitrd-like use whatever depmod/modprobe tells them it's
needed. So kmod still needs to know about it.
>
> It's really up to Lucas, since this affects him.
IMO saying "this is an optional dependency and we can work without"
doesn't buy us much. Distros will end up putting the soft dep in
/etc/modules.d, kmod will always use them anyway and failing to load
the soft dep will fail the module load. I'd like to have no distro
files in /etc/modules.d in future.
Lucas De Marchi
^ permalink raw reply
* Re: [PATCH V4 02/15] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
From: Dmitry Kasatkin @ 2013-09-17 22:29 UTC (permalink / raw)
To: Lee, Chun-Yi
Cc: linux-kernel@vger.kernel.org, linux-security-module, linux-efi,
linux-pm, linux-crypto, opensuse-kernel, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Pavel Machek,
Josh Boyer, Vojtech Pavlik, Matt Fleming, James Bottomley,
Greg KH, JKosina, Rusty Russell, Herbert Xu, David S. Miller,
H. Peter Anvin, Michal Marek, Gary Lin, Vivek Goyal, Lee, Chun-Yi
In-Reply-To: <1379206621-18639-3-git-send-email-jlee@suse.com>
On Sat, Sep 14, 2013 at 7:56 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> first step of signature generation operation (RSASSA-PKCS1-v1_5-SIGN).
>
> This patch is temporary set emLen to pks->k, and temporary set EM to
> pks->S for debugging. We will replace the above values to real signature
> after implement RSASP1.
>
> The naming of EMSA_PKCS1_v1_5_ENCODE and the variables used in this function
> accord PKCS#1 spec but not follow kernel naming convention, it useful when look
> at them with spec.
>
> Reference: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1v2/pkcs1ietffinal.txt
> Reference: http://www.emc.com/collateral/white-papers/h11300-pkcs-1v2-2-rsa-cryptography-standard-wp.pdf
>
> V2:
> - Clean up naming of variable: replace _EM by EM, replace EM by EM_tmp.
> - Add comment to EMSA_PKCS1-v1_5-ENCODE function.
>
> Cc: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Jiri Kosina <jkosina@suse.cz>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
> crypto/asymmetric_keys/rsa.c | 163 +++++++++++++++++++++++++++++++++++++++++-
> include/crypto/public_key.h | 2 +
> 2 files changed, 164 insertions(+), 1 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c
> index 47f3be4..352ba45 100644
> --- a/crypto/asymmetric_keys/rsa.c
> +++ b/crypto/asymmetric_keys/rsa.c
> @@ -13,6 +13,7 @@
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> +#include <crypto/hash.h>
> #include "public_key.h"
> #include "private_key.h"
>
> @@ -152,6 +153,132 @@ static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> }
>
> /*
> + * EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2]
> + * @M: message to be signed, an octet string
> + * @emLen: intended length in octets of the encoded message
> + * @hash_algo: hash function (option)
> + * @hash: true means hash M, otherwise M is already a digest
> + * @EM: encoded message, an octet string of length emLen
> + *
> + * This function is a implementation of the EMSA-PKCS1-v1_5 encoding operation
> + * in RSA PKCS#1 spec. It used by the signautre generation operation of
> + * RSASSA-PKCS1-v1_5 to encode message M to encoded message EM.
> + *
> + * The variables used in this function accord PKCS#1 spec but not follow kernel
> + * naming convention, it useful when look at them with spec.
> + */
> +static int EMSA_PKCS1_v1_5_ENCODE(const u8 *M, size_t emLen,
> + enum pkey_hash_algo hash_algo, const bool hash,
> + u8 **EM, struct public_key_signature *pks)
> +{
> + u8 *digest;
> + struct crypto_shash *tfm;
> + struct shash_desc *desc;
> + size_t digest_size, desc_size;
> + size_t tLen;
> + u8 *T, *PS, *EM_tmp;
> + int i, ret;
> +
> + pr_info("EMSA_PKCS1_v1_5_ENCODE start\n");
> +
> + if (!RSA_ASN1_templates[hash_algo].data)
> + ret = -ENOTSUPP;
> + else
> + pks->pkey_hash_algo = hash_algo;
> +
> + /* 1) Apply the hash function to the message M to produce a hash value H */
> + tfm = crypto_alloc_shash(pkey_hash_algo[hash_algo], 0, 0);
> + if (IS_ERR(tfm))
> + return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> +
> + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> + digest_size = crypto_shash_digestsize(tfm);
> +
> + ret = -ENOMEM;
> +
> + digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
> + if (!digest)
> + goto error_digest;
> + pks->digest = digest;
> + pks->digest_size = digest_size;
> +
> + if (hash) {
> + desc = (void *) digest + digest_size;
> + desc->tfm = tfm;
> + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> +
> + ret = crypto_shash_init(desc);
> + if (ret < 0)
> + goto error_shash;
> + ret = crypto_shash_finup(desc, M, sizeof(M), pks->digest);
> + if (ret < 0)
> + goto error_shash;
> + } else {
> + memcpy(pks->digest, M, pks->digest_size);
> + pks->digest_size = digest_size;
> + }
> + crypto_free_shash(tfm);
> +
> + /* 2) Encode the algorithm ID for the hash function and the hash value into
> + * an ASN.1 value of type DigestInfo with the DER. Let T be the DER encoding of
> + * the DigestInfo value and let tLen be the length in octets of T.
> + */
> + tLen = RSA_ASN1_templates[hash_algo].size + pks->digest_size;
> + T = kmalloc(tLen, GFP_KERNEL);
> + if (!T)
> + goto error_T;
> +
as I said, remove it... see bellow....
> + memcpy(T, RSA_ASN1_templates[hash_algo].data, RSA_ASN1_templates[hash_algo].size);
> + memcpy(T + RSA_ASN1_templates[hash_algo].size, pks->digest, pks->digest_size);
> +
> + /* 3) check If emLen < tLen + 11, output "intended encoded message length too short" */
> + if (emLen < tLen + 11) {
> + ret = -EINVAL;
> + goto error_emLen;
> + }
> +
> + /* 4) Generate an octet string PS consisting of emLen - tLen - 3 octets with 0xff. */
> + PS = kmalloc(emLen - tLen - 3, GFP_KERNEL);
> + if (!PS)
> + goto error_P;
> +
as I said remove it...
see bellow..
> + for (i = 0; i < (emLen - tLen - 3); i++)
> + PS[i] = 0xff;
> +
memset() does not work here?
> + /* 5) Concatenate PS, the DER encoding T, and other padding to form the encoded
> + * message EM as EM = 0x00 || 0x01 || PS || 0x00 || T
> + */
> + EM_tmp = kmalloc(3 + emLen - tLen - 3 + tLen, GFP_KERNEL);
> + if (!EM_tmp)
> + goto error_EM;
> +
> + EM_tmp[0] = 0x00;
> + EM_tmp[1] = 0x01;
> + memcpy(EM_tmp + 2, PS, emLen - tLen - 3);
> + EM_tmp[2 + emLen - tLen - 3] = 0x00;
above 2 lines can be replaced by:
PS = &EM_tmp[2];
PS_len = emLen - tLen - 3
memset(PS, 0xff, PS_len);
EM_tmp[2 + PS_len] = 0x00;
> + memcpy(EM_tmp + 2 + emLen - tLen - 3 + 1, T, tLen);
> +
This can be replaced by:
T = &EM_tmp[2+ PS_Len + 1];
memcpy(T, RSA_ASN1_templates[hash_algo].data,
RSA_ASN1_templates[hash_algo].size);
memcpy(T + RSA_ASN1_templates[hash_algo].size, pks->digest,
pks->digest_size);
> + *EM = EM_tmp;
> +
> + kfree(PS);
> + kfree(T);
> +
Right? So please remove unneeded allocations...
Dmitry
> + return 0;
> +
> +error_EM:
> + kfree(PS);
> +error_P:
> +error_emLen:
> + kfree(T);
> +error_T:
> +error_shash:
> + kfree(digest);
> +error_digest:
> + crypto_free_shash(tfm);
> + return ret;
> +}
> +
> +/*
> * Perform the RSA signature verification.
> * @H: Value of hash of data and metadata
> * @EM: The computed signature value
> @@ -275,9 +402,43 @@ static struct public_key_signature *RSA_generate_signature(
> const struct private_key *key, u8 *M,
> enum pkey_hash_algo hash_algo, const bool hash)
> {
> + struct public_key_signature *pks;
> + u8 *EM = NULL;
> + size_t emLen;
> + int ret;
> +
> pr_info("RSA_generate_signature start\n");
>
> - return 0;
> + ret = -ENOMEM;
> + pks = kzalloc(sizeof(*pks), GFP_KERNEL);
> + if (!pks)
> + goto error_no_pks;
> +
> + /* 1): EMSA-PKCS1-v1_5 encoding: */
> + /* Use the private key modulus size to be EM length */
> + emLen = mpi_get_nbits(key->rsa.n);
> + emLen = (emLen + 7) / 8;
> +
> + ret = EMSA_PKCS1_v1_5_ENCODE(M, emLen, hash_algo, hash, &EM, pks);
> + if (ret < 0)
> + goto error_v1_5_encode;
> +
> + /* TODO 2): m = OS2IP (EM) */
> +
> + /* TODO 3): s = RSASP1 (K, m) */
> +
> + /* TODO 4): S = I2OSP (s, k) */
> +
> + /* TODO: signature S to a u8* S or set to sig->rsa.s? */
> + pks->S = EM; /* TODO: temporary set S to EM */
> +
> + return pks;
> +
> +error_v1_5_encode:
> + kfree(pks);
> +error_no_pks:
> + pr_info("<==%s() = %d\n", __func__, ret);
> + return ERR_PTR(ret);
> }
>
> const struct public_key_algorithm RSA_public_key_algorithm = {
> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> index d44b29f..1cdf457 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -110,6 +110,8 @@ extern void public_key_destroy(void *payload);
> struct public_key_signature {
> u8 *digest;
> u8 digest_size; /* Number of bytes in digest */
> + u8 *S; /* signature S of length k octets */
> + size_t k; /* length k of signature S */
> u8 nr_mpi; /* Occupancy of mpi[] */
> enum pkey_hash_algo pkey_hash_algo : 8;
> union {
> --
> 1.6.0.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Dmitry
^ permalink raw reply
* Re: [PATCH] modules: add support for soft module dependencies
From: Rusty Russell @ 2013-09-18 2:10 UTC (permalink / raw)
To: Lucas De Marchi
Cc: Herbert Xu, Andreas Robinson, Linux Kernel Mailing List,
Zhao Hongjiang, David Miller, tim.c.chen, Andrew Morton,
gregkh@linuxfoundation.org, linux-crypto, linux-modules,
Tom Gundersen
In-Reply-To: <CAKi4VALAu0ek6tYhb_Ls34hNU1Omjhv=QJDaACAp_0gpRy98HA@mail.gmail.com>
Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
> On Thu, Sep 12, 2013 at 9:07 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>>> On Wed, Jul 24, 2013 at 11:03 PM, Herbert Xu
>>> <herbert@gondor.apana.org.au> wrote:
>>>> On Thu, Jul 25, 2013 at 09:32:02AM +0930, Rusty Russell wrote:
>>>>> Herbert Xu <herbert@gondor.apana.org.au> writes:
>>>>> > Hi Rusty:
>>>>> >
>>>>> > I don't know why this patch never went into the kernel, even
>>>>> > though the corresponding features have been added to modprobe
>>>>> > in most if not all distros.
>>>>>
>>>>> Because Andreas never sent me the patch? This is the first I've *heard*
>>>>> of this feature. Looks like it didn't hit lkml either. And what was
>>>>> 2/2?
>>>>
>>>> 2/2 was the patch to actually use this in crc32c.
>>>>
>>>>> It's not how I would have done this: post-deps are more flexibly done at
>>>>> runtime, because the module may have to do work to figure out what to
>>>>> pull in. But since it already exists, I'll apply this patch: it doesn't
>>>>> cost the kernel anything.
>>>
>>> But it did cause boot failures. The file modules.softdep file was
>>> supposed to be informational until now. That's why depmod put a
>>> comment saying to "copy on user's discretion to /etc/modules.d"
>>> instead of parsing it directly.
>>
>> I'm happy to change this macro to create a modinfo line like
>> "softdep:<modname>"
>
> how is that solving the issue that this macro can be used to designate
> a mandatory or optional dependency
> (https://lkml.org/lkml/2013/9/10/371)? If we decide the dependency is
> mandatory we can very well let modprobe use that dependency during
> module load
I'm very close to sending Linus a revert commit at this point, since
there's no consensus on what it's for.
*Clearly* softdep shouldn't indicate a mandatory dependency. We already
have a way (several) to make mandatory dependencies!
And the "pre:" vs "post:" thing is just weird. If a module wants a post
dependency, you can request_module() it from a workqueue.
>> ie. tools like mkinitrd could pick it up and try to find a matching
>> module, but depmod would ignore it.
>
> Some mkinitrd-like use whatever depmod/modprobe tells them it's
> needed. So kmod still needs to know about it.
It sounds like we should create a separate tool, which takes a list of
modules and spits out the full pathname of all dependencies. *That*
tool should include soft dependencies.
>> It's really up to Lucas, since this affects him.
>
> IMO saying "this is an optional dependency and we can work without"
> doesn't buy us much. Distros will end up putting the soft dep in
> /etc/modules.d, kmod will always use them anyway and failing to load
> the soft dep will fail the module load. I'd like to have no distro
> files in /etc/modules.d in future.
I assumed modprobe would handle soft dependencies in modules and try to
pull them in, but *not* fail if they don't work.
The previous way of doing this was:
install foo modprobe foo_softdep 2>/dev/null; modprobe --ignore-install foo $CMDLINE_OPTS
I agree this logic belongs in the kernel, we just have to figure out
exactly how.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH] modules: add support for soft module dependencies
From: Lucas De Marchi @ 2013-09-18 5:32 UTC (permalink / raw)
To: Rusty Russell
Cc: Lucas De Marchi, Herbert Xu, Andreas Robinson,
Linux Kernel Mailing List, Zhao Hongjiang, David Miller,
tim.c.chen, Andrew Morton, gregkh@linuxfoundation.org,
linux-crypto, linux-modules, Tom Gundersen
In-Reply-To: <878uyuzxp0.fsf@rustcorp.com.au>
On Tue, Sep 17, 2013 at 11:10 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>> On Thu, Sep 12, 2013 at 9:07 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>>>> On Wed, Jul 24, 2013 at 11:03 PM, Herbert Xu
>>>> <herbert@gondor.apana.org.au> wrote:
>>>>> On Thu, Jul 25, 2013 at 09:32:02AM +0930, Rusty Russell wrote:
>>>>>> Herbert Xu <herbert@gondor.apana.org.au> writes:
>>>>>> > Hi Rusty:
>>>>>> >
>>>>>> > I don't know why this patch never went into the kernel, even
>>>>>> > though the corresponding features have been added to modprobe
>>>>>> > in most if not all distros.
>>>>>>
>>>>>> Because Andreas never sent me the patch? This is the first I've *heard*
>>>>>> of this feature. Looks like it didn't hit lkml either. And what was
>>>>>> 2/2?
>>>>>
>>>>> 2/2 was the patch to actually use this in crc32c.
>>>>>
>>>>>> It's not how I would have done this: post-deps are more flexibly done at
>>>>>> runtime, because the module may have to do work to figure out what to
>>>>>> pull in. But since it already exists, I'll apply this patch: it doesn't
>>>>>> cost the kernel anything.
>>>>
>>>> But it did cause boot failures. The file modules.softdep file was
>>>> supposed to be informational until now. That's why depmod put a
>>>> comment saying to "copy on user's discretion to /etc/modules.d"
>>>> instead of parsing it directly.
>>>
>>> I'm happy to change this macro to create a modinfo line like
>>> "softdep:<modname>"
>>
>> how is that solving the issue that this macro can be used to designate
>> a mandatory or optional dependency
>> (https://lkml.org/lkml/2013/9/10/371)? If we decide the dependency is
>> mandatory we can very well let modprobe use that dependency during
>> module load
>
> I'm very close to sending Linus a revert commit at this point, since
> there's no consensus on what it's for.
>
> *Clearly* softdep shouldn't indicate a mandatory dependency. We already
> have a way (several) to make mandatory dependencies!
>
> And the "pre:" vs "post:" thing is just weird. If a module wants a post
> dependency, you can request_module() it from a workqueue.
>
>>> ie. tools like mkinitrd could pick it up and try to find a matching
>>> module, but depmod would ignore it.
>>
>> Some mkinitrd-like use whatever depmod/modprobe tells them it's
>> needed. So kmod still needs to know about it.
>
> It sounds like we should create a separate tool, which takes a list of
> modules and spits out the full pathname of all dependencies. *That*
> tool should include soft dependencies.
>
>>> It's really up to Lucas, since this affects him.
>>
>> IMO saying "this is an optional dependency and we can work without"
>> doesn't buy us much. Distros will end up putting the soft dep in
>> /etc/modules.d, kmod will always use them anyway and failing to load
>> the soft dep will fail the module load. I'd like to have no distro
>> files in /etc/modules.d in future.
>
> I assumed modprobe would handle soft dependencies in modules and try to
> pull them in, but *not* fail if they don't work.
Iff the module doesn't *exist*. If it failed to load or failed for any
other reason then we will abort trying to load the other module.
However this is one thing we can change in modprobe to make it
consistent and more predictable. But we really need to reach a
consensus.
>
> The previous way of doing this was:
> install foo modprobe foo_softdep 2>/dev/null; modprobe --ignore-install foo $CMDLINE_OPTS
I just hope this is in no way an incentive for people using install commands ;-)
Lucas De Marchi
^ permalink raw reply
* Re: [PATCH] modules: add support for soft module dependencies
From: Rusty Russell @ 2013-09-18 7:17 UTC (permalink / raw)
To: Lucas De Marchi
Cc: Lucas De Marchi, Herbert Xu, Andreas Robinson,
Linux Kernel Mailing List, Zhao Hongjiang, David Miller,
tim.c.chen-VuQAYsv1563Yd54FQh9/CA, Andrew Morton,
gregkh@linuxfoundation.org, linux-crypto-u79uwXL29TY76Z2rM5mHXA,
linux-modules, Tom Gundersen
In-Reply-To: <CAMOw1v7RhTfWu0K64ndxXdET9JQjg7Pr3DWKGMSM9gN9_pviJQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Lucas De Marchi <lucas.demarchi-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org> writes:
> On Tue, Sep 17, 2013 at 11:10 PM, Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> wrote:
>> Lucas De Marchi <lucas.de.marchi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>> On Thu, Sep 12, 2013 at 9:07 PM, Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> wrote:
>>>> I'm happy to change this macro to create a modinfo line like
>>>> "softdep:<modname>"
>>>
>>> how is that solving the issue that this macro can be used to designate
>>> a mandatory or optional dependency
>>> (https://lkml.org/lkml/2013/9/10/371)? If we decide the dependency is
>>> mandatory we can very well let modprobe use that dependency during
>>> module load
>>
>> I'm very close to sending Linus a revert commit at this point, since
>> there's no consensus on what it's for.
>>
>> *Clearly* softdep shouldn't indicate a mandatory dependency. We already
>> have a way (several) to make mandatory dependencies!
>>
>> And the "pre:" vs "post:" thing is just weird. If a module wants a post
>> dependency, you can request_module() it from a workqueue.
>>
>>>> ie. tools like mkinitrd could pick it up and try to find a matching
>>>> module, but depmod would ignore it.
>>>
>>> Some mkinitrd-like use whatever depmod/modprobe tells them it's
>>> needed. So kmod still needs to know about it.
>>
>> It sounds like we should create a separate tool, which takes a list of
>> modules and spits out the full pathname of all dependencies. *That*
>> tool should include soft dependencies.
>>
>>>> It's really up to Lucas, since this affects him.
>>>
>>> IMO saying "this is an optional dependency and we can work without"
>>> doesn't buy us much. Distros will end up putting the soft dep in
>>> /etc/modules.d, kmod will always use them anyway and failing to load
>>> the soft dep will fail the module load. I'd like to have no distro
>>> files in /etc/modules.d in future.
>>
>> I assumed modprobe would handle soft dependencies in modules and try to
>> pull them in, but *not* fail if they don't work.
>
> Iff the module doesn't *exist*. If it failed to load or failed for any
> other reason then we will abort trying to load the other module.
> However this is one thing we can change in modprobe to make it
> consistent and more predictable. But we really need to reach a
> consensus.
It is a bit of a corner case, but there are other conceivable reasons
why a module wouldn't load. The hardware it wants might not be present,
or busy, or it conflicts with an existing module, or needs user
configuration (eg. commandline params).
It might be worth reporting, but I wouldn't stop loading on softdep fails.
>> The previous way of doing this was:
>> install foo modprobe foo_softdep 2>/dev/null; modprobe --ignore-install foo $CMDLINE_OPTS
>
> I just hope this is in no way an incentive for people using install commands ;-)
Agreed. install commands were implemented because I had no idea what
people wanted to do in future. They definitely provide enough rope...
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH V4 02/15] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
From: joeyli @ 2013-09-18 9:08 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: linux-kernel@vger.kernel.org, linux-security-module, linux-efi,
linux-pm, linux-crypto, opensuse-kernel, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Pavel Machek,
Josh Boyer, Vojtech Pavlik, Matt Fleming, James Bottomley,
Greg KH, JKosina, Rusty Russell, Herbert Xu, David S. Miller,
H. Peter Anvin, Michal Marek, Gary Lin, Vivek Goyal
In-Reply-To: <CACE9dm-7HKz4VFR1bNTTFd-YpYhnkNVwiW81iXSJZbqjUTBR_Q@mail.gmail.com>
Hi Dmitry,
First, thanks for your time to review my patches!
於 二，2013-09-17 於 16:51 -0500，Dmitry Kasatkin 提到：
> Hello,
>
>
> On Sat, Sep 14, 2013 at 7:56 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> > first step of signature generation operation (RSASSA-PKCS1-v1_5-SIGN).
> >
> > This patch is temporary set emLen to pks->k, and temporary set EM to
> > pks->S for debugging. We will replace the above values to real signature
> > after implement RSASP1.
> >
> > The naming of EMSA_PKCS1_v1_5_ENCODE and the variables used in this function
> > accord PKCS#1 spec but not follow kernel naming convention, it useful when look
> > at them with spec.
> >
> > Reference: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1v2/pkcs1ietffinal.txt
> > Reference: http://www.emc.com/collateral/white-papers/h11300-pkcs-1v2-2-rsa-cryptography-standard-wp.pdf
> >
> > V2:
> > - Clean up naming of variable: replace _EM by EM, replace EM by EM_tmp.
> > - Add comment to EMSA_PKCS1-v1_5-ENCODE function.
> >
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Reviewed-by: Jiri Kosina <jkosina@suse.cz>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> > crypto/asymmetric_keys/rsa.c | 163 +++++++++++++++++++++++++++++++++++++++++-
> > include/crypto/public_key.h | 2 +
> > 2 files changed, 164 insertions(+), 1 deletions(-)
> >
> > diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c
> > index 47f3be4..352ba45 100644
> > --- a/crypto/asymmetric_keys/rsa.c
> > +++ b/crypto/asymmetric_keys/rsa.c
> > @@ -13,6 +13,7 @@
> > #include <linux/module.h>
> > #include <linux/kernel.h>
> > #include <linux/slab.h>
> > +#include <crypto/hash.h>
> > #include "public_key.h"
> > #include "private_key.h"
> >
> > @@ -152,6 +153,132 @@ static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> > }
> >
> > /*
> > + * EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2]
> > + * @M: message to be signed, an octet string
> > + * @emLen: intended length in octets of the encoded message
> > + * @hash_algo: hash function (option)
> > + * @hash: true means hash M, otherwise M is already a digest
> > + * @EM: encoded message, an octet string of length emLen
> > + *
> > + * This function is a implementation of the EMSA-PKCS1-v1_5 encoding operation
> > + * in RSA PKCS#1 spec. It used by the signautre generation operation of
> > + * RSASSA-PKCS1-v1_5 to encode message M to encoded message EM.
> > + *
> > + * The variables used in this function accord PKCS#1 spec but not follow kernel
> > + * naming convention, it useful when look at them with spec.
> > + */
> > +static int EMSA_PKCS1_v1_5_ENCODE(const u8 *M, size_t emLen,
> > + enum pkey_hash_algo hash_algo, const bool hash,
> > + u8 **EM, struct public_key_signature *pks)
> > +{
> > + u8 *digest;
> > + struct crypto_shash *tfm;
> > + struct shash_desc *desc;
> > + size_t digest_size, desc_size;
> > + size_t tLen;
> > + u8 *T, *PS, *EM_tmp;
> > + int i, ret;
> > +
> > + pr_info("EMSA_PKCS1_v1_5_ENCODE start\n");
> > +
> > + if (!RSA_ASN1_templates[hash_algo].data)
>
> What about checking hash_algo against PKEY_HASH__LAST, or it relies on
> the caller?
>
Yes, check PKEY_HASH__LAST is more easy and clear, I will change it.
Thanks!
>
> > + ret = -ENOTSUPP;
> > + else
> > + pks->pkey_hash_algo = hash_algo;
> > +
> > + /* 1) Apply the hash function to the message M to produce a hash value H */
> > + tfm = crypto_alloc_shash(pkey_hash_algo[hash_algo], 0, 0);
> > + if (IS_ERR(tfm))
> > + return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> > +
> > + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > + digest_size = crypto_shash_digestsize(tfm);
> > +
> > + ret = -ENOMEM;
> > +
> > + digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
> > + if (!digest)
> > + goto error_digest;
> > + pks->digest = digest;
> > + pks->digest_size = digest_size;
> > +
>
> Ok. You allocated tfm to get hash size, right?
>
> But why do you allocate descriptor even it might not be needed?
>
You are right, I should skip the code of allocate descriptor when the
hash is supported. I will modified it.
Thanks!
> > + if (hash) {
> > + desc = (void *) digest + digest_size;
> > + desc->tfm = tfm;
> > + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> > +
> > + ret = crypto_shash_init(desc);
> > + if (ret < 0)
> > + goto error_shash;
> > + ret = crypto_shash_finup(desc, M, sizeof(M), pks->digest);
>
> This is I completely fail to understand... You expect sizeof(M) to be
> the message length?????
> Have you ever tested it?
>
Sigh!
I just checked my test program for this code path, this stupid problem
causes by my test program doesn't feed the right hash result that should
used to verify signature. So, I didn't capture this bug.
And, the hibernate signature check mechanism doesn't run into this code
path because the hash generation is done by hibernate code but not in
here. So, I also didn't find this problem when running hibernate check.
Appreciate for your point out! I just fix it in next patch version.
> > + if (ret < 0)
> > + goto error_shash;
> > + } else {
> > + memcpy(pks->digest, M, pks->digest_size);
> > + pks->digest_size = digest_size;
> > + }
>
> Does caller use pks->digest and pks->digest_size after return?
> I think it needs encoded value, not the hash...
> So why do you pass pks?
>
I put the signature MPI to pks->rsa.s, and put the encoded signature to
pks->S. So caller can grab encoded signature.
I also pass the pks->digest and pks->digest_size to caller for
reference. Then caller can simply feed this pks to
RSA_verify_signature() for verify the signature result, don't need
generate hash again.
>
>
> > + crypto_free_shash(tfm);
> > +
> > + /* 2) Encode the algorithm ID for the hash function and the hash value into
> > + * an ASN.1 value of type DigestInfo with the DER. Let T be the DER encoding of
> > + * the DigestInfo value and let tLen be the length in octets of T.
> > + */
> > + tLen = RSA_ASN1_templates[hash_algo].size + pks->digest_size;
> > + T = kmalloc(tLen, GFP_KERNEL);
> > + if (!T)
> > + goto error_T;
> > +
>
> Why do you need T and PS memory allocations at all?
> You need only EM_tmp allocation and copy directly to the destination...
>
OK, I will change the code to allocate T and PS size in EM_tmp.
>
> > + memcpy(T, RSA_ASN1_templates[hash_algo].data, RSA_ASN1_templates[hash_algo].size);
> > + memcpy(T + RSA_ASN1_templates[hash_algo].size, pks->digest, pks->digest_size);
> > +
> > + /* 3) check If emLen < tLen + 11, output "intended encoded message length too short" */
> > + if (emLen < tLen + 11) {
> > + ret = -EINVAL;
> > + goto error_emLen;
> > + }
> > +
> > + /* 4) Generate an octet string PS consisting of emLen - tLen - 3 octets with 0xff. */
> > + PS = kmalloc(emLen - tLen - 3, GFP_KERNEL);
> > + if (!PS)
> > + goto error_P;
> > +
>
> ditto
OK, I will allocate PS with EM_tmp.
Thanks!
>
> > + for (i = 0; i < (emLen - tLen - 3); i++)
> > + PS[i] = 0xff;
> > +
> > + /* 5) Concatenate PS, the DER encoding T, and other padding to form the encoded
> > + * message EM as EM = 0x00 || 0x01 || PS || 0x00 || T
> > + */
> > + EM_tmp = kmalloc(3 + emLen - tLen - 3 + tLen, GFP_KERNEL);
> > + if (!EM_tmp)
> > + goto error_EM;
> > +
> > + EM_tmp[0] = 0x00;
> > + EM_tmp[1] = 0x01;
> > + memcpy(EM_tmp + 2, PS, emLen - tLen - 3);
> > + EM_tmp[2 + emLen - tLen - 3] = 0x00;
> > + memcpy(EM_tmp + 2 + emLen - tLen - 3 + 1, T, tLen);
> > +
> > + *EM = EM_tmp;
> > +
> > + kfree(PS);
> > + kfree(T);
>
> get rid of it...
>
OK!
>
> - Dmitry
>
> > +
> > + return 0;
> > +
> > +error_EM:
> > + kfree(PS);
> > +error_P:
> > +error_emLen:
> > + kfree(T);
> > +error_T:
> > +error_shash:
> > + kfree(digest);
> > +error_digest:
> > + crypto_free_shash(tfm);
> > + return ret;
> > +}
> > +
> > +/*
> > * Perform the RSA signature verification.
> > * @H: Value of hash of data and metadata
> > * @EM: The computed signature value
> > @@ -275,9 +402,43 @@ static struct public_key_signature *RSA_generate_signature(
> > const struct private_key *key, u8 *M,
> > enum pkey_hash_algo hash_algo, const bool hash)
> > {
> > + struct public_key_signature *pks;
> > + u8 *EM = NULL;
> > + size_t emLen;
> > + int ret;
> > +
> > pr_info("RSA_generate_signature start\n");
> >
> > - return 0;
> > + ret = -ENOMEM;
> > + pks = kzalloc(sizeof(*pks), GFP_KERNEL);
> > + if (!pks)
> > + goto error_no_pks;
> > +
> > + /* 1): EMSA-PKCS1-v1_5 encoding: */
> > + /* Use the private key modulus size to be EM length */
> > + emLen = mpi_get_nbits(key->rsa.n);
> > + emLen = (emLen + 7) / 8;
> > +
> > + ret = EMSA_PKCS1_v1_5_ENCODE(M, emLen, hash_algo, hash, &EM, pks);
> > + if (ret < 0)
> > + goto error_v1_5_encode;
> > +
> > + /* TODO 2): m = OS2IP (EM) */
> > +
> > + /* TODO 3): s = RSASP1 (K, m) */
> > +
> > + /* TODO 4): S = I2OSP (s, k) */
> > +
> > + /* TODO: signature S to a u8* S or set to sig->rsa.s? */
> > + pks->S = EM; /* TODO: temporary set S to EM */
> > +
> > + return pks;
> > +
> > +error_v1_5_encode:
> > + kfree(pks);
> > +error_no_pks:
> > + pr_info("<==%s() = %d\n", __func__, ret);
> > + return ERR_PTR(ret);
> > }
> >
> > const struct public_key_algorithm RSA_public_key_algorithm = {
> > diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> > index d44b29f..1cdf457 100644
> > --- a/include/crypto/public_key.h
> > +++ b/include/crypto/public_key.h
> > @@ -110,6 +110,8 @@ extern void public_key_destroy(void *payload);
> > struct public_key_signature {
> > u8 *digest;
> > u8 digest_size; /* Number of bytes in digest */
> > + u8 *S; /* signature S of length k octets */
> > + size_t k; /* length k of signature S */
> > u8 nr_mpi; /* Occupancy of mpi[] */
> > enum pkey_hash_algo pkey_hash_algo : 8;
> > union {
> > --
> > 1.6.0.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH V4 02/15] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
From: joeyli @ 2013-09-18 9:08 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
opensuse-kernel-stAJ6ESoqRxg9hUCZPvPmw, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Pavel Machek,
Josh Boyer, Vojtech Pavlik, Matt Fleming, James Bottomley,
Greg KH, JKosina-IBi9RG/b67k, Rusty Russell, Herbert Xu,
David S. Miller, H. Peter Anvin, Michal Marek, Gary Lin,
Vivek Goyal
In-Reply-To: <CACE9dm-7HKz4VFR1bNTTFd-YpYhnkNVwiW81iXSJZbqjUTBR_Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Dmitry,
First, thanks for your time to review my patches!
於 二，2013-09-17 於 16:51 -0500，Dmitry Kasatkin 提到：
> Hello,
>
>
> On Sat, Sep 14, 2013 at 7:56 PM, Lee, Chun-Yi <joeyli.kernel-Re5JQEeQqe8@public.gmane.orgm> wrote:
> > Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> > first step of signature generation operation (RSASSA-PKCS1-v1_5-SIGN).
> >
> > This patch is temporary set emLen to pks->k, and temporary set EM to
> > pks->S for debugging. We will replace the above values to real signature
> > after implement RSASP1.
> >
> > The naming of EMSA_PKCS1_v1_5_ENCODE and the variables used in this function
> > accord PKCS#1 spec but not follow kernel naming convention, it useful when look
> > at them with spec.
> >
> > Reference: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1v2/pkcs1ietffinal.txt
> > Reference: http://www.emc.com/collateral/white-papers/h11300-pkcs-1v2-2-rsa-cryptography-standard-wp.pdf
> >
> > V2:
> > - Clean up naming of variable: replace _EM by EM, replace EM by EM_tmp.
> > - Add comment to EMSA_PKCS1-v1_5-ENCODE function.
> >
> > Cc: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> > Reviewed-by: Jiri Kosina <jkosina-AlSwsSmVLrQ@public.gmane.org>
> > Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> > ---
> > crypto/asymmetric_keys/rsa.c | 163 +++++++++++++++++++++++++++++++++++++++++-
> > include/crypto/public_key.h | 2 +
> > 2 files changed, 164 insertions(+), 1 deletions(-)
> >
> > diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c
> > index 47f3be4..352ba45 100644
> > --- a/crypto/asymmetric_keys/rsa.c
> > +++ b/crypto/asymmetric_keys/rsa.c
> > @@ -13,6 +13,7 @@
> > #include <linux/module.h>
> > #include <linux/kernel.h>
> > #include <linux/slab.h>
> > +#include <crypto/hash.h>
> > #include "public_key.h"
> > #include "private_key.h"
> >
> > @@ -152,6 +153,132 @@ static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> > }
> >
> > /*
> > + * EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2]
> > + * @M: message to be signed, an octet string
> > + * @emLen: intended length in octets of the encoded message
> > + * @hash_algo: hash function (option)
> > + * @hash: true means hash M, otherwise M is already a digest
> > + * @EM: encoded message, an octet string of length emLen
> > + *
> > + * This function is a implementation of the EMSA-PKCS1-v1_5 encoding operation
> > + * in RSA PKCS#1 spec. It used by the signautre generation operation of
> > + * RSASSA-PKCS1-v1_5 to encode message M to encoded message EM.
> > + *
> > + * The variables used in this function accord PKCS#1 spec but not follow kernel
> > + * naming convention, it useful when look at them with spec.
> > + */
> > +static int EMSA_PKCS1_v1_5_ENCODE(const u8 *M, size_t emLen,
> > + enum pkey_hash_algo hash_algo, const bool hash,
> > + u8 **EM, struct public_key_signature *pks)
> > +{
> > + u8 *digest;
> > + struct crypto_shash *tfm;
> > + struct shash_desc *desc;
> > + size_t digest_size, desc_size;
> > + size_t tLen;
> > + u8 *T, *PS, *EM_tmp;
> > + int i, ret;
> > +
> > + pr_info("EMSA_PKCS1_v1_5_ENCODE start\n");
> > +
> > + if (!RSA_ASN1_templates[hash_algo].data)
>
> What about checking hash_algo against PKEY_HASH__LAST, or it relies on
> the caller?
>
Yes, check PKEY_HASH__LAST is more easy and clear, I will change it.
Thanks!
>
> > + ret = -ENOTSUPP;
> > + else
> > + pks->pkey_hash_algo = hash_algo;
> > +
> > + /* 1) Apply the hash function to the message M to produce a hash value H */
> > + tfm = crypto_alloc_shash(pkey_hash_algo[hash_algo], 0, 0);
> > + if (IS_ERR(tfm))
> > + return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> > +
> > + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > + digest_size = crypto_shash_digestsize(tfm);
> > +
> > + ret = -ENOMEM;
> > +
> > + digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
> > + if (!digest)
> > + goto error_digest;
> > + pks->digest = digest;
> > + pks->digest_size = digest_size;
> > +
>
> Ok. You allocated tfm to get hash size, right?
>
> But why do you allocate descriptor even it might not be needed?
>
You are right, I should skip the code of allocate descriptor when the
hash is supported. I will modified it.
Thanks!
> > + if (hash) {
> > + desc = (void *) digest + digest_size;
> > + desc->tfm = tfm;
> > + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> > +
> > + ret = crypto_shash_init(desc);
> > + if (ret < 0)
> > + goto error_shash;
> > + ret = crypto_shash_finup(desc, M, sizeof(M), pks->digest);
>
> This is I completely fail to understand... You expect sizeof(M) to be
> the message length?????
> Have you ever tested it?
>
Sigh!
I just checked my test program for this code path, this stupid problem
causes by my test program doesn't feed the right hash result that should
used to verify signature. So, I didn't capture this bug.
And, the hibernate signature check mechanism doesn't run into this code
path because the hash generation is done by hibernate code but not in
here. So, I also didn't find this problem when running hibernate check.
Appreciate for your point out! I just fix it in next patch version.
> > + if (ret < 0)
> > + goto error_shash;
> > + } else {
> > + memcpy(pks->digest, M, pks->digest_size);
> > + pks->digest_size = digest_size;
> > + }
>
> Does caller use pks->digest and pks->digest_size after return?
> I think it needs encoded value, not the hash...
> So why do you pass pks?
>
I put the signature MPI to pks->rsa.s, and put the encoded signature to
pks->S. So caller can grab encoded signature.
I also pass the pks->digest and pks->digest_size to caller for
reference. Then caller can simply feed this pks to
RSA_verify_signature() for verify the signature result, don't need
generate hash again.
>
>
> > + crypto_free_shash(tfm);
> > +
> > + /* 2) Encode the algorithm ID for the hash function and the hash value into
> > + * an ASN.1 value of type DigestInfo with the DER. Let T be the DER encoding of
> > + * the DigestInfo value and let tLen be the length in octets of T.
> > + */
> > + tLen = RSA_ASN1_templates[hash_algo].size + pks->digest_size;
> > + T = kmalloc(tLen, GFP_KERNEL);
> > + if (!T)
> > + goto error_T;
> > +
>
> Why do you need T and PS memory allocations at all?
> You need only EM_tmp allocation and copy directly to the destination...
>
OK, I will change the code to allocate T and PS size in EM_tmp.
>
> > + memcpy(T, RSA_ASN1_templates[hash_algo].data, RSA_ASN1_templates[hash_algo].size);
> > + memcpy(T + RSA_ASN1_templates[hash_algo].size, pks->digest, pks->digest_size);
> > +
> > + /* 3) check If emLen < tLen + 11, output "intended encoded message length too short" */
> > + if (emLen < tLen + 11) {
> > + ret = -EINVAL;
> > + goto error_emLen;
> > + }
> > +
> > + /* 4) Generate an octet string PS consisting of emLen - tLen - 3 octets with 0xff. */
> > + PS = kmalloc(emLen - tLen - 3, GFP_KERNEL);
> > + if (!PS)
> > + goto error_P;
> > +
>
> ditto
OK, I will allocate PS with EM_tmp.
Thanks!
>
> > + for (i = 0; i < (emLen - tLen - 3); i++)
> > + PS[i] = 0xff;
> > +
> > + /* 5) Concatenate PS, the DER encoding T, and other padding to form the encoded
> > + * message EM as EM = 0x00 || 0x01 || PS || 0x00 || T
> > + */
> > + EM_tmp = kmalloc(3 + emLen - tLen - 3 + tLen, GFP_KERNEL);
> > + if (!EM_tmp)
> > + goto error_EM;
> > +
> > + EM_tmp[0] = 0x00;
> > + EM_tmp[1] = 0x01;
> > + memcpy(EM_tmp + 2, PS, emLen - tLen - 3);
> > + EM_tmp[2 + emLen - tLen - 3] = 0x00;
> > + memcpy(EM_tmp + 2 + emLen - tLen - 3 + 1, T, tLen);
> > +
> > + *EM = EM_tmp;
> > +
> > + kfree(PS);
> > + kfree(T);
>
> get rid of it...
>
OK!
>
> - Dmitry
>
> > +
> > + return 0;
> > +
> > +error_EM:
> > + kfree(PS);
> > +error_P:
> > +error_emLen:
> > + kfree(T);
> > +error_T:
> > +error_shash:
> > + kfree(digest);
> > +error_digest:
> > + crypto_free_shash(tfm);
> > + return ret;
> > +}
> > +
> > +/*
> > * Perform the RSA signature verification.
> > * @H: Value of hash of data and metadata
> > * @EM: The computed signature value
> > @@ -275,9 +402,43 @@ static struct public_key_signature *RSA_generate_signature(
> > const struct private_key *key, u8 *M,
> > enum pkey_hash_algo hash_algo, const bool hash)
> > {
> > + struct public_key_signature *pks;
> > + u8 *EM = NULL;
> > + size_t emLen;
> > + int ret;
> > +
> > pr_info("RSA_generate_signature start\n");
> >
> > - return 0;
> > + ret = -ENOMEM;
> > + pks = kzalloc(sizeof(*pks), GFP_KERNEL);
> > + if (!pks)
> > + goto error_no_pks;
> > +
> > + /* 1): EMSA-PKCS1-v1_5 encoding: */
> > + /* Use the private key modulus size to be EM length */
> > + emLen = mpi_get_nbits(key->rsa.n);
> > + emLen = (emLen + 7) / 8;
> > +
> > + ret = EMSA_PKCS1_v1_5_ENCODE(M, emLen, hash_algo, hash, &EM, pks);
> > + if (ret < 0)
> > + goto error_v1_5_encode;
> > +
> > + /* TODO 2): m = OS2IP (EM) */
> > +
> > + /* TODO 3): s = RSASP1 (K, m) */
> > +
> > + /* TODO 4): S = I2OSP (s, k) */
> > +
> > + /* TODO: signature S to a u8* S or set to sig->rsa.s? */
> > + pks->S = EM; /* TODO: temporary set S to EM */
> > +
> > + return pks;
> > +
> > +error_v1_5_encode:
> > + kfree(pks);
> > +error_no_pks:
> > + pr_info("<==%s() = %d\n", __func__, ret);
> > + return ERR_PTR(ret);
> > }
> >
> > const struct public_key_algorithm RSA_public_key_algorithm = {
> > diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> > index d44b29f..1cdf457 100644
> > --- a/include/crypto/public_key.h
> > +++ b/include/crypto/public_key.h
> > @@ -110,6 +110,8 @@ extern void public_key_destroy(void *payload);
> > struct public_key_signature {
> > u8 *digest;
> > u8 digest_size; /* Number of bytes in digest */
> > + u8 *S; /* signature S of length k octets */
> > + size_t k; /* length k of signature S */
> > u8 nr_mpi; /* Occupancy of mpi[] */
> > enum pkey_hash_algo pkey_hash_algo : 8;
> > union {
> > --
> > 1.6.0.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH V4 02/15] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
From: joeyli @ 2013-09-18 9:08 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: linux-kernel@vger.kernel.org, linux-security-module, linux-efi,
linux-pm, linux-crypto, opensuse-kernel, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Pavel Machek,
Josh Boyer, Vojtech Pavlik, Matt Fleming, James Bottomley,
Greg KH, JKosina, Rusty Russell, Herbert Xu, David S. Miller,
H. Peter Anvin, Michal Marek, Gary Lin, Vivek Goyal
In-Reply-To: <CACE9dm-7HKz4VFR1bNTTFd-YpYhnkNVwiW81iXSJZbqjUTBR_Q@mail.gmail.com>
Hi Dmitry,
First, thanks for your time to review my patches!
於 二，2013-09-17 於 16:51 -0500，Dmitry Kasatkin 提到：
> Hello,
>
>
> On Sat, Sep 14, 2013 at 7:56 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> > first step of signature generation operation (RSASSA-PKCS1-v1_5-SIGN).
> >
> > This patch is temporary set emLen to pks->k, and temporary set EM to
> > pks->S for debugging. We will replace the above values to real signature
> > after implement RSASP1.
> >
> > The naming of EMSA_PKCS1_v1_5_ENCODE and the variables used in this function
> > accord PKCS#1 spec but not follow kernel naming convention, it useful when look
> > at them with spec.
> >
> > Reference: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1v2/pkcs1ietffinal.txt
> > Reference: http://www.emc.com/collateral/white-papers/h11300-pkcs-1v2-2-rsa-cryptography-standard-wp.pdf
> >
> > V2:
> > - Clean up naming of variable: replace _EM by EM, replace EM by EM_tmp.
> > - Add comment to EMSA_PKCS1-v1_5-ENCODE function.
> >
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Reviewed-by: Jiri Kosina <jkosina@suse.cz>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> > crypto/asymmetric_keys/rsa.c | 163 +++++++++++++++++++++++++++++++++++++++++-
> > include/crypto/public_key.h | 2 +
> > 2 files changed, 164 insertions(+), 1 deletions(-)
> >
> > diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c
> > index 47f3be4..352ba45 100644
> > --- a/crypto/asymmetric_keys/rsa.c
> > +++ b/crypto/asymmetric_keys/rsa.c
> > @@ -13,6 +13,7 @@
> > #include <linux/module.h>
> > #include <linux/kernel.h>
> > #include <linux/slab.h>
> > +#include <crypto/hash.h>
> > #include "public_key.h"
> > #include "private_key.h"
> >
> > @@ -152,6 +153,132 @@ static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> > }
> >
> > /*
> > + * EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2]
> > + * @M: message to be signed, an octet string
> > + * @emLen: intended length in octets of the encoded message
> > + * @hash_algo: hash function (option)
> > + * @hash: true means hash M, otherwise M is already a digest
> > + * @EM: encoded message, an octet string of length emLen
> > + *
> > + * This function is a implementation of the EMSA-PKCS1-v1_5 encoding operation
> > + * in RSA PKCS#1 spec. It used by the signautre generation operation of
> > + * RSASSA-PKCS1-v1_5 to encode message M to encoded message EM.
> > + *
> > + * The variables used in this function accord PKCS#1 spec but not follow kernel
> > + * naming convention, it useful when look at them with spec.
> > + */
> > +static int EMSA_PKCS1_v1_5_ENCODE(const u8 *M, size_t emLen,
> > + enum pkey_hash_algo hash_algo, const bool hash,
> > + u8 **EM, struct public_key_signature *pks)
> > +{
> > + u8 *digest;
> > + struct crypto_shash *tfm;
> > + struct shash_desc *desc;
> > + size_t digest_size, desc_size;
> > + size_t tLen;
> > + u8 *T, *PS, *EM_tmp;
> > + int i, ret;
> > +
> > + pr_info("EMSA_PKCS1_v1_5_ENCODE start\n");
> > +
> > + if (!RSA_ASN1_templates[hash_algo].data)
>
> What about checking hash_algo against PKEY_HASH__LAST, or it relies on
> the caller?
>
Yes, check PKEY_HASH__LAST is more easy and clear, I will change it.
Thanks!
>
> > + ret = -ENOTSUPP;
> > + else
> > + pks->pkey_hash_algo = hash_algo;
> > +
> > + /* 1) Apply the hash function to the message M to produce a hash value H */
> > + tfm = crypto_alloc_shash(pkey_hash_algo[hash_algo], 0, 0);
> > + if (IS_ERR(tfm))
> > + return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> > +
> > + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > + digest_size = crypto_shash_digestsize(tfm);
> > +
> > + ret = -ENOMEM;
> > +
> > + digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
> > + if (!digest)
> > + goto error_digest;
> > + pks->digest = digest;
> > + pks->digest_size = digest_size;
> > +
>
> Ok. You allocated tfm to get hash size, right?
>
> But why do you allocate descriptor even it might not be needed?
>
You are right, I should skip the code of allocate descriptor when the
hash is supported. I will modified it.
Thanks!
> > + if (hash) {
> > + desc = (void *) digest + digest_size;
> > + desc->tfm = tfm;
> > + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> > +
> > + ret = crypto_shash_init(desc);
> > + if (ret < 0)
> > + goto error_shash;
> > + ret = crypto_shash_finup(desc, M, sizeof(M), pks->digest);
>
> This is I completely fail to understand... You expect sizeof(M) to be
> the message length?????
> Have you ever tested it?
>
Sigh!
I just checked my test program for this code path, this stupid problem
causes by my test program doesn't feed the right hash result that should
used to verify signature. So, I didn't capture this bug.
And, the hibernate signature check mechanism doesn't run into this code
path because the hash generation is done by hibernate code but not in
here. So, I also didn't find this problem when running hibernate check.
Appreciate for your point out! I just fix it in next patch version.
> > + if (ret < 0)
> > + goto error_shash;
> > + } else {
> > + memcpy(pks->digest, M, pks->digest_size);
> > + pks->digest_size = digest_size;
> > + }
>
> Does caller use pks->digest and pks->digest_size after return?
> I think it needs encoded value, not the hash...
> So why do you pass pks?
>
I put the signature MPI to pks->rsa.s, and put the encoded signature to
pks->S. So caller can grab encoded signature.
I also pass the pks->digest and pks->digest_size to caller for
reference. Then caller can simply feed this pks to
RSA_verify_signature() for verify the signature result, don't need
generate hash again.
>
>
> > + crypto_free_shash(tfm);
> > +
> > + /* 2) Encode the algorithm ID for the hash function and the hash value into
> > + * an ASN.1 value of type DigestInfo with the DER. Let T be the DER encoding of
> > + * the DigestInfo value and let tLen be the length in octets of T.
> > + */
> > + tLen = RSA_ASN1_templates[hash_algo].size + pks->digest_size;
> > + T = kmalloc(tLen, GFP_KERNEL);
> > + if (!T)
> > + goto error_T;
> > +
>
> Why do you need T and PS memory allocations at all?
> You need only EM_tmp allocation and copy directly to the destination...
>
OK, I will change the code to allocate T and PS size in EM_tmp.
>
> > + memcpy(T, RSA_ASN1_templates[hash_algo].data, RSA_ASN1_templates[hash_algo].size);
> > + memcpy(T + RSA_ASN1_templates[hash_algo].size, pks->digest, pks->digest_size);
> > +
> > + /* 3) check If emLen < tLen + 11, output "intended encoded message length too short" */
> > + if (emLen < tLen + 11) {
> > + ret = -EINVAL;
> > + goto error_emLen;
> > + }
> > +
> > + /* 4) Generate an octet string PS consisting of emLen - tLen - 3 octets with 0xff. */
> > + PS = kmalloc(emLen - tLen - 3, GFP_KERNEL);
> > + if (!PS)
> > + goto error_P;
> > +
>
> ditto
OK, I will allocate PS with EM_tmp.
Thanks!
>
> > + for (i = 0; i < (emLen - tLen - 3); i++)
> > + PS[i] = 0xff;
> > +
> > + /* 5) Concatenate PS, the DER encoding T, and other padding to form the encoded
> > + * message EM as EM = 0x00 || 0x01 || PS || 0x00 || T
> > + */
> > + EM_tmp = kmalloc(3 + emLen - tLen - 3 + tLen, GFP_KERNEL);
> > + if (!EM_tmp)
> > + goto error_EM;
> > +
> > + EM_tmp[0] = 0x00;
> > + EM_tmp[1] = 0x01;
> > + memcpy(EM_tmp + 2, PS, emLen - tLen - 3);
> > + EM_tmp[2 + emLen - tLen - 3] = 0x00;
> > + memcpy(EM_tmp + 2 + emLen - tLen - 3 + 1, T, tLen);
> > +
> > + *EM = EM_tmp;
> > +
> > + kfree(PS);
> > + kfree(T);
>
> get rid of it...
>
OK!
>
> - Dmitry
>
> > +
> > + return 0;
> > +
> > +error_EM:
> > + kfree(PS);
> > +error_P:
> > +error_emLen:
> > + kfree(T);
> > +error_T:
> > +error_shash:
> > + kfree(digest);
> > +error_digest:
> > + crypto_free_shash(tfm);
> > + return ret;
> > +}
> > +
> > +/*
> > * Perform the RSA signature verification.
> > * @H: Value of hash of data and metadata
> > * @EM: The computed signature value
> > @@ -275,9 +402,43 @@ static struct public_key_signature *RSA_generate_signature(
> > const struct private_key *key, u8 *M,
> > enum pkey_hash_algo hash_algo, const bool hash)
> > {
> > + struct public_key_signature *pks;
> > + u8 *EM = NULL;
> > + size_t emLen;
> > + int ret;
> > +
> > pr_info("RSA_generate_signature start\n");
> >
> > - return 0;
> > + ret = -ENOMEM;
> > + pks = kzalloc(sizeof(*pks), GFP_KERNEL);
> > + if (!pks)
> > + goto error_no_pks;
> > +
> > + /* 1): EMSA-PKCS1-v1_5 encoding: */
> > + /* Use the private key modulus size to be EM length */
> > + emLen = mpi_get_nbits(key->rsa.n);
> > + emLen = (emLen + 7) / 8;
> > +
> > + ret = EMSA_PKCS1_v1_5_ENCODE(M, emLen, hash_algo, hash, &EM, pks);
> > + if (ret < 0)
> > + goto error_v1_5_encode;
> > +
> > + /* TODO 2): m = OS2IP (EM) */
> > +
> > + /* TODO 3): s = RSASP1 (K, m) */
> > +
> > + /* TODO 4): S = I2OSP (s, k) */
> > +
> > + /* TODO: signature S to a u8* S or set to sig->rsa.s? */
> > + pks->S = EM; /* TODO: temporary set S to EM */
> > +
> > + return pks;
> > +
> > +error_v1_5_encode:
> > + kfree(pks);
> > +error_no_pks:
> > + pr_info("<==%s() = %d\n", __func__, ret);
> > + return ERR_PTR(ret);
> > }
> >
> > const struct public_key_algorithm RSA_public_key_algorithm = {
> > diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> > index d44b29f..1cdf457 100644
> > --- a/include/crypto/public_key.h
> > +++ b/include/crypto/public_key.h
> > @@ -110,6 +110,8 @@ extern void public_key_destroy(void *payload);
> > struct public_key_signature {
> > u8 *digest;
> > u8 digest_size; /* Number of bytes in digest */
> > + u8 *S; /* signature S of length k octets */
> > + size_t k; /* length k of signature S */
> > u8 nr_mpi; /* Occupancy of mpi[] */
> > enum pkey_hash_algo pkey_hash_algo : 8;
> > union {
> > --
> > 1.6.0.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH V4 02/15] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
From: joeyli @ 2013-09-18 9:08 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: linux-kernel@vger.kernel.org, linux-security-module, linux-efi,
linux-pm, linux-crypto, opensuse-kernel, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Pavel Machek,
Josh Boyer, Vojtech Pavlik, Matt Fleming, James Bottomley,
Greg KH, JKosina, Rusty Russell, Herbert Xu, David S. Miller,
H. Peter Anvin, Michal Marek, Gary Lin, Vivek Goyal
In-Reply-To: <CACE9dm-7HKz4VFR1bNTTFd-YpYhnkNVwiW81iXSJZbqjUTBR_Q@mail.gmail.com>
Hi Dmitry,
First, thanks for your time to review my patches!
於 二，2013-09-17 於 16:51 -0500，Dmitry Kasatkin 提到：
> Hello,
>
>
> On Sat, Sep 14, 2013 at 7:56 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> > first step of signature generation operation (RSASSA-PKCS1-v1_5-SIGN).
> >
> > This patch is temporary set emLen to pks->k, and temporary set EM to
> > pks->S for debugging. We will replace the above values to real signature
> > after implement RSASP1.
> >
> > The naming of EMSA_PKCS1_v1_5_ENCODE and the variables used in this function
> > accord PKCS#1 spec but not follow kernel naming convention, it useful when look
> > at them with spec.
> >
> > Reference: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1v2/pkcs1ietffinal.txt
> > Reference: http://www.emc.com/collateral/white-papers/h11300-pkcs-1v2-2-rsa-cryptography-standard-wp.pdf
> >
> > V2:
> > - Clean up naming of variable: replace _EM by EM, replace EM by EM_tmp.
> > - Add comment to EMSA_PKCS1-v1_5-ENCODE function.
> >
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Reviewed-by: Jiri Kosina <jkosina@suse.cz>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> > crypto/asymmetric_keys/rsa.c | 163 +++++++++++++++++++++++++++++++++++++++++-
> > include/crypto/public_key.h | 2 +
> > 2 files changed, 164 insertions(+), 1 deletions(-)
> >
> > diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c
> > index 47f3be4..352ba45 100644
> > --- a/crypto/asymmetric_keys/rsa.c
> > +++ b/crypto/asymmetric_keys/rsa.c
> > @@ -13,6 +13,7 @@
> > #include <linux/module.h>
> > #include <linux/kernel.h>
> > #include <linux/slab.h>
> > +#include <crypto/hash.h>
> > #include "public_key.h"
> > #include "private_key.h"
> >
> > @@ -152,6 +153,132 @@ static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> > }
> >
> > /*
> > + * EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2]
> > + * @M: message to be signed, an octet string
> > + * @emLen: intended length in octets of the encoded message
> > + * @hash_algo: hash function (option)
> > + * @hash: true means hash M, otherwise M is already a digest
> > + * @EM: encoded message, an octet string of length emLen
> > + *
> > + * This function is a implementation of the EMSA-PKCS1-v1_5 encoding operation
> > + * in RSA PKCS#1 spec. It used by the signautre generation operation of
> > + * RSASSA-PKCS1-v1_5 to encode message M to encoded message EM.
> > + *
> > + * The variables used in this function accord PKCS#1 spec but not follow kernel
> > + * naming convention, it useful when look at them with spec.
> > + */
> > +static int EMSA_PKCS1_v1_5_ENCODE(const u8 *M, size_t emLen,
> > + enum pkey_hash_algo hash_algo, const bool hash,
> > + u8 **EM, struct public_key_signature *pks)
> > +{
> > + u8 *digest;
> > + struct crypto_shash *tfm;
> > + struct shash_desc *desc;
> > + size_t digest_size, desc_size;
> > + size_t tLen;
> > + u8 *T, *PS, *EM_tmp;
> > + int i, ret;
> > +
> > + pr_info("EMSA_PKCS1_v1_5_ENCODE start\n");
> > +
> > + if (!RSA_ASN1_templates[hash_algo].data)
>
> What about checking hash_algo against PKEY_HASH__LAST, or it relies on
> the caller?
>
Yes, check PKEY_HASH__LAST is more easy and clear, I will change it.
Thanks!
>
> > + ret = -ENOTSUPP;
> > + else
> > + pks->pkey_hash_algo = hash_algo;
> > +
> > + /* 1) Apply the hash function to the message M to produce a hash value H */
> > + tfm = crypto_alloc_shash(pkey_hash_algo[hash_algo], 0, 0);
> > + if (IS_ERR(tfm))
> > + return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> > +
> > + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > + digest_size = crypto_shash_digestsize(tfm);
> > +
> > + ret = -ENOMEM;
> > +
> > + digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
> > + if (!digest)
> > + goto error_digest;
> > + pks->digest = digest;
> > + pks->digest_size = digest_size;
> > +
>
> Ok. You allocated tfm to get hash size, right?
>
> But why do you allocate descriptor even it might not be needed?
>
You are right, I should skip the code of allocate descriptor when the
hash is supported. I will modified it.
Thanks!
> > + if (hash) {
> > + desc = (void *) digest + digest_size;
> > + desc->tfm = tfm;
> > + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> > +
> > + ret = crypto_shash_init(desc);
> > + if (ret < 0)
> > + goto error_shash;
> > + ret = crypto_shash_finup(desc, M, sizeof(M), pks->digest);
>
> This is I completely fail to understand... You expect sizeof(M) to be
> the message length?????
> Have you ever tested it?
>
Sigh!
I just checked my test program for this code path, this stupid problem
causes by my test program doesn't feed the right hash result that should
used to verify signature. So, I didn't capture this bug.
And, the hibernate signature check mechanism doesn't run into this code
path because the hash generation is done by hibernate code but not in
here. So, I also didn't find this problem when running hibernate check.
Appreciate for your point out! I just fix it in next patch version.
> > + if (ret < 0)
> > + goto error_shash;
> > + } else {
> > + memcpy(pks->digest, M, pks->digest_size);
> > + pks->digest_size = digest_size;
> > + }
>
> Does caller use pks->digest and pks->digest_size after return?
> I think it needs encoded value, not the hash...
> So why do you pass pks?
>
I put the signature MPI to pks->rsa.s, and put the encoded signature to
pks->S. So caller can grab encoded signature.
I also pass the pks->digest and pks->digest_size to caller for
reference. Then caller can simply feed this pks to
RSA_verify_signature() for verify the signature result, don't need
generate hash again.
>
>
> > + crypto_free_shash(tfm);
> > +
> > + /* 2) Encode the algorithm ID for the hash function and the hash value into
> > + * an ASN.1 value of type DigestInfo with the DER. Let T be the DER encoding of
> > + * the DigestInfo value and let tLen be the length in octets of T.
> > + */
> > + tLen = RSA_ASN1_templates[hash_algo].size + pks->digest_size;
> > + T = kmalloc(tLen, GFP_KERNEL);
> > + if (!T)
> > + goto error_T;
> > +
>
> Why do you need T and PS memory allocations at all?
> You need only EM_tmp allocation and copy directly to the destination...
>
OK, I will change the code to allocate T and PS size in EM_tmp.
>
> > + memcpy(T, RSA_ASN1_templates[hash_algo].data, RSA_ASN1_templates[hash_algo].size);
> > + memcpy(T + RSA_ASN1_templates[hash_algo].size, pks->digest, pks->digest_size);
> > +
> > + /* 3) check If emLen < tLen + 11, output "intended encoded message length too short" */
> > + if (emLen < tLen + 11) {
> > + ret = -EINVAL;
> > + goto error_emLen;
> > + }
> > +
> > + /* 4) Generate an octet string PS consisting of emLen - tLen - 3 octets with 0xff. */
> > + PS = kmalloc(emLen - tLen - 3, GFP_KERNEL);
> > + if (!PS)
> > + goto error_P;
> > +
>
> ditto
OK, I will allocate PS with EM_tmp.
Thanks!
>
> > + for (i = 0; i < (emLen - tLen - 3); i++)
> > + PS[i] = 0xff;
> > +
> > + /* 5) Concatenate PS, the DER encoding T, and other padding to form the encoded
> > + * message EM as EM = 0x00 || 0x01 || PS || 0x00 || T
> > + */
> > + EM_tmp = kmalloc(3 + emLen - tLen - 3 + tLen, GFP_KERNEL);
> > + if (!EM_tmp)
> > + goto error_EM;
> > +
> > + EM_tmp[0] = 0x00;
> > + EM_tmp[1] = 0x01;
> > + memcpy(EM_tmp + 2, PS, emLen - tLen - 3);
> > + EM_tmp[2 + emLen - tLen - 3] = 0x00;
> > + memcpy(EM_tmp + 2 + emLen - tLen - 3 + 1, T, tLen);
> > +
> > + *EM = EM_tmp;
> > +
> > + kfree(PS);
> > + kfree(T);
>
> get rid of it...
>
OK!
>
> - Dmitry
>
> > +
> > + return 0;
> > +
> > +error_EM:
> > + kfree(PS);
> > +error_P:
> > +error_emLen:
> > + kfree(T);
> > +error_T:
> > +error_shash:
> > + kfree(digest);
> > +error_digest:
> > + crypto_free_shash(tfm);
> > + return ret;
> > +}
> > +
> > +/*
> > * Perform the RSA signature verification.
> > * @H: Value of hash of data and metadata
> > * @EM: The computed signature value
> > @@ -275,9 +402,43 @@ static struct public_key_signature *RSA_generate_signature(
> > const struct private_key *key, u8 *M,
> > enum pkey_hash_algo hash_algo, const bool hash)
> > {
> > + struct public_key_signature *pks;
> > + u8 *EM = NULL;
> > + size_t emLen;
> > + int ret;
> > +
> > pr_info("RSA_generate_signature start\n");
> >
> > - return 0;
> > + ret = -ENOMEM;
> > + pks = kzalloc(sizeof(*pks), GFP_KERNEL);
> > + if (!pks)
> > + goto error_no_pks;
> > +
> > + /* 1): EMSA-PKCS1-v1_5 encoding: */
> > + /* Use the private key modulus size to be EM length */
> > + emLen = mpi_get_nbits(key->rsa.n);
> > + emLen = (emLen + 7) / 8;
> > +
> > + ret = EMSA_PKCS1_v1_5_ENCODE(M, emLen, hash_algo, hash, &EM, pks);
> > + if (ret < 0)
> > + goto error_v1_5_encode;
> > +
> > + /* TODO 2): m = OS2IP (EM) */
> > +
> > + /* TODO 3): s = RSASP1 (K, m) */
> > +
> > + /* TODO 4): S = I2OSP (s, k) */
> > +
> > + /* TODO: signature S to a u8* S or set to sig->rsa.s? */
> > + pks->S = EM; /* TODO: temporary set S to EM */
> > +
> > + return pks;
> > +
> > +error_v1_5_encode:
> > + kfree(pks);
> > +error_no_pks:
> > + pr_info("<==%s() = %d\n", __func__, ret);
> > + return ERR_PTR(ret);
> > }
> >
> > const struct public_key_algorithm RSA_public_key_algorithm = {
> > diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> > index d44b29f..1cdf457 100644
> > --- a/include/crypto/public_key.h
> > +++ b/include/crypto/public_key.h
> > @@ -110,6 +110,8 @@ extern void public_key_destroy(void *payload);
> > struct public_key_signature {
> > u8 *digest;
> > u8 digest_size; /* Number of bytes in digest */
> > + u8 *S; /* signature S of length k octets */
> > + size_t k; /* length k of signature S */
> > u8 nr_mpi; /* Occupancy of mpi[] */
> > enum pkey_hash_algo pkey_hash_algo : 8;
> > union {
> > --
> > 1.6.0.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH V4 02/15] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
From: joeyli @ 2013-09-18 9:08 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: linux-kernel@vger.kernel.org, linux-security-module, linux-efi,
linux-pm, linux-crypto, opensuse-kernel, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Pavel Machek,
Josh Boyer, Vojtech Pavlik, Matt Fleming, James Bottomley,
Greg KH, JKosina, Rusty Russell, Herbert Xu, David S. Miller,
H. Peter Anvin, Michal Marek, Gary Lin, Vivek Goyal
In-Reply-To: <CACE9dm-7HKz4VFR1bNTTFd-YpYhnkNVwiW81iXSJZbqjUTBR_Q@mail.gmail.com>
Hi Dmitry,
First, thanks for your time to review my patches!
於 二，2013-09-17 於 16:51 -0500，Dmitry Kasatkin 提到：
> Hello,
>
>
> On Sat, Sep 14, 2013 at 7:56 PM, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> > first step of signature generation operation (RSASSA-PKCS1-v1_5-SIGN).
> >
> > This patch is temporary set emLen to pks->k, and temporary set EM to
> > pks->S for debugging. We will replace the above values to real signature
> > after implement RSASP1.
> >
> > The naming of EMSA_PKCS1_v1_5_ENCODE and the variables used in this function
> > accord PKCS#1 spec but not follow kernel naming convention, it useful when look
> > at them with spec.
> >
> > Reference: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1v2/pkcs1ietffinal.txt
> > Reference: http://www.emc.com/collateral/white-papers/h11300-pkcs-1v2-2-rsa-cryptography-standard-wp.pdf
> >
> > V2:
> > - Clean up naming of variable: replace _EM by EM, replace EM by EM_tmp.
> > - Add comment to EMSA_PKCS1-v1_5-ENCODE function.
> >
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Reviewed-by: Jiri Kosina <jkosina@suse.cz>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> > crypto/asymmetric_keys/rsa.c | 163 +++++++++++++++++++++++++++++++++++++++++-
> > include/crypto/public_key.h | 2 +
> > 2 files changed, 164 insertions(+), 1 deletions(-)
> >
> > diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c
> > index 47f3be4..352ba45 100644
> > --- a/crypto/asymmetric_keys/rsa.c
> > +++ b/crypto/asymmetric_keys/rsa.c
> > @@ -13,6 +13,7 @@
> > #include <linux/module.h>
> > #include <linux/kernel.h>
> > #include <linux/slab.h>
> > +#include <crypto/hash.h>
> > #include "public_key.h"
> > #include "private_key.h"
> >
> > @@ -152,6 +153,132 @@ static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> > }
> >
> > /*
> > + * EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2]
> > + * @M: message to be signed, an octet string
> > + * @emLen: intended length in octets of the encoded message
> > + * @hash_algo: hash function (option)
> > + * @hash: true means hash M, otherwise M is already a digest
> > + * @EM: encoded message, an octet string of length emLen
> > + *
> > + * This function is a implementation of the EMSA-PKCS1-v1_5 encoding operation
> > + * in RSA PKCS#1 spec. It used by the signautre generation operation of
> > + * RSASSA-PKCS1-v1_5 to encode message M to encoded message EM.
> > + *
> > + * The variables used in this function accord PKCS#1 spec but not follow kernel
> > + * naming convention, it useful when look at them with spec.
> > + */
> > +static int EMSA_PKCS1_v1_5_ENCODE(const u8 *M, size_t emLen,
> > + enum pkey_hash_algo hash_algo, const bool hash,
> > + u8 **EM, struct public_key_signature *pks)
> > +{
> > + u8 *digest;
> > + struct crypto_shash *tfm;
> > + struct shash_desc *desc;
> > + size_t digest_size, desc_size;
> > + size_t tLen;
> > + u8 *T, *PS, *EM_tmp;
> > + int i, ret;
> > +
> > + pr_info("EMSA_PKCS1_v1_5_ENCODE start\n");
> > +
> > + if (!RSA_ASN1_templates[hash_algo].data)
>
> What about checking hash_algo against PKEY_HASH__LAST, or it relies on
> the caller?
>
Yes, check PKEY_HASH__LAST is more easy and clear, I will change it.
Thanks!
>
> > + ret = -ENOTSUPP;
> > + else
> > + pks->pkey_hash_algo = hash_algo;
> > +
> > + /* 1) Apply the hash function to the message M to produce a hash value H */
> > + tfm = crypto_alloc_shash(pkey_hash_algo[hash_algo], 0, 0);
> > + if (IS_ERR(tfm))
> > + return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> > +
> > + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > + digest_size = crypto_shash_digestsize(tfm);
> > +
> > + ret = -ENOMEM;
> > +
> > + digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
> > + if (!digest)
> > + goto error_digest;
> > + pks->digest = digest;
> > + pks->digest_size = digest_size;
> > +
>
> Ok. You allocated tfm to get hash size, right?
>
> But why do you allocate descriptor even it might not be needed?
>
You are right, I should skip the code of allocate descriptor when the
hash is supported. I will modified it.
Thanks!
> > + if (hash) {
> > + desc = (void *) digest + digest_size;
> > + desc->tfm = tfm;
> > + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> > +
> > + ret = crypto_shash_init(desc);
> > + if (ret < 0)
> > + goto error_shash;
> > + ret = crypto_shash_finup(desc, M, sizeof(M), pks->digest);
>
> This is I completely fail to understand... You expect sizeof(M) to be
> the message length?????
> Have you ever tested it?
>
Sigh!
I just checked my test program for this code path, this stupid problem
causes by my test program doesn't feed the right hash result that should
used to verify signature. So, I didn't capture this bug.
And, the hibernate signature check mechanism doesn't run into this code
path because the hash generation is done by hibernate code but not in
here. So, I also didn't find this problem when running hibernate check.
Appreciate for your point out! I just fix it in next patch version.
> > + if (ret < 0)
> > + goto error_shash;
> > + } else {
> > + memcpy(pks->digest, M, pks->digest_size);
> > + pks->digest_size = digest_size;
> > + }
>
> Does caller use pks->digest and pks->digest_size after return?
> I think it needs encoded value, not the hash...
> So why do you pass pks?
>
I put the signature MPI to pks->rsa.s, and put the encoded signature to
pks->S. So caller can grab encoded signature.
I also pass the pks->digest and pks->digest_size to caller for
reference. Then caller can simply feed this pks to
RSA_verify_signature() for verify the signature result, don't need
generate hash again.
>
>
> > + crypto_free_shash(tfm);
> > +
> > + /* 2) Encode the algorithm ID for the hash function and the hash value into
> > + * an ASN.1 value of type DigestInfo with the DER. Let T be the DER encoding of
> > + * the DigestInfo value and let tLen be the length in octets of T.
> > + */
> > + tLen = RSA_ASN1_templates[hash_algo].size + pks->digest_size;
> > + T = kmalloc(tLen, GFP_KERNEL);
> > + if (!T)
> > + goto error_T;
> > +
>
> Why do you need T and PS memory allocations at all?
> You need only EM_tmp allocation and copy directly to the destination...
>
OK, I will change the code to allocate T and PS size in EM_tmp.
>
> > + memcpy(T, RSA_ASN1_templates[hash_algo].data, RSA_ASN1_templates[hash_algo].size);
> > + memcpy(T + RSA_ASN1_templates[hash_algo].size, pks->digest, pks->digest_size);
> > +
> > + /* 3) check If emLen < tLen + 11, output "intended encoded message length too short" */
> > + if (emLen < tLen + 11) {
> > + ret = -EINVAL;
> > + goto error_emLen;
> > + }
> > +
> > + /* 4) Generate an octet string PS consisting of emLen - tLen - 3 octets with 0xff. */
> > + PS = kmalloc(emLen - tLen - 3, GFP_KERNEL);
> > + if (!PS)
> > + goto error_P;
> > +
>
> ditto
OK, I will allocate PS with EM_tmp.
Thanks!
>
> > + for (i = 0; i < (emLen - tLen - 3); i++)
> > + PS[i] = 0xff;
> > +
> > + /* 5) Concatenate PS, the DER encoding T, and other padding to form the encoded
> > + * message EM as EM = 0x00 || 0x01 || PS || 0x00 || T
> > + */
> > + EM_tmp = kmalloc(3 + emLen - tLen - 3 + tLen, GFP_KERNEL);
> > + if (!EM_tmp)
> > + goto error_EM;
> > +
> > + EM_tmp[0] = 0x00;
> > + EM_tmp[1] = 0x01;
> > + memcpy(EM_tmp + 2, PS, emLen - tLen - 3);
> > + EM_tmp[2 + emLen - tLen - 3] = 0x00;
> > + memcpy(EM_tmp + 2 + emLen - tLen - 3 + 1, T, tLen);
> > +
> > + *EM = EM_tmp;
> > +
> > + kfree(PS);
> > + kfree(T);
>
> get rid of it...
>
OK!
>
> - Dmitry
>
> > +
> > + return 0;
> > +
> > +error_EM:
> > + kfree(PS);
> > +error_P:
> > +error_emLen:
> > + kfree(T);
> > +error_T:
> > +error_shash:
> > + kfree(digest);
> > +error_digest:
> > + crypto_free_shash(tfm);
> > + return ret;
> > +}
> > +
> > +/*
> > * Perform the RSA signature verification.
> > * @H: Value of hash of data and metadata
> > * @EM: The computed signature value
> > @@ -275,9 +402,43 @@ static struct public_key_signature *RSA_generate_signature(
> > const struct private_key *key, u8 *M,
> > enum pkey_hash_algo hash_algo, const bool hash)
> > {
> > + struct public_key_signature *pks;
> > + u8 *EM = NULL;
> > + size_t emLen;
> > + int ret;
> > +
> > pr_info("RSA_generate_signature start\n");
> >
> > - return 0;
> > + ret = -ENOMEM;
> > + pks = kzalloc(sizeof(*pks), GFP_KERNEL);
> > + if (!pks)
> > + goto error_no_pks;
> > +
> > + /* 1): EMSA-PKCS1-v1_5 encoding: */
> > + /* Use the private key modulus size to be EM length */
> > + emLen = mpi_get_nbits(key->rsa.n);
> > + emLen = (emLen + 7) / 8;
> > +
> > + ret = EMSA_PKCS1_v1_5_ENCODE(M, emLen, hash_algo, hash, &EM, pks);
> > + if (ret < 0)
> > + goto error_v1_5_encode;
> > +
> > + /* TODO 2): m = OS2IP (EM) */
> > +
> > + /* TODO 3): s = RSASP1 (K, m) */
> > +
> > + /* TODO 4): S = I2OSP (s, k) */
> > +
> > + /* TODO: signature S to a u8* S or set to sig->rsa.s? */
> > + pks->S = EM; /* TODO: temporary set S to EM */
> > +
> > + return pks;
> > +
> > +error_v1_5_encode:
> > + kfree(pks);
> > +error_no_pks:
> > + pr_info("<==%s() = %d\n", __func__, ret);
> > + return ERR_PTR(ret);
> > }
> >
> > const struct public_key_algorithm RSA_public_key_algorithm = {
> > diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> > index d44b29f..1cdf457 100644
> > --- a/include/crypto/public_key.h
> > +++ b/include/crypto/public_key.h
> > @@ -110,6 +110,8 @@ extern void public_key_destroy(void *payload);
> > struct public_key_signature {
> > u8 *digest;
> > u8 digest_size; /* Number of bytes in digest */
> > + u8 *S; /* signature S of length k octets */
> > + size_t k; /* length k of signature S */
> > u8 nr_mpi; /* Occupancy of mpi[] */
> > enum pkey_hash_algo pkey_hash_algo : 8;
> > union {
> > --
> > 1.6.0.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V4 13/15] Hibernate: introduced SNAPSHOT_SIG_HASH config for select hash algorithm
From: Pavel Machek @ 2013-09-18 13:45 UTC (permalink / raw)
To: Lee, Chun-Yi
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
opensuse-kernel-stAJ6ESoqRxg9hUCZPvPmw, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Josh Boyer,
Vojtech Pavlik, Matt Fleming, James Bottomley, Greg KH,
JKosina-IBi9RG/b67k, Rusty Russell, Herbert Xu, David S. Miller,
H. Peter Anvin, Michal Marek, Gary Lin, Vivek Goyal, Lee, Chun-Yi
In-Reply-To: <1379206621-18639-14-git-send-email-jlee-IBi9RG/b67k@public.gmane.org>
On Sun 2013-09-15 08:56:59, Lee, Chun-Yi wrote:
> This patch introduced SNAPSHOT_SIG_HASH config for user to select which
> hash algorithm will be used during signature generation of snapshot.
This series is big enough already... and who is going to test it?
There's no need to make hash configurable. Just select one that works.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
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