Re: crypto: FIPS 200 mode

[Stephan Mueller]

Am Dienstag, dem 30.03.2021 um 15:26 -0700 schrieb Randy Dunlap:
> 
> The Kconfig help text for CRYPTO_FIPS says
> 
> config CRYPTO_FIPS
> bool "FIPS 200 compliance"
> ...
> help
>   This option enables the fips boot option which is
>   required if you want the system to operate in a FIPS 200
>   certification.  You should say no unless you know what
>   this is.
> 
> This seems confusing to me since it says "compliance" in one place and
> "certification" in another place. And AFAICT, those two words don't
> mean the same thing as far as NIST & FIPS are concerned.
> 
> 
> Should it say "compliance" in both places?  E.g.
> 
> help
>   This option enables the fips boot option which is
>   required if you want the system to operate in FIPS 200
>   compliance mode.  You should say no unless you know what
>   this is.

Sounds good to me.

Ciao
Stephan
> 
> 
> thanks.

Re: [PATCH v2] Documentation: crypto: add info about "fips=" boot option

[Stephan Mueller]

Am Dienstag, dem 30.03.2021 um 15:44 -0700 schrieb Eric Biggers:
> On Tue, Mar 30, 2021 at 09:38:55AM -0700, Randy Dunlap wrote:
> > On 3/29/21 10:29 PM, Eric Biggers wrote:
> > > On Mon, Mar 29, 2021 at 10:06:51PM -0700, Randy Dunlap wrote:
> > > > Having just seen a report of using "fips=1" on the kernel command
> > > > line,
> > > > I could not find it documented anywhere, so add some help for it.
> > > > 
> > > > Signed-off-by: Randy Dunlap 
> > > > Cc: Dexuan Cui 
> > > > Cc: linux-cry...@vger.kernel.org
> > > > Cc: Eric Biggers 
> > > > Cc: Herbert Xu 
> > > > Cc: "David S. Miller" 
> > > > Cc: Jonathan Corbet 
> > > > Cc: linux-...@vger.kernel.org
> > > > ---
> > > > Updates/corrections welcome.
> > > > 
> > > > v2: drop comment that "fips_enabled can cause some tests to be
> > > > skipped".
> > > > 
> > > >  Documentation/admin-guide/kernel-parameters.txt |   14 ++
> > > >  1 file changed, 14 insertions(+)
> > > > 
> > > > --- linux-next-20210329.orig/Documentation/admin-guide/kernel-
> > > > parameters.txt
> > > > +++ linux-next-20210329/Documentation/admin-guide/kernel-
> > > > parameters.txt
> > > > @@ -1370,6 +1370,20 @@
> > > > See Documentation/admin-guide/sysctl/net.rst
> > > > for
> > > > fb_tunnels_only_for_init_ns
> > > >  
> > > > +   fips=   Format: { 0 | 1}
> > > > +   Use to disable (0) or enable (1) FIPS mode.
> > > > +   If enabled, any process that is waiting on the
> > > > +   'fips_fail_notif_chain' will be notified of
> > > > fips
> > > > +   failures.
> > > > +   This setting can also be modified via sysctl
> > > > at
> > > > +   /proc/sysctl/crypto/fips_enabled, i.e.,
> > > > +   crypto.fips_enabled.
> > > > +   If fips_enabled = 1 and a test fails, it will
> > > > cause a
> > > > +   kernel panic.
> > > > +   If fips_enabled = 1, RSA test requires a key
> > > > size of
> > > > +   2K or larger.
> > > > +   It can also effect which ECC curve is used.
> > > 
> > > This doesn't really explain why anyone would want to give this option.
> > > What high-level thing is this option meant to be accomplishing?
> > > That's what the documentation should explain.
> > 
> > Yes, clearly, even to me.
> > 
> > But I could not find anything in the kernel source tree that would help me
> > explain that.  So to repeat:
> > 
> > > > Updates/corrections welcome.
> > 
> > thanks.
> > -- 
> 
> I'm by no means an expert on this, but the main thing I have in mind is that
> (IIUC) the "fips" option is only useful if your whole kernel binary is
> certified
> as a "FIPS cryptographic module", *and* you actually need the FIPS
> compliance.
> And the upstream kernel doesn't have a FIPS certification out of the box;
> that's
> a task for specific Linux distributors like Red Hat, SUSE, Ubuntu, who get
> specific kernel binaries certified.
> 
> So, compiling a kernel and using the "fips" option is useless by itself, as
> your
> kernel image won't actually have a FIPS certification in that case anyway.
> 
> So, I would expect an explanation like that about under what circumstances
> the
> "fips" option is actually useful and intended for.
> 
> The people who actually use this option should be able to explain it
> properly
> though; the above is just my understanding...


The fips=1 flag serves the following purposes:

In-kernel:

- it restricts crypto algos to those which are marked as .fips_allowed in the
testmgr.c

- it causes the panic() if the signature verification of a KO providing a
crypto algo implementation fails

- it causes a specific behavior in driver/char/random.c (which was correct
till 4.8 but then got modified - patches to correct it in current kernels were
ignored)

- elevates the priority of crypto/drbg.c to ensure that when using stdrng the
DRBG is invoked

- ensures that the Jitter RNG is allocated as one seed source for
crypto/drbg.c

In user space:

- Various crypto libraries (OpenSSL, GnuTLS, libgcrypt, NSS) use the flag as
the trigger point to enable their FIPS-compliance with the goal to have one
central "knob" that enables the FIPS mode system-wide

- The boot system (e.g. dracut) starts its FIPS work (see dracut-fips).

Ciao
Stephan
> 
> - Eric

Re: [PATCH 3/5] crypto: add RFC5869 HKDF

[Stephan Mueller]

Am Mittwoch, dem 06.01.2021 um 23:30 -0800 schrieb Eric Biggers:
> On Mon, Jan 04, 2021 at 10:49:13PM +0100, Stephan Müller wrote:
> > RFC5869 specifies an extract and expand two-step key derivation
> > function. The HKDF implementation is provided as a service function that
> > operates on a caller-provided HMAC cipher handle.
> 
> HMAC isn't a "cipher".
> 
> > The extract function is invoked via the crypto_hkdf_setkey call.
> 
> Any reason not to call this crypto_hkdf_extract(), to match the
> specification?

I named it to match the other KDF implementation. But you are right, I will
name it accordingly.

> 
> > RFC5869
> > allows two optional parameters to be provided to the extract operation:
> > the salt and additional information. Both are to be provided with the
> > seed parameter where the salt is the first entry of the seed parameter
> > and all subsequent entries are handled as additional information. If
> > the caller intends to invoke the HKDF without salt, it has to provide a
> > NULL/0 entry as first entry in seed.
> 
> Where does "additional information" for extract come from?  RFC 5869 has:
> 
> HKDF-Extract(salt, IKM) -> PRK
> 
> Inputs:
>   salt optional salt value (a non-secret random value);
>    if not provided, it is set to a string of HashLen
> zeros.
>   IKM  input keying material
> 
> There's no "additional information".

I used the terminology from SP800-108. I will update the description
accordingly. 
> 
> > 
> > The expand function is invoked via the crypto_hkdf_generate and can be
> > invoked multiple times. This function allows the caller to provide a
> > context for the key derivation operation. As specified in RFC5869, it is
> > optional. In case such context is not provided, the caller must provide
> > NULL / 0 for the info / info_nvec parameters.
> 
> Any reason not to call this crypto_hkdf_expand() to match the specification?

I will update the function name.

Thanks
Stephan
> 
> - Eric

Re: [PATCH 5/5] fs: use HKDF implementation from kernel crypto API

[Stephan Mueller]

Am Mittwoch, dem 06.01.2021 um 23:19 -0800 schrieb Eric Biggers:
> On Mon, Jan 04, 2021 at 10:50:49PM +0100, Stephan Müller wrote:
> > As the kernel crypto API implements HKDF, replace the
> > file-system-specific HKDF implementation with the generic HKDF
> > implementation.
> > 
> > Signed-off-by: Stephan Mueller 
> > ---
> >  fs/crypto/Kconfig   |   2 +-
> >  fs/crypto/fscrypt_private.h |   4 +-
> >  fs/crypto/hkdf.c    | 108 +---
> >  3 files changed, 30 insertions(+), 84 deletions(-)
> > 
> > diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> > index a5f5c30368a2..9450e958f1d1 100644
> > --- a/fs/crypto/Kconfig
> > +++ b/fs/crypto/Kconfig
> > @@ -2,7 +2,7 @@
> >  config FS_ENCRYPTION
> > bool "FS Encryption (Per-file encryption)"
> > select CRYPTO
> > -   select CRYPTO_HASH
> > +   select CRYPTO_HKDF
> > select CRYPTO_SKCIPHER
> > select CRYPTO_LIB_SHA256
> > select KEYS
> > diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> > index 3fa965eb3336..0d6871838099 100644
> > --- a/fs/crypto/fscrypt_private.h
> > +++ b/fs/crypto/fscrypt_private.h
> > @@ -304,7 +304,7 @@ struct fscrypt_hkdf {
> > struct crypto_shash *hmac_tfm;
> >  };
> >  
> > -int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key,
> > +int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, u8 *master_key,
> >   unsigned int master_key_size);
> 
> It shouldn't be necessary to remove const here.

Unfortunately it is when adding the pointer to struct kvec
> 
> >  
> >  /*
> > @@ -323,7 +323,7 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const
> > u8 *master_key,
> >  #define HKDF_CONTEXT_INODE_HASH_KEY7 /* info=   */
> >  
> >  int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
> > -   const u8 *info, unsigned int infolen,
> > +   u8 *info, unsigned int infolen,
> > u8 *okm, unsigned int okmlen);
> 
> Likewise.  In fact some callers rely on 'info' not being modified.

