[PATCH 1/2 v2] scripts/coccinelle: catch freeing cryptographic structures via kfree
Structures allocated by crypto_alloc_* must be freed using crypto_free_*. Signed-off-by: Konstantin Khlebnikov k.khlebni...@samsung.com --- scripts/coccinelle/free/crypto_free.cocci | 64 + 1 file changed, 64 insertions(+) create mode 100644 scripts/coccinelle/free/crypto_free.cocci diff --git a/scripts/coccinelle/free/crypto_free.cocci b/scripts/coccinelle/free/crypto_free.cocci new file mode 100644 index 000..a717070 --- /dev/null +++ b/scripts/coccinelle/free/crypto_free.cocci @@ -0,0 +1,64 @@ +/// +/// Structures allocated by crypto_alloc_* must be freed using crypto_free_*. +/// This finds freeing them by kfree. +/// +// Confidence: Moderate +// Copyright: (C) 2014 Konstantin Khlebnikov, GPLv2. +// Comments: There are false positives in crypto/ where they are actually freed. +// Keywords: crypto, kfree +// Options: --no-includes --include-headers + +virtual org +virtual report +virtual context + +@r depends on context || org || report@ +expression x; +@@ + +( + x = crypto_alloc_base(...) +| + x = crypto_alloc_cipher(...) +| + x = crypto_alloc_ablkcipher(...) +| + x = crypto_alloc_aead(...) +| + x = crypto_alloc_instance(...) +| + x = crypto_alloc_instance2(...) +| + x = crypto_alloc_comp(...) +| + x = crypto_alloc_pcomp(...) +| + x = crypto_alloc_hash(...) +| + x = crypto_alloc_ahash(...) +| + x = crypto_alloc_shash(...) +| + x = crypto_alloc_rng(...) +) + +@pb@ +expression r.x; +position p; +@@ + +* kfree@p(x) + +@script:python depends on org@ +p pb.p; +@@ + +msg=WARNING: invalid free of crypto_alloc_* allocated data +coccilib.org.print_todo(p[0], msg) + +@script:python depends on report@ +p pb.p; +@@ + +msg=WARNING: invalid free of crypto_alloc_* allocated data +coccilib.report.print_report(p[0], msg) -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/10] crypto: AF_ALG: add user space interface for AEAD
On Sun, Nov 16, 2014 at 03:23:50AM +0100, Stephan Mueller wrote: AEAD requires the following data in addition to normal symmetric ciphers: * Associated authentication data of arbitrary length * Authentication tag for decryption * Length of authentication tag for encryption The authentication tag data is communicated as part of the actual ciphertext as mandated by the kernel crypto API. Therefore we only need to provide a user space interface for the associated authentication data as well as for the authentication tag length. This patch adds both as a setsockopt interface that is identical to the AF_ALG interface for setting an IV and for selecting the cipher operation type (encrypt or decrypt). Signed-off-by: Stephan Mueller smuel...@chronox.de I don't like the fact that we're putting arbitrary limits on the AD, as well as the fact that the way you're doing it the AD has to be copied. How about simply saying that the first X bytes of the input shall be the AD? Cheers, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/10] crypto: AF_ALG: make setkey optional
On Sun, Nov 16, 2014 at 03:26:58AM +0100, Stephan Mueller wrote: The current AF_ALG implementation requires that a userspace interface implementation must provide a callback for setkey. Such a call is not appliable to random number generators. To prepare AF_ALG for the addition of a random number generator user space interface, this function callback invocation is made optional. Signed-off-by: Stephan Mueller smuel...@chronox.de Did you actually try this? AFAICS setkey is already optional. Cheers, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
Andrew,
Thanks for the review
-Original Message-
From: abres...@google.com [mailto:abres...@google.com] On Behalf Of
Andrew Bresticker
Sent: 14 November 2014 23:59
To: James Hartley
Cc: Herbert Xu; da...@davemloft.net; Grant Likely; Rob Herring;
a...@linux-foundation.org; Greg Kroah-Hartman; j...@perches.com;
mche...@osg.samsung.com; Antti Palosaari; jg1@samsung.com; linux-
cry...@vger.kernel.org; devicet...@vger.kernel.org; Pawel Moll; Mark
Rutland; Ian Campbell; Kumar Gala; Ezequiel Garcia
Subject: Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash
accelerator
Hi James,
On Mon, Nov 10, 2014 at 4:10 AM, James Hartley
james.hart...@imgtec.com wrote:
This adds support for the Imagination Technologies hash accelerator
that provides hardware acceleration for
SHA1 SHA224 SHA256 and MD5 Hashes.
Signed-off-by: James Hartley james.hart...@imgtec.com
It's going to take me a little longer to get through the crypto API parts of
the
driver, but here are some initial comments.
diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index
2fb0fdf..4b931eb 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -436,4 +436,17 @@ config CRYPTO_DEV_QCE
hardware. To compile this driver as a module, choose M here. The
module will be called qcrypto.
+config CRYPTO_DEV_IMGTEC_HASH
+tristate Imagination Technologies hardware hash accelerator
Is there no meaningful depends-on here? There's no MACH_PISTACHIO yet,
but perhaps MIPS for now? Also, {readl,writel}_relaxed aren't available on
all
architectures.
I have added MIPS || COMPILE_TEST for now.
On the basis of Arnd's comments I'll leave in the readl, writel.
+select CRYPTO_ALG_API
+select CRYPTO_MD5
+select CRYPTO_SHA1
+select CRYPTO_SHA224
+select CRYPTO_SHA256
+select CRYPTO_HASH
+help
+ This driver interfaces with the Imagination Technologies
+ hardware hash accelerator. Supporting MD5/SHA1/SHA224/SHA256
+ hashing algorithms.
+
endif # CRYPTO_HW
diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c new
file mode 100644 index 000..e58c81a
--- /dev/null
+++ b/drivers/crypto/img-hash.c
@@ -0,0 +1,1048 @@
+/*
+* Copyright (c) 2014 Imagination Technologies
+* Author: Will Thomas
+*
+* This program is free software; you can redistribute it and/or modify
+* it under the terms of the GNU General Public License version 2 as
published
+* by the Free Software Foundation.
+*
+* Interface structure taken from omap-sham driver
+*/
Comment style is incorrect here, the *s in multi-line comments should
be aligned.
Fixed
+#include linux/kernel.h
+#include linux/module.h
+#include linux/clk.h
+#include linux/platform_device.h
+#include linux/io.h
+#include linux/scatterlist.h
+#include linux/interrupt.h
+#include linux/of_device.h
+#include crypto/sha.h
+#include crypto/md5.h
+#include crypto/internal/hash.h
#includes should be sorted in alphabetical order, with a newline
separating the linux/ and crypto/ includes.
Ok, done.
+#define MD5_BLOCK_SIZE 64
There's already MD5_HMAC_BLOCK_SIZE #define'd in crypto/md5.h.
Ah yes, modified now.
+#define CR_RESET 0
+#define CR_RESET_SET 1
+#define CR_RESET_UNSET 0
+
+#define CR_MESSAGE_LENGTH_H0x4
+#define CR_MESSAGE_LENGTH_L0x8
+
+#defineCR_CONTROL 0xc
+#define CR_CONTROL_BYTE_ORDER_3210 0
+#define CR_CONTROL_BYTE_ORDER_0123 1
+#define CR_CONTROL_BYTE_ORDER_2310 2
+#define CR_CONTROL_BYTE_ORDER_1032 3
+#define CR_CONTROL_ALGO_MD50
+#define CR_CONTROL_ALGO_SHA1 1
+#define CR_CONTROL_ALGO_SHA224 2
+#define CR_CONTROL_ALGO_SHA256 3
+
+#define CR_INTSTAT 0x10
+#define CR_INTENAB 0x14
+#define CR_INTCLEAR0x18
+#define CR_INT_RESULTS_AVAILABLE (10)
+#define CR_INT_NEW_RESULTS_SET (11)
+#define CR_INT_RESULT_READ_ERR (12)
+#define CR_INT_MESSAGE_WRITE_ERROR (13)
+#define CR_INT_STATUS (18)
Use the BIT() macro for single-bit fields like this.
Yes, done now.
+#define CR_RESULT_QUEUE0x1c
+#define CR_RSD00x40
+#define CR_CORE_REV0x50
+#define CR_CORE_DES1 0x60
+#define CR_CORE_DES2 0x70
+
+#define DRIVER_FLAGS_BUSY BIT(0)
+#define DRIVER_FLAGS_FINAL BIT(1)
+#define DRIVER_FLAGS_DMA_ACTIVEBIT(2)
+#define DRIVER_FLAGS_OUTPUT_READY BIT(3)
+#define DRIVER_FLAGS_INIT BIT(4)
+#define DRIVER_FLAGS_CPU BIT(5)
+#define
RE: [PATCH 2/2] Documentation: crypto: Add DT binding info for the img hw hash accelerator
Hi Andrew
-Original Message-
From: abres...@google.com [mailto:abres...@google.com] On Behalf Of
Andrew Bresticker
Sent: 10 November 2014 17:30
To: James Hartley
Cc: herb...@gondor.apana.org.au; da...@davemloft.net; Grant Likely; Rob
Herring; a...@linux-foundation.org; Greg Kroah-Hartman;
j...@perches.com; mche...@osg.samsung.com; cr...@iki.fi;
jg1@samsung.com; linux-crypto@vger.kernel.org;
devicet...@vger.kernel.org; Pawel Moll; Mark Rutland; Ian Campbell; Kumar
Gala; Ezequiel Garcia
Subject: Re: [PATCH 2/2] Documentation: crypto: Add DT binding info for the
img hw hash accelerator
Hi James,
On Mon, Nov 10, 2014 at 4:10 AM, James Hartley
james.hart...@imgtec.com wrote:
Signed-off-by: James Hartley james.hart...@imgtec.com
A brief commit message describing the hardware and where it's found would
be nice.
diff --git a/Documentation/devicetree/bindings/crypto/img-hash.txt
b/Documentation/devicetree/bindings/crypto/img-hash.txt
@@ -0,0 +1,28 @@
+* Imagination Technologies Ltd. Hash Accelerator
+
+The hash accelerator provides hardware hashing acceleration for SHA1,
+SHA224, SHA256 and MD5 hashes
+
+Required properties:
+
+- compatible : img,img-hash-accelerator-rev1
I know I mentioned in the internal review that it would be good to have
some sort of version indicator, but it looks like from the TRM that the
version
is probable (CR_HASH_CORE_REV). If we expect probing for the revision
number to be sufficient, then perhaps rev1 can be dropped? Also, the
second img is redundant.
Yes the core ID and versions are available, so I'll drop rev-1, and remove the
second img.
+- reg : Offset and length of the register set for the module, and the
+DMA port
+- interrupts : The designated IRQ line for the hashing module.
+- dmas : DMA specifier as per
+Documentation/devicetree/bindings/dma/dma.txt
+- dma-names : Should be tx
+- bus-addr : The bus address for the input data for hashing block
I think this can be dropped. This is the same as the second reg
entry above, is it not?
Yes, that should not have made it through to the patch - it will be removed.
Thanks,
James.
N�r��yb�X��ǧv�^�){.n�+{�r����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf
[PATCH V2 0/2] crypto: Add support for the IMG hash accelerator
This adds support for the Imagination Technologies hash accelerator which provides hardware acceleration for SHA1 SHA244 SHA256 and MD5 hashes. Changes from V1: * Addressed review comments from Andrew Bresticker and Vladimir Zapolskiy * rebased to current linux-next James Hartley (2): crypto: Add Imagination Technologies hw hash accelerator Documentation: crypto: Add DT binding info for the img hw hash accelerator .../devicetree/bindings/crypto/img-hash.txt| 26 + MAINTAINERS|5 + drivers/crypto/Kconfig | 14 + drivers/crypto/Makefile|1 + drivers/crypto/img-hash.c | 1061 5 files changed, 1108 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/img-hash.txt create mode 100644 drivers/crypto/img-hash.c -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2 2/2] Documentation: crypto: Add DT binding info for the img hw hash accelerator
This adds the binding documenation for the Imagination Technologies
hash accelerator that provides hardware acceleration for
SHA1/SHA224/SHA256/MD5 hashes. This hardware will be present in
the upcoming pistachio SoC.
Signed-off-by: James Hartley james.hart...@imgtec.com
---
.../devicetree/bindings/crypto/img-hash.txt| 26
1 file changed, 26 insertions(+)
create mode 100644 Documentation/devicetree/bindings/crypto/img-hash.txt
diff --git a/Documentation/devicetree/bindings/crypto/img-hash.txt
b/Documentation/devicetree/bindings/crypto/img-hash.txt
new file mode 100644
index 000..72764a8
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/img-hash.txt
@@ -0,0 +1,26 @@
+* Imagination Technologies Ltd. Hash Accelerator
+
+The hash accelerator provides hardware hashing acceleration for
+SHA1, SHA224, SHA256 and MD5 hashes
+
+Required properties:
+
+- compatible : img,hash-accelerator
+- reg : Offset and length of the register set for the module, and the DMA port
+- interrupts : The designated IRQ line for the hashing module.
+- dmas : DMA specifier as per Documentation/devicetree/bindings/dma/dma.txt
+- dma-names : Should be tx
+- clocks : Clock specifier
+- clock-names : hash_clk Used to clock data through the accelerator
+
+Example:
+
+ hash: hash@18149600 {
+ compatible = img,hash-accelerator;
+ reg = 0x18149600 0x100, 0x18101100 0x4;
+ interrupts = 59 4 2;
+ dmas = dma 8 0x;
+ dma-names = tx;
+ clocks = hash_clk;
+ clock-names = hash_clk;
+ };
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2 1/2] crypto: Add Imagination Technologies hw hash accelerator
This adds support for the Imagination Technologies hash accelerator which provides hardware acceleratopm for SHA1 SHA224 SHA256 and MD5 hashes. Signed-off-by: James Hartley james.hart...@imgtec.com --- MAINTAINERS |5 + drivers/crypto/Kconfig| 14 + drivers/crypto/Makefile |1 + drivers/crypto/img-hash.c | 1061 + 4 files changed, 1081 insertions(+) create mode 100644 drivers/crypto/img-hash.c diff --git a/MAINTAINERS b/MAINTAINERS index 0662378..45a3b53 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4743,6 +4743,11 @@ S: Maintained F: drivers/iio/ F: drivers/staging/iio/ +IMAGINATION TECHNOLOGIES HASH ACCELERATOR DRIVER +M: James Hartley james.hart...@imgtec.com +S: Supported +F: drivers/crypto/img-hash.c + IKANOS/ADI EAGLE ADSL USB DRIVER M: Matthieu Castet castet.matth...@free.fr M: Stanislaw Gruszka stf...@wp.pl diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index 2fb0fdf..ee43ba3 100644 --- a/drivers/crypto/Kconfig +++ b/drivers/crypto/Kconfig @@ -436,4 +436,18 @@ config CRYPTO_DEV_QCE hardware. To compile this driver as a module, choose M here. The module will be called qcrypto. +config CRYPTO_DEV_IMGTEC_HASH + tristate Imagination Technologies hardware hash accelerator + depends on MIPS || COMPILE_TEST + select CRYPTO_ALG_API + select CRYPTO_MD5 + select CRYPTO_SHA1 + select CRYPTO_SHA224 + select CRYPTO_SHA256 + select CRYPTO_HASH + help + This driver interfaces with the Imagination Technologies + hardware hash accelerator. Supporting MD5/SHA1/SHA224/SHA256 + hashing algorithms + endif # CRYPTO_HW diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile index 3924f93..1c34fff 100644 --- a/drivers/crypto/Makefile +++ b/drivers/crypto/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_CRYPTO_DEV_CCP) += ccp/ obj-$(CONFIG_CRYPTO_DEV_FSL_CAAM) += caam/ obj-$(CONFIG_CRYPTO_DEV_GEODE) += geode-aes.o obj-$(CONFIG_CRYPTO_DEV_HIFN_795X) += hifn_795x.o +obj-$(CONFIG_CRYPTO_DEV_IMGTEC_HASH) += img-hash.o obj-$(CONFIG_CRYPTO_DEV_IXP4XX) += ixp4xx_crypto.o obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o obj-$(CONFIG_CRYPTO_DEV_MXS_DCP) += mxs-dcp.o diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c new file mode 100644 index 000..3ca6fce --- /dev/null +++ b/drivers/crypto/img-hash.c @@ -0,0 +1,1061 @@ +/* + * Copyright (c) 2014 Imagination Technologies + * Author: Will Thomas + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as published + * by the Free Software Foundation. + * + * Interface structure taken from omap-sham driver + */ + +#include linux/kernel.h +#include linux/module.h +#include linux/clk.h +#include linux/platform_device.h +#include linux/io.h +#include linux/scatterlist.h +#include linux/interrupt.h +#include linux/of_device.h + +#include crypto/sha.h +#include crypto/md5.h +#include crypto/internal/hash.h + +#define CR_RESET 0 +#define CR_RESET_SET 1 +#define CR_RESET_UNSET 0 + +#define CR_MESSAGE_LENGTH_H0x4 +#define CR_MESSAGE_LENGTH_L0x8 + +#define CR_CONTROL 0xc +#define CR_CONTROL_BYTE_ORDER_3210 0 +#define CR_CONTROL_BYTE_ORDER_0123 1 +#define CR_CONTROL_BYTE_ORDER_2310 2 +#define CR_CONTROL_BYTE_ORDER_1032 3 +#define CR_CONTROL_BYTE_ORDER_SHIFT8 +#define CR_CONTROL_ALGO_MD50 +#define CR_CONTROL_ALGO_SHA1 1 +#define CR_CONTROL_ALGO_SHA224 2 +#define CR_CONTROL_ALGO_SHA256 3 + +#define CR_INTSTAT 0x10 +#define CR_INTENAB 0x14 +#define CR_INTCLEAR0x18 +#define CR_INT_RESULTS_AVAILABLE BIT(0) +#define CR_INT_NEW_RESULTS_SET BIT(1) +#define CR_INT_RESULT_READ_ERR BIT(2) +#define CR_INT_MESSAGE_WRITE_ERROR BIT(3) +#define CR_INT_STATUS BIT(8) + +#define CR_RESULT_QUEUE0x1c +#define CR_RSD00x40 +#define CR_CORE_REV0x50 +#define CR_CORE_DES1 0x60 +#define CR_CORE_DES2 0x70 + +#define DRIVER_FLAGS_BUSY BIT(0) +#define DRIVER_FLAGS_FINAL BIT(1) +#define DRIVER_FLAGS_DMA_ACTIVEBIT(2) +#define DRIVER_FLAGS_OUTPUT_READY BIT(3) +#define DRIVER_FLAGS_INIT BIT(4) +#define DRIVER_FLAGS_CPU BIT(5) +#define DRIVER_FLAGS_DMA_READY BIT(6) +#define DRIVER_FLAGS_ERROR BIT(7) +#define DRIVER_FLAGS_SGBIT(8) +#define DRIVER_FLAGS_FINUP BIT(9) +#define DRIVER_FLAGS_SHA1 BIT(18) +#define DRIVER_FLAGS_SHA224BIT(19) +#define DRIVER_FLAGS_SHA256BIT(20) +#define DRIVER_FLAGS_MD5
Re: [PATCH v2] crypto: prefix module autoloading with crypto-
On 18 November 2014 01:45, Kees Cook keesc...@chromium.org wrote: On Mon, Nov 17, 2014 at 3:20 PM, Mathias Krause mini...@googlemail.com wrote: On 17 November 2014 21:02, Kees Cook keesc...@chromium.org wrote: This prefixes all crypto module loading with crypto- so we never run the risk of exposing module auto-loading to userspace via a crypto API, as demonstrated by Mathias Krause: https://lkml.org/lkml/2013/3/4/70 Signed-off-by: Kees Cook keesc...@chromium.org --- v2: - added missing #include, thanks to minipli - built with allmodconfig [...snip...] diff --git a/include/linux/crypto.h b/include/linux/crypto.h index d45e949699ea..d14230f6e977 100644 --- a/include/linux/crypto.h +++ b/include/linux/crypto.h @@ -26,6 +26,13 @@ #include linux/uaccess.h /* + * Autoloaded crypto modules should only use a prefixed name to avoid allowing + * arbitrary modules to be loaded. + */ +#define MODULE_ALIAS_CRYPTO(name) \ + MODULE_ALIAS(crypto- name) This would break userland relying on the old aliases, e.g. 'modprobe aes' no longer works. Why not have both aliases, one with the crypto- prefix for on-demand loading within the crypto API and one without for manual loading from userland? E.g., something like this: #define MODULE_ALIAS_CRYPTO(name) \ MODULE_ALIAS(name); \ MODULE_ALIAS(crypto- name) That would prevent the userland breakage and still achieves the goal of restricting the request_module() call offered by the means of the AF_ALG API. That was my intention originally, and I should go back to it. The trouble is with the use of __UNIQUE_ID in the MODULE_ALIAS macro. It uses __LINE__ to produce the id, so the suggested macro expansion (which is what I started with) won't work on non-gcc compilers. I haven't found any solutions for C89 version of gcc's __COUNTER__, and I haven't found any C89 ways to force a macro to be expanded as being multi-line. Well, clang should support it as well, according to [1]. But still, a compiler independent solution would be nice. Anyway, the __COUNTER__ support is gcc = 4.3 only. So, according to Documentation/Changes, stating gcc 3.2 is the minimum supported version for compiling the kernel, this would be a no-go, too. [1] http://clang.llvm.org/docs/LanguageExtensions.html#builtin-macros I'd like to avoid having to open-code both MODULE_ALIAS and MODULE_ALIAS_CRYPTO in each module's source. Anyone see some sneaky way to accomplish this? Unfortunately, I do not ... beside this, maybe: #define MODULE_ALIAS_CRYPTO(name) \ __MODULE_INFO(alias, alias_userland, name); \ __MODULE_INFO(alias, alias_crypto, crypto- name) Looks ugly, but works. ;) Regards, Mathias -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] crypto: prefix module autoloading with crypto-
On Tue, Nov 18, 2014 at 3:12 PM, Mathias Krause mini...@googlemail.com wrote: On 18 November 2014 01:45, Kees Cook keesc...@chromium.org wrote: On Mon, Nov 17, 2014 at 3:20 PM, Mathias Krause mini...@googlemail.com wrote: On 17 November 2014 21:02, Kees Cook keesc...@chromium.org wrote: This prefixes all crypto module loading with crypto- so we never run the risk of exposing module auto-loading to userspace via a crypto API, as demonstrated by Mathias Krause: https://lkml.org/lkml/2013/3/4/70 Signed-off-by: Kees Cook keesc...@chromium.org --- v2: - added missing #include, thanks to minipli - built with allmodconfig [...snip...] diff --git a/include/linux/crypto.h b/include/linux/crypto.h index d45e949699ea..d14230f6e977 100644 --- a/include/linux/crypto.h +++ b/include/linux/crypto.h @@ -26,6 +26,13 @@ #include linux/uaccess.h /* + * Autoloaded crypto modules should only use a prefixed name to avoid allowing + * arbitrary modules to be loaded. + */ +#define MODULE_ALIAS_CRYPTO(name) \ + MODULE_ALIAS(crypto- name) This would break userland relying on the old aliases, e.g. 'modprobe aes' no longer works. Why not have both aliases, one with the crypto- prefix for on-demand loading within the crypto API and one without for manual loading from userland? E.g., something like this: #define MODULE_ALIAS_CRYPTO(name) \ MODULE_ALIAS(name); \ MODULE_ALIAS(crypto- name) That would prevent the userland breakage and still achieves the goal of restricting the request_module() call offered by the means of the AF_ALG API. That was my intention originally, and I should go back to it. The trouble is with the use of __UNIQUE_ID in the MODULE_ALIAS macro. It uses __LINE__ to produce the id, so the suggested macro expansion (which is what I started with) won't work on non-gcc compilers. I haven't found any solutions for C89 version of gcc's __COUNTER__, and I haven't found any C89 ways to force a macro to be expanded as being multi-line. Well, clang should support it as well, according to [1]. But still, a compiler independent solution would be nice. Anyway, the __COUNTER__ support is gcc = 4.3 only. So, according to Documentation/Changes, stating gcc 3.2 is the minimum supported version for compiling the kernel, this would be a no-go, too. [1] http://clang.llvm.org/docs/LanguageExtensions.html#builtin-macros Yeah, it's the avr32 auto builder that throws the warnings, so I assume it's using a pre-4.3 gcc. I'd like to avoid having to open-code both MODULE_ALIAS and MODULE_ALIAS_CRYPTO in each module's source. Anyone see some sneaky way to accomplish this? Unfortunately, I do not ... beside this, maybe: #define MODULE_ALIAS_CRYPTO(name) \ __MODULE_INFO(alias, alias_userland, name); \ __MODULE_INFO(alias, alias_crypto, crypto- name) Looks ugly, but works. ;) AAh! Yes, that's perfect! I will spin it this way. Thank you thank you! -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/10] crypto: AF_ALG: add user space interface for AEAD
Am Dienstag, 18. November 2014, 22:06:31 schrieb Herbert Xu: Hi Herbert, On Sun, Nov 16, 2014 at 03:23:50AM +0100, Stephan Mueller wrote: AEAD requires the following data in addition to normal symmetric ciphers: * Associated authentication data of arbitrary length * Authentication tag for decryption * Length of authentication tag for encryption The authentication tag data is communicated as part of the actual ciphertext as mandated by the kernel crypto API. Therefore we only need to provide a user space interface for the associated authentication data as well as for the authentication tag length. This patch adds both as a setsockopt interface that is identical to the AF_ALG interface for setting an IV and for selecting the cipher operation type (encrypt or decrypt). Signed-off-by: Stephan Mueller smuel...@chronox.de I don't like the fact that we're putting arbitrary limits on the AD, as well as the fact that the way you're doing it the AD has to be copied. How about simply saying that the first X bytes of the input shall be the AD? That is a very good idea. To cover that approach, the kernel needs to be informed about the length of the authentication data size to separate the ciphertext/plaintext from the authentication data. To cover that, I would recommend to simply send a u32 value to the kernel for the AD size instead of the AD. The kernel then can adjust the pointers as necessary. I will update the patch in the next days to cover that scenario. Thanks -- Ciao Stephan -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/10] crypto: AF_ALG: user space interface for cipher info
Am Dienstag, 18. November 2014, 22:08:23 schrieb Herbert Xu: Hi Herbert, On Sun, Nov 16, 2014 at 03:24:25AM +0100, Stephan Mueller wrote: The AF_ALG interface allows normal cipher (hash, encrypt, decrypt). However, it does not allow user space to obtain the following generic information about the currently active cipher: * block size of the cipher * IV size of the cipher * for AEAD, the maximum authentication tag size The patch adds a getsockopt interface for the symmetric ciphers to answer such information requests from user space. The kernel crypto API function calls are used to obtain the real data. As all data are simple integer values, the getsockopt handler function uses put_user() to return the integer value to user space in the *optval parameter of getsockopt. Signed-off-by: Stephan Mueller smuel...@chronox.de We already have crypto_user so you should be extending that to cover what's missing. Looking into that, I think nothing is missing -- thanks for the pointer. Thus I think I can withdraw that patch and just simply update libkcapi to use that user space netlink interface. Though, I yet have to try using that interface ;-) I am wondering why cryptouser.h is in include/linux -- shouldn't it be in include/uapi/linux? Aren't the definitions in that header file needed for userspace to talk to the netlink socket? I guess that is also the reason why I do not see the interface API details in /usr/linux on my F21 system. PS These paramters should not vary depending on the implementation, if they do then one of the implementations must be buggy. Cheers, -- Ciao Stephan -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/10] crypto: AF_ALG: user space interface for cipher info
On Wed, Nov 19, 2014 at 02:02:33AM +0100, Stephan Mueller wrote: I am wondering why cryptouser.h is in include/linux -- shouldn't it be in include/uapi/linux? Aren't the definitions in that header file needed for userspace to talk to the netlink socket? I guess that is also the reason why I It should be. I think crypto_user predates the uapi split so that's probably why it's in the wrong place. Cheers, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/10] crypto: AF_ALG: make setkey optional
Am Dienstag, 18. November 2014, 22:10:13 schrieb Herbert Xu: Hi Herbert, On Sun, Nov 16, 2014 at 03:26:58AM +0100, Stephan Mueller wrote: The current AF_ALG implementation requires that a userspace interface implementation must provide a callback for setkey. Such a call is not appliable to random number generators. To prepare AF_ALG for the addition of a random number generator user space interface, this function callback invocation is made optional. Signed-off-by: Stephan Mueller smuel...@chronox.de Did you actually try this? AFAICS setkey is already optional. You are correct. I tested the kernel without my patch and the setkey on the RNG handle is rejected. I now also see the check already present in the alg_setkey function. This patch will be removed from a new patchset. Cheers, -- Ciao Stephan -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/10] crypto: AF_ALG: add user space interface for AEAD
Am Dienstag, 18. November 2014, 22:06:31 schrieb Herbert Xu: Hi Herbert, On Sun, Nov 16, 2014 at 03:23:50AM +0100, Stephan Mueller wrote: AEAD requires the following data in addition to normal symmetric ciphers: * Associated authentication data of arbitrary length * Authentication tag for decryption * Length of authentication tag for encryption The authentication tag data is communicated as part of the actual ciphertext as mandated by the kernel crypto API. Therefore we only need to provide a user space interface for the associated authentication data as well as for the authentication tag length. This patch adds both as a setsockopt interface that is identical to the AF_ALG interface for setting an IV and for selecting the cipher operation type (encrypt or decrypt). Signed-off-by: Stephan Mueller smuel...@chronox.de I don't like the fact that we're putting arbitrary limits on the AD, as well as the fact that the way you're doing it the AD has to be copied. How about simply saying that the first X bytes of the input shall be the AD? When looking deeper into skcipher_sendmsg, I see that the input data is copied into the kernel using memcpy_fromiovec. The memory is allocated before the memcpy call by skcipher_alloc_sgl. The AD is input data and therefore needs to be copied into the kernel just like plaintext. Of course it is possible to require user space to concatenate the AD with the plaintext (or ciphertext in case of decryption). However, I see the following drawbacks when we do that: - either user space has to do a very good memory allocation or it has to copy the data into the buffer before the plaintext. Also, if the plaintext buffer is not allocated in the right way, even the plaintext has to be copied to the newly created buffer that concatenates the AD with the plaintext. So, unless user space is very careful, at least some memcpy must be done in user space to accommodate the requirement that AD and plaintext is concatenated. - The kernel function of skcipher_sendmsg would now become very complicated, because a similar logic currently applied to plaintext needs to be also implemented to copy and track the initial AD into the kernel. However I see your point as the sendmsg approach to handle AD currently implements two memcpy() calls: one is the copy_from_user of the sendmsg system call framework to copy in msg and then the memcpy in skcipher_sendmsg. To avoid the cluttering of the skcipher_sendmsg function (which already is complex), may I propose that a setsockopt call is used just like the SET_IV call? When using the setsockopt call, I see the following advantages: - only one memcpy (the copy_from_user) is needed to move the data into kernel land -- this would be then en-par with the skcipher_sendmsg approach. - the implementation of the setsockopt approach would be very clean and small as just one copy_from_user call is to be made followed by a aead_request_set_assoc call. - user space memory handling is very clean as user space does not need a very specific memory setup to deliver AD. So, if the memory layout is not as specific as needed for the AEAD call, with the setsockopt approach, we do not need additional memcpy calls in user space. In any case, we need to set some upper boundary for the maximum size of the AD. As I do not know of any standardized upper limit for the AD size, I would consider the standard CAVP testing requirements. These tests have the maximum AD size of 116. When using the setsockopt call approach we can allocate the in-kernel AD memory at the setsockopt call time. IMHO it would be save now to limit the maximum AD size to 116 at this point. Cheers, -- Ciao Stephan -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/10] crypto: AF_ALG: add user space interface for AEAD
On Wed, Nov 19, 2014 at 05:20:42AM +0100, Stephan Mueller wrote: When looking deeper into skcipher_sendmsg, I see that the input data is copied into the kernel using memcpy_fromiovec. The memory is allocated before the memcpy call by skcipher_alloc_sgl. Zero-copy is done through sendpage. Cheers, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/10] crypto: AF_ALG: add user space interface for AEAD
Am Mittwoch, 19. November 2014, 12:27:04 schrieb Herbert Xu: Hi Herbert, On Wed, Nov 19, 2014 at 05:20:42AM +0100, Stephan Mueller wrote: When looking deeper into skcipher_sendmsg, I see that the input data is copied into the kernel using memcpy_fromiovec. The memory is allocated before the memcpy call by skcipher_alloc_sgl. Zero-copy is done through sendpage. I am slightly at a loss here -- if you could give me a hint on how you think it can be implemented, I would be grateful. Let us assume the AD || plaintext buffer is known to the kernel, either through sendpage or sendmsg. The entire buffer is split into chunks of scatterlists with ctx-tsgl. After processing one scatterlist from ctx-tsgl, that scatterlist is released via skcipher_pull_sgl. Now, for AD, we need to consider: - AD can span multiple ctx-tsgl chunks - these AD scatterlist chunks cannot be released after a normal encryption operation. The associated data must be available for multiple operations. So, while plaintext data is still flowing in, we need to keep operating with the same AD. Thus I am wondering how the rather static nature of the AD can fit with the dynamic nature of the plaintext given the current implementation on how plaintext is handled in the kernel. To me, AD in league with an IV considering its rather static nature. Having said that, the IV is also not transported via the plaintext interface, but via a setsockopt. Shouldn't the AD be handled the same way? Cheers, -- Ciao Stephan -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