Same here.
> 
> > -/*
> > + *
> >   * Compute HKDF-Extract using the given master key as the input keying
> > material,
> >   * and prepare an HMAC transform object keyed by the resulting
> > pseudorandom key.
> >   *
> >   * Afterwards, the keyed HMAC transform object can be used for HKDF-
> > Expand many
> >   * times without having to recompute HKDF-Extract each time.
> >   */
> > -int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key,
> > +int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, u8 *master_key,
> >   unsigned int master_key_size)
> >  {
> > +   /* HKDF-Extract (RFC 5869 section 2.2), unsalted */
> > +   const struct kvec seed[] = { {
> > +   .iov_base = NULL,
> > +   .iov_len = 0
> > +   }, {
> > +   .iov_base = master_key,
> > +   .iov_len = master_key_size
> > +   } };
> > struct crypto_shash *hmac_tfm;
> > -   u8 prk[HKDF_HASHLEN];
> > int err;
> >  
> > hmac_tfm = crypto_alloc_shash(HKDF_HMAC_ALG, 0, 0);
> > @@ -74,16 +65,12 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const
> > u8 *master_key,
> > return PTR_ERR(hmac_tfm);
> > }
> >  
> > -   if (WARN_ON(crypto_shash_digestsize(hmac_tfm) != sizeof(prk))) {
> > +   if (WARN_ON(crypto_shash_digestsize(hmac_tfm) != HKDF_HASHLEN)) {
> > err = -EINVAL;
> > goto err_free_tfm;
> > }
> >  
> > -   err = hkdf_extract(hmac_tfm, master_key, master_key_size, prk);
> > -   if (err)
> > -   goto err_free_tfm;
> > -
> > -   err = crypto_shash_setkey(hmac_tfm, prk, sizeof(prk));
> > +   err = crypto_hkdf_setkey(hmac_tfm, seed, ARRAY_SIZE(seed));
> > if (err)
> > goto err_free_tfm;
> 
> It's weird that the salt and key have to be passed in a kvec.
> Why not just have normal function parameters like:
> 
> int crypto_hkdf_setkey(struct crypto_shash *hmac_tfm,
>    const u8 *key, size_t keysize,
>    const u8 *salt, size_t saltsize);

I wanted to have an identical interface for all types of KDFs to allow turning
them into a template eventually. For example,

Re: [PATCH 0/5] Add KDF implementations to crypto API

[Stephan Mueller]

Am Montag, dem 04.01.2021 um 14:20 -0800 schrieb Eric Biggers:
> On Mon, Jan 04, 2021 at 10:45:57PM +0100, Stephan Müller wrote:
> > The HKDF addition is used to replace the implementation in the filesystem
> > crypto extension. This code was tested by using an EXT4 encrypted file
> > system that was created and contains files written to by the current
> > implementation. Using the new implementation a successful read of the
> > existing files was possible and new files / directories were created
> > and read successfully. These newly added file system objects could be
> > successfully read using the current code. Yet if there is a test suite
> > to validate whether the invokcation of the HKDF calculates the same
> > result as the existing implementation, I would be happy to validate
> > the implementation accordingly.
> 
> See https://www.kernel.org/doc/html/latest/filesystems/fscrypt.html#tests
> for how to run the fscrypt tests.  'kvm-xfstests -c ext4 generic/582' should
> be
> enough for this, though you could run all the tests if you want.

I ran the $(kvm-xfstests -c encrypt -g auto) on 5.11-rc2 with and without my
HKDF changes. I.e. the testing shows the same results for both kernels which
seems to imply that my HKDF changes do not change the behavior.

I get the following errors in both occasions - let me know if I should dig a
bit more.


[failed, exit status 1] [06:19:21]- output mismatch (see
/results/ext4/results-encrypt/ext4/023.out.bad)
--- tests/ext4/023.out  2020-03-20 02:31:32.0 +
+++ /results/ext4/results-encrypt/ext4/023.out.bad  2021-01-07
06:19:21.292339438 +
@@ -1,3 +1,2 @@
 QA output created by 023
 Format and populate
-Mount
...
(Run 'diff -u /root/xfstests/tests/ext4/023.out /results/ext4/results-
encrypt/ext4/023.out.bad'  to see the entire )

[failed, exit status 1] [06:19:28]- output mismatch (see
/results/ext4/results-encrypt/ext4/028.out.bad)
--- tests/ext4/028.out  2020-03-20 02:31:32.0 +
+++ /results/ext4/results-encrypt/ext4/028.out.bad  2021-01-07
06:19:28.762339424 +
@@ -1,3 +1,2 @@
 QA output created by 028
 Format and mount
-Compare fsmap
...
(Run 'diff -u /root/xfstests/tests/ext4/028.out /results/ext4/results-
encrypt/ext4/028.out.bad'  to see the entire )

[failed, exit status 1] [06:21:02]- output mismatch (see
/results/ext4/results-encrypt/ext4/044.out.bad)
--- tests/ext4/044.out  2020-03-20 02:31:32.0 +
+++ /results/ext4/results-encrypt/ext4/044.out.bad  2021-01-07
06:21:02.215672727 +
@@ -1,2 +1,5 @@
 QA output created by 044
 Silence is golden
+mount: /vdc: wrong fs type, bad option, bad superblock on /dev/vdc,
missing codepage or helper program, or other e.
+ext3 mount failed
+(see /results/ext4/results-encrypt/ext4/044.full for details)
...
(Run 'diff -u /root/xfstests/tests/ext4/044.out /results/ext4/results-
encrypt/ext4/044.out.bad'  to see the entire )


generic/085 [06:32:40][  849.654788] run fstests generic/085 at
2021-01-07 06:32:40
[  849.903286] EXT4-fs (vdd): Test dummy encryption mode enabled
[  849.915355] EXT4-fs (vdd): mounted filesystem with ordered data mode. Opts:
acl,user_xattr,block_validity,test_dummy.
[  850.267282] dm-0: detected capacity change from 524288 to 0
[  850.369101] EXT4-fs (dm-0): mounted filesystem with ordered data mode.
Opts: (null). Quota mode: none.
[  850.370106] ext4 filesystem being mounted at /vdc supports timestamps until
2038 (0x7fff)
[  850.479981] EXT4-fs (dm-0): mounted filesystem with ordered data mode.
Opts: (null). Quota mode: none.
[  850.480782] ext4 filesystem being mounted at /vdc supports timestamps until
2038 (0x7fff)
[  850.530734] BUG: kernel NULL pointer dereference, address: 0058
[  850.531241] #PF: supervisor read access in kernel mode
[  850.531613] #PF: error_code(0x) - not-present page
[  850.532020] PGD 2a496067 P4D 2a496067 PUD 0 
[  850.532336] Oops:  [#1] SMP NOPTI
[  850.532604] CPU: 1 PID: 19542 Comm: dmsetup Not tainted 5.11.0-rc2-xfstests
#8
[  850.533156] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.14.0-1.fc33 04/01/2014
[  850.533780] RIP: 0010:thaw_bdev+0x47/0x90
[  850.534106] Code: 8b 83 d8 04 00 00 85 c0 74 57 83 e8 01 45 31 e4 85 c0 89
83 d8 04 00 00 7f 2d 48 8b bb 80 05 00 007
[  850.535447] RSP: 0018:b97586c2bcd8 EFLAGS: 00010286
[  850.535822] RAX:  RBX: 9df4a2e74240 RCX:
b97586c2bbdc
[  850.536361] RDX: 9df4fdc17e80 RSI: 9df4a2e74790 RDI:
9df48b0bf000
[  850.536864] RBP: 9df4a2e74720 R08:  R09:
00040216
[  850.537410] R10:  R11:  R12:

[  850.537950] R13:  R14: 0006 R15:
0001
[  850.538455] FS:  () GS:9df4fdc0(0063)
knlGS:f7a487c0
[  850.539063] CS:  0010 DS: 002b ES: 

Re: drivers/char/random.c needs a (new) maintainer

[Stephan Mueller]

Am Mittwoch, dem 23.12.2020 um 15:32 +0100 schrieb Jason A. Donenfeld:
> 
> I would, however, be interested in a keccak-based construction. But
> just using the keccak permutation does not automatically make it
> "SHA-3", so we're back at the same issue again. FIPS is simply not
> interesting for our requirements.

Your requirements? Interesting approach.

Using non-assessed cryptography? Sounds dangerous to me even though it may be
based on some well-known construction.

I thought Linux in general and crypto in particular is about allowing user (or
the vendor) to decide about the used algorithm. So, let us have a mechanism
that gives them this freedom.

Thus the proposed idea sounds to me like a dangerous proposition upon which
almost all cryptography shall rest. This will surely invite even more
fragmentation.

Ciao
Stephan

PS: This entire discussion is NOT about the crypto side of the random numbers,
but about how get the entropy for the random numbers.

Re: [PATCH v4 4/5] crypto: hisilicon/hpre - add 'ECDH' algorithm

[Stephan Mueller]

Am Mittwoch, dem 16.12.2020 um 10:39 +0800 schrieb yumeng:
> 
> 
> 
> > Am Freitag, den 11.12.2020, 14:30 +0800 schrieb Meng Yu:
> > > 
> > > +/* size in bytes of the n prime */
> > > +#define HPRE_ECC_NIST_P128_N_SIZE  16
> > 
> > Do we truly need P-128? Besides, I do not see that curve being defined in
> > contemporary cipher specs.
> > 
> > > +#define HPRE_ECC_NIST_P192_N_SIZE  24
> > > +#define HPRE_ECC_NIST_P224_N_SIZE  28
> > > +#define HPRE_ECC_NIST_P256_N_SIZE  32
> > > +#define HPRE_ECC_NIST_P320_N_SIZE  40
> > 
> > Do we truly need P-320? Besides, I do not see that curve being defined in
> > contemporary cipher specs.
> 
> Yes, in rfc 5903, only P-256, P-384 and P-521 is defined, but in
> 'rfc5639' and  "SEC 2: Recommended Elliptic Curve Domain Parameters",
> other curves like P-128, P-192, P-224, and P-320 curve parameters are
> found, and they are used in 'openssl';
> How about your idea?

Who is going to use that curve considering that common protocols that are
implemented in the kernel do not use it?

Thanks
Stephan

Re: [PATCH v4 5/5] crypto: hisilicon/hpre - add 'CURVE25519' algorithm

[Stephan Mueller]

Am Freitag, den 11.12.2020, 14:30 +0800 schrieb Meng Yu:
>   
> +/* curve25519 */
> +static u64 curve25519_g_x[] = { 0x0009, 0x,
> +   0x, 0x };
> +static u64 curve25519_p[] = { 0xffed, 0x,
> +   0x, 0x7fff };
> +static u64 curve25519_a[] = { 0x0001DB41, 0x,
> +   0x, 0x };
> +static const struct ecc_curve ecc_25519 = {
> +   .name = "curve25519",
> +   .g = {
> +   .x = curve25519_g_x,
> +   .ndigits = 4,
> +   },
> +   .p = curve25519_p,
> +   .a = curve25519_a,
> +};

With this definition, I am not sure whether ecc_is_pubkey_valid_partial would
work correctly. At least it *seems* that there would be a NULL-pointer
dereference in vli_add with the undefined .b value. Did you test and can you
confirm?

Thanks
Stephan

Re: [PATCH v4 4/5] crypto: hisilicon/hpre - add 'ECDH' algorithm

[Stephan Mueller]

Am Freitag, den 11.12.2020, 14:30 +0800 schrieb Meng Yu:
> 
> +/* size in bytes of the n prime */
> +#define HPRE_ECC_NIST_P128_N_SIZE  16

Do we truly need P-128? Besides, I do not see that curve being defined in
contemporary cipher specs.

> +#define HPRE_ECC_NIST_P192_N_SIZE  24
> +#define HPRE_ECC_NIST_P224_N_SIZE  28
> +#define HPRE_ECC_NIST_P256_N_SIZE  32
> +#define HPRE_ECC_NIST_P320_N_SIZE  40

Do we truly need P-320? Besides, I do not see that curve being defined in
contemporary cipher specs.

> +#define HPRE_ECC_NIST_P384_N_SIZE  48
> +#define HPRE_ECC_NIST_P521_N_SIZE  66
> +
> +/* size in bytes */
> +#define HPRE_ECC_HW256_KSZ_B   32
> +#define HPRE_ECC_HW384_KSZ_B   48
> +#define HPRE_ECC_HW576_KSZ_B   72
> +
> +#define HPRE_ECDH_MAX_SZ   HPRE_ECC_HW576_KSZ_B]

Ciao
Stephan

Re: [PATCH v3 4/5] crypto: hisilicon/hpre - add 'ECDH' algorithm

[Stephan Mueller]

Am Mittwoch, den 18.11.2020, 11:47 +0800 schrieb Meng Yu:

Hi Meng,

> Enable 'ECDH' algorithm in Kunpeng 930.
> 
> Signed-off-by: Meng Yu 
> Reviewed-by: Zaibo Xu 
> ---
>  drivers/crypto/hisilicon/hpre/hpre.h    |   2 +-
>  drivers/crypto/hisilicon/hpre/hpre_crypto.c | 802
> +++-
>  drivers/crypto/hisilicon/hpre/hpre_main.c   |   1 +
>  3 files changed, 800 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/crypto/hisilicon/hpre/hpre.h
> b/drivers/crypto/hisilicon/hpre/hpre.h
> index 02193e1..50e6b2e 100644
> --- a/drivers/crypto/hisilicon/hpre/hpre.h
> +++ b/drivers/crypto/hisilicon/hpre/hpre.h
> @@ -83,6 +83,7 @@ enum hpre_alg_type {
> HPRE_ALG_KG_CRT = 0x3,
> HPRE_ALG_DH_G2 = 0x4,
> HPRE_ALG_DH = 0x5,
> +   HPRE_ALG_ECC_MUL = 0xD,
>  };
>  
>  struct hpre_sqe {
> @@ -104,5 +105,4 @@ struct hisi_qp *hpre_create_qp(u8 type);
>  int hpre_algs_register(struct hisi_qm *qm);
>  void hpre_algs_unregister(struct hisi_qm *qm);
>  
> -
>  #endif
> diff --git a/drivers/crypto/hisilicon/hpre/hpre_crypto.c
> b/drivers/crypto/hisilicon/hpre/hpre_crypto.c
> index 712bea9..b7814ce 100644
> --- a/drivers/crypto/hisilicon/hpre/hpre_crypto.c
> +++ b/drivers/crypto/hisilicon/hpre/hpre_crypto.c
> @@ -2,6 +2,7 @@
>  /* Copyright (c) 2019 HiSilicon Limited. */
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -36,6 +37,342 @@ struct hpre_ctx;
>  #define HPRE_DFX_SEC_TO_US 100
>  #define HPRE_DFX_US_TO_NS  1000
>  
> +/* HPRE support 7 curves (include curve P192 and P256 in ecdh.h) */
> +#define HPRE_ECC_CURVE_NIST_P128   0X0003
> +#define HPRE_ECC_CURVE_NIST_P320   0X0004
> +#define HPRE_ECC_CURVE_NIST_P384   0X0005
> +#define HPRE_ECC_CURVE_NIST_P521   0X0006
> +#define HPRE_ECC_CURVE_NIST_P224   0X0007
> +
> +/* size in bytes of the n prime */
> +#define HPRE_ECC_NIST_P128_N_SIZE  16
> +#define HPRE_ECC_NIST_P192_N_SIZE  24
> +#define HPRE_ECC_NIST_P224_N_SIZE  28
> +#define HPRE_ECC_NIST_P256_N_SIZE  32
> +#define HPRE_ECC_NIST_P320_N_SIZE  40
> +#define HPRE_ECC_NIST_P384_N_SIZE  48
> +#define HPRE_ECC_NIST_P521_N_SIZE  66
> +
> +/* size in bytes */
> +#define HPRE_ECC_HW256_KSZ_B   32
> +#define HPRE_ECC_HW384_KSZ_B   48
> +#define HPRE_ECC_HW576_KSZ_B   72
> +
> +#define HPRE_ECDH_MAX_SZ   HPRE_ECC_HW576_KSZ_B
> +
> +struct curve_param_desc {
> +   __u32 id;
> +   const unsigned char *p;
> +   const unsigned char *a;
> +   const unsigned char *b;
> +   const unsigned char *gx;
> +   const unsigned char *gy;
> +   const unsigned char *n;
> +};
> +
> +/* ECC CURVE PARAMS */
> +/* 128 bits */
> +static const unsigned char ecdh_p128_p[] = {
> +   0xFF, 0xFF, 0xFF, 0xFD, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> 0xFF,
> +   0xFF, 0xFF, 0xFF, 0xFF
> +};
> +static const unsigned char ecdh_p128_a[] = {
> +   0xFF, 0xFF, 0xFF, 0xFD, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> 0xFF,
> +   0xFF, 0xFF, 0xFF, 0xFC
> +};
> +static const unsigned char ecdh_p128_b[] = {
> +   0xE8, 0x75, 0x79, 0xC1, 0x10, 0x79, 0xF4, 0x3D, 0xD8, 0x24, 0x99,
> 0x3C,
> +   0x2C, 0xEE, 0x5E, 0xD3
> +};
> +static const unsigned char ecdh_p128_x[] = {
> +   0x16, 0x1F, 0xF7, 0x52, 0x8B, 0x89, 0x9B, 0x2D, 0x0C, 0x28, 0x60,
> 0x7C,
> +   0xA5, 0x2C, 0x5B, 0x86
> +};
> +static const unsigned char ecdh_p128_y[] = {
> +   0xcf, 0x5a, 0xc8, 0x39, 0x5b, 0xaf, 0xeb, 0x13, 0xc0, 0x2d, 0xa2,
> 0x92,
> +   0xdd, 0xed, 0x7a, 0x83
> +};
> +static const unsigned char ecdh_p128_n[] = {
> +   0xFF, 0xFF, 0xFF, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x75, 0xA3, 0x0D,
> 0x1B,
> +   0x90, 0x38, 0xA1, 0x15
> +};
> +
> +/* 192 bits */
> +static const unsigned char ecdh_p192_p[] = {
> +   0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> 0xFF,
> +   0xFF, 0xFF, 0xFF, 0xFE, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> 0xFF
> +};
> +static const unsigned char ecdh_p192_a[] = {
> +   0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> 0xFF,
> +   0xFF, 0xFF, 0xFF, 0xFE, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> 0xFC
> +};
> +static const unsigned char ecdh_p192_b[] = {
> +   0x64, 0x21, 0x05, 0x19, 0xE5, 0x9C, 0x80, 0xE7, 0x0F, 0xA7, 0xE9,
> 0xAB,
> +   0x72, 0x24, 0x30, 0x49, 0xFE, 0xB8, 0xDE, 0xEC, 0xC1, 0x46, 0xB9,
> 0xB1
> +};
> +static const unsigned char ecdh_p192_x[] = {
> +   0x18, 0x8D, 0xA8, 0x0E, 0xB0, 0x30, 0x90, 0xF6, 0x7C, 0xBF, 0x20,
> 0xEB,
> +   0x43, 0xA1, 0x88, 0x00, 0xF4, 0xFF, 0x0A, 0xFD, 0x82, 0xFF, 0x10,
> 0x12
> +};
> +static const unsigned char ecdh_p192_y[] = {
> +   0x07, 0x19, 0x2b, 0x95, 0xff, 0xc8, 0xda, 0x78, 0x63, 0x10, 0x11,
> 0xed,
> +   0x6b, 0x24, 0xcd, 0xd5, 0x73, 0xf9, 0x77, 0xa1, 0x1e, 0x79, 0x48,
> 0x11
> +};
> +static const unsigned char ecdh_p192_n[] = {
> +   0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> 0xFF,
> +  

Re: [PATCH v36 00/13] /dev/random - a new approach

[Stephan Mueller]

Am Montag, 19. Oktober 2020, 21:28:50 CET schrieb Stephan Müller:

Hi,
> 
> * Performance
> 
>  - Faster by up to 75% in the critical code path of the interrupt handler
>depending on data collection size configurable at kernel compile time -
>the default is about equal in performance with existing /dev/random as
>outlined in [2] section 4.2.

By streamlining the implementation a bit, the LRNG interrupt handler now 
operates about 130% faster than the existing /dev/random (average of 97 cycles 
of the existing /dev/random code vs. an average of 42 cycles of the LRNG). 
This fast operation is the default now due to patch [2]. The conceptual data 
handling outlined in [3] section 2.2 remains unchanged.

Even the addition of health tests applied to the noise source data would still 
result in a faster interrupt handling code (average of 97 cycles of the 
existing /dev/random code vs on average 78 cycles of the LRNG).

[1] https://github.com/smuellerDD/lrng/commit/
10b74b242950371273e38df78060e258d9d3ea40

[2] https://github.com/smuellerDD/lrng/commit/
383b087653c21cf20984f5508befa57e96f685ba

[3] https://chronox.de/lrng/doc/lrng.pdf

Ciao
Stephan

Re: jitterentropy: `jent_mod_init()` takes 17 ms

[Stephan Mueller]

Am Dienstag, 10. November 2020, 10:37:02 CET schrieb Paul Menzel:

Hi Paul,

> Dear Stephan,
> 
> 
> Thank you for the quick reply.
> 
> Am 10.11.20 um 10:25 schrieb Stephan Mueller:
> > Am Montag, 9. November 2020, 20:31:02 CET schrieb Paul Menzel:
> >> By mistake I built `XFRM_ESP` into the Linux kernel, resulting in
> >> 
> >>   CONFIG_CRYPTO_SEQIV=y
> >>   CONFIG_CRYPTO_ECHAINIV=y
> >> 
> >> and also the Jitterentropy RNG to be built in.
> >> 
> >>   CRYPTO_JITTERENTROPY=y
> >> 
> >> So, on the Asus F2A85-M PRO starting Linux 4.10-rc3 with
> >> `initcall_debug`, the init method is run unconditionally, and it takes
> >> 17.5 ms, which is over ten percent of the overall 900 ms the Linux
> > 
> > Hm, 17.5 / 900 = 2%, or am I missing something?
> 
> Indeed, that is embarrassing. My bad.
> 
> >> kernel needs until loading the init process.
> >> 
> >>   [0.300544] calling  jent_mod_init+0x0/0x2c @ 1
> >>   [0.318438] initcall jent_mod_init+0x0/0x2c returned 0 after
> >>   17471 usecs
> >> 
> >> Looking at the output of systemd-bootchart, it looks like, that this
> >> indeed delayed the boot a little, as the other init methods seem to be
> >> ordered after it.
> >> 
> >> I am now building it as a module, but am wondering if the time can be
> >> reduced to below ten milliseconds.
> > 
> > What you see is the test whether the Jitter RNG has a proper noise source.
> > The function jent_entropy_init() is the cause of the operation. It
> > performs 1024 times a test to validate the appropriateness of the noise
> > source. You can adjust that with the TESTLOOPCOUNT in this function. But
> > I am not sure adjusting is a wise course of action.
> 
> Out of curiosity, why 1024 and not, for example, 128 or 2048? Is there
> some statistics behind it?

See [1] section 4.3 bullet 4 is the culprit. The startup test includes the 
referenced test logic.

[1] https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-90B.pdf
> 
> 
> Kind regards,
> 
> Paul


Ciao
Stephan

Re: jitterentropy: `jent_mod_init()` takes 17 ms

[Stephan Mueller]

Am Montag, 9. November 2020, 20:31:02 CET schrieb Paul Menzel:

Hi Paul,

> Dear Linux folks,
> 
> 
> By mistake I built `XFRM_ESP` into the Linux kernel, resulting in
> 
>  CONFIG_CRYPTO_SEQIV=y
>  CONFIG_CRYPTO_ECHAINIV=y
> 
> and also the Jitterentropy RNG to be built in.
> 
>  CRYPTO_JITTERENTROPY=y
> 
> So, on the Asus F2A85-M PRO starting Linux 4.10-rc3 with
> `initcall_debug`, the init method is run unconditionally, and it takes
> 17.5 ms, which is over ten percent of the overall 900 ms the Linux

Hm, 17.5 / 900 = 2%, or am I missing something?

> kernel needs until loading the init process.
> 
>  [0.300544] calling  jent_mod_init+0x0/0x2c @ 1
>  [0.318438] initcall jent_mod_init+0x0/0x2c returned 0 after
> 17471 usecs
> 
> Looking at the output of systemd-bootchart, it looks like, that this
> indeed delayed the boot a little, as the other init methods seem to be
> ordered after it.
> 
> I am now building it as a module, but am wondering if the time can be
> reduced to below ten milliseconds.

What you see is the test whether the Jitter RNG has a proper noise source. The 
function jent_entropy_init() is the cause of the operation. It performs 1024 
times a test to validate the appropriateness of the noise source. You can 
adjust that with the TESTLOOPCOUNT in this function. But I am not sure 
adjusting is a wise course of action.

Ciao
Stephan

Re: [DISCUSSION PATCH 00/41] random: possible ways towards NIST SP800-90B compliance

[Stephan Mueller]

Am Mittwoch, 7. Oktober 2020, 06:24:09 CEST schrieb Eric Biggers:

Hi Eric,
> 
> Note that having multiple RNG implementations would cause fragmentation,
> more maintenance burden, etc.  So IMO, that should be a last resort. 
> Instead we should try to find an implementation that works for everyone. 
> I.e., at least to me, Nicolai's patchset seems more on the right track than
> Stephan's patchset...

Thank you for sharing your considerations.

If you say that only one implementation should be there, I am wondering why 
not considering an implementation that as significant advantages over the 
existing implementation as outlined in my cover letter to patch v35. In the 
default configuration, it compiles no code at all that has any bearing on 
government standards. Yet it has a more cryptographic sound approach to handle 
entropy. In addition is meant to be extensible allowing each user to pick and 
chose what he wants. Yet, users who do not want these extensions should not 
suffer from it (neither performance-wise, nor should they suffer from an 
unnecessary complex code that builds all options into one C file).

And speaking of fragmentation, if it is not *possible* to allow users to pick 
what they want and need (and yes, in some parts of the world or for some users 
these government standards are simply a necessity), we surely invite 
fragmentation. In the LRNG, I tried to have all operations critical to entropy 
compression and random number generation modularized so that the a can be 
replaced or extended if needed without fragmentation.

PS: The reason why I started the LRNG was not government standards, but the 
result of performing two studies. The one study was about entropy in 
virtualized environment which showed that we have significant entropy in 
virtual environments and yet the existing /dev/random implementation thinks 
there is much less available. Another study I maintain for years also shows 
that the entire entropy collection and heuristic on bare metal systems is also 
in need of advancements. Initially I provided patches to the existing /dev/
random implementation, but basically all were silently ignored.

Ciao
Stephan

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Stephan Mueller]

Am Montag, 5. Oktober 2020, 08:44:39 CEST schrieb Ard Biesheuvel:

Hi Ard,

> On Mon, 5 Oct 2020 at 08:40, Stephan Mueller  wrote:
> > Am Montag, 5. Oktober 2020, 08:24:46 CEST schrieb Ard Biesheuvel:
> > 
> > Hi Ard,
> > 
> > > If jitterentropy is a special case, we could put a alternate
> > > non-'static inline' version of random_get_entropy() in the core
> > > kernel, and only export it if JITTER_ENTROPY is built as a module in
> > > the first place. But I'd prefer it if jitterentropy switches to an API
> > > that is suitable for driver consumption.
> > 
> > Which API do you have in mind? In user space, I use
> > clock_gettime(CLOCK_REALTIME) which also considers the clock source.
> 
> AFAICT, that call is backed by ktime_get_real_ts64(), which is already
> being exported to modules.
> 
> Could you please check whether that works for your driver?

Yes, will do. Thanks.

Ciao
Stephan

Re: [PATCH] crypto: jitterentropy - bind statically into kernel

[Stephan Mueller]

Am Montag, 5. Oktober 2020, 08:24:46 CEST schrieb Ard Biesheuvel:

Hi Ard,

> If jitterentropy is a special case, we could put a alternate
> non-'static inline' version of random_get_entropy() in the core
> kernel, and only export it if JITTER_ENTROPY is built as a module in
> the first place. But I'd prefer it if jitterentropy switches to an API
> that is suitable for driver consumption.

Which API do you have in mind? In user space, I use 
clock_gettime(CLOCK_REALTIME) which also considers the clock source.

Thanks
Stephan

Re: [DISCUSSION PATCH 00/41] random: possible ways towards NIST SP800-90B compliance

[Stephan Mueller]

Am Freitag, 2. Oktober 2020, 15:15:55 CEST schrieb Willy Tarreau:

Hi Willy,

> > And this is all ???
> 
> Possibly a lot of people got used to seeing the numerous versions
> and are less attentive to new series, it's possible that your message
> will wake everyone up.

I think that points to my patch series. My patch series which provide a 
complete separate, API and ABI compliant drop in replacement of /dev/random, 
nobody from the gatekeepers cared to even answer. It would not touch the 
existing code.

After waiting some time without changing the code (e.g. after Andi Lutomirski 
commented), I got no answer at all from the gatekeepers, not even any 
indication in what direction I should move if something was not desired in the 
patch series.

Thus I continued adding the features that I think are necessary and for which 
I received comments from mathematicians. What else should I do?

With the patch set v35 of my patch series, I see all my goals finally 
achieved at I expect the code to be stable from here on. The last one was the 
hardest: to get rid of all non-cryptographic conditioning operations and yet 
retain performance en par or even superior to the existing /dev/random 
implementation.

Ciao
Stephan

Re: get_cycles from modular code in jitterentropy, was Re: [PATCH] clocksource: clint: Export clint_time_val for modules

[Stephan Mueller]

Am Freitag, 2. Oktober 2020, 08:49:05 CEST schrieb Christoph Hellwig:

Hi Christoph,

> On Tue, Sep 29, 2020 at 11:56:18PM -0700, Palmer Dabbelt wrote:
> > clint_time_val will soon be used by the RISC-V implementation of
> > random_get_entropy(), which is a static inline function that may be used
> > by
> > modules (at least CRYPTO_JITTERENTROPY=m).
> 
> At very least this needs to be an EXPORT_SYMBOL_GPL.  But I really don't
> think modules have any business using get_cycles, so I'd much rather
> fix CRYPTO_JITTERENTROPY to be required to be build in.

Changing CRYPTO_JITTERENTROPY from tistate to bool should be no problem.

I will provide a patch.

Ciao
Stephan

Re: [DISCUSSION PATCH 00/41] random: possible ways towards NIST SP800-90B compliance

[Stephan Mueller]

Am Montag, 21. September 2020, 09:58:16 CEST schrieb Nicolai Stange:

Hi Nicolai,

> Hi all,
> 
> first of all, my apologies for the patch bomb following up in reply to this
> mail here -- it's not meant to receive any serious review at all, but only
> to support the discussion I'm hoping to get going.

Thank you for this effort!
> 
> As some of you might already be aware of, all new submissions for FIPS
> certification will be required to comply with NIST SP800-90B from Nov 7th
> on ([1], sec. 7.18 "Entropy Estimation and Compliance with SP 800-90B").
> For reference: broadly speaking, NIST SP800-90B is about noise sources,
> SP800-90A about the DRBG algorithms stacked on top and SP800-90C about how
> everything is supposed to be glued together. The main requirements from
> SP800-90B are
> - no correlations between different noise sources,
> - to continuously run certain health tests on a noise source's output and
> - to provide an interface enabling access to the raw noise samples for
>   validation purposes.
> 
> To my knowledge, all SP800-90B compliant noise sources available on Linux
> today are either based on the Jitter RNG one way or another or on
> architectural RNGs like e.g. x86's RDSEED or arm64's RNDRRS. Currently,
> there's an in-kernel Jitter RNG implementation getting registered (c.f.
> crypto/drbg.c, (*)) with the Crypto RNG API, which is also accessible from
> userspace via AF_ALG. The userspace haveged ([2]) or jitterentropy
> integrations ([3]) are worth mentioning in this context, too. So in
> summary, I think that for the in-kernel entropy consumers falling under the
> scope of FIPS, the currently only way to stay compliant would be to draw it
> from said Crypto API RNG. For userspace applications there's the additional
> option to invoke haveged and alike.
> 
> OTOH, CPU jitter based techniques are not uncontroversial ([4]). In any
> case, it would certainly be a good idea to mix (xor or whatever) any jitter
> output with entropy obtained from /dev/random (**). If I'm not mistaken,
> the mentioned Crypto API RNG implementation (crypto/drbg.c) follows exactly
> this approach, but doesn't enforce it yet: there's no
> wait_for_random_bytes() and early DRBG invocations could in principle run
> on seeds dominated entirely by jitterentropy. However, this can probably
> get sorted quite easily and thus, one reasonable way towards maintaining
> FIPS resp. SP800-90 compliance would be to
> - make crypto/drbg.c invoke wait_for_random_bytes(),
> - make all relevant in-kernel consumers to draw their random numbers from
>   the Crypto RNG API, if not already the case and
> - convert all relevant userspace to use a SP800-90B conforming Jitter RNG
>   style noise source for compliance reasons, either by invoking the
>   kernel's Crypto RNG API or by diffent means, and mix that with
>   /dev/random.
> 
> Even though this would probably be feasible, I'm not sure that giving up on
> /dev/random being the primary, well established source of randomness in
> favor of each and every userspace crypto library rolling its own entropy
> collection scheme is necessarily the best solution (it might very well be
> though).
> 
> An obvious alternative would be to make /dev/random conform to SP800-90B.
> Stephan Müller posted his "LRNG" patchset ([5]), in which he proposed to
> introduce a second, independent implementation aiming at SP800-90[A-C]
> conformance. However, it's in the 35th iteration now and my impression is
> that there's hardly any discussion happening around this for quite a while
> now. I haven't followed the earlier development, but I can imagine several
> reasons for that:
> - people are not really interested in FIPS or even questioning the whole
>   concept in the first place (c.f. Theodore Ts'o remarks on this topic
>   at [6]),
> - potential reviewers got merely discouraged by the diffstat or

Maybe I followed the Linux principle a bit to much here? Release early, 
release often.

But with the v35, all goals I tried to achieve are now in (namely the last was 
to get rid of any non-cryptographic conditioning functions) and to have a very 
clean data processing / entropy analysis. I do not expect big changes any 
more.

> - people dislike the approach of having two competing implementations for
>   what is basically the same functionality in the kernel.

Is this really so bad considering the security implications on this topic? We 
also have multiple file systems, multiple memory allocators, etc...
> 
> In either case, I figured it might perhaps help further discussion to
> provide at least a rough idea of how bad the existing /dev/random
> implementation would get cluttered when worked towards SP800-90B
> compliance. So I implemented the required health tests for the interrupt
> noise source -- the resulting patches can be found in reply to this mail.
> I'd like to stress(!) that this should really only be considered a first
> step and that there would still be a long way towards a complete 

Re: [PATCH v35 01/13] Linux Random Number Generator

[Stephan Mueller]

Am Freitag, 18. September 2020, 15:02:17 CEST schrieb kernel test robot:

Hi,

> All errors (new ones prefixed by >>):
> >> drivers/char/lrng/lrng_chacha20.c:33:8: error: structure variable
> >> 'chacha20' with 'latent_entropy' attribute has a non-integer field
> >> 'block'
>   33 | struct chacha20_state chacha20 __latent_entropy;
> 
>  |^~
> 
> #
> https://github.com/0day-ci/linux/commit/ecb964754fd80cca434d6d2ad6db8f28a15
> 92fa1 git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review
> Stephan-M-ller/dev-random-a-new-approach/20200918-181505 git checkout
> ecb964754fd80cca434d6d2ad6db8f28a1592fa1
> vim +33 drivers/char/lrng/lrng_chacha20.c
> 
> 27
> 28/*
> 29 * Have a static memory blocks for the ChaCha20 DRNG instance to
> avoid calling 30   * kmalloc too early in the boot cycle. For 
subsequent
> allocation requests, 31* such as per-NUMA-node DRNG instances, 
kmalloc
> will be used. 32   */
> 
>   > 33struct chacha20_state chacha20 __latent_entropy;

I do not think this report is valid. The following definitions apply:

struct chacha20_state {
struct chacha20_block block;
};

struct chacha20_block {
u32 constants[4];
union {
#define CHACHA_KEY_SIZE_WORDS (CHACHA_KEY_SIZE / sizeof(u32))
u32 u[CHACHA_KEY_SIZE_WORDS];
u8  b[CHACHA_KEY_SIZE];
} key;
u32 counter;
u32 nonce[3];
};


This implies that struct chacha20_state and thus the chacha20 variable is a 
linear buffer with in total 4 + 8 + 1 + 3  = 16 32-bit integers which are at 
least aligned on a 32-bit boundary and are designated as u32 integers.

Please let me know if I need to make a tweak to the definitions to convince 
the code analyzer it is a flat linear buffer consisting of integers and thus 
to understand the structure correctly.

Thanks a lot.

Ciao
Stephan

Re: [PATCH v33 01/12] Linux Random Number Generator

[Stephan Mueller]

Am Mittwoch, 26. August 2020, 16:27:42 CEST schrieb kernel test robot:

Hi,

> >> drivers/char/lrng/lrng_chacha20.c:54:47: sparse: sparse: cast to
> >> restricted __le32
>drivers/char/lrng/lrng_chacha20.c:58:47: sparse: sparse: cast to
> restricted __le32 --

Thanks for the reports. Both, the __poll_t and the __le32 issues are fixed in 
v34.

Ciao
Stephan

Re: [PATCH v34 01/12] Linux Random Number Generator

[Stephan Mueller]

Am Dienstag, 25. August 2020, 13:28:53 CEST schrieb kernel test robot:

Hi,

> All warnings (new ones prefixed by >>):
> >> drivers/char/lrng/lrng_drng.c:381:6: warning: no previous prototype for
> >> 'lrng_reset' [-Wmissing-prototypes]
>  381 | void lrng_reset(void)
> 
>  |  ^~

The prototype is covered in an ifdef in lrng_internal.h as it is only needed 
for a specific configuration. I have moved the prototype out of that 
configuration conditional now.

Thanks.

Ciao
Stephan

Re: [PATCH] crypto: drbg: check blocklen is non zero

[Stephan Mueller]

Am Sonntag, 2. August 2020, 19:12:47 CEST schrieb t...@redhat.com:

Hi Tom,

> From: Tom Rix 
> 
> Clang static analysis reports this error
> 
> crypto/drbg.c:441:40: warning: Division by zero
> padlen = (inputlen + sizeof(L_N) + 1) % (drbg_blocklen(drbg));
>  ~^~~
> 
> When drbg_bocklen fails it returns 0.
> 
>   if (drbg && drbg->core)
>   return drbg->core->blocklen_bytes;
>   return 0;
> 
> In many places in drbg_ctr_df drbg_bocklen is assumed to be non zero.
> So turn the assumption into a check.
> 
> Fixes: 541af946fe13 ("crypto: drbg - SP800-90A Deterministic Random Bit
> Generator")
> 
> Signed-off-by: Tom Rix 

Thank you.

Reviewed-by: Stephan Mueller 

Ciao
Stephan

Re: [PATCH v31 00/12] /dev/random - a new approach with full SP800-90B

[Stephan Mueller]

Am Dienstag, 28. Juli 2020, 22:40:44 CEST schrieb Pavel Machek:

Hi Pavel,

> Hi!
> 
> > The following patch set provides a different approach to /dev/random which
> > is called Linux Random Number Generator (LRNG) to collect entropy within
> > the Linux kernel. The main improvements compared to the existing
> > /dev/random is to provide sufficient entropy during boot time as well as
> > in virtual environments and when using SSDs. A secondary design goal is
> > to limit the impact of the entropy collection on massive parallel systems
> > and also allow the use accelerated cryptographic primitives. Also, all
> > steps of the entropic data processing are testable.
> 
> That sounds good.. maybe too good. Where does LRNG get the entropy? That is
> the part that should be carefully documented..
> 
>   Pavel

The entire description of the LRNG is given in [1].

[1] section 2.1 outlines the general architecture specifying that there are 
currently 3 noise sources. Per default, the interrupt-based noise source is 
the main source.

Section 2.4 outlines the details of the interrupt noise source handling. The 
key now is unlike the existing implementation that there is no separate block/
HID noise collection because they are "just" derivatives of the interrupt 
noise source which would imply that noise events are double credited with 
entropy. This allows for a massively higher valuation of the entropy rate that 
exists in interrupt events.

To support the design, a large scale noise source analysis is performed in 
chapter 3 [1]. Specifically sections 3.2.3 and 3.2.4 provide quantitative 
statements which are further analyzed in subsequent sections. This includes 
reboot tests as well as runtime tests.

[1] appendix C performs these measurements on other CPU architectures, 
including very small environments which could be expected to have too little 
entropy (specifically the first listed ARM system mentioned there and the MIPS 
system are older embedded devices that yet show sufficient entropy). Also, the 
entropy available in virtual environments is shown in appendix C.

The tools perform the aforementioned measurements are provided with the 
enabling of CONFIG_LRNG_RAW_ENTROPY supported by [2]. This allows everybody to 
re-perform the analysis on the system of his choice.

Also, the entire entropy assessment of the LRNG is supported by the entropy 
analysis of the existing implementation in [3]. Specifically section 6.1 shows 
that the existing implementation has much more entropy available in the 
interrupt events than it credits. Yet, due to the design of the existing 
implementation with the fast pool (for which we have no assessment how much 
entropy is lost by it) and the fact of double counting of entropy with HID/
block devices, the massive underestimation of existing entropy with the 
existing /dev/random implementation is warranted.

Lastly, [4] performs the entropy assessment of the existing /dev/random 
implementation in virtualized environments showing that still sufficient 
entropy is available in interrupt events supporting the approach taken in the 
LRNG. Writing the assessment of [4] was the initial trigger point for me to 
start the LRNG implementation.

The second noise source that, however, is credited much less entropy is 
documented in [5] including its entropy assessment.

[1] https://chronox.de/lrng/doc/lrng.pdf

[2] https://chronox.de/lrng/lrng-tests-20200415.tar.xz

[3] https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/Publications/Studies/
LinuxRNG/LinuxRNG_EN.pdf?__blob=publicationFile

[4] https://www.bsi.bund.de/SharedDocs/Downloads/DE/BSI/Publikationen/Studien/
ZufallinVMS/Randomness-in-VMs.pdf?__blob=publicationFile

[5] https://chronox.de/jent/doc/CPU-Jitter-NPTRNG.pdf

Ciao
Stephan

Re: [PATCH v2 13/14] crypto: sun8i-ce: Add support for the PRNG

[Stephan Mueller]

Am Montag, 15. Juni 2020, 15:02:53 CEST schrieb LABBE Corentin:

Hi,


> I still dont see any memset_secure in kzfree (mm/slab_common.c).
> Does I miss something ?

Nope, you do not miss anything, it seems that the patch that I had seen did 
not go in.
> 
> Regards


Ciao
Stephan

Re: [PATCH v2] crypto: DRBG - always try to free Jitter RNG instance

[Stephan Mueller]

Am Freitag, 5. Juni 2020, 08:16:46 CEST schrieb Eric Biggers:

Hi Eric,

> On Fri, Jun 05, 2020 at 07:58:15AM +0200, Stephan Mueller wrote:
> > Am Freitag, 5. Juni 2020, 02:43:36 CEST schrieb Eric Biggers:
> > 
> > Hi Eric,
> > 
> > > On Thu, Jun 04, 2020 at 08:41:00AM +0200, Stephan Müller wrote:
> > > > The Jitter RNG is unconditionally allocated as a seed source follwoing
> > > > the patch 97f2650e5040. Thus, the instance must always be deallocated.
> > > > 
> > > > Reported-by: syzbot+2e635807decef724a...@syzkaller.appspotmail.com
> > > > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B
> > > > ...")
> > > > Signed-off-by: Stephan Mueller 
> > > > ---
> > > > 
> > > >  crypto/drbg.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/crypto/drbg.c b/crypto/drbg.c
> > > > index 37526eb8c5d5..8a0f16950144 100644
> > > > --- a/crypto/drbg.c
> > > > +++ b/crypto/drbg.c
> > > > @@ -1631,6 +1631,9 @@ static int drbg_uninstantiate(struct drbg_state
> > > > *drbg)>
> > > > 
> > > > if (drbg->random_ready.func) {
> > > > 
> > > > del_random_ready_callback(>random_ready);
> > > > cancel_work_sync(>seed_work);
> > > > 
> > > > +   }
> > > > +
> > > > +   if (!IS_ERR_OR_NULL(drbg->jent)) {
> > > > 
> > > > crypto_free_rng(drbg->jent);
> > > > drbg->jent = NULL;
> > > > 
> > > > }
> > > 
> > > It it okay that ->jent can be left as an ERR_PTR() value?
> > > 
> > > Perhaps it should always be set to NULL?
> > 
> > The error value is used in the drbg_instantiate function. There it is
> > checked whether -ENOENT (i.e. the cipher is not available) or any other
> > error is present. I am not sure we should move that check.
> > 
> > Thanks for the review.
> 
> drbg_seed() and drbg_async_seed() check for drbg->jent being NULL.
> 
> Will that now break due it drbg->jent possibly being an ERR_PTR()?
> 
> Hence why I'm asking whether drbg_uninstantiate() should set it to NULL.

The allocation happens in drbg_prepare_hrng that is only invoked by 
drbg_instantiate.

drbg_instantiate checks for the ERR_PTR and sets it to NULL in case the error 
is deemed ok.

Thus, any subsequent functions would see either a valid pointer or NULL. The 
only exception is drbg_uninstantiate when invoked from the error case 

ret = drbg_prepare_hrng(drbg);
if (ret)
goto free_everything;

Thus, I think that the two functions you mention will never see any values 
other than NULL or a valid pointer.

Thanks


Ciao
Stephan

Re: [PATCH v2] crypto: DRBG - always try to free Jitter RNG instance

[Stephan Mueller]

Am Freitag, 5. Juni 2020, 02:43:36 CEST schrieb Eric Biggers:

Hi Eric,

> On Thu, Jun 04, 2020 at 08:41:00AM +0200, Stephan Müller wrote:
> > The Jitter RNG is unconditionally allocated as a seed source follwoing
> > the patch 97f2650e5040. Thus, the instance must always be deallocated.
> > 
> > Reported-by: syzbot+2e635807decef724a...@syzkaller.appspotmail.com
> > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
> > Signed-off-by: Stephan Mueller 
> > ---
> > 
> >  crypto/drbg.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/crypto/drbg.c b/crypto/drbg.c
> > index 37526eb8c5d5..8a0f16950144 100644
> > --- a/crypto/drbg.c
> > +++ b/crypto/drbg.c
> > @@ -1631,6 +1631,9 @@ static int drbg_uninstantiate(struct drbg_state
> > *drbg)> 
> > if (drbg->random_ready.func) {
> > 
> > del_random_ready_callback(>random_ready);
> > cancel_work_sync(>seed_work);
> > 
> > +   }
> > +
> > +   if (!IS_ERR_OR_NULL(drbg->jent)) {
> > 
> > crypto_free_rng(drbg->jent);
> > drbg->jent = NULL;
> > 
> > }
> 
> It it okay that ->jent can be left as an ERR_PTR() value?
> 
> Perhaps it should always be set to NULL?

The error value is used in the drbg_instantiate function. There it is checked 
whether -ENOENT (i.e. the cipher is not available) or any other error is 
present. I am not sure we should move that check.

Thanks for the review.
> 
> - Eric


Ciao
Stephan

Re: [PATCH] crypto: DRBG - always try to free Jitter RNG instance

[Stephan Mueller]

Am Mittwoch, 3. Juni 2020, 13:09:19 CEST schrieb Dan Carpenter:

Hi Dan,

> On Wed, Jun 03, 2020 at 10:08:56AM +0200, Stephan Müller wrote:
> > The Jitter RNG is unconditionally allocated as a seed source follwoing
> > the patch 97f2650e5040. Thus, the instance must always be deallocated.
> > 
> > Reported-by: syzbot+2e635807decef724a...@syzkaller.appspotmail.com
> > Fixes: 97f2650e5040 ("crypto: drbg - always seeded with SP800-90B ...")
> > Signed-off-by: Stephan Mueller 
> > ---
> > 
> >  crypto/drbg.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/crypto/drbg.c b/crypto/drbg.c
> > index 37526eb8c5d5..33d28016da2d 100644
> > --- a/crypto/drbg.c
> > +++ b/crypto/drbg.c
> > @@ -1631,6 +1631,9 @@ static int drbg_uninstantiate(struct drbg_state
> > *drbg)> 
> > if (drbg->random_ready.func) {
> > 
> > del_random_ready_callback(>random_ready);
> > cancel_work_sync(>seed_work);
> > 
> > +   }
> > +
> > +   if (drbg->jent) {
> > 
> > crypto_free_rng(drbg->jent);
> > drbg->jent = NULL;
> > 
> > }
> 
> free_everything functions never work.  For example, "drbg->jent" can be
> an error pointer at this point.
> 
> crypto/drbg.c
>   1577  if (!drbg->core) {
>   1578  drbg->core = _cores[coreref];
>   1579  drbg->pr = pr;
>   1580  drbg->seeded = false;
>   1581  drbg->reseed_threshold = drbg_max_requests(drbg);
>   1582
>   1583  ret = drbg_alloc_state(drbg);
>   1584  if (ret)
>   1585  goto unlock;
>   1586
>   1587  ret = drbg_prepare_hrng(drbg);
>   1588  if (ret)
>   1589  goto free_everything;
> 
> If we hit two failures inside drbg_prepare_hrng() then "drbg->jent" can
> be an error pointer.
> 
>   1590
>   1591  if (IS_ERR(drbg->jent)) {
>   1592  ret = PTR_ERR(drbg->jent);
>   1593  drbg->jent = NULL;
>   1594  if (fips_enabled || ret != -ENOENT)
>   1595  goto free_everything;
>   1596  pr_info("DRBG: Continuing without Jitter
> RNG\n"); 1597  }
>   1598
>   1599  reseed = false;
>   1600  }
>   1601
>   1602  ret = drbg_seed(drbg, pers, reseed);
>   1603
>   1604  if (ret && !reseed)
>   1605  goto free_everything;
>   1606
>   1607  mutex_unlock(>drbg_mutex);
>   1608  return ret;
>   1609
>   1610  unlock:
>   1611  mutex_unlock(>drbg_mutex);
>   1612  return ret;
>   1613
>   1614  free_everything:
>   1615  mutex_unlock(>drbg_mutex);
>   1616  drbg_uninstantiate(drbg);
>
> Leading to an Oops.

Thanks a lot for the hint - a version 2 should come shortly.
> 
>   1617  return ret;
>   1618  }
> 
> regards,
> dan carpenter


Ciao
Stephan

Re: [PATCH v2 1/2] hwrng: iproc-rng200 - Set the quality value

[Stephan Mueller]

Am Mittwoch, 20. Mai 2020, 14:00:01 CEST schrieb Krzysztof Kozlowski:

Hi Krzysztof,

> On Wed, 20 May 2020 at 13:53, Stephan Mueller  wrote:
> > > > That said, the illustrated example is typical for hardware RNGs. Yet
> > > > it is never guaranteed to work that way. Thus, if you can point to
> > > > architecture documentation of your specific hardware RNGs showing that
> > > > the data read from the hardware is pure unconditioned noise data, then
> > > > I have no objections to the patch.
> > > 
> > > I can tell for sure that this is the case for exynos-trng[1].
> > 
> > So you are saying that the output for the exynos-trng is straight from a
> > ring oscillator without any post-processing of any kind?
> 
> Hi,
> 
> I think we will never be able to state this because the manual is
> quite limited in sharing internals. What the driver does and probably
> Lukasz wanted to say is that there is "post processing" block and
> feature which can be disabled. The manual is saying the TRNG block
> generates random data from thermal noise but not how much in a direct
> way. There could be some simple post-processing or not (except the one
> able to on/off). Also manual says this post processing block is there
> to remove statistical weakness from the TRNG block. To me it does not
> prove enough that raw data is really raw...

Unterstood, but can't that statement be added to the commit message?
> 
> Best regards,
> Krzysztof


Ciao
Stephan

Re: [PATCH v2 1/2] hwrng: iproc-rng200 - Set the quality value

[Stephan Mueller]

Am Mittwoch, 20. Mai 2020, 12:44:33 CEST schrieb Lukasz Stelmach:

Hi Lukasz,

> It was <2020-05-20 śro 11:18>, when Stephan Mueller wrote:
> > Am Mittwoch, 20. Mai 2020, 11:10:32 CEST schrieb Lukasz Stelmach:
> >> It was <2020-05-20 śro 08:23>, when Stephan Mueller wrote:
> >>> Am Dienstag, 19. Mai 2020, 23:25:51 CEST schrieb Łukasz Stelmach:
> >>>> The value was estimaded with ea_iid[1] using on 10485760 bytes read
> >>>> from the RNG via /dev/hwrng. The min-entropy value calculated using
> >>>> the most common value estimate (NIST SP 800-90P[2], section 6.3.1)
> >>>> was 7.964464.
> >>> 
> >>> I am sorry, but I think I did not make myself clear: testing random
> >>> numbers post-processing with the statistical tools does NOT give any
> >>> idea about the entropy rate. Thus, all that was calculated is the
> >>> proper implementation of the post-processing operation and not the
> >>> actual noise source.
> >>> 
> >>> What needs to happen is that we need access to raw, unconditioned
> >>> data from the noise source that is analyzed with the statistical
> >>> methods.
> >> 
> >> I did understand you and I assure you the data I tested were obtained
> >> directly from RNGs. As I pointed before[1], that is how /dev/hwrng
> >> works[2].
> > 
> > I understand that /dev/hwrng pulls the data straight from the
> > hardware. But the data from the hardware usually is not obtained
> > straight from the noise source.
> > 
> > Typically you have a noise source (e.g. a ring oscillator) whose data
> > is digitized then fed into a compression function like an LFSR or a
> > hash. Then a cryptographic operation like a CBC-MAC, hash or even a
> > DRBG is applied to that data when the caller wants to have random
> > numbers.
> 
> I do understand your point (but not entirely, see below). [opinion]
> However, I am really not sure that this is a "typical" setting for a HW
> RNG, at least not among RNGs supported by Linux. Otherwise there would
> be no hw_random framework and no rngd(8) which are suppsed to
> post-process imperfectly random data from HW. [/opinion]

The hw_random framework only makes these hardware RNG accessible for in-kernel 
as well as user space use.
> 
> > In order to estimate entropy, we need the raw unconditioned data from
> > the, say, ring oscillator and not from the (cryptographic) output
> > operation.
> 
> Can you tell, why it matters in this case? If I understand correctly,
> the quality field describes not the randomness created by the noise
> generator but the one delivered by the driver to other software
> components.

The quality field is used by add_hwgenerator_randomness to increase the Linux 
RNG entropy estimator accordingly. This is the issue.

And giving an entropy rate based on post-processed data is meaningless.

The concern is, for example, that you use a DRBG that you seeded with, say, a 
zero buffer. You get perfect random data from it that no statistical test can 
disprove. Yet we know this data stream has zero entropy. Thus, we need to get 
to the source and assess its entropy.

> 
> > That said, the illustrated example is typical for hardware RNGs. Yet
> > it is never guaranteed to work that way. Thus, if you can point to
> > architecture documentation of your specific hardware RNGs showing that
> > the data read from the hardware is pure unconditioned noise data, then
> > I have no objections to the patch.
> 
> I can tell for sure that this is the case for exynos-trng[1].

So you are saying that the output for the exynos-trng is straight from a ring 
oscillator without any post-processing of any kind?

If this is the case, I would like to suggest you add that statement to the git 
commit message with that reference. If so, then I would withdraw my objection.

> There is a
> post-processor which I have forgotten about since writing the driver,
> because from the very beginning I didn't intend to use it. I knew there
> is the software framework for post-processing and simply didn't bother.
> 
> With regards to iproc-rng200 I cannot be sure.
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dri
> vers/char/hw_random/exynos-trng.c?h=v5.6#n100
> 
> Kind regards,


Ciao
Stephan

Re: [PATCH v2 1/2] hwrng: iproc-rng200 - Set the quality value

[Stephan Mueller]

Am Mittwoch, 20. Mai 2020, 11:10:32 CEST schrieb Lukasz Stelmach:

Hi Lukasz,

> It was <2020-05-20 śro 08:23>, when Stephan Mueller wrote:
> > Am Dienstag, 19. Mai 2020, 23:25:51 CEST schrieb Łukasz Stelmach:
> >> The value was estimaded with ea_iid[1] using on 10485760 bytes read from
> >> the RNG via /dev/hwrng. The min-entropy value calculated using the most
> >> common value estimate (NIST SP 800-90P[2], section 6.3.1) was 7.964464.
> > 
> > I am sorry, but I think I did not make myself clear: testing random
> > numbers
> > post-processing with the statistical tools does NOT give any idea about
> > the
> > entropy rate. Thus, all that was calculated is the proper implementation
> > of
> > the post-processing operation and not the actual noise source.
> > 
> > What needs to happen is that we need access to raw, unconditioned data
> > from
> > the noise source that is analyzed with the statistical methods.
> 
> I did understand you and I assure you the data I tested were obtained
> directly from RNGs. As I pointed before[1], that is how /dev/hwrng
> works[2].

I understand that /dev/hwrng pulls the data straight from the hardware. But 
the data from the hardware usually is not obtained straight from the noise 
source.

Typically you have a noise source (e.g. a ring oscillator) whose data is 
digitized then fed into a compression function like an LFSR or a hash. Then a 
cryptographic operation like a CBC-MAC, hash or even a DRBG is applied to that 
data when the caller wants to have random numbers.

In order to estimate entropy, we need the raw unconditioned data from the, 
say, ring oscillator and not from the (cryptographic) output operation.

That said, the illustrated example is typical for hardware RNGs. Yet it is 
never guaranteed to work that way. Thus, if you can point to architecture 
documentation of your specific hardware RNGs showing that the data read from 
the hardware is pure unconditioned noise data, then I have no objections to 
the patch.
> 
> If I am wrong, do show me the code that processes the data from a HW RNG
> before copying them to user provided buffer[3].

I am not talking about any software post-processing. I am talking about post-
processing within the hardware.
> 
> [1] https://lkml.org/lkml/2020/5/15/252
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Doc
> umentation/admin-guide/hw_random.rst?h=v5.6 [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dri
> vers/char/hw_random/core.c?h=v5.6#n251
> 
> Kind regards,


Ciao
Stephan

Re: [PATCH v2 1/2] hwrng: iproc-rng200 - Set the quality value

[Stephan Mueller]

Am Dienstag, 19. Mai 2020, 23:25:51 CEST schrieb Łukasz Stelmach:

Hi Łukasz,

> The value was estimaded with ea_iid[1] using on 10485760 bytes read from
> the RNG via /dev/hwrng. The min-entropy value calculated using the most
> common value estimate (NIST SP 800-90P[2], section 6.3.1) was 7.964464.

I am sorry, but I think I did not make myself clear: testing random numbers 
post-processing with the statistical tools does NOT give any idea about the 
entropy rate. Thus, all that was calculated is the proper implementation of 
the post-processing operation and not the actual noise source.

What needs to happen is that we need access to raw, unconditioned data from 
the noise source that is analyzed with the statistical methods.

Ciao
Stephan

Re: [PATCH 1/2] hwrng: iproc-rng200 - Set the quality value

[Stephan Mueller]

Am Freitag, 15. Mai 2020, 11:01:48 CEST schrieb Lukasz Stelmach:

Hi Lukasz,


As I mentioned, all that is or seems to be analyzed here is the quality of the 
cryptographic post-processing. Thus none of the data can be used for getting 
an idea of the entropy content.

That said, the ent value indeed looks too low which seems to be an issue in 
the tool itself.

Note, for an entropy assessment commonly at least 1 million traces from the 
raw noise source are needed.

See for examples on how such entropy assessments are conducted in the LRNG 
documentation [1] or the Linux /dev/random implementation in [2]

[1] appendix C of https://www.chronox.de/lrng/doc/lrng.pdf

[2] chapter 6 of https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/
Publications/Studies/LinuxRNG/LinuxRNG_EN.pdf

Ciao
Stephan

Re: [PATCH 1/2] hwrng: iproc-rng200 - Set the quality value

[Stephan Mueller]

Am Freitag, 15. Mai 2020, 00:18:41 CEST schrieb Lukasz Stelmach:

Hi Lukasz,
> 
> I am running tests using SP800-90B tools and the first issue I can see
> is the warning that samples contain less than 1e6 bytes of data. I know
> little about maths behind random number generators, but I have noticed
> that the bigger chunk of data from an RNG I feed into either ent or ea_iid
> the higher entropy they report. That is why I divided the data into 1024
> bit chunks in the first place. To get worse results. With ea_iid they
> get even worse (128 bytes of random data)

I read that you seem to just take the output data from the RNG. If this is 
correct, I think we can stop right here. The output of an RNG is usually after 
post-processing commonly provided by a cryptographic function.

Thus, when processing the output of the RNG all what we measure here is the 
quality of the cryptographic post-processing and not the entropy that may be 
present in the data.

What we need is to access the noise source and analyze this with the given 
tool set. And yes, the analysis may require adjusting the data to a format 
that can be consumed and analyzed by the statistical tests.

Ciao
Stephan

Re: [PATCH 1/2] hwrng: iproc-rng200 - Set the quality value

[Stephan Mueller]

Am Donnerstag, 14. Mai 2020, 21:07:33 CEST schrieb Łukasz Stelmach:

Hi Łukasz,

> The value has been estimaded by obtainig 1024 chunks of data 128 bytes
> (1024 bits) each from the generator and finding chunk with minimal
> entropy using the ent(1) tool. The value was 6.327820 bits of entropy
> in each 8 bits of data.

I am not sure we should use the ent tool to define the entropy level. Ent 
seems to use a very coarse entropy estimation.

I would feel more comfortable when using other measures like SP800-90B which 
even provides a tool for the analysis.

I understand that entropy estimates, well, are estimates. But the ent data is 
commonly not very conservative.

[1] https://github.com/usnistgov/SP800-90B_EntropyAssessment

Ciao
Stephan

Re: [PATCH 2/2] hwrng: exynos - Set the quality value

[Stephan Mueller]

Am Donnerstag, 14. Mai 2020, 21:07:34 CEST schrieb Łukasz Stelmach:

Hi Łukasz,

> The value has been estimaded by obtainig 1024 chunks of data 128 bytes
> (1024 bits) each from the generator and finding chunk with minimal
> entropy using the ent(1) tool. The value was 6.332937 bits of entropy
> in each 8 bits of data.

Dto - see the other comment.

Ciao
Stephan

Re: [PATCH 5.4 regression fix] x86/boot: Provide memzero_explicit

[Stephan Mueller]

Am Montag, 7. Oktober 2019, 11:06:04 CEST schrieb Hans de Goede:

Hi Hans,

> Hi Stephan,
> 
> On 07-10-2019 10:59, Stephan Mueller wrote:
> > Am Montag, 7. Oktober 2019, 10:55:01 CEST schrieb Hans de Goede:
> > 
> > Hi Hans,
> > 
> >> The purgatory code now uses the shared lib/crypto/sha256.c sha256
> >> implementation. This needs memzero_explicit, implement this.
> >> 
> >> Reported-by: Arvind Sankar 
> >> Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get
> >> input, memzero_explicit") Signed-off-by: Hans de Goede
> >> 
> >> ---
> >> 
> >>   arch/x86/boot/compressed/string.c | 5 +
> >>   1 file changed, 5 insertions(+)
> >> 
> >> diff --git a/arch/x86/boot/compressed/string.c
> >> b/arch/x86/boot/compressed/string.c index 81fc1eaa3229..511332e279fe
> >> 100644
> >> --- a/arch/x86/boot/compressed/string.c
> >> +++ b/arch/x86/boot/compressed/string.c
> >> @@ -50,6 +50,11 @@ void *memset(void *s, int c, size_t n)
> >> 
> >>return s;
> >>   
> >>   }
> >> 
> >> +void memzero_explicit(void *s, size_t count)
> >> +{
> >> +  memset(s, 0, count);
> > 
> > May I ask how it is guaranteed that this memset is not optimized out by
> > the
> > compiler, e.g. for stack variables?
> 
> The function and the caller live in different compile units, so unless
> LTO is used this cannot happen.

Agreed in this case.

I would just be worried that this memzero_explicit implementation is assumed 
to be protected against optimization when used elsewhere since other 
implementations of memzero_explicit are provided with the goal to be protected 
against optimizations.
> 
> Also note that the previous purgatory private (vs shared) sha256
> implementation had:
> 
>  /* Zeroize sensitive information. */
>  memset(sctx, 0, sizeof(*sctx));
> 
> In the place where the new shared 256 code uses memzero_explicit() and the
> new shared sha256 code is the only user of the
> arch/x86/boot/compressed/string.c memzero_explicit() implementation.
> 
> With that all said I'm open to suggestions for improving this.

What speaks against the common memzero_explicit implementation? If you cannot 
use it, what about adding a barrier in the memzero_explicit implementation? Or 
what about adding some compiler magic as attached to this email?


> 
> Regards,
> 
> Hans



Ciao
Stephan/*
 * Copyright (C) 2015, Stephan Mueller 
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 * 1. Redistributions of source code must retain the above copyright
 *notice, and the entire permission notice in its entirety,
 *including the disclaimer of warranties.
 * 2. Redistributions in binary form must reproduce the above copyright
 *notice, this list of conditions and the following disclaimer in the
 *documentation and/or other materials provided with the distribution.
 * 3. The name of the author may not be used to endorse or promote
 *products derived from this software without specific prior
 *written permission.
 *
 * ALTERNATIVELY, this product may be distributed under the terms of
 * the GNU General Public License, in which case the provisions of the GPL2
 * are required INSTEAD OF the above restrictions.  (This clause is
 * necessary due to a potential bad interaction between the GPL and
 * the restrictions contained in a BSD-style copyright.)
 *
 * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
 * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
 * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE, ALL OF
 * WHICH ARE HEREBY DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR BE
 * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
 * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT
 * OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
 * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
 * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
 * USE OF THIS SOFTWARE, EVEN IF NOT ADVISED OF THE POSSIBILITY OF SUCH
 * DAMAGE.
 */

#include 

/*
 * Tested following code:
 *
 * (1) __asm__ __volatile__("" : "=r" (s) : "0" (s));
 * (2) __asm__ __volatile__("": : :"memory");
 * (3) __asm__ __volatile__("" : "=r" (s) : "0" (s) : "memory");
 * (4) __asm__ __volatile__("" : : "r" (s) : "memory");
 *
 * Requred result:
 *
 * gcc -O3: objdump -d shows the following:
 *
 * 

Re: [PATCH v2 0/6] General Key Derivation Function Support

[Stephan Mueller]

Am Freitag, 8. Februar 2019, 09:05:47 CET schrieb Herbert Xu:

Hi Herbert,
> 
> > Also, shall we add the signature verification enforcemnt to the helper as
> > well?
> 
> What do you mean by that?

We need to invoke the function crypto_check_module_sig when the module is 
loaded. Do you have any concerns invoking it from the module init function?

Ciao
Stephan

Re: [PATCH v2 0/6] General Key Derivation Function Support

[Stephan Mueller]

Am Freitag, 8. Februar 2019, 08:45:58 CET schrieb Herbert Xu:

Hi Herbert,

> We could easily add self-tests for the helper.

Thanks for the clarification. And do you have a suggestion how we can should 
ensure that the self-tests are run only once?

Also, shall we add the signature verification enforcemnt to the helper as 
well?


Ciao
Stephan

Re: [PATCH v2 0/6] General Key Derivation Function Support

[Stephan Mueller]

Am Mittwoch, 30. Januar 2019, 11:08:54 CET schrieb Herbert Xu:

Hi Herbert,

> I'm still not convinced why this needs to go into the crypto API
> instead of being hosted in a helper which should achieve pretty
> much the same result.

How do you propose to handle the FIPS 140-2 related requirements of enforcing 
the integrity test result and the known-answer self tests?

Thanks
Stephan

Re: [PATCH v2 0/6] General Key Derivation Function Support

[Stephan Mueller]

Am Mittwoch, 16. Januar 2019, 12:06:54 CET schrieb Stephan Müller:

Hi Herbert,

> Changes v2:
> * Incorporation of all comments from Eric Biggers
> 
> Stephan Mueller (6):
>   crypto: add template handling for RNGs
>   crypto: kdf - SP800-108 Key Derivation Function
>   crypto: kdf - add known answer tests
>   crypto: hkdf - HMAC-based Extract-and-Expand KDF
>   crypto: hkdf - add known answer tests
>   crypto: tcrypt - add KDF test invocation
> 
>  crypto/Kconfig|  13 +
>  crypto/Makefile   |   2 +
>  crypto/hkdf.c | 272 +++
>  crypto/kdf_sp800108.c | 491 ++
>  crypto/rng.c  |  44 +++
>  crypto/tcrypt.c   |   8 +
>  crypto/testmgr.c  | 245 +
>  crypto/testmgr.h  | 198 ++
>  include/crypto/internal/rng.h |  26 ++
>  9 files changed, 1299 insertions(+)
>  create mode 100644 crypto/hkdf.c
>  create mode 100644 crypto/kdf_sp800108.c

Do you happen to have any comments on this patch set?

Ciao
Stephan

Re: [RFC PATCH 4/5] crypto: Adds user space interface for ALG_SET_KEY_TYPE

[Stephan Mueller]

Am Donnerstag, 17. Januar 2019, 08:02:20 CET schrieb Kalyani Akula:

Hi Kalyani,

> ALG_SET_KEY_TYPE requires caller to pass the key_type to be used
> for AES encryption/decryption.
> 
> Sometimes the cipher key will be stored in the
> device's hardware. So, there is a need to specify
> the information about the key to use for AES operations.
> 
> In Xilinx ZynqMP SoC, below key types are available
> 
> 1. Device key, which is flashed in the HW.
> 
> 2. PUF KEK, which can be regenerated using the
>helper data programmed in the HW.
> 
> 3. User supplied key.
> 
> So to choose the AES key to be used, this patch adds key-type attribute.

You expose your particular driver interface to user space. So, user space 
would need the details of you driver to know what to set. If another driver 
has such key type support, user space would need to know about that, too. I do 
not think this is a wise idea.

If we are going to have such a keytype selection, there must be a common user 
space interface for all drivers. I.e. define common key types the drivers then 
can map to their particular key type interface.

Besides, seem to be more a key handling issue. Wouldn't it make sense to 
rather have such issue solved with key rings than in the kernel crypto API?

Ciao
Stephan

Re: [PATCH 4/6] crypto: hkdf - RFC5869 Key Derivation Function

[Stephan Mueller]

Am Montag, 14. Januar 2019, 18:53:16 CET schrieb Eric Biggers:

Hi Eric,

> > I would not suggest this, because that rounds contrary to the concept of
> > the kernel crypto API IMHO. The caller has to provide the wrapping
> > cipher. It is perfectly viable to allow a caller to invoke a specific
> > keyed message digest.
> Sure, but it would not conform to the HKDF specification.  Are you sure it
> is okay to specify an arbitrary keyed hash?

Technically, I see no issue why this should not be possible. You see that with 
the SP800-108 KDF implementations where using CMAC is perfectly legal (and 
which I also test).

Though, using another keyed hash implementation like CMAC is not covered by 
the HKDF spec. If a caller would use hkdf(cmac(aes)), it would produce 
cryptographically strong values. Though this implementation does not conform 
to any standard. I do not think we should prevent a caller to select such 
combination in the kernel crypto API.

IMHO there would even be valid reasons why one would use cmac(aes) for a kdf. 
For example, when you would want to use a hardware AES which somehow also 
employs a hardware key that is inaccessible to software in order to tie the 
KDF result to the local hardware. This could even be a valid use case for Ext4 
FBE encryption where you derive a key. The KDF could be used to link the 
derived key to the local hardware to prevent the encrypted data could be 
copied to another system and decrypted successfully there.

Ciao
Stephan

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

[Stephan Mueller]

Am Mittwoch, 9. Januar 2019, 18:34:55 CET schrieb Eric Biggers:

Hi Eric,

> That would not meet my performance requirements as I want to precompute
> HKDF-Extract, and then do HKDF-Expand many times.  Also the HKDF-Expand part
> should be thread-safe and not require allocating memory, especially not a
> whole crypto_shash tfm every time.
> 
> So presumably with crypto_rng, crypto_rng_reset() would need to take the
> input keyring material and salt and do HKDF-Extract (like my
> fscrypt_init_hkdf()), and crypto_rng_generate() would need to take the
> application-specific info string and do HKDF-Expand (like my
> fscrypt_hkdf_expand()).

Great, that was the idea I had in mind as well. Maybe the example was not 
right to convey that. Let me work on that.
> 
> It is ugly though.  Please also consider just having simple crypto_hkdf_*()
> helper functions which wrap a HMAC tfm along the lines of my patch, rather
> than shoehorning it into the crypto_rng API.
> 
> - Eric



Ciao
Stephan

Re: [PATCH 0/5 v2][RFC] Encryption and authentication for hibernate snapshot image

[Stephan Mueller]

Am Mittwoch, 9. Januar 2019, 17:39:58 CET schrieb joeyli:

Hi joeyli,

> 
> I am doing encrypt-then-MAC.

Note, this is what the authenc() AEAD cipher does.

Ciao
Stephan

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

[Stephan Mueller]

Am Mittwoch, 9. Januar 2019, 09:21:04 CET schrieb Eric Biggers:

Hi Eric,
> 
> FWIW, it's been very slow going since I've been working on other projects
> and I also need to be very sure to get the API changes right, but I still
> plan to change the KDF in fscrypt (a.k.a. ext4/f2fs/ubifs encryption) to
> HKDF-SHA512 as part of a larger set of improvements to how fscrypt
> encryption keys are managed. I sent the last patchset a year ago
> (https://marc.info/?l=linux-fsdevel=150879493206257) but I'm working to
> revive it.  In the work-in-progress version in my git tree, this is the
> commit that adds a HKDF implementation as fs/crypto/hkdf.c:
> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/commit/?i
> d=e8a78767131c9717ee838f0c4e307948d65a4427 It basically just wraps a
> crypto_shash for "hmac(sha512)".
> 
> I'd be fine with using a common implementation instead, provided that it
> gives the same functionality, including supporting user-specified salt and
> application-specific info strings, and isn't slower or more complex to use.
> 
> (This comment is solely on the tangential discussion about KDF
> implementations; I've not looked at the hibernation image encryption stuff
> yet.)

Thanks for the clarification. I have started a generic HKDF implementation for 
the kernel crypto API which lead to the questions above. I would then also try 
to provide a HKDF proposal.

To use the (H)KDF, I currently envision 2 calls apart from alloc/free. The 
following code would serve as an example.

 * Example without proper error handling:
 *  char *keying_material = "\x00\x11\x22\x33\x44\x55\x66\x77";
 *  char *label_context = "\xde\xad\xbe\xef\x00\xde\xad\xbe\xef";
 *  kdf = crypto_alloc_rng(name, 0, 0);
 *  crypto_rng_reset(kdf, keying_material, 8);
 *  crypto_rng_generate(kdf, label_context, 9, outbuf, outbuflen);

That hopefully should be simple enough.

For HKDF, as mentioned, I would envision to use a struct instead of a char * 
for the label_context to communicate IKM, Salt, and the label/info 
information.

Ciao
Stephan

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

[Stephan Mueller]

Am Mittwoch, 9. Januar 2019, 07:58:28 CET schrieb James Bottomley:

Hi James,

> On Wed, 2019-01-09 at 07:45 +0100, Stephan Mueller wrote:
> > Am Mittwoch, 9. Januar 2019, 01:44:31 CET schrieb James Bottomley:
> > 
> > Hi James,
> > 
> > > Actually, it would be enormously helpful if we could reuse these
> > > pieces for the TPM as well.
> > 
> > Could you please help me understand whether the KDFs in TPM are
> > directly usable as a standalone cipher primitive or does it go
> > together with additional  key generation operations?
> 
> They're used as generators ... which means they deterministically
> produce keys from what the TPM calls seeds so we can get crypto agility
> of TPM 2.0 ... well KDFa does.  KDFe is simply what NIST recommends you
> do when using EC for a shared key agreement ... and really we shouldn't
> be using ECDH in the kernel without it.
> 

Thanks for clarifying. That would mean that indeed we would have hardware-
provided KDF implementations that may be usable with the kernel crypto API.

[...]
> 
> > Would it be appropriate, to implement a type cast to a structure from
> > the u8 pointer?
> > 
> > E.g. for the aforementioned label/context data, we could define
> > something like
> > 
> > struct crypto_kdf_ctr {
> > 
> > char *label;
> > size_t label_len;
> > u8 *contextU;
> > size_t contextU_len;
> > u8 *contextV;
> > size_t contextV_len;
> > 
> > };
> > 
> > And the implementation of the generate function for CTR KDF would
> > 
> > then have a  type cast along the following lines:
> > if (slen != sizeof(struct crypto_kdf_ctr))
> > 
> > return -EINVAL;
> > 
> > const struct crypto_kdf_ctr *kdf_ctr_input = (struct
> > 
> > crypto_kdf_ctr *)src;
> > 
> > 
> > For different KDFs, different structs would be needed.
> 
> Actually, we probably just need the input key (or secret material), the
> concatenation and the number of output bits.

Thanks for confirming. Though, when it comes to HKDF (not that I see it being 
needed or required in the kernel), there is a need to split up the src pointer 
since the mentioned input is used in different ways.

In order to try to get the implementation and thus the interface right, I 
would suggest to at least have a consensus on how to handle such situations.

Thus, would the proposal be acceptable for such KDFs that may need to have 
different components communicated as input to the KDF?

Ciao
Stephan

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

[Stephan Mueller]

Am Mittwoch, 9. Januar 2019, 01:44:31 CET schrieb James Bottomley:

Hi James,

> Actually, it would be enormously helpful if we could reuse these pieces
> for the TPM as well. 

Could you please help me understand whether the KDFs in TPM are directly 
usable as a standalone cipher primitive or does it go together with additional 
key generation operations?

> It has two KDFs: KDFa, which is the CTR-KDF from
> SP800-108 and KDFe which is the SP800-56A KDF for elliptic curve one
> pass Diffie Hellman, so if we're going to do the former, I'd really
> like the latter as well.
> 
> The way the TPM parametrises input to both KDFs is
> 
> (hashAlg, key, label, contextU, contextV, bits)
> 
> Where
> 
> hashAlg  = the hash algorithm used as the PRF
> key  = the input parameter of variable bit size or
>the x co-ordinate of the shared point
> label= An ASCII string representing the use
> contextU = public input U
> contextV = public input V
> bits = number of output bits

When implementing KDFs as an extension of the kernel crypto API's RNG facility 
we currently have to handle the limitiation of the existing API. The label/
context data (and when considering RFC 5869 HKDF requring IKM, salt and 
additional information as input) currently cannot directly be communicated 
through the API.

The issue is that the RNG facility currently has the following prototype 
defined:

int (*generate)(struct crypto_rng *tfm,
const u8 *src, unsigned int slen,
u8 *dst, unsigned int dlen);

The src pointer would need to take the label/context data.

Would it be appropriate, to implement a type cast to a structure from the u8 
pointer?

E.g. for the aforementioned label/context data, we could define something like

struct crypto_kdf_ctr {
char *label;
size_t label_len;
u8 *contextU;
size_t contextU_len;
u8 *contextV;
size_t contextV_len;
};

And the implementation of the generate function for CTR KDF would then have a 
type cast along the following lines:

if (slen != sizeof(struct crypto_kdf_ctr))
return -EINVAL;
const struct crypto_kdf_ctr *kdf_ctr_input = (struct crypto_kdf_ctr 
*)src;


For different KDFs, different structs would be needed.

Ciao
Stephan

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

[Stephan Mueller]

Am Mittwoch, 9. Januar 2019, 00:54:22 CET schrieb Andy Lutomirski:

Hi Andy,
> 
> I think that, if the crypto API is going to grow a KDF facility, it should
> be done right. Have a key type or flag or whatever that says “this key may
> *only* be used to derive keys using such-and-such algorithm”, and have a
> helper to derive a key.  That helper should take some useful parameters and
> mix them in:
> 
> - What type of key is being derived?  ECDSA signing key?  HMAC key?  AES
> key?
> 
> - Can user code access the derived key?
> 
> - What is the key’s purpose?  “Encrypt and authenticate a hibernation image”
> would be a purpose.
> 
> - Number of bytes.
> 
> All of these parameters should be mixed in to the key derivation.
> 
> Also, an AE key, even for AES+HMAC, should be just one derived key.  If you
> need 512 bits, ask for a 512-bit key, not two 256-bit keys.

I concur with your requirements. However, is the kernel crypto API the right 
place to enforce such policies? To me, the kernel crypto API is a tinker-toy 
set of ciphers.

The real policy enforcer would or should be the keyring facility. Thus, may I 
propose to:

- implement the cryptographic primitive of the KDF in the kernel crypto API

- implement the policy system how to use the KDF in the keyring facility

Ciao
Stephan

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

[Stephan Mueller]

Am Dienstag, 8. Januar 2019, 06:03:58 CET schrieb Herbert Xu:

Hi Herbert,

> Are we going to have multiple implementations for the same KDF?
> If not then the crypto API is not a good fit.  To consolidate
> multiple implementations of the same KDF, simply provide helpers
> for them.

It is unlikely to have multiple implementations of a KDF. However, KDFs relate 
to hashes like block chaining modes to raw block ciphers. Thus a KDF can be 
applied with different hashes.

My idea was to add template support to RNGs (because KDFs are effectively a 
type of RNG since they produce an arbitrary output from a fixed input). The 
KDFs would be a template wrapping hashes. For example, the CTR-KDF from 
SP800-108 could be instantiated like kdf-ctr(sha256).

Ciao
Stephan

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

[Stephan Mueller]

Am Montag, 7. Januar 2019, 16:33:27 CET schrieb joeyli:

Hi Herbert,

> 
> > use an official KDF type like SP800-108 or HKDF?
> > 
> > You find the counter-KDF according to SP800-108 in security/keys/dh.c
> > (search for functions *kdf*).
> > 
> > Or we may start pulling in KDF support into the kernel crypto API via the
> > patches along the line of [1].
> > 
> > [1] http://www.chronox.de/kdf.html
> 
> Thanks for your suggestion. I didn't touch any key derivation standard
> before. I will study it.
> 
> But I still want to use my original function currently. Because the same
> logic is also used in trusted key. I will happy to move to SP800-108 or
> HKDF when it's available in kernel.

Would it make sense to polish these mentioned KDF patches and add them to the 
kernel crypto API? The sprawl of key derivation logic here and there which 
seemingly does not comply to any standard and thus possibly have issues should 
be prevented and cleaned up.

Ciao
Stephan

Re: [RFC PATCH 4/4] crypto: Add EC-RDSA algorithm

[Stephan Mueller]

Am Montag, 7. Januar 2019, 09:07:10 CET schrieb Vitaly Chikunov:

Hi Vitaly,

> > Why do you manually parse the ASN.1 structure instead of using the ASN.1
> > parser?
> 
> I am not sure this worth effort and will not be most degenerate use of
> asn1_ber_decoder, since 1) I only need to parse one type in each case:
> OCTET STRING string above code, and OIDs in below code; 2) this data is
> said to be in DER format, which asn1_ber_decoder can not enforce. Surely
> this will also produce more code and files.

RSA public keys also only contain n and e in the ASN.1 structure for which the 
ASN.1 parser is used (see linux/crypto/rsapubkey.asn1). As ASN.1 parsing is 
always having security issues, I would rather suggest to have this parsing 
implemented in one spot and not here and there.

Regarding your comment (2), I am not sure I understand. Why do you say that 
the DER format cannot be parsed by the kernel's ASN.1 parser? For example, 
when you generate RSA keys in DER format with, say, openssl, the kernel ASN.1 
parser will happily use them. Also, when I created my (not accepted) patch to 
load PQG domain parameters for DH using the ASN.1 parser, the PQG domain 
parameters created by openssl in DER format were processed well.

Ciao
Stephan

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

[Stephan Mueller]

Am Sonntag, 6. Januar 2019, 09:01:27 CET schrieb Stephan Mueller:

Hi,

> > +   memcpy(skey.key, ukp->data, ukp->datalen);
> 
> Where would skey.key be destroyed again?

Now I see it - it is in patch 4. Please disregard my comment.

Ciao
Stephan

Re: [PATCH 3/5] PM / hibernate: Encrypt snapshot image

[Stephan Mueller]

Am Donnerstag, 3. Januar 2019, 15:32:25 CET schrieb Lee, Chun-Yi:

Hi Chun,

> To protect the secret in memory snapshot image, this patch adds the
> logic to encrypt snapshot pages by AES-CTR. Using AES-CTR is because
> it's simple, fast and parallelizable. But this patch didn't implement
> parallel encryption.
> 
> The encrypt key is derived from the snapshot key. And the initialization
> vector will be kept in snapshot header for resuming.
> 
> Cc: "Rafael J. Wysocki" 
> Cc: Pavel Machek 
> Cc: Chen Yu 
> Cc: Oliver Neukum 
> Cc: Ryan Chen 
> Cc: David Howells 
> Cc: Giovanni Gherdovich 
> Cc: Randy Dunlap 
> Cc: Jann Horn 
> Cc: Andy Lutomirski 
> Signed-off-by: "Lee, Chun-Yi" 
> ---
>  kernel/power/hibernate.c |   8 ++-
>  kernel/power/power.h |   6 ++
>  kernel/power/snapshot.c  | 154
> ++- 3 files changed, 164
> insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 0dda6a9f0af1..5ac2ab6f4a0e 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -275,10 +275,14 @@ static int create_image(int platform_mode)
>   if (error)
>   return error;
> 
> + error = snapshot_prepare_crypto(false, true);
> + if (error)
> + goto finish_hash;
> +
>   error = dpm_suspend_end(PMSG_FREEZE);
>   if (error) {
>   pr_err("Some devices failed to power down, aborting 
> hibernation\n");
> - goto finish_hash;
> + goto finish_crypto;
>   }
> 
>   error = platform_pre_snapshot(platform_mode);
> @@ -335,6 +339,8 @@ static int create_image(int platform_mode)
>   dpm_resume_start(in_suspend ?
>   (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
> 
> + finish_crypto:
> + snapshot_finish_crypto();
>   finish_hash:
>   snapshot_finish_hash();
> 
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index c614b0a294e3..41263fdd3a54 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -5,6 +5,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  /* The max size of encrypted key blob */
>  #define KEY_BLOB_BUFF_LEN 512
> @@ -24,6 +25,7 @@ struct swsusp_info {
>   unsigned long   pages;
>   unsigned long   size;
>   unsigned long   trampoline_pfn;
> + u8  iv[AES_BLOCK_SIZE];
>   u8  signature[SNAPSHOT_DIGEST_SIZE];
>  } __aligned(PAGE_SIZE);
> 
> @@ -44,6 +46,8 @@ extern void __init hibernate_image_size_init(void);
>  #ifdef CONFIG_HIBERNATION_ENC_AUTH
>  /* kernel/power/snapshot.c */
>  extern int snapshot_image_verify_decrypt(void);
> +extern int snapshot_prepare_crypto(bool may_sleep, bool create_iv);
> +extern void snapshot_finish_crypto(void);
>  extern int snapshot_prepare_hash(bool may_sleep);
>  extern void snapshot_finish_hash(void);
>  /* kernel/power/snapshot_key.c */
> @@ -53,6 +57,8 @@ extern int snapshot_get_auth_key(u8 *auth_key, bool
> may_sleep); extern int snapshot_get_enc_key(u8 *enc_key, bool may_sleep);
>  #else
>  static inline int snapshot_image_verify_decrypt(void) { return 0; }
> +static inline int snapshot_prepare_crypto(bool may_sleep, bool create_iv) {
> return 0; } +static inline void snapshot_finish_crypto(void) {}
>  static inline int snapshot_prepare_hash(bool may_sleep) { return 0; }
>  static inline void snapshot_finish_hash(void) {}
>  static inline int snapshot_key_init(void) { return 0; }
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index e817c035f378..cd10ab5e4850 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -41,7 +41,11 @@
>  #include 
>  #include 
>  #ifdef CONFIG_HIBERNATION_ENC_AUTH
> +#include 
> +#include 
> +#include 
>  #include 
> +#include 
>  #endif
> 
>  #include "power.h"
> @@ -1413,6 +1417,127 @@ static unsigned int nr_copy_pages;
>  static void **h_buf;
> 
>  #ifdef CONFIG_HIBERNATION_ENC_AUTH
> +static struct skcipher_request *sk_req;
> +static u8 iv[AES_BLOCK_SIZE];

May I ask for a different name here? The variable iv is used throughout the 
kernel crypto API and it is always a challenge when doing code reviews to 
trace the right variable when using common names :-)

> +static void *c_buffer;
> +
> +static void init_iv(struct swsusp_info *info)
> +{
> + memcpy(info->iv, iv, AES_BLOCK_SIZE);
> +}
> +
> +static void load_iv(struct swsusp_info *info)
> +{
> + memcpy(iv, info->iv, AES_BLOCK_SIZE);
> +}
> +
> +int snapshot_prepare_crypto(bool may_sleep, bool create_iv)
> +{
> + char enc_key[DERIVED_KEY_SIZE];
> + struct crypto_skcipher *tfm;
> + int ret = 0;
> +
> + ret = snapshot_get_enc_key(enc_key, may_sleep);
> + if (ret) {
> + pr_warn_once("enc key is invalid\n");
> + return -EINVAL;
> + }
> +
> + c_buffer = (void *)get_zeroed_page(GFP_KERNEL);
> + if (!c_buffer) {
> + pr_err("Allocate crypto buffer 

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

[Stephan Mueller]

Am Donnerstag, 3. Januar 2019, 15:32:23 CET schrieb Lee, Chun-Yi:

Hi Chun,

> This patch adds a snapshot keys handler for using the key retention
> service api to create keys for snapshot image encryption and
> authentication.
> 
> This handler uses TPM trusted key as the snapshot master key, and the
> encryption key and authentication key are derived from the snapshot
> key. The user defined key can also be used as the snapshot master key
> , but user must be aware that the security of user key relies on user
> space.
> 
> The name of snapshot key is fixed to "swsusp-kmk". User should use
> the keyctl tool to load the key blob to root's user keyring. e.g.
> 
>  # /bin/keyctl add trusted swsusp-kmk "load `cat swsusp-kmk.blob`" @u
> 
> or create a new user key. e.g.
> 
>  # /bin/keyctl add user swsusp-kmk password @u
> 
> Then the disk_kmk sysfs file can be used to trigger the initialization
> of snapshot key:
> 
>  # echo 1 > /sys/power/disk_kmk
> 
> After the initialization be triggered, the secret in the payload of
> swsusp-key will be copied by hibernation and be erased. Then user can
> use keyctl to remove swsusp-kmk key from root's keyring.
> 
> If user does not trigger the initialization by disk_kmk file after
> swsusp-kmk be loaded to kernel. Then the snapshot key will be
> initialled when hibernation be triggered.
> 
> v2:
> - Fixed bug of trusted_key_init's return value.
> - Fixed wording in Kconfig
> - Removed VLA usage
> - Removed the checking of capability for writing disk_kmk.
> - Fixed Kconfig, select trusted key.
> - Add memory barrier before setting key initialized flag.
> 
> Cc: "Rafael J. Wysocki" 
> Cc: Pavel Machek 
> Cc: Chen Yu 
> Cc: Oliver Neukum 
> Cc: Ryan Chen 
> Cc: David Howells 
> Cc: Giovanni Gherdovich 
> Cc: Randy Dunlap 
> Cc: Jann Horn 
> Cc: Andy Lutomirski 
> Signed-off-by: "Lee, Chun-Yi" 
> ---
>  kernel/power/Kconfig|  14 +++
>  kernel/power/Makefile   |   1 +
>  kernel/power/hibernate.c|  33 ++
>  kernel/power/power.h|  16 +++
>  kernel/power/snapshot_key.c | 245
>  5 files changed, 309
> insertions(+)
>  create mode 100644 kernel/power/snapshot_key.c
> 
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index f8fe57d1022e..506a3c5a7a0d 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -76,6 +76,20 @@ config HIBERNATION
> 
> For more information take a look at
> .
> 
> +config HIBERNATION_ENC_AUTH
> + bool "Hibernation encryption and authentication"
> + depends on HIBERNATION
> + select TRUSTED_KEYS
> + select CRYPTO_AES
> + select CRYPTO_HMAC
> + select CRYPTO_SHA512
> + help
> +   This option will encrypt and authenticate the memory snapshot image
> +   of hibernation. It prevents that the snapshot image be arbitrarily
> +   modified. A user can use the TPM's trusted key or user defined key
> +   as the master key of hibernation. The TPM trusted key depends on TPM.
> +   The security of user defined key relies on user space.
> +
>  config ARCH_SAVE_PAGE_KEYS
>   bool
> 
> diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> index e7e47d9be1e5..d949adbaf580 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_FREEZER)   += process.o
>  obj-$(CONFIG_SUSPEND)+= suspend.o
>  obj-$(CONFIG_PM_TEST_SUSPEND)+= suspend_test.o
>  obj-$(CONFIG_HIBERNATION)+= hibernate.o snapshot.o swap.o user.o
> +obj-$(CONFIG_HIBERNATION_ENC_AUTH)   += snapshot_key.o
>  obj-$(CONFIG_PM_AUTOSLEEP)   += autosleep.o
>  obj-$(CONFIG_PM_WAKELOCKS)   += wakelock.o
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index abef759de7c8..ecc31e8e40d0 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -1034,6 +1034,36 @@ static ssize_t disk_store(struct kobject *kobj,
> struct kobj_attribute *attr,
> 
>  power_attr(disk);
> 
> +#ifdef CONFIG_HIBERNATION_ENC_AUTH
> +static ssize_t disk_kmk_show(struct kobject *kobj, struct kobj_attribute
> *attr, +   char *buf)
> +{
> + if (snapshot_key_initialized())
> + return sprintf(buf, "initialized\n");
> + else
> + return sprintf(buf, "uninitialized\n");
> +}
> +
> +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute
> *attr, +const char *buf, size_t n)
> +{
> + int error = 0;
> + char *p;
> + int len;
> +
> + p = memchr(buf, '\n', n);
> + len = p ? p - buf : n;
> + if (strncmp(buf, "1", len))
> + return -EINVAL;
> +
> + error = snapshot_key_init();
> +
> + return error ? error : n;
> +}
> +
> +power_attr(disk_kmk);
> +#endif /* !CONFIG_HIBERNATION_ENC_AUTH */
> +
>  static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute
> *attr, char *buf)
>  {
> @@ -1138,6 +1168,9 @@ power_attr(reserved_size);
> 

Re: [PATCH 2/5] PM / hibernate: Generate and verify signature for snapshot image

[Stephan Mueller]

Am Donnerstag, 3. Januar 2019, 15:32:24 CET schrieb Lee, Chun-Yi:

Hi Chun,

> +int snapshot_image_verify_decrypt(void)
> +{
> + int ret, i;
> +
> + if (!h_buf) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + ret = snapshot_key_init();
> + if (ret)
> + goto error_prep;
> +
> + ret = snapshot_prepare_hash(true);
> + if (ret || !s4_verify_desc)
> + goto error_prep;
> +
> + for (i = 0; i < nr_copy_pages; i++) {
> + ret = crypto_shash_update(s4_verify_desc, *(h_buf + i), 
> PAGE_SIZE);
> + if (ret)
> + goto error_shash;
> + }
> +
> + ret = crypto_shash_final(s4_verify_desc, s4_verify_digest);
> + if (ret)
> + goto error_shash;
> +
> + pr_debug("Signature %*phN\n", SNAPSHOT_DIGEST_SIZE, signature);
> + pr_debug("Digest%*phN\n", SNAPSHOT_DIGEST_SIZE, s4_verify_digest);
> + if (memcmp(signature, s4_verify_digest, SNAPSHOT_DIGEST_SIZE))
> + ret = -EKEYREJECTED;
> +
> + error_shash:
> + snapshot_finish_hash();
> +
> + error_prep:
> + vfree(h_buf);
> + if (ret)
> + pr_warn("Signature verification failed: %d\n", ret);
> + error:
> + sig_verify_ret = ret;
> + return ret;
> +}

May I ask why the authentication part is done manually here? Why not using an 
AEAD cipher like the authenc() ciphers, or CCM (I would not recommend GCM 
though)? In this case, the encryption/decryption operation would automatically 
perform the creation of the hash and the verification of the hash. I.e. 
decryption can return EBADMSG which indicates an authentication failure.

> +
> +static int
> +__copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap
> *orig_bm) +{
> + unsigned long pfn, dst_pfn;
> + struct page *d_page;
> + void *crypto_buffer = NULL;
> + int ret = 0;
> +
> + memory_bm_position_reset(orig_bm);
> + memory_bm_position_reset(copy_bm);
> + for (;;) {
> + pfn = memory_bm_next_pfn(orig_bm);
> + if (unlikely(pfn == BM_END_OF_MAP))
> + break;
> + dst_pfn = memory_bm_next_pfn(copy_bm);
> + copy_data_page(dst_pfn, pfn);
> +
> + /* Setup buffer */
> + d_page = pfn_to_page(dst_pfn);
> + if (PageHighMem(d_page)) {
> + void *kaddr = kmap_atomic(d_page);
> +
> + copy_page(buffer, kaddr);
> + kunmap_atomic(kaddr);
> + crypto_buffer = buffer;
> + } else {
> + crypto_buffer = page_address(d_page);
> + }
> +
> + /* Generate digest */
> + if (!s4_verify_desc)
> + continue;
> + ret = crypto_shash_update(s4_verify_desc, crypto_buffer,
> +   PAGE_SIZE);

Same here, the creation of the hash would be implicit during the encryption.

Ciao
Stephan

Re: [PATCH] crypto: mark cts(cbc(aes)) as FIPS allowed

[Stephan Mueller]

Am Sonntag, 4. November 2018, 11:05:24 CET schrieb Gilad Ben-Yossef:

Hi Gilad,

> As per Sp800-38A addendum from Oct 2010[1], cts(cbc(aes)) is
> allowed as a FIPS mode algorithm. Mark it as such.
> 
> [1] https://csrc.nist.gov/publications/detail/sp/800-38a/addendum/final

There are several types of CTS approaches. Only three of those are listed in 
the SP800-38A addendum. The source code only refers to some RFCs.

Did you check whether the CTS implementation matches one or more of the types 
listed in the addendum? If yes, may I suggest to add a small statement in the 
code noting this fact?

Thanks a lot.

Ciao
Stephan

Re: [PATCH] crypto: mark cts(cbc(aes)) as FIPS allowed

[Stephan Mueller]

Am Sonntag, 4. November 2018, 11:05:24 CET schrieb Gilad Ben-Yossef:

Hi Gilad,

> As per Sp800-38A addendum from Oct 2010[1], cts(cbc(aes)) is
> allowed as a FIPS mode algorithm. Mark it as such.
> 
> [1] https://csrc.nist.gov/publications/detail/sp/800-38a/addendum/final

There are several types of CTS approaches. Only three of those are listed in 
the SP800-38A addendum. The source code only refers to some RFCs.

Did you check whether the CTS implementation matches one or more of the types 
listed in the addendum? If yes, may I suggest to add a small statement in the 
code noting this fact?

Thanks a lot.

Ciao
Stephan

Re: [PATCH 1/5] random: fix crng_ready() test

[Stephan Mueller]

Am Freitag, 13. April 2018, 14:53:13 CEST schrieb Theodore Y. Ts'o:

Hi Theodore,
> 
> This was always the original design intent, but I screwed up and no
> one noticed until Jann reached out to be and said, "Hey this
> doesn't seem to make much sense".

I disagree, but I guess you would have expected that. But this is not the 
issue.

What I would like to point out that more and more folks change to 
getrandom(2). As this call will now unblock much later in the boot cycle, 
these systems see a significant departure from the current system behavior.

E.g. an sshd using getrandom(2) would be ready shortly after the boot finishes 
as of now. Now it can be a matter minutes before it responds. Thus, is such 
change in the kernel behavior something for stable?

Ciao
Stephan

Re: [PATCH 1/5] random: fix crng_ready() test

[Stephan Mueller]

Am Freitag, 13. April 2018, 14:53:13 CEST schrieb Theodore Y. Ts'o:

Hi Theodore,
> 
> This was always the original design intent, but I screwed up and no
> one noticed until Jann reached out to be and said, "Hey this
> doesn't seem to make much sense".

I disagree, but I guess you would have expected that. But this is not the 
issue.

What I would like to point out that more and more folks change to 
getrandom(2). As this call will now unblock much later in the boot cycle, 
these systems see a significant departure from the current system behavior.

E.g. an sshd using getrandom(2) would be ready shortly after the boot finishes 
as of now. Now it can be a matter minutes before it responds. Thus, is such 
change in the kernel behavior something for stable?

Ciao
Stephan

Re: [PATCH 1/5] random: fix crng_ready() test

[Stephan Mueller]

Am Freitag, 13. April 2018, 03:30:42 CEST schrieb Theodore Ts'o:

Hi Theodore,

> The crng_init variable has three states:
> 
> 0: The CRNG is not initialized at all
> 1: The CRNG has a small amount of entropy, hopefully good enough for
>early-boot, non-cryptographical use cases
> 2: The CRNG is fully initialized and we are sure it is safe for
>cryptographic use cases.
> 
> The crng_ready() function should only return true once we are in the
> last state.
> 
Do I see that correctly that getrandom(2) will now unblock after the 
input_pool has obtained 128 bits of entropy? Similarly for 
get_random_bytes_wait.

As this seems to be the only real use case for crng_ready (apart from 
logging), what is the purpose of crng_init == 1?

Ciao
Stephan

Re: [PATCH 1/5] random: fix crng_ready() test

[Stephan Mueller]

Am Freitag, 13. April 2018, 03:30:42 CEST schrieb Theodore Ts'o:

Hi Theodore,

> The crng_init variable has three states:
> 
> 0: The CRNG is not initialized at all
> 1: The CRNG has a small amount of entropy, hopefully good enough for
>early-boot, non-cryptographical use cases
> 2: The CRNG is fully initialized and we are sure it is safe for
>cryptographic use cases.
> 
> The crng_ready() function should only return true once we are in the
> last state.
> 
Do I see that correctly that getrandom(2) will now unblock after the 
input_pool has obtained 128 bits of entropy? Similarly for 
get_random_bytes_wait.

As this seems to be the only real use case for crng_ready (apart from 
logging), what is the purpose of crng_init == 1?

Ciao
Stephan

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Mueller]

Am Mittwoch, 11. April 2018, 14:29:45 CEST schrieb Dmitry Vyukov:

Hi Dmitry,
> 
> What do you mean by description of the fault?
> It's kernel standard FAULT_INJECTION facility, it injects faults
> mainly into kmalloc/slab_alloc (also in a bunch of other things, but
> in this case this seems to be kmalloc). In the repro you can see that
> it's injecting a fault into 8-th malloc in the setsockopt syscall.

I am now able to reproduce it.

I think I have a smoking gun, but let me test it first.

Ciao
Stephan

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Mueller]

Am Mittwoch, 11. April 2018, 14:29:45 CEST schrieb Dmitry Vyukov:

Hi Dmitry,
> 
> What do you mean by description of the fault?
> It's kernel standard FAULT_INJECTION facility, it injects faults
> mainly into kmalloc/slab_alloc (also in a bunch of other things, but
> in this case this seems to be kmalloc). In the repro you can see that
> it's injecting a fault into 8-th malloc in the setsockopt syscall.

I am now able to reproduce it.

I think I have a smoking gun, but let me test it first.

Ciao
Stephan

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Mueller]

Am Dienstag, 10. April 2018, 17:23:46 CEST schrieb Dmitry Vyukov:

Hi Dmitry,

> Stephan,
> 
> Do you have any hypothesis as to why this is not detected by KASAN and
> causes silent corruptions?
> We generally try to understand such cases and improve KASAN so that it
> catches such cases more reliably and they do not cause splashes of
> random crashes on syzbot.

I do not have any hypothesis at this point. I know that you induce some fault. 
As you mentioned the drbg_kcapi_seed function, I was looking through the error 
code paths to see whether some error handlers trip over each other. But all is 
guesswork so far. And I am not even sure whether the bug is in the DRBG code 
base.

Looking into the trace you sent, I see a NULL pointer dereference. At one 
point there is also the drbg_init_hash_kernel that is called. But nowhere I 
see any smoking gun.

Could you please give me a description of the fault you are inducing?

Ciao
Stephan

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Mueller]

Am Dienstag, 10. April 2018, 17:23:46 CEST schrieb Dmitry Vyukov:

Hi Dmitry,

> Stephan,
> 
> Do you have any hypothesis as to why this is not detected by KASAN and
> causes silent corruptions?
> We generally try to understand such cases and improve KASAN so that it
> catches such cases more reliably and they do not cause splashes of
> random crashes on syzbot.

I do not have any hypothesis at this point. I know that you induce some fault. 
As you mentioned the drbg_kcapi_seed function, I was looking through the error 
code paths to see whether some error handlers trip over each other. But all is 
guesswork so far. And I am not even sure whether the bug is in the DRBG code 
base.

Looking into the trace you sent, I see a NULL pointer dereference. At one 
point there is also the drbg_init_hash_kernel that is called. But nowhere I 
see any smoking gun.

Could you please give me a description of the fault you are inducing?

Ciao
Stephan

Re: [PATCH] AF_ALG: register completely initialized request in list

[Stephan Mueller]

Am Montag, 9. April 2018, 09:51:13 CEST schrieb Dmitry Vyukov:

Hi Dmitry,

> You can ask syzbot to test by replying to its report email with a test
> command, see:
> https://github.com/google/syzkaller/blob/master/docs/syzbot.md#communication
> -with-syzbot
> 
> Note that all testing of KMSAN bugs needs to go to KMSAN tree, for details
> see:
> https://github.com/google/syzkaller/blob/master/docs/syzbot.md#kmsan-bugs

Thank you. I will resend the patch later today with the proper tags.

Ciao
Stephan

Re: [PATCH] AF_ALG: register completely initialized request in list

[Stephan Mueller]

Am Montag, 9. April 2018, 09:51:13 CEST schrieb Dmitry Vyukov:

Hi Dmitry,

> You can ask syzbot to test by replying to its report email with a test
> command, see:
> https://github.com/google/syzkaller/blob/master/docs/syzbot.md#communication
> -with-syzbot
> 
> Note that all testing of KMSAN bugs needs to go to KMSAN tree, for details
> see:
> https://github.com/google/syzkaller/blob/master/docs/syzbot.md#kmsan-bugs

Thank you. I will resend the patch later today with the proper tags.

Ciao
Stephan

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Mueller]

Am Montag, 9. April 2018, 00:46:03 CEST schrieb Theodore Y. Ts'o:

Hi Theodore,
> 
> So the syzbot will run while the patch goes through the normal e-mail
> review process, which is kind of neat.  :-)

Thank you very much for the hint. That is a neat feature indeed.

As I came late to the party and I missed the original mails, I am wondering 
about which GIT repo was used and which branch of it. With that, I would be 
happy to resubmit with the test line.

Ciao
Stephan

Re: [PATCH] crypto: DRBG - guard uninstantion by lock

[Stephan Mueller]

Am Montag, 9. April 2018, 00:46:03 CEST schrieb Theodore Y. Ts'o:

Hi Theodore,
> 
> So the syzbot will run while the patch goes through the normal e-mail
> review process, which is kind of neat.  :-)

Thank you very much for the hint. That is a neat feature indeed.

As I came late to the party and I missed the original mails, I am wondering 
about which GIT repo was used and which branch of it. With that, I would be 
happy to resubmit with the test line.

Ciao
Stephan

Re: [PATCH] crypto: ctr: avoid VLA use

[Stephan Mueller]

Am Mittwoch, 14. März 2018, 14:46:29 CET schrieb Salvatore Mesoraca:

Hi Salvatore,

> 2018-03-14 14:31 GMT+01:00 Stephan Mueller <smuel...@chronox.de>:
> > Am Mittwoch, 14. März 2018, 14:17:30 CET schrieb Salvatore Mesoraca:
> > 
> > Hi Salvatore,
> > 
> >>   if (walk.nbytes) {
> >> 
> >> - crypto_ctr_crypt_final(, child);
> >> - err = blkcipher_walk_done(desc, , 0);
> >> + err = crypto_ctr_crypt_final(, child);
> >> + err = blkcipher_walk_done(desc, , err);
> > 
> > I guess you either want to handle the error from crypto_ctr_crypt_final or
> > do an err |= blkcipher_walk_done.
> 
> I think that blkcipher_walk_done handles and returns the error for me.
> Am I wrong?

You are right as you want to finalize the crypto operation even though the 
encryption fails.

Please disregard my comment.
> 
> Best regards,
> 
> Salvatore



Ciao
Stephan

Re: [PATCH] crypto: ctr: avoid VLA use

[Stephan Mueller]

Am Mittwoch, 14. März 2018, 14:46:29 CET schrieb Salvatore Mesoraca:

Hi Salvatore,

> 2018-03-14 14:31 GMT+01:00 Stephan Mueller :
> > Am Mittwoch, 14. März 2018, 14:17:30 CET schrieb Salvatore Mesoraca:
> > 
> > Hi Salvatore,
> > 
> >>   if (walk.nbytes) {
> >> 
> >> - crypto_ctr_crypt_final(, child);
> >> - err = blkcipher_walk_done(desc, , 0);
> >> + err = crypto_ctr_crypt_final(, child);
> >> + err = blkcipher_walk_done(desc, , err);
> > 
> > I guess you either want to handle the error from crypto_ctr_crypt_final or
> > do an err |= blkcipher_walk_done.
> 
> I think that blkcipher_walk_done handles and returns the error for me.
> Am I wrong?

You are right as you want to finalize the crypto operation even though the 
encryption fails.

Please disregard my comment.
> 
> Best regards,
> 
> Salvatore



Ciao
Stephan

Re: [PATCH] crypto: ctr: avoid VLA use

[Stephan Mueller]

Am Mittwoch, 14. März 2018, 14:17:30 CET schrieb Salvatore Mesoraca:

Hi Salvatore,

>   if (walk.nbytes) {
> - crypto_ctr_crypt_final(, child);
> - err = blkcipher_walk_done(desc, , 0);
> + err = crypto_ctr_crypt_final(, child);
> + err = blkcipher_walk_done(desc, , err);

I guess you either want to handle the error from crypto_ctr_crypt_final or do 
an err |= blkcipher_walk_done.

>   }
> 
>   return err;



Ciao
Stephan

Re: [PATCH] crypto: ctr: avoid VLA use

[Stephan Mueller]

Am Mittwoch, 14. März 2018, 14:17:30 CET schrieb Salvatore Mesoraca:

Hi Salvatore,

>   if (walk.nbytes) {
> - crypto_ctr_crypt_final(, child);
> - err = blkcipher_walk_done(desc, , 0);
> + err = crypto_ctr_crypt_final(, child);
> + err = blkcipher_walk_done(desc, , err);

I guess you either want to handle the error from crypto_ctr_crypt_final or do 
an err |= blkcipher_walk_done.

>   }
> 
>   return err;



Ciao
Stephan

Re: [PATCH v3] input: bcm5974 - Add driver for Apple Magic Trackpad 2

[Stephan Mueller]

Am Dienstag, 27. Februar 2018, 17:37:45 CET schrieb Stephan Mueller:

Hi Jiri, Dimity, Henrik,

> Am Sonntag, 21. Januar 2018, 23:06:55 CET schrieb Stephan Müller:
> 
> Hi Jiri, Dimity, Henrik,
> 
> > Hi,
> > 
> > Changes v3:
> > * port to 4.15-rc8
> > * small code cleanups (isolation of type casts to functions pertaining
> > 
> >   to the Apple Magic Trackpad 2
> > 
> > * clean up all checkpatch.pl errors and warnings (except those
> > 
> >   where the patch uses the structure of existing code fragments)
> > 
> > * updated horizontal and vertical limits to capture start of movements
> > 
> >   in the outer areas of the pad
> > 
> > ---8<---
> > 
> > Add support for Apple Magic Trackpad 2 in bcm5974 (MacBook Tochpad)
> > driver.
> > The Magic Trackpad 2 needs to be switched into the finger-reporting-mode,
> > just like the other macbook touchpads as well. But the format is different
> > to the ones before. The Header is 12 Bytes long and each reported finger
> > is additional 9 Bytes. The data order reported by the hardware is
> > different as well.
> 
> May I ask whether there is an issue in the patch? I have not received any
> feedback and would like to inquire about the status of the patch.
> 
> I would like to have Touchpad 2 properly supported.



Ciao
Stephan

Re: [PATCH v3] input: bcm5974 - Add driver for Apple Magic Trackpad 2

[Stephan Mueller]

Am Dienstag, 27. Februar 2018, 17:37:45 CET schrieb Stephan Mueller:

Hi Jiri, Dimity, Henrik,

> Am Sonntag, 21. Januar 2018, 23:06:55 CET schrieb Stephan Müller:
> 
> Hi Jiri, Dimity, Henrik,
> 
> > Hi,
> > 
> > Changes v3:
> > * port to 4.15-rc8
> > * small code cleanups (isolation of type casts to functions pertaining
> > 
> >   to the Apple Magic Trackpad 2
> > 
> > * clean up all checkpatch.pl errors and warnings (except those
> > 
> >   where the patch uses the structure of existing code fragments)
> > 
> > * updated horizontal and vertical limits to capture start of movements
> > 
> >   in the outer areas of the pad
> > 
> > ---8<---
> > 
> > Add support for Apple Magic Trackpad 2 in bcm5974 (MacBook Tochpad)
> > driver.
> > The Magic Trackpad 2 needs to be switched into the finger-reporting-mode,
> > just like the other macbook touchpads as well. But the format is different
> > to the ones before. The Header is 12 Bytes long and each reported finger
> > is additional 9 Bytes. The data order reported by the hardware is
> > different as well.
> 
> May I ask whether there is an issue in the patch? I have not received any
> feedback and would like to inquire about the status of the patch.
> 
> I would like to have Touchpad 2 properly supported.



Ciao
Stephan

Re: [PATCH v3] input: bcm5974 - Add driver for Apple Magic Trackpad 2

[Stephan Mueller]

Am Sonntag, 21. Januar 2018, 23:06:55 CET schrieb Stephan Müller:

Hi Jiri, Dimity, Henrik,

> Hi,
> 
> Changes v3:
> * port to 4.15-rc8
> * small code cleanups (isolation of type casts to functions pertaining
>   to the Apple Magic Trackpad 2
> * clean up all checkpatch.pl errors and warnings (except those
>   where the patch uses the structure of existing code fragments)
> * updated horizontal and vertical limits to capture start of movements
>   in the outer areas of the pad
> 
> ---8<---
> 
> Add support for Apple Magic Trackpad 2 in bcm5974 (MacBook Tochpad) driver.
> The Magic Trackpad 2 needs to be switched into the finger-reporting-mode,
> just like the other macbook touchpads as well. But the format is different
> to the ones before. The Header is 12 Bytes long and each reported finger
> is additional 9 Bytes. The data order reported by the hardware is
> different as well.

May I ask whether there is an issue in the patch? I have not received any 
feedback and would like to inquire about the status of the patch.

I would like to have Touchpad 2 properly supported.

Thanks a lot
Stephan

Re: [PATCH v3] input: bcm5974 - Add driver for Apple Magic Trackpad 2

[Stephan Mueller]

Am Sonntag, 21. Januar 2018, 23:06:55 CET schrieb Stephan Müller:

Hi Jiri, Dimity, Henrik,

> Hi,
> 
> Changes v3:
> * port to 4.15-rc8
> * small code cleanups (isolation of type casts to functions pertaining
>   to the Apple Magic Trackpad 2
> * clean up all checkpatch.pl errors and warnings (except those
>   where the patch uses the structure of existing code fragments)
> * updated horizontal and vertical limits to capture start of movements
>   in the outer areas of the pad
> 
> ---8<---
> 
> Add support for Apple Magic Trackpad 2 in bcm5974 (MacBook Tochpad) driver.
> The Magic Trackpad 2 needs to be switched into the finger-reporting-mode,
> just like the other macbook touchpads as well. But the format is different
> to the ones before. The Header is 12 Bytes long and each reported finger
> is additional 9 Bytes. The data order reported by the hardware is
> different as well.

May I ask whether there is an issue in the patch? I have not received any 
feedback and would like to inquire about the status of the patch.

I would like to have Touchpad 2 properly supported.

Thanks a lot
Stephan

Re: [PATCH 14/14] x86/crypto: aesni: Update aesni-intel_glue to use scatter/gather

[Stephan Mueller]

Am Montag, 12. Februar 2018, 20:51:28 CET schrieb Dave Watson:

Hi Dave,

> Add gcmaes_en/decrypt_sg routines, that will do scatter/gather
> by sg. Either src or dst may contain multiple buffers, so
> iterate over both at the same time if they are different.
> If the input is the same as the output, iterate only over one.
> 
> Currently both the AAD and TAG must be linear, so copy them out
> with scatterlist_map_and_copy.
> 
> Only the SSE routines are updated so far, so leave the previous
> gcmaes_en/decrypt routines, and branch to the sg ones if the
> keysize is inappropriate for avx, or we are SSE only.
> 
> Signed-off-by: Dave Watson 
> ---
>  arch/x86/crypto/aesni-intel_glue.c | 166
> + 1 file changed, 166 insertions(+)
> 
> diff --git a/arch/x86/crypto/aesni-intel_glue.c
> b/arch/x86/crypto/aesni-intel_glue.c index de986f9..1e32fbe 100644
> --- a/arch/x86/crypto/aesni-intel_glue.c
> +++ b/arch/x86/crypto/aesni-intel_glue.c
> @@ -791,6 +791,82 @@ static int generic_gcmaes_set_authsize(struct
> crypto_aead *tfm, return 0;
>  }
> 
> +static int gcmaes_encrypt_sg(struct aead_request *req, unsigned int
> assoclen, +   u8 *hash_subkey, u8 *iv, void *aes_ctx)
> +{
> + struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> + unsigned long auth_tag_len = crypto_aead_authsize(tfm);
> + struct gcm_context_data data AESNI_ALIGN_ATTR;
> + struct scatter_walk dst_sg_walk = {};
> + unsigned long left = req->cryptlen;
> + unsigned long len, srclen, dstlen;
> + struct scatter_walk src_sg_walk;
> + struct scatterlist src_start[2];
> + struct scatterlist dst_start[2];
> + struct scatterlist *src_sg;
> + struct scatterlist *dst_sg;
> + u8 *src, *dst, *assoc;
> + u8 authTag[16];
> +
> + assoc = kmalloc(assoclen, GFP_ATOMIC);
> + if (unlikely(!assoc))
> + return -ENOMEM;
> + scatterwalk_map_and_copy(assoc, req->src, 0, assoclen, 0);

Have you tested that this code does not barf when assoclen is 0?

Maybe it is worth while to finally add a test vector to testmgr.h which 
validates such scenario. If you would like, here is a vector you could add to 
testmgr:

https://github.com/smuellerDD/libkcapi/blob/master/test/test.sh#L315

This is a decryption of gcm(aes) with no message, no AAD and just a tag. The 
result should be EBADMSG.
> +
> + src_sg = scatterwalk_ffwd(src_start, req->src, req->assoclen);

Why do you use assoclen in the map_and_copy, and req->assoclen in the ffwd?

> + scatterwalk_start(_sg_walk, src_sg);
> + if (req->src != req->dst) {
> + dst_sg = scatterwalk_ffwd(dst_start, req->dst, req->assoclen);

Dto: req->assoclen or assoclen?

> + scatterwalk_start(_sg_walk, dst_sg);
> + }
> +
> + kernel_fpu_begin();
> + aesni_gcm_init(aes_ctx, , iv,
> + hash_subkey, assoc, assoclen);
> + if (req->src != req->dst) {
> + while (left) {
> + src = scatterwalk_map(_sg_walk);
> + dst = scatterwalk_map(_sg_walk);
> + srclen = scatterwalk_clamp(_sg_walk, left);
> + dstlen = scatterwalk_clamp(_sg_walk, left);
> + len = min(srclen, dstlen);
> + if (len)
> + aesni_gcm_enc_update(aes_ctx, ,
> +  dst, src, len);
> + left -= len;
> +
> + scatterwalk_unmap(src);
> + scatterwalk_unmap(dst);
> + scatterwalk_advance(_sg_walk, len);
> + scatterwalk_advance(_sg_walk, len);
> + scatterwalk_done(_sg_walk, 0, left);
> + scatterwalk_done(_sg_walk, 1, left);
> + }
> + } else {
> + while (left) {
> + dst = src = scatterwalk_map(_sg_walk);
> + len = scatterwalk_clamp(_sg_walk, left);
> + if (len)
> + aesni_gcm_enc_update(aes_ctx, ,
> +  src, src, len);
> + left -= len;
> + scatterwalk_unmap(src);
> + scatterwalk_advance(_sg_walk, len);
> + scatterwalk_done(_sg_walk, 1, left);
> + }
> + }
> + aesni_gcm_finalize(aes_ctx, , authTag, auth_tag_len);
> + kernel_fpu_end();
> +
> + kfree(assoc);
> +
> + /* Copy in the authTag */
> + scatterwalk_map_and_copy(authTag, req->dst,
> + req->assoclen + req->cryptlen,
> + auth_tag_len, 1);
> + return 0;
> +}
> +
>  static int gcmaes_encrypt(struct aead_request *req, unsigned int assoclen,
> u8 *hash_subkey, u8 *iv, void *aes_ctx)
>  {
> @@ -802,6 +878,11 @@ static int gcmaes_encrypt(struct aead_request 

Re: [PATCH 14/14] x86/crypto: aesni: Update aesni-intel_glue to use scatter/gather

[Stephan Mueller]

Am Montag, 12. Februar 2018, 20:51:28 CET schrieb Dave Watson:

Hi Dave,

> Add gcmaes_en/decrypt_sg routines, that will do scatter/gather
> by sg. Either src or dst may contain multiple buffers, so
> iterate over both at the same time if they are different.
> If the input is the same as the output, iterate only over one.
> 
> Currently both the AAD and TAG must be linear, so copy them out
> with scatterlist_map_and_copy.
> 
> Only the SSE routines are updated so far, so leave the previous
> gcmaes_en/decrypt routines, and branch to the sg ones if the
> keysize is inappropriate for avx, or we are SSE only.
> 
> Signed-off-by: Dave Watson 
> ---
>  arch/x86/crypto/aesni-intel_glue.c | 166
> + 1 file changed, 166 insertions(+)
> 
> diff --git a/arch/x86/crypto/aesni-intel_glue.c
> b/arch/x86/crypto/aesni-intel_glue.c index de986f9..1e32fbe 100644
> --- a/arch/x86/crypto/aesni-intel_glue.c
> +++ b/arch/x86/crypto/aesni-intel_glue.c
> @@ -791,6 +791,82 @@ static int generic_gcmaes_set_authsize(struct
> crypto_aead *tfm, return 0;
>  }
> 
> +static int gcmaes_encrypt_sg(struct aead_request *req, unsigned int
> assoclen, +   u8 *hash_subkey, u8 *iv, void *aes_ctx)
> +{
> + struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> + unsigned long auth_tag_len = crypto_aead_authsize(tfm);
> + struct gcm_context_data data AESNI_ALIGN_ATTR;
> + struct scatter_walk dst_sg_walk = {};
> + unsigned long left = req->cryptlen;
> + unsigned long len, srclen, dstlen;
> + struct scatter_walk src_sg_walk;
> + struct scatterlist src_start[2];
> + struct scatterlist dst_start[2];
> + struct scatterlist *src_sg;
> + struct scatterlist *dst_sg;
> + u8 *src, *dst, *assoc;
> + u8 authTag[16];
> +
> + assoc = kmalloc(assoclen, GFP_ATOMIC);
> + if (unlikely(!assoc))
> + return -ENOMEM;
> + scatterwalk_map_and_copy(assoc, req->src, 0, assoclen, 0);

Have you tested that this code does not barf when assoclen is 0?

Maybe it is worth while to finally add a test vector to testmgr.h which 
validates such scenario. If you would like, here is a vector you could add to 
testmgr:

https://github.com/smuellerDD/libkcapi/blob/master/test/test.sh#L315

This is a decryption of gcm(aes) with no message, no AAD and just a tag. The 
result should be EBADMSG.
> +
> + src_sg = scatterwalk_ffwd(src_start, req->src, req->assoclen);

Why do you use assoclen in the map_and_copy, and req->assoclen in the ffwd?

> + scatterwalk_start(_sg_walk, src_sg);
> + if (req->src != req->dst) {
> + dst_sg = scatterwalk_ffwd(dst_start, req->dst, req->assoclen);

Dto: req->assoclen or assoclen?

> + scatterwalk_start(_sg_walk, dst_sg);
> + }
> +
> + kernel_fpu_begin();
> + aesni_gcm_init(aes_ctx, , iv,
> + hash_subkey, assoc, assoclen);
> + if (req->src != req->dst) {
> + while (left) {
> + src = scatterwalk_map(_sg_walk);
> + dst = scatterwalk_map(_sg_walk);
> + srclen = scatterwalk_clamp(_sg_walk, left);
> + dstlen = scatterwalk_clamp(_sg_walk, left);
> + len = min(srclen, dstlen);
> + if (len)
> + aesni_gcm_enc_update(aes_ctx, ,
> +  dst, src, len);
> + left -= len;
> +
> + scatterwalk_unmap(src);
> + scatterwalk_unmap(dst);
> + scatterwalk_advance(_sg_walk, len);
> + scatterwalk_advance(_sg_walk, len);
> + scatterwalk_done(_sg_walk, 0, left);
> + scatterwalk_done(_sg_walk, 1, left);
> + }
> + } else {
> + while (left) {
> + dst = src = scatterwalk_map(_sg_walk);
> + len = scatterwalk_clamp(_sg_walk, left);
> + if (len)
> + aesni_gcm_enc_update(aes_ctx, ,
> +  src, src, len);
> + left -= len;
> + scatterwalk_unmap(src);
> + scatterwalk_advance(_sg_walk, len);
> + scatterwalk_done(_sg_walk, 1, left);
> + }
> + }
> + aesni_gcm_finalize(aes_ctx, , authTag, auth_tag_len);
> + kernel_fpu_end();
> +
> + kfree(assoc);
> +
> + /* Copy in the authTag */
> + scatterwalk_map_and_copy(authTag, req->dst,
> + req->assoclen + req->cryptlen,
> + auth_tag_len, 1);
> + return 0;
> +}
> +
>  static int gcmaes_encrypt(struct aead_request *req, unsigned int assoclen,
> u8 *hash_subkey, u8 *iv, void *aes_ctx)
>  {
> @@ -802,6 +878,11 @@ static int gcmaes_encrypt(struct aead_request *req,
> unsigned int 

Re: AIO operation and CMSG

[Stephan Mueller]

Am Mittwoch, 17. Januar 2018, 20:22:13 CET schrieb Christoph Hellwig:

Hi Christoph,

> On Sun, Jan 14, 2018 at 09:01:00AM +0100, Stephan Müller wrote:
> > The syscall io_submit sends data to the kernel and invokes the respective
> > handler function in the kernel such as the recvmsg handler. What I am
> > wondering is whether there is a way to send CMSG data along with the
> > io_submit syscall? If not, is CMSG handling with the AIO syscalls
> > possible at all?
> Not as-is, but it could be easily added by repurposing unused fields
> in the iocb.  The big question is if it should be done to the existing
> IOCB_CMD_PREADV/IOCB_CMD_PWRITEV types, or if new SENDMSG/RECVMSG ones
> should be added instead.

Thank you for the clarification. I think for the moment we have found another 
solution that we are discussing at linux-crypto. Therefore, I would currently 
not see the need for an additional support of CMSG in AIO.

Ciao
Stephan

Re: AIO operation and CMSG

[Stephan Mueller]

Am Mittwoch, 17. Januar 2018, 20:22:13 CET schrieb Christoph Hellwig:

Hi Christoph,

> On Sun, Jan 14, 2018 at 09:01:00AM +0100, Stephan Müller wrote:
> > The syscall io_submit sends data to the kernel and invokes the respective
> > handler function in the kernel such as the recvmsg handler. What I am
> > wondering is whether there is a way to send CMSG data along with the
> > io_submit syscall? If not, is CMSG handling with the AIO syscalls
> > possible at all?
> Not as-is, but it could be easily added by repurposing unused fields
> in the iocb.  The big question is if it should be done to the existing
> IOCB_CMD_PREADV/IOCB_CMD_PWRITEV types, or if new SENDMSG/RECVMSG ones
> should be added instead.

Thank you for the clarification. I think for the moment we have found another 
solution that we are discussing at linux-crypto. Therefore, I would currently 
not see the need for an additional support of CMSG in AIO.

Ciao
Stephan

Re: [PATCH v2] net/core: Increase default optmem_max limit

[Stephan Mueller]

Am Dienstag, 16. Januar 2018, 18:16:43 CET schrieb Björn 'besser82' Esser:

Hi Björn,

> With the new Linux Kernel Crypto API User Space Interface
> and its underlying AF_ALG socket, the current default value
> for `net.core.optmem_max` can be exhausted pretty quick when
> using asynchronous IO; on 32 bit systems it is not even enough
> for sending about 10 IOVECs at once to the socket interface.
> 
> To provide consumers of this new user space interface a well
> sufficient and reasonable maximum ancillary buffer size per
> socket by default, the limit is increased to four times of
> the previous setting:
> 
>   * 32 bit systems:  from 10240 bytes to 40960 bytes
>   * 64 bit systems:  from 20480 bytes to 81920 bytes
> 
> This allows for sending 32/64 (32/64 bit) parallel IOVECs at
> once to the socket interface, which should be enough for use
> in real world applications.
> 
> Signed-off-by: Björn Esser <besse...@fedoraproject.org>

Considering NR_FILE defining the default maximum number of file descriptors, 
at max 335 MB of RAM (32 bit) or 670 MB (64 bit) could be allocated which I 
would assume to be ok in current systems.

Reviewed-by: Stephan Mueller <smuel...@chronox.de>

Ciao
Stephan

Re: [PATCH v2] net/core: Increase default optmem_max limit

[Stephan Mueller]

Am Dienstag, 16. Januar 2018, 18:16:43 CET schrieb Björn 'besser82' Esser:

Hi Björn,

> With the new Linux Kernel Crypto API User Space Interface
> and its underlying AF_ALG socket, the current default value
> for `net.core.optmem_max` can be exhausted pretty quick when
> using asynchronous IO; on 32 bit systems it is not even enough
> for sending about 10 IOVECs at once to the socket interface.
> 
> To provide consumers of this new user space interface a well
> sufficient and reasonable maximum ancillary buffer size per
> socket by default, the limit is increased to four times of
> the previous setting:
> 
>   * 32 bit systems:  from 10240 bytes to 40960 bytes
>   * 64 bit systems:  from 20480 bytes to 81920 bytes
> 
> This allows for sending 32/64 (32/64 bit) parallel IOVECs at
> once to the socket interface, which should be enough for use
> in real world applications.
> 
> Signed-off-by: Björn Esser 

Considering NR_FILE defining the default maximum number of file descriptors, 
at max 335 MB of RAM (32 bit) or 670 MB (64 bit) could be allocated which I 
would assume to be ok in current systems.

Reviewed-by: Stephan Mueller 

Ciao
Stephan

Re: [PATCH 1/2] crypto: Implement a generic crypto statistics

[Stephan Mueller]

Am Freitag, 12. Januar 2018, 10:07:30 CET schrieb LABBE Corentin:

Hi LABBE,

> 
> > > diff --git a/include/uapi/linux/cryptouser.h
> > > b/include/uapi/linux/cryptouser.h index 19bf0ca6d635..15e51ccb3679
> > > 100644
> > > --- a/include/uapi/linux/cryptouser.h
> > > +++ b/include/uapi/linux/cryptouser.h
> > > @@ -73,6 +73,8 @@ struct crypto_report_hash {
> > > 
> > >   char type[CRYPTO_MAX_NAME];
> > >   unsigned int blocksize;
> > >   unsigned int digestsize;
> > > 
> > > + __u64 stat_hash;
> > 
> > Why do you use __u64? The atomic_t variable is an int, i.e. 32 bit. Thus I
> > would think that __u32 would suffice?
> 
> You are right, I will downgrade to __u32
> But I think I will set len stats to atomic64/__u64 and keep count on
> atomic_t/__u32.

Fine with me.

> > > + __u64 stat_hash_tlen;
> > > 
> > >  };
> > 
> > What I am slightly unsure here is: how should user space detect whether
> > these additional parameters are part of the NETLINK_USER API or not? I
> > use that interface in my libkcapi whose binary may be used on multiple
> > different kernel versions. How should that library operate if one kernel
> > has these parameters and another does not?
> 
> Userspace could check for kernel version and know if stat are present or
> not. Another way is to add a new netlink request.

Well, I am not sure that checking the kernel version is good enough. Distros 
and other vendors may backport this patch. This means that for some older 
kernel versions this interface is present.

Hence I would rather opt for a separate stat message where the user spacee 
caller receives an error on kernels that does not support it.

Ciao
Stephan

Re: [PATCH 1/2] crypto: Implement a generic crypto statistics

[Stephan Mueller]

Am Freitag, 12. Januar 2018, 10:07:30 CET schrieb LABBE Corentin:

Hi LABBE,

> 
> > > diff --git a/include/uapi/linux/cryptouser.h
> > > b/include/uapi/linux/cryptouser.h index 19bf0ca6d635..15e51ccb3679
> > > 100644
> > > --- a/include/uapi/linux/cryptouser.h
> > > +++ b/include/uapi/linux/cryptouser.h
> > > @@ -73,6 +73,8 @@ struct crypto_report_hash {
> > > 
> > >   char type[CRYPTO_MAX_NAME];
> > >   unsigned int blocksize;
> > >   unsigned int digestsize;
> > > 
> > > + __u64 stat_hash;
> > 
> > Why do you use __u64? The atomic_t variable is an int, i.e. 32 bit. Thus I
> > would think that __u32 would suffice?
> 
> You are right, I will downgrade to __u32
> But I think I will set len stats to atomic64/__u64 and keep count on
> atomic_t/__u32.

Fine with me.

> > > + __u64 stat_hash_tlen;
> > > 
> > >  };
> > 
> > What I am slightly unsure here is: how should user space detect whether
> > these additional parameters are part of the NETLINK_USER API or not? I
> > use that interface in my libkcapi whose binary may be used on multiple
> > different kernel versions. How should that library operate if one kernel
> > has these parameters and another does not?
> 
> Userspace could check for kernel version and know if stat are present or
> not. Another way is to add a new netlink request.

Well, I am not sure that checking the kernel version is good enough. Distros 
and other vendors may backport this patch. This means that for some older 
kernel versions this interface is present.

Hence I would rather opt for a separate stat message where the user spacee 
caller receives an error on kernels that does not support it.

Ciao
Stephan

Re: [PATCH 1/2] crypto: Implement a generic crypto statistics

[Stephan Mueller]

Am Donnerstag, 11. Januar 2018, 20:56:56 CET schrieb Corentin Labbe:

Hi Corentin,

> This patch implement a generic way to get statistics about all crypto
> usages.
> 
> Signed-off-by: Corentin Labbe 
> ---
>  crypto/Kconfig  | 11 
>  crypto/ablkcipher.c |  9 +++
>  crypto/acompress.c  |  9 +++
>  crypto/aead.c   | 10 
>  crypto/ahash.c  |  8 ++
>  crypto/akcipher.c   | 13 ++
>  crypto/algapi.c |  6 +
>  crypto/blkcipher.c  |  9 +++
>  crypto/crypto_user.c| 28 +
>  crypto/kpp.c|  7 ++
>  crypto/rng.c|  8 ++
>  crypto/scompress.c  |  9 +++
>  crypto/shash.c  |  5 
>  crypto/skcipher.c   |  9 +++
>  include/crypto/acompress.h  | 22 
>  include/crypto/aead.h   | 22 
>  include/crypto/akcipher.h   | 42 +++
>  include/crypto/hash.h   | 21 
>  include/crypto/kpp.h| 28 +
>  include/crypto/rng.h| 17 +
>  include/crypto/skcipher.h   | 22 
>  include/linux/crypto.h  | 56
> + include/uapi/linux/cryptouser.h |
> 34 +
>  23 files changed, 405 insertions(+)
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 971d558494c3..3b88fba14b59 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1780,6 +1780,17 @@ config CRYPTO_USER_API_AEAD
> This option enables the user-spaces interface for AEAD
> cipher algorithms.
> 
> +config CRYPTO_STATS
> + bool "Crypto usage statistics for User-space"
> + help
> +   This option enables the gathering of crypto stats.
> +   This will collect:
> +   - encrypt/decrypt size and numbers of symmeric operations
> +   - compress/decompress size and numbers of compress operations
> +   - size and numbers of hash operations
> +   - encrypt/decrypt/sign/verify numbers for asymmetric operations
> +   - generate/seed numbers for rng operations
> +
>  config CRYPTO_HASH_INFO
>   bool
> 
> diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
> index d880a4897159..f6d20e4ca977 100644
> --- a/crypto/ablkcipher.c
> +++ b/crypto/ablkcipher.c
> @@ -369,6 +369,7 @@ static int crypto_init_ablkcipher_ops(struct crypto_tfm
> *tfm, u32 type, static int crypto_ablkcipher_report(struct sk_buff *skb,
> struct crypto_alg *alg) {
>   struct crypto_report_blkcipher rblkcipher;
> + u64 v;
> 
>   strncpy(rblkcipher.type, "ablkcipher", sizeof(rblkcipher.type));
>   strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "",
> @@ -378,6 +379,14 @@ static int crypto_ablkcipher_report(struct sk_buff
> *skb, struct crypto_alg *alg) rblkcipher.min_keysize =
> alg->cra_ablkcipher.min_keysize;
>   rblkcipher.max_keysize = alg->cra_ablkcipher.max_keysize;
>   rblkcipher.ivsize = alg->cra_ablkcipher.ivsize;
> + v = atomic_read(>encrypt_cnt);
> + rblkcipher.stat_encrypt_cnt = v;
> + v = atomic_read(>encrypt_tlen);
> + rblkcipher.stat_encrypt_tlen = v;
> + v = atomic_read(>decrypt_cnt);
> + rblkcipher.stat_decrypt_cnt = v;
> + v = atomic_read(>decrypt_tlen);
> + rblkcipher.stat_decrypt_tlen = v;
> 
>   if (nla_put(skb, CRYPTOCFGA_REPORT_BLKCIPHER,
>   sizeof(struct crypto_report_blkcipher), ))
> diff --git a/crypto/acompress.c b/crypto/acompress.c
> index 1544b7c057fb..524c8a3e3f80 100644
> --- a/crypto/acompress.c
> +++ b/crypto/acompress.c
> @@ -32,8 +32,17 @@ static const struct crypto_type crypto_acomp_type;
>  static int crypto_acomp_report(struct sk_buff *skb, struct crypto_alg *alg)
> {
>   struct crypto_report_acomp racomp;
> + u64 v;
> 
>   strncpy(racomp.type, "acomp", sizeof(racomp.type));
> + v = atomic_read(>compress_cnt);
> + racomp.stat_compress_cnt = v;
> + v = atomic_read(>compress_tlen);
> + racomp.stat_compress_tlen = v;
> + v = atomic_read(>decompress_cnt);
> + racomp.stat_decompress_cnt = v;
> + v = atomic_read(>decompress_tlen);
> + racomp.stat_decompress_tlen = v;
> 
>   if (nla_put(skb, CRYPTOCFGA_REPORT_ACOMP,
>   sizeof(struct crypto_report_acomp), ))
> diff --git a/crypto/aead.c b/crypto/aead.c
> index fe00cbd7243d..de13bd345d8b 100644
> --- a/crypto/aead.c
> +++ b/crypto/aead.c
> @@ -109,6 +109,7 @@ static int crypto_aead_report(struct sk_buff *skb,
> struct crypto_alg *alg) {
>   struct crypto_report_aead raead;
>   struct aead_alg *aead = container_of(alg, struct aead_alg, base);
> + u64 v;
> 
>   strncpy(raead.type, "aead", sizeof(raead.type));
>   strncpy(raead.geniv, "", sizeof(raead.geniv));
> @@ -116,6 +117,15 @@ static int 

Re: [PATCH 1/2] crypto: Implement a generic crypto statistics

[Stephan Mueller]

Am Donnerstag, 11. Januar 2018, 20:56:56 CET schrieb Corentin Labbe:

Hi Corentin,

> This patch implement a generic way to get statistics about all crypto
> usages.
> 
> Signed-off-by: Corentin Labbe 
> ---
>  crypto/Kconfig  | 11 
>  crypto/ablkcipher.c |  9 +++
>  crypto/acompress.c  |  9 +++
>  crypto/aead.c   | 10 
>  crypto/ahash.c  |  8 ++
>  crypto/akcipher.c   | 13 ++
>  crypto/algapi.c |  6 +
>  crypto/blkcipher.c  |  9 +++
>  crypto/crypto_user.c| 28 +
>  crypto/kpp.c|  7 ++
>  crypto/rng.c|  8 ++
>  crypto/scompress.c  |  9 +++
>  crypto/shash.c  |  5 
>  crypto/skcipher.c   |  9 +++
>  include/crypto/acompress.h  | 22 
>  include/crypto/aead.h   | 22 
>  include/crypto/akcipher.h   | 42 +++
>  include/crypto/hash.h   | 21 
>  include/crypto/kpp.h| 28 +
>  include/crypto/rng.h| 17 +
>  include/crypto/skcipher.h   | 22 
>  include/linux/crypto.h  | 56
> + include/uapi/linux/cryptouser.h |
> 34 +
>  23 files changed, 405 insertions(+)
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 971d558494c3..3b88fba14b59 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1780,6 +1780,17 @@ config CRYPTO_USER_API_AEAD
> This option enables the user-spaces interface for AEAD
> cipher algorithms.
> 
> +config CRYPTO_STATS
> + bool "Crypto usage statistics for User-space"
> + help
> +   This option enables the gathering of crypto stats.
> +   This will collect:
> +   - encrypt/decrypt size and numbers of symmeric operations
> +   - compress/decompress size and numbers of compress operations
> +   - size and numbers of hash operations
> +   - encrypt/decrypt/sign/verify numbers for asymmetric operations
> +   - generate/seed numbers for rng operations
> +
>  config CRYPTO_HASH_INFO
>   bool
> 
> diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
> index d880a4897159..f6d20e4ca977 100644
> --- a/crypto/ablkcipher.c
> +++ b/crypto/ablkcipher.c
> @@ -369,6 +369,7 @@ static int crypto_init_ablkcipher_ops(struct crypto_tfm
> *tfm, u32 type, static int crypto_ablkcipher_report(struct sk_buff *skb,
> struct crypto_alg *alg) {
>   struct crypto_report_blkcipher rblkcipher;
> + u64 v;
> 
>   strncpy(rblkcipher.type, "ablkcipher", sizeof(rblkcipher.type));
>   strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "",
> @@ -378,6 +379,14 @@ static int crypto_ablkcipher_report(struct sk_buff
> *skb, struct crypto_alg *alg) rblkcipher.min_keysize =
> alg->cra_ablkcipher.min_keysize;
>   rblkcipher.max_keysize = alg->cra_ablkcipher.max_keysize;
>   rblkcipher.ivsize = alg->cra_ablkcipher.ivsize;
> + v = atomic_read(>encrypt_cnt);
> + rblkcipher.stat_encrypt_cnt = v;
> + v = atomic_read(>encrypt_tlen);
> + rblkcipher.stat_encrypt_tlen = v;
> + v = atomic_read(>decrypt_cnt);
> + rblkcipher.stat_decrypt_cnt = v;
> + v = atomic_read(>decrypt_tlen);
> + rblkcipher.stat_decrypt_tlen = v;
> 
>   if (nla_put(skb, CRYPTOCFGA_REPORT_BLKCIPHER,
>   sizeof(struct crypto_report_blkcipher), ))
> diff --git a/crypto/acompress.c b/crypto/acompress.c
> index 1544b7c057fb..524c8a3e3f80 100644
> --- a/crypto/acompress.c
> +++ b/crypto/acompress.c
> @@ -32,8 +32,17 @@ static const struct crypto_type crypto_acomp_type;
>  static int crypto_acomp_report(struct sk_buff *skb, struct crypto_alg *alg)
> {
>   struct crypto_report_acomp racomp;
> + u64 v;
> 
>   strncpy(racomp.type, "acomp", sizeof(racomp.type));
> + v = atomic_read(>compress_cnt);
> + racomp.stat_compress_cnt = v;
> + v = atomic_read(>compress_tlen);
> + racomp.stat_compress_tlen = v;
> + v = atomic_read(>decompress_cnt);
> + racomp.stat_decompress_cnt = v;
> + v = atomic_read(>decompress_tlen);
> + racomp.stat_decompress_tlen = v;
> 
>   if (nla_put(skb, CRYPTOCFGA_REPORT_ACOMP,
>   sizeof(struct crypto_report_acomp), ))
> diff --git a/crypto/aead.c b/crypto/aead.c
> index fe00cbd7243d..de13bd345d8b 100644
> --- a/crypto/aead.c
> +++ b/crypto/aead.c
> @@ -109,6 +109,7 @@ static int crypto_aead_report(struct sk_buff *skb,
> struct crypto_alg *alg) {
>   struct crypto_report_aead raead;
>   struct aead_alg *aead = container_of(alg, struct aead_alg, base);
> + u64 v;
> 
>   strncpy(raead.type, "aead", sizeof(raead.type));
>   strncpy(raead.geniv, "", sizeof(raead.geniv));
> @@ -116,6 +117,15 @@ static int 

Re: [PATCH 3/7] crypto: ccree: add ablkcipher support

[Stephan Mueller]

Am Donnerstag, 11. Januar 2018, 10:17:10 CET schrieb Gilad Ben-Yossef:

Hi Gilad,

> + // verify weak keys
> + if (ctx_p->flow_mode == S_DIN_to_DES) {
> + if (!des_ekey(tmp, key) &&
> + (crypto_tfm_get_flags(tfm) & CRYPTO_TFM_REQ_WEAK_KEY)) {
> + tfm->crt_flags |= CRYPTO_TFM_RES_WEAK_KEY;
> + dev_dbg(dev, "weak DES key");
> + return -EINVAL;
> + }
> + }
> + if (ctx_p->cipher_mode == DRV_CIPHER_XTS &&
> + xts_check_key(tfm, key, keylen)) {
> + dev_dbg(dev, "weak XTS key");
> + return -EINVAL;
> + }
> + if (ctx_p->flow_mode == S_DIN_to_DES &&
> + keylen == DES3_EDE_KEY_SIZE &&
> + cc_verify_3des_keys(key, keylen)) {
> + dev_dbg(dev, "weak 3DES key");
> + return -EINVAL;
> + }

For the DES key, wouldn't it make sense to use __des3_ede_setkey?

Note, I would plan to release a patch for review to change that function to 
disallow key1 == key2 or key1 == key3 or key2 == key3 in FIPS mode.

Ciao
Stephan

Re: [PATCH 3/7] crypto: ccree: add ablkcipher support

[Stephan Mueller]

Am Donnerstag, 11. Januar 2018, 10:17:10 CET schrieb Gilad Ben-Yossef:

Hi Gilad,

> + // verify weak keys
> + if (ctx_p->flow_mode == S_DIN_to_DES) {
> + if (!des_ekey(tmp, key) &&
> + (crypto_tfm_get_flags(tfm) & CRYPTO_TFM_REQ_WEAK_KEY)) {
> + tfm->crt_flags |= CRYPTO_TFM_RES_WEAK_KEY;
> + dev_dbg(dev, "weak DES key");
> + return -EINVAL;
> + }
> + }
> + if (ctx_p->cipher_mode == DRV_CIPHER_XTS &&
> + xts_check_key(tfm, key, keylen)) {
> + dev_dbg(dev, "weak XTS key");
> + return -EINVAL;
> + }
> + if (ctx_p->flow_mode == S_DIN_to_DES &&
> + keylen == DES3_EDE_KEY_SIZE &&
> + cc_verify_3des_keys(key, keylen)) {
> + dev_dbg(dev, "weak 3DES key");
> + return -EINVAL;
> + }

For the DES key, wouldn't it make sense to use __des3_ede_setkey?

Note, I would plan to release a patch for review to change that function to 
disallow key1 == key2 or key1 == key3 or key2 == key3 in FIPS mode.

Ciao
Stephan

Re: [PATCH v2] crypto: AF_ALG - limit mask and type

[Stephan Mueller]

Am Freitag, 22. Dezember 2017, 08:36:07 CET schrieb Herbert Xu:

Hi Herbert,

> On Tue, Dec 19, 2017 at 07:25:04AM +0100, Stephan Müller wrote:
> > The user space interface allows specifying the type and the mask field
> > used to allocate the cipher. As user space can precisely select the
> > desired cipher by using either the name or the driver name, additional
> > selection options for cipher are not considered necessary and relevant
> > for user space.
> > 
> > This fixes a bug where user space is able to cause one cipher to be
> > registered multiple times potentially exhausting kernel memory.
> > 
> > Reported-by: syzbot <syzkal...@googlegroups.com>
> > Cc: <sta...@vger.kernel.org>
> > Signed-off-by: Stephan Mueller <smuel...@chronox.de>
> 
> This will break users of CRYPTO_ALG_KERN_DRIVER_ONLY.  I think
> we should add CRYPTO_ALG_TESTED to the blacklist since there is
> no sane reason to use it here.

Shouldn't we then rather use a white list instead of a black list?
> 
> Most other problems however would be bugs in the template code.
> The first thing a template does when it creates an instance is
> to check whether the resulting algorithm would fulfil the requested
> type/mask using crypto_check_attr_type.  So if that's not working
> then we should fix it there as it may also be triggered via other
> code paths that can create instances.

I think I understand that, but does user space need to utilize this logic?
> 
> Thanks,



Ciao
Stephan

Re: [PATCH v2] crypto: AF_ALG - limit mask and type

[Stephan Mueller]

Am Freitag, 22. Dezember 2017, 08:36:07 CET schrieb Herbert Xu:

Hi Herbert,

> On Tue, Dec 19, 2017 at 07:25:04AM +0100, Stephan Müller wrote:
> > The user space interface allows specifying the type and the mask field
> > used to allocate the cipher. As user space can precisely select the
> > desired cipher by using either the name or the driver name, additional
> > selection options for cipher are not considered necessary and relevant
> > for user space.
> > 
> > This fixes a bug where user space is able to cause one cipher to be
> > registered multiple times potentially exhausting kernel memory.
> > 
> > Reported-by: syzbot 
> > Cc: 
> > Signed-off-by: Stephan Mueller 
> 
> This will break users of CRYPTO_ALG_KERN_DRIVER_ONLY.  I think
> we should add CRYPTO_ALG_TESTED to the blacklist since there is
> no sane reason to use it here.

Shouldn't we then rather use a white list instead of a black list?
> 
> Most other problems however would be bugs in the template code.
> The first thing a template does when it creates an instance is
> to check whether the resulting algorithm would fulfil the requested
> type/mask using crypto_check_attr_type.  So if that's not working
> then we should fix it there as it may also be triggered via other
> code paths that can create instances.

I think I understand that, but does user space need to utilize this logic?
> 
> Thanks,



Ciao
Stephan

Re: [RFC] syzbot process

[Stephan Mueller]

Am Donnerstag, 21. Dezember 2017, 14:22:40 CET schrieb Andrey Ryabinin:

Hi Andrey,

> 2017-12-21 15:52 GMT+03:00 Dmitry Vyukov :
> > Any other proposals, thoughts, ideas?
> 
> a) Assume that patches send in replies to the bug report are fixes.
> 
> b) Almost the same as your "syzbot-fix: HASH"  proposal, but slightly
> closer to normal kernel development workflow.
>  Add hash/bug id into the From field of email, i.e.
> 
>  instead of
>  From: syzbot 
> 
>  make it
>  From: syzbot-{hash} 
> 
>  And ask to include "Reported-by: syzbot-{hash}
> " tag in a changelog.
> 
> a) doesn't exclude b) or "#syz: fix " emails, and vise versa

One additional suggestion: Rerun all already generated reproducer tests on, 
say, each updated kernel (e.g. newer rc or even full new version). If an issue 
is detected again, send a reply to the original message to indicate that the 
issue is still there. Note, I sometimes even fear that a patch that ought to 
fix the reported issue may not completely fix it considering races.

The problem with the current approach (at least to me) is that on mailing 
lists with some volume, such reports get easily buried.

Ciao
Stephan

Re: [RFC] syzbot process

[Stephan Mueller]

Am Donnerstag, 21. Dezember 2017, 14:22:40 CET schrieb Andrey Ryabinin:

Hi Andrey,

> 2017-12-21 15:52 GMT+03:00 Dmitry Vyukov :
> > Any other proposals, thoughts, ideas?
> 
> a) Assume that patches send in replies to the bug report are fixes.
> 
> b) Almost the same as your "syzbot-fix: HASH"  proposal, but slightly
> closer to normal kernel development workflow.
>  Add hash/bug id into the From field of email, i.e.
> 
>  instead of
>  From: syzbot 
> 
>  make it
>  From: syzbot-{hash} 
> 
>  And ask to include "Reported-by: syzbot-{hash}
> " tag in a changelog.
> 
> a) doesn't exclude b) or "#syz: fix " emails, and vise versa

One additional suggestion: Rerun all already generated reproducer tests on, 
say, each updated kernel (e.g. newer rc or even full new version). If an issue 
is detected again, send a reply to the original message to indicate that the 
issue is still there. Note, I sometimes even fear that a patch that ought to 
fix the reported issue may not completely fix it considering races.

The problem with the current approach (at least to me) is that on mailing 
lists with some volume, such reports get easily buried.

Ciao
Stephan

Re: [PATCH RFC 2/3] crypto: Implement a generic crypto statistics

[Stephan Mueller]

Am Mittwoch, 20. Dezember 2017, 21:09:26 CET schrieb Corentin Labbe:

Hi Corentin,

> This patch implement a generic way to get statistics about all crypto
> usages.
> 
> Signed-off-by: Corentin Labbe 
> ---
>  crypto/Kconfig |  11 +++
>  crypto/ahash.c |  18 +
>  crypto/algapi.c| 186
> + crypto/rng.c   | 
>  3 +
>  include/crypto/acompress.h |  10 +++
>  include/crypto/akcipher.h  |  12 +++
>  include/crypto/kpp.h   |   9 +++
>  include/crypto/rng.h   |   5 ++
>  include/crypto/skcipher.h  |   8 ++
>  include/linux/crypto.h |  22 ++
>  10 files changed, 284 insertions(+)
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index d6e9b60fc063..69f1822a026b 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1781,6 +1781,17 @@ config CRYPTO_USER_API_AEAD
> This option enables the user-spaces interface for AEAD
> cipher algorithms.
> 
> +config CRYPTO_STATS
> + bool "Crypto usage statistics for User-space"
> + help
> +   This option enables the gathering of crypto stats.
> +   This will collect:
> +   - encrypt/decrypt size and numbers of symmeric operations
> +   - compress/decompress size and numbers of compress operations
> +   - size and numbers of hash operations
> +   - encrypt/decrypt/sign/verify numbers for asymmetric operations
> +   - generate/seed numbers for rng operations
> +
>  config CRYPTO_HASH_INFO
>   bool
> 
> diff --git a/crypto/ahash.c b/crypto/ahash.c
> index 3a35d67de7d9..93b56892f1b8 100644
> --- a/crypto/ahash.c
> +++ b/crypto/ahash.c
> @@ -356,18 +356,36 @@ static int crypto_ahash_op(struct ahash_request *req,
> 
>  int crypto_ahash_final(struct ahash_request *req)
>  {
> +#ifdef CONFIG_CRYPTO_STATS
> + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +
> + tfm->base.__crt_alg->enc_cnt++;
> + tfm->base.__crt_alg->enc_tlen += req->nbytes;
> +#endif
>   return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->final);
>  }
>  EXPORT_SYMBOL_GPL(crypto_ahash_final);
> 
>  int crypto_ahash_finup(struct ahash_request *req)
>  {
> +#ifdef CONFIG_CRYPTO_STATS
> + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +
> + tfm->base.__crt_alg->enc_cnt++;
> + tfm->base.__crt_alg->enc_tlen += req->nbytes;
> +#endif
>   return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->finup);
>  }
>  EXPORT_SYMBOL_GPL(crypto_ahash_finup);
> 
>  int crypto_ahash_digest(struct ahash_request *req)
>  {
> +#ifdef CONFIG_CRYPTO_STATS
> + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +
> + tfm->base.__crt_alg->enc_cnt++;
> + tfm->base.__crt_alg->enc_tlen += req->nbytes;
> +#endif

This is ugly. May I ask that one inlne function is made that has these ifdefs 
instead of adding them throughout multiple functions? This comment would apply 
to the other instrumentation code below as well. The issue is that these 
ifdefs should not be spread around through the code.

Besides, I would think that the use of normal integers is not thread-safe. 
What about using atomic_t?

>   return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->digest);
>  }
>  EXPORT_SYMBOL_GPL(crypto_ahash_digest);
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index b8f6122f37e9..4fca4576af78 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -20,11 +20,158 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  #include "internal.h"
> 
>  static LIST_HEAD(crypto_template_list);
> 
> +#ifdef CONFIG_CRYPTO_STATS

Instead of ifdefing in the code, may I suggest adding a new file that is 
compiled / not compiled via the Makefile?

> +static struct kobject *crypto_root_kobj;
> +
> +static struct kobj_type dynamic_kobj_ktype = {
> + .sysfs_ops  = _sysfs_ops,
> +};
> +
> +static ssize_t fcrypto_priority(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct crypto_alg *alg;
> +
> + alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> + return snprintf(buf, 9, "%d\n", alg->cra_priority);
> +}
> +
> +static ssize_t fcrypto_stat_enc_cnt(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct crypto_alg *alg;
> +
> + alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> + return snprintf(buf, 9, "%lu\n", alg->enc_cnt);
> +}
> +
> +static ssize_t fcrypto_stat_enc_tlen(struct kobject *kobj,
> +  struct kobj_attribute *attr, char *buf)
> +{
> + struct crypto_alg *alg;
> +
> + alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> + return snprintf(buf, 9, "%lu\n", alg->enc_tlen);
> +}
> +
> +static ssize_t fcrypto_stat_dec_cnt(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct crypto_alg *alg;
> +
> + alg = 

Re: [PATCH RFC 2/3] crypto: Implement a generic crypto statistics

[Stephan Mueller]

Am Mittwoch, 20. Dezember 2017, 21:09:26 CET schrieb Corentin Labbe:

Hi Corentin,

> This patch implement a generic way to get statistics about all crypto
> usages.
> 
> Signed-off-by: Corentin Labbe 
> ---
>  crypto/Kconfig |  11 +++
>  crypto/ahash.c |  18 +
>  crypto/algapi.c| 186
> + crypto/rng.c   | 
>  3 +
>  include/crypto/acompress.h |  10 +++
>  include/crypto/akcipher.h  |  12 +++
>  include/crypto/kpp.h   |   9 +++
>  include/crypto/rng.h   |   5 ++
>  include/crypto/skcipher.h  |   8 ++
>  include/linux/crypto.h |  22 ++
>  10 files changed, 284 insertions(+)
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index d6e9b60fc063..69f1822a026b 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1781,6 +1781,17 @@ config CRYPTO_USER_API_AEAD
> This option enables the user-spaces interface for AEAD
> cipher algorithms.
> 
> +config CRYPTO_STATS
> + bool "Crypto usage statistics for User-space"
> + help
> +   This option enables the gathering of crypto stats.
> +   This will collect:
> +   - encrypt/decrypt size and numbers of symmeric operations
> +   - compress/decompress size and numbers of compress operations
> +   - size and numbers of hash operations
> +   - encrypt/decrypt/sign/verify numbers for asymmetric operations
> +   - generate/seed numbers for rng operations
> +
>  config CRYPTO_HASH_INFO
>   bool
> 
> diff --git a/crypto/ahash.c b/crypto/ahash.c
> index 3a35d67de7d9..93b56892f1b8 100644
> --- a/crypto/ahash.c
> +++ b/crypto/ahash.c
> @@ -356,18 +356,36 @@ static int crypto_ahash_op(struct ahash_request *req,
> 
>  int crypto_ahash_final(struct ahash_request *req)
>  {
> +#ifdef CONFIG_CRYPTO_STATS
> + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +
> + tfm->base.__crt_alg->enc_cnt++;
> + tfm->base.__crt_alg->enc_tlen += req->nbytes;
> +#endif
>   return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->final);
>  }
>  EXPORT_SYMBOL_GPL(crypto_ahash_final);
> 
>  int crypto_ahash_finup(struct ahash_request *req)
>  {
> +#ifdef CONFIG_CRYPTO_STATS
> + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +
> + tfm->base.__crt_alg->enc_cnt++;
> + tfm->base.__crt_alg->enc_tlen += req->nbytes;
> +#endif
>   return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->finup);
>  }
>  EXPORT_SYMBOL_GPL(crypto_ahash_finup);
> 
>  int crypto_ahash_digest(struct ahash_request *req)
>  {
> +#ifdef CONFIG_CRYPTO_STATS
> + struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +
> + tfm->base.__crt_alg->enc_cnt++;
> + tfm->base.__crt_alg->enc_tlen += req->nbytes;
> +#endif

This is ugly. May I ask that one inlne function is made that has these ifdefs 
instead of adding them throughout multiple functions? This comment would apply 
to the other instrumentation code below as well. The issue is that these 
ifdefs should not be spread around through the code.

Besides, I would think that the use of normal integers is not thread-safe. 
What about using atomic_t?

>   return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->digest);
>  }
>  EXPORT_SYMBOL_GPL(crypto_ahash_digest);
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index b8f6122f37e9..4fca4576af78 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -20,11 +20,158 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  #include "internal.h"
> 
>  static LIST_HEAD(crypto_template_list);
> 
> +#ifdef CONFIG_CRYPTO_STATS

Instead of ifdefing in the code, may I suggest adding a new file that is 
compiled / not compiled via the Makefile?

> +static struct kobject *crypto_root_kobj;
> +
> +static struct kobj_type dynamic_kobj_ktype = {
> + .sysfs_ops  = _sysfs_ops,
> +};
> +
> +static ssize_t fcrypto_priority(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct crypto_alg *alg;
> +
> + alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> + return snprintf(buf, 9, "%d\n", alg->cra_priority);
> +}
> +
> +static ssize_t fcrypto_stat_enc_cnt(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct crypto_alg *alg;
> +
> + alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> + return snprintf(buf, 9, "%lu\n", alg->enc_cnt);
> +}
> +
> +static ssize_t fcrypto_stat_enc_tlen(struct kobject *kobj,
> +  struct kobj_attribute *attr, char *buf)
> +{
> + struct crypto_alg *alg;
> +
> + alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> + return snprintf(buf, 9, "%lu\n", alg->enc_tlen);
> +}
> +
> +static ssize_t fcrypto_stat_dec_cnt(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct crypto_alg *alg;
> +
> + alg = container_of(kobj, struct 

Re: [PATCH RFC 1/3] crypto: Prevent to register duplicate cra_driver_name

[Stephan Mueller]

Am Mittwoch, 20. Dezember 2017, 21:09:25 CET schrieb Corentin Labbe:

Hi Corentin,

> Each crypto algorithm "cra_name" can have multiple implementation called
> "cra_driver_name".
> If two different implementation have the same cra_driver_name, nothing
> can easily differentiate them.
> Furthermore the mechanism for getting a crypto algorithm with its
> implementation name (crypto_alg_match() in crypto/crypto_user.c) will
> get only the first one found.
> 
> So this patch prevent the registration of two implementation with the
> same cra_driver_name.

Have you seen that happen? The driver_name should be an unambiguous name that 
is unique throughout all crypto implementations.

If you have seen a duplication, then this duplication should be fixed.

Ciao
Stephan

Re: [PATCH RFC 1/3] crypto: Prevent to register duplicate cra_driver_name

[Stephan Mueller]

Am Mittwoch, 20. Dezember 2017, 21:09:25 CET schrieb Corentin Labbe:

Hi Corentin,

> Each crypto algorithm "cra_name" can have multiple implementation called
> "cra_driver_name".
> If two different implementation have the same cra_driver_name, nothing
> can easily differentiate them.
> Furthermore the mechanism for getting a crypto algorithm with its
> implementation name (crypto_alg_match() in crypto/crypto_user.c) will
> get only the first one found.
> 
> So this patch prevent the registration of two implementation with the
> same cra_driver_name.

Have you seen that happen? The driver_name should be an unambiguous name that 
is unique throughout all crypto implementations.

If you have seen a duplication, then this duplication should be fixed.

Ciao
Stephan

Re: KASAN: use-after-free Read in crypto_aead_free_instance

[Stephan Mueller]

Am Mittwoch, 20. Dezember 2017, 11:15:38 CET schrieb Dmitry Vyukov:

Hi Dmitry,

> 
> What will be its meaning? How will it differ from fix?

Maybe a short clarification would help: what is the meaning of the syz fix 
marker? Depending on this answer, all that I am thinking of is to mark bug 
reports for which there are fixes actively discussed, but yet not integrated. 
Thus, such marker should only help others to point them to active discussions 
instead of them trying to find fixes alone.

Ciao
Stephan

Re: KASAN: use-after-free Read in crypto_aead_free_instance

[Stephan Mueller]

Am Mittwoch, 20. Dezember 2017, 11:15:38 CET schrieb Dmitry Vyukov:

Hi Dmitry,

> 
> What will be its meaning? How will it differ from fix?

Maybe a short clarification would help: what is the meaning of the syz fix 
marker? Depending on this answer, all that I am thinking of is to mark bug 
reports for which there are fixes actively discussed, but yet not integrated. 
Thus, such marker should only help others to point them to active discussions 
instead of them trying to find fixes alone.

Ciao
Stephan