2020-07-13 16:49:36

by Elena Petrova

[permalink] [raw]
Subject: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface

This patch extends the userspace RNG interface to make it usable for
CAVS testing. This is achieved by adding ALG_SET_DRBG_ENTROPY
option to the setsockopt interface for specifying the entropy, and using
sendmsg syscall for specifying the additional data.

See libkcapi patch [1] to test the added functionality. The libkcapi
patch is not intended for merging into libkcapi as is: it is only a
quick plug to manually verify that the extended AF_ALG RNG interface
generates the expected output on DRBG800-90A CAVS inputs.

[1] https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531

Elena Petrova (1):
crypto: af_alg - add extra parameters for DRBG interface

Documentation/crypto/userspace-if.rst | 14 +++-
crypto/af_alg.c | 8 +++
crypto/algif_rng.c | 99 +++++++++++++++++++++++++--
include/crypto/if_alg.h | 3 +-
include/uapi/linux/if_alg.h | 1 +
5 files changed, 115 insertions(+), 10 deletions(-)

--
2.27.0.383.g050319c2ae-goog


2020-07-13 16:49:36

by Elena Petrova

[permalink] [raw]
Subject: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface

Extending the userspace RNG interface:
1. adding ALG_SET_DRBG_ENTROPY setsockopt option for entropy input;
2. using sendmsg syscall for specifying the additional data.

Signed-off-by: Elena Petrova <[email protected]>
---
Documentation/crypto/userspace-if.rst | 14 +++-
crypto/af_alg.c | 8 +++
crypto/algif_rng.c | 99 +++++++++++++++++++++++++--
include/crypto/if_alg.h | 3 +-
include/uapi/linux/if_alg.h | 1 +
5 files changed, 115 insertions(+), 10 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index ff86befa61e0..9ee9e93e9207 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -296,15 +296,20 @@ follows:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
- .salg_type = "rng", /* this selects the symmetric cipher */
- .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
+ .salg_type = "rng", /* this selects the random number generator */
+ .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
};


Depending on the RNG type, the RNG must be seeded. The seed is provided
using the setsockopt interface to set the key. For example, the
ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
-seeded.
+seeded. The seed is also known as a *Personalization String* in DRBG800-90A
+standard.
+
+For the purpose of CAVS testing, the concatenation of *Entropy* and *Nonce*
+can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface.
+*Additional Data* can be provided using the send()/sendmsg() system calls.

Using the read()/recvmsg() system calls, random numbers can be obtained.
The kernel generates at most 128 bytes in one call. If user space
@@ -377,6 +382,9 @@ mentioned optname:
provided ciphertext is assumed to contain an authentication tag of
the given size (see section about AEAD memory layout below).

+- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
+ This option is applicable to RNG cipher type only.
+
User space API example
----------------------

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index b1cd3535c525..27d6248ca447 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -260,6 +260,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+ break;
+ case ALG_SET_DRBG_ENTROPY:
+ if (sock->state == SS_CONNECTED)
+ goto unlock;
+ if (!type->setentropy)
+ goto unlock;
+
+ err = type->setentropy(ask->private, optval, optlen);
}

unlock:
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 087c0ad09d38..c3d1667db367 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -53,8 +53,24 @@ struct rng_ctx {
#define MAXSIZE 128
unsigned int len;
struct crypto_rng *drng;
+ u8 *addtl;
+ size_t addtl_len;
};

+struct rng_parent_ctx {
+ struct crypto_rng *drng;
+ u8 *entropy;
+};
+
+static void reset_addtl(struct rng_ctx *ctx)
+{
+ if (ctx->addtl) {
+ kzfree(ctx->addtl);
+ ctx->addtl = NULL;
+ }
+ ctx->addtl_len = 0;
+}
+
static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
int flags)
{
@@ -82,16 +98,39 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
* seeding as they automatically seed. The X9.31 DRNG will return
* an error if it was not seeded properly.
*/
- genlen = crypto_rng_get_bytes(ctx->drng, result, len);
+ genlen = crypto_rng_generate(ctx->drng, ctx->addtl, ctx->addtl_len,
+ result, len);
if (genlen < 0)
return genlen;

err = memcpy_to_msg(msg, result, len);
memzero_explicit(result, len);
+ reset_addtl(ctx);

return err ? err : len;
}

+static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sock->sk);
+ struct rng_ctx *ctx = ask->private;
+
+ reset_addtl(ctx);
+ ctx->addtl = kzalloc(len, GFP_KERNEL);
+ if (!ctx->addtl)
+ return -ENOMEM;
+
+ err = memcpy_from_msg(ctx->addtl, msg, len);
+ if (err) {
+ reset_addtl(ctx);
+ return err;
+ }
+ ctx->addtl_len = len;
+
+ return 0;
+}
+
static struct proto_ops algif_rng_ops = {
.family = PF_ALG,

@@ -106,21 +145,40 @@ static struct proto_ops algif_rng_ops = {
.bind = sock_no_bind,
.accept = sock_no_accept,
.setsockopt = sock_no_setsockopt,
- .sendmsg = sock_no_sendmsg,
.sendpage = sock_no_sendpage,

.release = af_alg_release,
.recvmsg = rng_recvmsg,
+ .sendmsg = rng_sendmsg,
};

static void *rng_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_rng(name, type, mask);
+ struct rng_parent_ctx *pctx;
+ void *err_ptr;
+
+ pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+ if (!pctx)
+ return ERR_PTR(-ENOMEM);
+
+ pctx->drng = crypto_alloc_rng(name, type, mask);
+ if (!IS_ERR(pctx->drng))
+ return pctx;
+
+ err_ptr = pctx->drng;
+ kfree(pctx);
+ return err_ptr;
}

static void rng_release(void *private)
{
- crypto_free_rng(private);
+ struct rng_parent_ctx *pctx = private;
+ if (unlikely(!pctx))
+ return;
+ crypto_free_rng(pctx->drng);
+ if (pctx->entropy)
+ kzfree(pctx->entropy);
+ kzfree(pctx);
}

static void rng_sock_destruct(struct sock *sk)
@@ -128,6 +186,7 @@ static void rng_sock_destruct(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;

+ reset_addtl(ctx);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -135,6 +194,7 @@ static void rng_sock_destruct(struct sock *sk)
static int rng_accept_parent(void *private, struct sock *sk)
{
struct rng_ctx *ctx;
+ struct rng_parent_ctx *pctx = private;
struct alg_sock *ask = alg_sk(sk);
unsigned int len = sizeof(*ctx);

@@ -150,7 +210,9 @@ static int rng_accept_parent(void *private, struct sock *sk)
* state of the RNG.
*/

- ctx->drng = private;
+ ctx->drng = pctx->drng;
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
ask->private = ctx;
sk->sk_destruct = rng_sock_destruct;

@@ -159,11 +221,35 @@ static int rng_accept_parent(void *private, struct sock *sk)

static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
{
+ struct rng_parent_ctx *pctx = private;
/*
* Check whether seedlen is of sufficient size is done in RNG
* implementations.
*/
- return crypto_rng_reset(private, seed, seedlen);
+ return crypto_rng_reset(pctx->drng, seed, seedlen);
+}
+
+static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
+{
+ struct rng_parent_ctx *pctx = private;
+ u8 *kentropy = NULL;
+
+ if (pctx->entropy)
+ return -EINVAL;
+
+ if (entropy && len) {
+ kentropy = kzalloc(len, GFP_KERNEL);
+ if (!kentropy)
+ return -ENOMEM;
+ if (copy_from_user(kentropy, entropy, len)) {
+ kzfree(kentropy);
+ return -EFAULT;
+ }
+ }
+
+ crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
+ pctx->entropy = kentropy;
+ return 0;
}

static const struct af_alg_type algif_type_rng = {
@@ -171,6 +257,7 @@ static const struct af_alg_type algif_type_rng = {
.release = rng_release,
.accept = rng_accept_parent,
.setkey = rng_setkey,
+ .setentropy = rng_setentropy,
.ops = &algif_rng_ops,
.name = "rng",
.owner = THIS_MODULE
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 56527c85d122..312fdb3469cf 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -46,6 +46,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
@@ -123,7 +124,7 @@ struct af_alg_async_req {
* @tsgl_list: Link to TX SGL
* @iv: IV for cipher operation
* @aead_assoclen: Length of AAD for AEAD cipher operations
- * @completion: Work queue for synchronous operation
+ * @wait: Wait on completion of async crypto ops
* @used: TX bytes sent to kernel. This variable is used to
* ensure that user space cannot cause the kernel
* to allocate too much memory in sendmsg operation.
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..60b7c2efd921 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_DRBG_ENTROPY 6

/* Operations */
#define ALG_OP_DECRYPT 0
--
2.27.0.383.g050319c2ae-goog

2020-07-13 17:12:43

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface

Just some bike-shedding:

On Mon, Jul 13, 2020 at 05:48:57PM +0100, Elena Petrova wrote:
> Extending the userspace RNG interface:
> 1. adding ALG_SET_DRBG_ENTROPY setsockopt option for entropy input;
> 2. using sendmsg syscall for specifying the additional data.
>
> Signed-off-by: Elena Petrova <[email protected]>

A cover letter shouldn't really be used for a single patch.
Just put the details here in the commit message.

> diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
> index 087c0ad09d38..c3d1667db367 100644
> --- a/crypto/algif_rng.c
> +++ b/crypto/algif_rng.c
> @@ -53,8 +53,24 @@ struct rng_ctx {
> #define MAXSIZE 128
> unsigned int len;
> struct crypto_rng *drng;
> + u8 *addtl;
> + size_t addtl_len;
> };
>
> +struct rng_parent_ctx {
> + struct crypto_rng *drng;
> + u8 *entropy;
> +};
> +
> +static void reset_addtl(struct rng_ctx *ctx)
> +{
> + if (ctx->addtl) {
> + kzfree(ctx->addtl);
> + ctx->addtl = NULL;
> + }
> + ctx->addtl_len = 0;
> +}

It's recommended to prefix function names. So, reset_addtl => rng_reset_addtl.

> +static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> +{
> + int err;
> + struct alg_sock *ask = alg_sk(sock->sk);
> + struct rng_ctx *ctx = ask->private;
> +
> + reset_addtl(ctx);
> + ctx->addtl = kzalloc(len, GFP_KERNEL);
> + if (!ctx->addtl)
> + return -ENOMEM;

Shouldn't the length be limited here?

Also, kmalloc would be sufficient since the memcpy_from_msg() immediately below
initializes the memory.

> +
> + err = memcpy_from_msg(ctx->addtl, msg, len);
> + if (err) {
> + reset_addtl(ctx);
> + return err;
> + }
> + ctx->addtl_len = len;
> +
> + return 0;
> +}

> static void *rng_bind(const char *name, u32 type, u32 mask)
> {
> - return crypto_alloc_rng(name, type, mask);
> + struct rng_parent_ctx *pctx;
> + void *err_ptr;
> +
> + pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
> + if (!pctx)
> + return ERR_PTR(-ENOMEM);
> +
> + pctx->drng = crypto_alloc_rng(name, type, mask);
> + if (!IS_ERR(pctx->drng))
> + return pctx;
> +
> + err_ptr = pctx->drng;
> + kfree(pctx);
> + return err_ptr;
> }

The error handling here is weird. It would be more conventional to do something
like:

static void *rng_bind(const char *name, u32 type, u32 mask)
{
struct rng_parent_ctx *pctx;
struct crypto_rng *rng;

pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
if (!pctx)
return ERR_PTR(-ENOMEM);

rng = crypto_alloc_rng(name, type, mask);
if (IS_ERR(rng)) {
kfree(pctx);
return ERR_CAST(rng);
}

pctx->drng = rng;
return pctx;
}

>
> static void rng_release(void *private)
> {
> - crypto_free_rng(private);
> + struct rng_parent_ctx *pctx = private;
> + if (unlikely(!pctx))
> + return;

There should be a blank line between declarations and statements.

> + crypto_free_rng(pctx->drng);
> + if (pctx->entropy)
> + kzfree(pctx->entropy);

No need to check for NULL before calling kzfree().

> +static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
> +{
> + struct rng_parent_ctx *pctx = private;
> + u8 *kentropy = NULL;
> +
> + if (pctx->entropy)
> + return -EINVAL;
> +
> + if (entropy && len) {

Best to check just 'len', so that users get an error as expected if they
accidentally pass entry=NULL len=nonzero.

> + kentropy = kzalloc(len, GFP_KERNEL);
> + if (!kentropy)
> + return -ENOMEM;
> + if (copy_from_user(kentropy, entropy, len)) {
> + kzfree(kentropy);
> + return -EFAULT;
> + }

This can use memdup_user(). Also, should there be a length limit?

> + }
> +
> + crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
> + pctx->entropy = kentropy;

pctx->entropy could just be a bool 'has_entropy', right? The actual value
doesn't need to be saved.

- Eric

2020-07-13 17:25:41

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface

On Mon, Jul 13, 2020 at 05:48:57PM +0100, Elena Petrova wrote:
> +static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> +{
> + int err;
> + struct alg_sock *ask = alg_sk(sock->sk);
> + struct rng_ctx *ctx = ask->private;
> +
> + reset_addtl(ctx);
> + ctx->addtl = kzalloc(len, GFP_KERNEL);
> + if (!ctx->addtl)
> + return -ENOMEM;
> +
> + err = memcpy_from_msg(ctx->addtl, msg, len);
> + if (err) {
> + reset_addtl(ctx);
> + return err;
> + }
> + ctx->addtl_len = len;
> +
> + return 0;
> +}

This is also missing any sort of locking, both between concurrent calls to
rng_sendmsg(), and between rng_sendmsg() and rng_recvmsg().

lock_sock() would solve the former. I'm not sure what should be done about
rng_recvmsg(). It apparently relies on the crypto_rng doing its own locking,
but maybe it should just use lock_sock() too.

- Eric

2020-07-14 05:18:07

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface

Am Montag, 13. Juli 2020, 18:48:56 CEST schrieb Elena Petrova:

Hi Elena,

> This patch extends the userspace RNG interface to make it usable for
> CAVS testing. This is achieved by adding ALG_SET_DRBG_ENTROPY
> option to the setsockopt interface for specifying the entropy, and using
> sendmsg syscall for specifying the additional data.
>
> See libkcapi patch [1] to test the added functionality. The libkcapi
> patch is not intended for merging into libkcapi as is: it is only a
> quick plug to manually verify that the extended AF_ALG RNG interface
> generates the expected output on DRBG800-90A CAVS inputs.

As I am responsible for developing such CAVS/ACVP harness as well, I played
with the idea of going through AF_ALG. I discarded it because I do not see the
benefit why we should add an interface solely for the purpose of testing.
Further, it is a potentially dangerous one because the created instance of the
DRBG is "seeded" from data provided by the caller.

Thus, I do not see the benefit from adding that extension, widening a user
space interface solely for the purpose of CAVS testing. I would not see any
other benefit we have with this extension. In particular, this interface would
then be always there. What I could live with is an interface that can be
enabled at compile time for those who want it.

Besides, when you want to do CAVS testing, the following ciphers are still not
testable and thus this patch would only be a partial solution to get the
testing covered:

- AES KW (you cannot get the final IV out of the kernel - I played with the
idea to return the IV through AF_ALG, but discarded it because of the concern
above)

- OFB/CFB MCT testing (you need the IV from the last round - same issue as for
AES KW)

- RSA

- DH

- ECDH

With these issues, I would assume you are better off creating your own kernel
module just like I did that externalizes the crypto API to user space but is
only available on your test kernel and will not affect all other users.

Ciao
Stephan


2020-07-14 15:23:56

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface

Hi Stephan,

On Tue, 14 Jul 2020 at 06:17, Stephan Mueller <[email protected]> wrote:
>
> Am Montag, 13. Juli 2020, 18:48:56 CEST schrieb Elena Petrova:
>
> Hi Elena,
>
> > This patch extends the userspace RNG interface to make it usable for
> > CAVS testing. This is achieved by adding ALG_SET_DRBG_ENTROPY
> > option to the setsockopt interface for specifying the entropy, and using
> > sendmsg syscall for specifying the additional data.
> >
> > See libkcapi patch [1] to test the added functionality. The libkcapi
> > patch is not intended for merging into libkcapi as is: it is only a
> > quick plug to manually verify that the extended AF_ALG RNG interface
> > generates the expected output on DRBG800-90A CAVS inputs.
>
> As I am responsible for developing such CAVS/ACVP harness as well, I played
> with the idea of going through AF_ALG. I discarded it because I do not see the
> benefit why we should add an interface solely for the purpose of testing.
> Further, it is a potentially dangerous one because the created instance of the
> DRBG is "seeded" from data provided by the caller.
>
> Thus, I do not see the benefit from adding that extension, widening a user
> space interface solely for the purpose of CAVS testing. I would not see any
> other benefit we have with this extension. In particular, this interface would
> then be always there. What I could live with is an interface that can be
> enabled at compile time for those who want it.

Thanks for reviewing this patch. I understand your concern about the erroneous
use of the entropy input by non-testing applications. This was an approach that
I had discussed with Ard. I should have included you, my apologies. I'll post
v2 with the CAVS testing stuff hidden under CONFIG_ option with appropriate help
text.

With regards to the usefulness, let me elaborate. This effort of extending the
drbg interface is driven by Android needs for having the kernel crypto
certified. I started from having an out-of-tree chrdev driver for Google Pixel
kernels that was exposing the required crypto functionality, and it wasn't ideal
in the following ways:
* it primarily consisted of copypasted code from testmgr.c
* it was hard for me to keep the code up to date because I'm not aware of
ongoing changes to crypto
* it is hard for other people and/or organisations to re-use it, hense a lot of
duplicated effort is going into CAVS: you have a private driver, I have mine,
Jaya from HP <[email protected]>, who's been asking linux-crypto a few
CAVS questions, has to develop theirs...

In general Android is trying to eliminate out-of-tree code. CAVS testing
functionality in particular needs to be upstream because we are switching all
Android devices to using a Generic Kernel Image (GKI)
[https://lwn.net/Articles/771974/] based on the upstream kernel.

> Besides, when you want to do CAVS testing, the following ciphers are still not
> testable and thus this patch would only be a partial solution to get the
> testing covered:
>
> - AES KW (you cannot get the final IV out of the kernel - I played with the
> idea to return the IV through AF_ALG, but discarded it because of the concern
> above)
>
> - OFB/CFB MCT testing (you need the IV from the last round - same issue as for
> AES KW)
>
> - RSA
>
> - DH
>
> - ECDH

For Android certification purposes, we only need to test the modules which are
actually being used. In our case it's what af_alg already exposes plus DRBG.
If, say, HP would need RSA, they could submit their own patch.

As for exposing bits of the internal state for some algorithms, I hope guarding
the testing functionality with a CONFIG_ option would solve the security part of
the problem.

> With these issues, I would assume you are better off creating your own kernel
> module just like I did that externalizes the crypto API to user space but is
> only available on your test kernel and will not affect all other users.

I considered publishing my kernel driver on GitHub, but there appears to be a
sufficiently large number of users to justify having this functionality
upstream.

Hope that addresses your concerns.

> Ciao
> Stephan

Thanks,
Elena

2020-07-14 15:35:36

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface

Am Dienstag, 14. Juli 2020, 17:23:05 CEST schrieb Elena Petrova:

Hi Elena,

> Hi Stephan,
>
> On Tue, 14 Jul 2020 at 06:17, Stephan Mueller <[email protected]> wrote:
> > Am Montag, 13. Juli 2020, 18:48:56 CEST schrieb Elena Petrova:
> >
> > Hi Elena,
> >
> > > This patch extends the userspace RNG interface to make it usable for
> > > CAVS testing. This is achieved by adding ALG_SET_DRBG_ENTROPY
> > > option to the setsockopt interface for specifying the entropy, and using
> > > sendmsg syscall for specifying the additional data.
> > >
> > > See libkcapi patch [1] to test the added functionality. The libkcapi
> > > patch is not intended for merging into libkcapi as is: it is only a
> > > quick plug to manually verify that the extended AF_ALG RNG interface
> > > generates the expected output on DRBG800-90A CAVS inputs.
> >
> > As I am responsible for developing such CAVS/ACVP harness as well, I
> > played
> > with the idea of going through AF_ALG. I discarded it because I do not see
> > the benefit why we should add an interface solely for the purpose of
> > testing. Further, it is a potentially dangerous one because the created
> > instance of the DRBG is "seeded" from data provided by the caller.
> >
> > Thus, I do not see the benefit from adding that extension, widening a user
> > space interface solely for the purpose of CAVS testing. I would not see
> > any
> > other benefit we have with this extension. In particular, this interface
> > would then be always there. What I could live with is an interface that
> > can be enabled at compile time for those who want it.
>
> Thanks for reviewing this patch. I understand your concern about the
> erroneous use of the entropy input by non-testing applications. This was an
> approach that I had discussed with Ard. I should have included you, my
> apologies. I'll post v2 with the CAVS testing stuff hidden under CONFIG_
> option with appropriate help text.
>
> With regards to the usefulness, let me elaborate. This effort of extending
> the drbg interface is driven by Android needs for having the kernel crypto
> certified. I started from having an out-of-tree chrdev driver for Google
> Pixel kernels that was exposing the required crypto functionality, and it
> wasn't ideal in the following ways:
> * it primarily consisted of copypasted code from testmgr.c
> * it was hard for me to keep the code up to date because I'm not aware of
> ongoing changes to crypto
> * it is hard for other people and/or organisations to re-use it, hense a
> lot of duplicated effort is going into CAVS: you have a private driver, I
> have mine, Jaya from HP <[email protected]>, who's been asking
> linux-crypto a few CAVS questions, has to develop theirs...
>
> In general Android is trying to eliminate out-of-tree code. CAVS testing
> functionality in particular needs to be upstream because we are switching
> all Android devices to using a Generic Kernel Image (GKI)
> [https://lwn.net/Articles/771974/] based on the upstream kernel.

Thank you for the explanation.
>
> > Besides, when you want to do CAVS testing, the following ciphers are still
> > not testable and thus this patch would only be a partial solution to get
> > the testing covered:
> >
> > - AES KW (you cannot get the final IV out of the kernel - I played with
> > the
> > idea to return the IV through AF_ALG, but discarded it because of the
> > concern above)
> >
> > - OFB/CFB MCT testing (you need the IV from the last round - same issue as
> > for AES KW)
> >
> > - RSA
> >
> > - DH
> >
> > - ECDH
>
> For Android certification purposes, we only need to test the modules which
> are actually being used. In our case it's what af_alg already exposes plus
> DRBG. If, say, HP would need RSA, they could submit their own patch.
>
> As for exposing bits of the internal state for some algorithms, I hope
> guarding the testing functionality with a CONFIG_ option would solve the
> security part of the problem.

Yes, for all other users.

But if you are planning to enable this option for all Android devices across
the board I am not sure here. In this case, wouldn't it make sense to require
capable(CAP_SYS_ADMIN) for the DRBG reset operation just as an additional
precaution? Note, the issue with the reset is that you loose all previous
state (which is good for testing, but bad for security as I guess you agree
:-) ).
>
> > With these issues, I would assume you are better off creating your own
> > kernel module just like I did that externalizes the crypto API to user
> > space but is only available on your test kernel and will not affect all
> > other users.
> I considered publishing my kernel driver on GitHub, but there appears to be
> a sufficiently large number of users to justify having this functionality
> upstream.

So, I should then dust off my AF_ALG KPP and AF_ALG akcipher patches then? :-D
>
> Hope that addresses your concerns.
>
> > Ciao
> > Stephan
>
> Thanks,
> Elena


Ciao
Stephan


2020-07-16 14:24:58

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface

Thank you for the review, Eric, I'll address your comments in v2.

On Mon, 13 Jul 2020 at 18:10, Eric Biggers <[email protected]> wrote:
>
> Just some bike-shedding:
>
> On Mon, Jul 13, 2020 at 05:48:57PM +0100, Elena Petrova wrote:
> > Extending the userspace RNG interface:
> > 1. adding ALG_SET_DRBG_ENTROPY setsockopt option for entropy input;
> > 2. using sendmsg syscall for specifying the additional data.
> >
> > Signed-off-by: Elena Petrova <[email protected]>
>
> A cover letter shouldn't really be used for a single patch.
> Just put the details here in the commit message.

Ack

> > diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
> > index 087c0ad09d38..c3d1667db367 100644
> > --- a/crypto/algif_rng.c
> > +++ b/crypto/algif_rng.c
> > @@ -53,8 +53,24 @@ struct rng_ctx {
> > #define MAXSIZE 128
> > unsigned int len;
> > struct crypto_rng *drng;
> > + u8 *addtl;
> > + size_t addtl_len;
> > };
> >
> > +struct rng_parent_ctx {
> > + struct crypto_rng *drng;
> > + u8 *entropy;
> > +};
> > +
> > +static void reset_addtl(struct rng_ctx *ctx)
> > +{
> > + if (ctx->addtl) {
> > + kzfree(ctx->addtl);
> > + ctx->addtl = NULL;
> > + }
> > + ctx->addtl_len = 0;
> > +}
>
> It's recommended to prefix function names. So, reset_addtl => rng_reset_addtl.

Ack

> > +static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> > +{
> > + int err;
> > + struct alg_sock *ask = alg_sk(sock->sk);
> > + struct rng_ctx *ctx = ask->private;
> > +
> > + reset_addtl(ctx);
> > + ctx->addtl = kzalloc(len, GFP_KERNEL);
> > + if (!ctx->addtl)
> > + return -ENOMEM;
>
> Shouldn't the length be limited here?
>
> Also, kmalloc would be sufficient since the memcpy_from_msg() immediately below
> initializes the memory.

Good point, I'll use the same limit as for the recv(). Ack kzalloc/kmalloc.

> > +
> > + err = memcpy_from_msg(ctx->addtl, msg, len);
> > + if (err) {
> > + reset_addtl(ctx);
> > + return err;
> > + }
> > + ctx->addtl_len = len;
> > +
> > + return 0;
> > +}
>
> > static void *rng_bind(const char *name, u32 type, u32 mask)
> > {
> > - return crypto_alloc_rng(name, type, mask);
> > + struct rng_parent_ctx *pctx;
> > + void *err_ptr;
> > +
> > + pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
> > + if (!pctx)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + pctx->drng = crypto_alloc_rng(name, type, mask);
> > + if (!IS_ERR(pctx->drng))
> > + return pctx;
> > +
> > + err_ptr = pctx->drng;
> > + kfree(pctx);
> > + return err_ptr;
> > }
>
> The error handling here is weird. It would be more conventional to do something
> like:
>
> static void *rng_bind(const char *name, u32 type, u32 mask)
> {
> struct rng_parent_ctx *pctx;
> struct crypto_rng *rng;
>
> pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
> if (!pctx)
> return ERR_PTR(-ENOMEM);
>
> rng = crypto_alloc_rng(name, type, mask);
> if (IS_ERR(rng)) {
> kfree(pctx);
> return ERR_CAST(rng);
> }
>
> pctx->drng = rng;
> return pctx;
> }

Thanks, I will use your variant.

> > static void rng_release(void *private)
> > {
> > - crypto_free_rng(private);
> > + struct rng_parent_ctx *pctx = private;
> > + if (unlikely(!pctx))
> > + return;
>
> There should be a blank line between declarations and statements.

Ack

> > + crypto_free_rng(pctx->drng);
> > + if (pctx->entropy)
> > + kzfree(pctx->entropy);
>
> No need to check for NULL before calling kzfree().

Ack

> > +static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
> > +{
> > + struct rng_parent_ctx *pctx = private;
> > + u8 *kentropy = NULL;
> > +
> > + if (pctx->entropy)
> > + return -EINVAL;
> > +
> > + if (entropy && len) {
>
> Best to check just 'len', so that users get an error as expected if they
> accidentally pass entry=NULL len=nonzero.

Ack

> > + kentropy = kzalloc(len, GFP_KERNEL);
> > + if (!kentropy)
> > + return -ENOMEM;
> > + if (copy_from_user(kentropy, entropy, len)) {
> > + kzfree(kentropy);
> > + return -EFAULT;
> > + }
>
> This can use memdup_user(). Also, should there be a length limit?

Alright, changed to memdup_user() and added the same limit as in send and recv.

> > + }
> > +
> > + crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
> > + pctx->entropy = kentropy;
>
> pctx->entropy could just be a bool 'has_entropy', right? The actual value
> doesn't need to be saved.

I need to keep the pointer to free it after use. DRBG saves the
pointer in one of its internal structures, but doesn't do any memory
management. So I had to either change drbg code to deal with the
memory, or save the pointer somewhere inside the socket. I opted for
the latter.

> > +static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> > +{
> > + int err;
> > + struct alg_sock *ask = alg_sk(sock->sk);
> > + struct rng_ctx *ctx = ask->private;
> > +
> > + reset_addtl(ctx);
> > + ctx->addtl = kzalloc(len, GFP_KERNEL);
> > + if (!ctx->addtl)
> > + return -ENOMEM;
> > +
> > + err = memcpy_from_msg(ctx->addtl, msg, len);
> > + if (err) {
> > + reset_addtl(ctx);
> > + return err;
> > + }
> > + ctx->addtl_len = len;
> > +
> > + return 0;
> > +}
>
> This is also missing any sort of locking, both between concurrent calls to
> rng_sendmsg(), and between rng_sendmsg() and rng_recvmsg().
>
> lock_sock() would solve the former. I'm not sure what should be done about
> rng_recvmsg(). It apparently relies on the crypto_rng doing its own locking,
> but maybe it should just use lock_sock() too.

Thanks, I've added lock_sock() to both.

> - Eric

2020-07-16 14:44:44

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface

Hi Stephan,

On Tue, 14 Jul 2020 at 16:34, Stephan Mueller <[email protected]> wrote:
>
> Am Dienstag, 14. Juli 2020, 17:23:05 CEST schrieb Elena Petrova:
>
> Hi Elena,
>
> > Hi Stephan,
> >
> > On Tue, 14 Jul 2020 at 06:17, Stephan Mueller <[email protected]> wrote:
> > > Am Montag, 13. Juli 2020, 18:48:56 CEST schrieb Elena Petrova:
> > >
> > > Hi Elena,
> > >
> > > > This patch extends the userspace RNG interface to make it usable for
> > > > CAVS testing. This is achieved by adding ALG_SET_DRBG_ENTROPY
> > > > option to the setsockopt interface for specifying the entropy, and using
> > > > sendmsg syscall for specifying the additional data.
> > > >
> > > > See libkcapi patch [1] to test the added functionality. The libkcapi
> > > > patch is not intended for merging into libkcapi as is: it is only a
> > > > quick plug to manually verify that the extended AF_ALG RNG interface
> > > > generates the expected output on DRBG800-90A CAVS inputs.
> > >
> > > As I am responsible for developing such CAVS/ACVP harness as well, I
> > > played
> > > with the idea of going through AF_ALG. I discarded it because I do not see
> > > the benefit why we should add an interface solely for the purpose of
> > > testing. Further, it is a potentially dangerous one because the created
> > > instance of the DRBG is "seeded" from data provided by the caller.
> > >
> > > Thus, I do not see the benefit from adding that extension, widening a user
> > > space interface solely for the purpose of CAVS testing. I would not see
> > > any
> > > other benefit we have with this extension. In particular, this interface
> > > would then be always there. What I could live with is an interface that
> > > can be enabled at compile time for those who want it.
> >
> > Thanks for reviewing this patch. I understand your concern about the
> > erroneous use of the entropy input by non-testing applications. This was an
> > approach that I had discussed with Ard. I should have included you, my
> > apologies. I'll post v2 with the CAVS testing stuff hidden under CONFIG_
> > option with appropriate help text.
> >
> > With regards to the usefulness, let me elaborate. This effort of extending
> > the drbg interface is driven by Android needs for having the kernel crypto
> > certified. I started from having an out-of-tree chrdev driver for Google
> > Pixel kernels that was exposing the required crypto functionality, and it
> > wasn't ideal in the following ways:
> > * it primarily consisted of copypasted code from testmgr.c
> > * it was hard for me to keep the code up to date because I'm not aware of
> > ongoing changes to crypto
> > * it is hard for other people and/or organisations to re-use it, hense a
> > lot of duplicated effort is going into CAVS: you have a private driver, I
> > have mine, Jaya from HP <[email protected]>, who's been asking
> > linux-crypto a few CAVS questions, has to develop theirs...
> >
> > In general Android is trying to eliminate out-of-tree code. CAVS testing
> > functionality in particular needs to be upstream because we are switching
> > all Android devices to using a Generic Kernel Image (GKI)
> > [https://lwn.net/Articles/771974/] based on the upstream kernel.
>
> Thank you for the explanation.
> >
> > > Besides, when you want to do CAVS testing, the following ciphers are still
> > > not testable and thus this patch would only be a partial solution to get
> > > the testing covered:
> > >
> > > - AES KW (you cannot get the final IV out of the kernel - I played with
> > > the
> > > idea to return the IV through AF_ALG, but discarded it because of the
> > > concern above)
> > >
> > > - OFB/CFB MCT testing (you need the IV from the last round - same issue as
> > > for AES KW)
> > >
> > > - RSA
> > >
> > > - DH
> > >
> > > - ECDH
> >
> > For Android certification purposes, we only need to test the modules which
> > are actually being used. In our case it's what af_alg already exposes plus
> > DRBG. If, say, HP would need RSA, they could submit their own patch.
> >
> > As for exposing bits of the internal state for some algorithms, I hope
> > guarding the testing functionality with a CONFIG_ option would solve the
> > security part of the problem.
>
> Yes, for all other users.
>
> But if you are planning to enable this option for all Android devices across
> the board I am not sure here. In this case, wouldn't it make sense to require
> capable(CAP_SYS_ADMIN) for the DRBG reset operation just as an additional
> precaution? Note, the issue with the reset is that you loose all previous
> state (which is good for testing, but bad for security as I guess you agree
> :-) ).

I believe that for Android, since CAVS is a one-off on demand
operation, we can create a separate build with CONFIG_CRYPTO_CAVS_DRBG
enabled, which won't go to the users. Thanks for suggesting
capabilities check, happy to add `capable(CAP_SYS_ADMIN)` regardless.

> > > With these issues, I would assume you are better off creating your own
> > > kernel module just like I did that externalizes the crypto API to user
> > > space but is only available on your test kernel and will not affect all
> > > other users.
> > I considered publishing my kernel driver on GitHub, but there appears to be
> > a sufficiently large number of users to justify having this functionality
> > upstream.
>
> So, I should then dust off my AF_ALG KPP and AF_ALG akcipher patches then? :-D

Sure :)

> >
> > Hope that addresses your concerns.
> >
> > > Ciao
> > > Stephan
> >
> > Thanks,
> > Elena
>
>
> Ciao
> Stephan
>
>

2020-07-16 14:50:33

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface

Am Donnerstag, 16. Juli 2020, 16:41:26 CEST schrieb Elena Petrova:

Hi Herbert,

> > > > With these issues, I would assume you are better off creating your own
> > > > kernel module just like I did that externalizes the crypto API to user
> > > > space but is only available on your test kernel and will not affect
> > > > all
> > > > other users.
> > >
> > > I considered publishing my kernel driver on GitHub, but there appears to
> > > be
> > > a sufficiently large number of users to justify having this
> > > functionality
> > > upstream.
> >
> > So, I should then dust off my AF_ALG KPP and AF_ALG akcipher patches then?
> > :-D
> Sure :)

Long time ago when I released the patches now found in [1] and [2] they where
rejected as it was said, the official route to access the RSA/ECDSA and the
DH/ECDH ciphers is through the keyring.

Obviously this interface of the keyring is not suitable for testing these
algorithms. Considering the request that the kernel crypto API ciphers should
be testable with arbitrary test vectors, would the dusted-off patches for
AF_ALG KPP and akcipher be accepted?

Ciao
Stephan

[1] https://github.com/smuellerDD/libkcapi/tree/master/kernel-patches/4.15-rc3/asym

[2] https://github.com/smuellerDD/libkcapi/tree/master/kernel-patches/4.15-rc3/kpp


2020-07-16 15:01:05

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface

Am Donnerstag, 16. Juli 2020, 16:49:52 CEST schrieb Stephan Mueller:

Hi Herbert,

(resent, adding Herbert to the list and fix the keyrings address)

> Am Donnerstag, 16. Juli 2020, 16:41:26 CEST schrieb Elena Petrova:
>
> Hi Herbert,
>
> > > > > With these issues, I would assume you are better off creating your
> > > > > own
> > > > > kernel module just like I did that externalizes the crypto API to
> > > > > user
> > > > > space but is only available on your test kernel and will not affect
> > > > > all
> > > > > other users.
> > > >
> > > > I considered publishing my kernel driver on GitHub, but there appears
> > > > to
> > > > be
> > > > a sufficiently large number of users to justify having this
> > > > functionality
> > > > upstream.
> > >
> > > So, I should then dust off my AF_ALG KPP and AF_ALG akcipher patches
> > > then?
> > >
> > > :-D
> >
> > Sure :)
>
> Long time ago when I released the patches now found in [1] and [2] they
> where rejected as it was said, the official route to access the RSA/ECDSA
> and the DH/ECDH ciphers is through the keyring.
>
> Obviously this interface of the keyring is not suitable for testing these
> algorithms. Considering the request that the kernel crypto API ciphers
> should be testable with arbitrary test vectors, would the dusted-off
> patches for AF_ALG KPP and akcipher be accepted?
>
> Ciao
> Stephan
>
> [1]
> https://github.com/smuellerDD/libkcapi/tree/master/kernel-patches/4.15-rc3/
> asym
>
> [2]
> https://github.com/smuellerDD/libkcapi/tree/master/kernel-patches/4.15-rc3/
> kpp


Ciao
Stephan


2020-07-16 16:42:22

by Elena Petrova

[permalink] [raw]
Subject: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface

Extending the userspace RNG interface:
1. adding ALG_SET_DRBG_ENTROPY setsockopt option for entropy input;
2. using sendmsg syscall for specifying the additional data.

Signed-off-by: Elena Petrova <[email protected]>
---

libkcapi patch for testing:
https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531

Updates in v2:
1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
2) Requiring CAP_SYS_ADMIN for entropy reset.
3) Locking for send and recv.
4) Length checks added for send and setentropy; send and setentropy now return
number of bytes accepted.
5) Minor code style corrections.

Documentation/crypto/userspace-if.rst | 17 +++-
crypto/Kconfig | 8 ++
crypto/af_alg.c | 8 ++
crypto/algif_rng.c | 112 ++++++++++++++++++++++++--
include/crypto/if_alg.h | 3 +-
include/uapi/linux/if_alg.h | 1 +
6 files changed, 139 insertions(+), 10 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index ff86befa61e0..c3695d2c7e0b 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -296,15 +296,23 @@ follows:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
- .salg_type = "rng", /* this selects the symmetric cipher */
- .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
+ .salg_type = "rng", /* this selects the random number generator */
+ .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
};


Depending on the RNG type, the RNG must be seeded. The seed is provided
using the setsockopt interface to set the key. For example, the
ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
-seeded.
+seeded. The seed is also known as a *Personalization String* in DRBG800-90A
+standard.
+
+For the purpose of CAVS testing, the concatenation of *Entropy* and *Nonce*
+can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface. This
+requires a kernel built with CONFIG_CRYPTO_CAVS_DRBG, and CAP_SYS_ADMIN
+permission.
+
+*Additional Data* can be provided using the send()/sendmsg() system calls.

Using the read()/recvmsg() system calls, random numbers can be obtained.
The kernel generates at most 128 bytes in one call. If user space
@@ -377,6 +385,9 @@ mentioned optname:
provided ciphertext is assumed to contain an authentication tag of
the given size (see section about AEAD memory layout below).

+- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
+ This option is applicable to RNG cipher type only.
+
User space API example
----------------------

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 091c0a0bbf26..8484617596d1 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1896,6 +1896,14 @@ config CRYPTO_STATS
config CRYPTO_HASH_INFO
bool

+config CRYPTO_CAVS_DRBG
+ tristate "Enable CAVS testing of DRBG"
+ depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
+ help
+ This option enables the resetting of DRBG entropy via the user-space
+ interface. This should only be enabled for CAVS testing. You should say
+ no unless you know what this is.
+
source "lib/crypto/Kconfig"
source "drivers/crypto/Kconfig"
source "crypto/asymmetric_keys/Kconfig"
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index b1cd3535c525..27d6248ca447 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -260,6 +260,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+ break;
+ case ALG_SET_DRBG_ENTROPY:
+ if (sock->state == SS_CONNECTED)
+ goto unlock;
+ if (!type->setentropy)
+ goto unlock;
+
+ err = type->setentropy(ask->private, optval, optlen);
}

unlock:
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 087c0ad09d38..8a3b0eb45a85 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -38,6 +38,7 @@
* DAMAGE.
*/

+#include <linux/capability.h>
#include <linux/module.h>
#include <crypto/rng.h>
#include <linux/random.h>
@@ -53,8 +54,22 @@ struct rng_ctx {
#define MAXSIZE 128
unsigned int len;
struct crypto_rng *drng;
+ u8 *addtl;
+ size_t addtl_len;
};

+struct rng_parent_ctx {
+ struct crypto_rng *drng;
+ u8 *entropy;
+};
+
+static void rng_reset_addtl(struct rng_ctx *ctx)
+{
+ kzfree(ctx->addtl);
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
+}
+
static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
int flags)
{
@@ -65,6 +80,7 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
int genlen = 0;
u8 result[MAXSIZE];

+ lock_sock(sock->sk);
if (len == 0)
return 0;
if (len > MAXSIZE)
@@ -82,16 +98,45 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
* seeding as they automatically seed. The X9.31 DRNG will return
* an error if it was not seeded properly.
*/
- genlen = crypto_rng_get_bytes(ctx->drng, result, len);
+ genlen = crypto_rng_generate(ctx->drng, ctx->addtl, ctx->addtl_len,
+ result, len);
if (genlen < 0)
return genlen;

err = memcpy_to_msg(msg, result, len);
memzero_explicit(result, len);
+ rng_reset_addtl(ctx);
+ release_sock(sock->sk);

return err ? err : len;
}

+static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sock->sk);
+ struct rng_ctx *ctx = ask->private;
+
+ lock_sock(sock->sk);
+ if (len > MAXSIZE)
+ len = MAXSIZE;
+
+ rng_reset_addtl(ctx);
+ ctx->addtl = kmalloc(len, GFP_KERNEL);
+ if (!ctx->addtl)
+ return -ENOMEM;
+
+ err = memcpy_from_msg(ctx->addtl, msg, len);
+ if (err) {
+ rng_reset_addtl(ctx);
+ return err;
+ }
+ ctx->addtl_len = len;
+ release_sock(sock->sk);
+
+ return len;
+}
+
static struct proto_ops algif_rng_ops = {
.family = PF_ALG,

@@ -106,21 +151,41 @@ static struct proto_ops algif_rng_ops = {
.bind = sock_no_bind,
.accept = sock_no_accept,
.setsockopt = sock_no_setsockopt,
- .sendmsg = sock_no_sendmsg,
.sendpage = sock_no_sendpage,

.release = af_alg_release,
.recvmsg = rng_recvmsg,
+ .sendmsg = rng_sendmsg,
};

static void *rng_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_rng(name, type, mask);
+ struct rng_parent_ctx *pctx;
+ struct crypto_rng *rng;
+
+ pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+ if (!pctx)
+ return ERR_PTR(-ENOMEM);
+
+ rng = crypto_alloc_rng(name, type, mask);
+ if (IS_ERR(rng)) {
+ kfree(pctx);
+ return ERR_CAST(rng);
+ }
+
+ pctx->drng = rng;
+ return pctx;
}

static void rng_release(void *private)
{
- crypto_free_rng(private);
+ struct rng_parent_ctx *pctx = private;
+
+ if (unlikely(!pctx))
+ return;
+ crypto_free_rng(pctx->drng);
+ kzfree(pctx->entropy);
+ kzfree(pctx);
}

static void rng_sock_destruct(struct sock *sk)
@@ -128,6 +193,7 @@ static void rng_sock_destruct(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;

+ rng_reset_addtl(ctx);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -135,6 +201,7 @@ static void rng_sock_destruct(struct sock *sk)
static int rng_accept_parent(void *private, struct sock *sk)
{
struct rng_ctx *ctx;
+ struct rng_parent_ctx *pctx = private;
struct alg_sock *ask = alg_sk(sk);
unsigned int len = sizeof(*ctx);

@@ -150,7 +217,9 @@ static int rng_accept_parent(void *private, struct sock *sk)
* state of the RNG.
*/

- ctx->drng = private;
+ ctx->drng = pctx->drng;
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
ask->private = ctx;
sk->sk_destruct = rng_sock_destruct;

@@ -159,18 +228,49 @@ static int rng_accept_parent(void *private, struct sock *sk)

static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
{
+ struct rng_parent_ctx *pctx = private;
/*
* Check whether seedlen is of sufficient size is done in RNG
* implementations.
*/
- return crypto_rng_reset(private, seed, seedlen);
+ return crypto_rng_reset(pctx->drng, seed, seedlen);
+}
+
+#ifdef CONFIG_CRYPTO_CAVS_DRBG
+static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
+{
+ struct rng_parent_ctx *pctx = private;
+ u8 *kentropy = NULL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (pctx->entropy)
+ return -EINVAL;
+
+ if (len > MAXSIZE)
+ len = MAXSIZE;
+
+ if (len) {
+ kentropy = memdup_user(entropy, len);
+ if (IS_ERR(kentropy))
+ return PTR_ERR(kentropy);
+ }
+
+ crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
+ pctx->entropy = kentropy;
+ return len;
}
+#endif

static const struct af_alg_type algif_type_rng = {
.bind = rng_bind,
.release = rng_release,
.accept = rng_accept_parent,
.setkey = rng_setkey,
+#ifdef CONFIG_CRYPTO_CAVS_DRBG
+ .setentropy = rng_setentropy,
+#endif
.ops = &algif_rng_ops,
.name = "rng",
.owner = THIS_MODULE
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 56527c85d122..312fdb3469cf 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -46,6 +46,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
@@ -123,7 +124,7 @@ struct af_alg_async_req {
* @tsgl_list: Link to TX SGL
* @iv: IV for cipher operation
* @aead_assoclen: Length of AAD for AEAD cipher operations
- * @completion: Work queue for synchronous operation
+ * @wait: Wait on completion of async crypto ops
* @used: TX bytes sent to kernel. This variable is used to
* ensure that user space cannot cause the kernel
* to allocate too much memory in sendmsg operation.
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..60b7c2efd921 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_DRBG_ENTROPY 6

/* Operations */
#define ALG_OP_DECRYPT 0
--
2.27.0.389.gc38d7665816-goog

2020-07-20 17:36:40

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface

Am Donnerstag, 16. Juli 2020, 18:40:28 CEST schrieb Elena Petrova:

Hi Elena,

sorry for the delay in answering.

> Extending the userspace RNG interface:
> 1. adding ALG_SET_DRBG_ENTROPY setsockopt option for entropy input;
> 2. using sendmsg syscall for specifying the additional data.
>
> Signed-off-by: Elena Petrova <[email protected]>
> ---
>
> libkcapi patch for testing:
>
> https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa59
> 2cb531

If that kernel patch is taken, I would be happy to take the libkcapi patch
with the following suggested changes:

- add full documentation of kcapi_rng_set_entropy and kcapi_rng_send_addtl to
kcapi.h

- move test_cavp into either test/kcapi-main.c or its own application inside
test/ where the caller provides the entropy_input, personalization string,
additional inputs
>
> Updates in v2:
> 1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
> 2) Requiring CAP_SYS_ADMIN for entropy reset.
> 3) Locking for send and recv.
> 4) Length checks added for send and setentropy; send and setentropy now
> return number of bytes accepted.
> 5) Minor code style corrections.
>
> Documentation/crypto/userspace-if.rst | 17 +++-
> crypto/Kconfig | 8 ++
> crypto/af_alg.c | 8 ++
> crypto/algif_rng.c | 112 ++++++++++++++++++++++++--
> include/crypto/if_alg.h | 3 +-
> include/uapi/linux/if_alg.h | 1 +
> 6 files changed, 139 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/crypto/userspace-if.rst
> b/Documentation/crypto/userspace-if.rst index ff86befa61e0..c3695d2c7e0b
> 100644
> --- a/Documentation/crypto/userspace-if.rst
> +++ b/Documentation/crypto/userspace-if.rst
> @@ -296,15 +296,23 @@ follows:
>
> struct sockaddr_alg sa = {
> .salg_family = AF_ALG,
> - .salg_type = "rng", /* this selects the symmetric cipher */
> - .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
> + .salg_type = "rng", /* this selects the random number generator */
> + .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
> };
>
>
> Depending on the RNG type, the RNG must be seeded. The seed is provided
> using the setsockopt interface to set the key. For example, the
> ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
> -seeded.
> +seeded. The seed is also known as a *Personalization String* in DRBG800-90A
> +standard.
> +
> +For the purpose of CAVS testing, the concatenation of *Entropy* and *Nonce*
> +can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface.
> This +requires a kernel built with CONFIG_CRYPTO_CAVS_DRBG, and
> CAP_SYS_ADMIN +permission.
> +
> +*Additional Data* can be provided using the send()/sendmsg() system calls.
>
> Using the read()/recvmsg() system calls, random numbers can be obtained.
> The kernel generates at most 128 bytes in one call. If user space
> @@ -377,6 +385,9 @@ mentioned optname:
> provided ciphertext is assumed to contain an authentication tag of
> the given size (see section about AEAD memory layout below).
>
> +- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number
> generator. + This option is applicable to RNG cipher type only.
> +
> User space API example
> ----------------------
>
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 091c0a0bbf26..8484617596d1 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1896,6 +1896,14 @@ config CRYPTO_STATS
> config CRYPTO_HASH_INFO
> bool
>
> +config CRYPTO_CAVS_DRBG

CAVS is dead, long live ACVT :-) So, maybe use CAVP or ACVT as abbreviations?

As the config option applies to AF_ALG, wouldn't it be better to use an
appropriate name here like CRYPTO_USER_API_CAVP_DRBG or similar?

Note, if indeed we would add akcipher or even KPP with CAVP support, maybe we
need additional config options here. So, I would recommend to use this
separate name space.


> + tristate "Enable CAVS testing of DRBG"

Dto: replace CAVS

> + depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
> + help
> + This option enables the resetting of DRBG entropy via the user-space
> + interface. This should only be enabled for CAVS testing. You should
say
> + no unless you know what this is.
> +
> source "lib/crypto/Kconfig"
> source "drivers/crypto/Kconfig"
> source "crypto/asymmetric_keys/Kconfig"
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index b1cd3535c525..27d6248ca447 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -260,6 +260,14 @@ static int alg_setsockopt(struct socket *sock, int
> level, int optname, if (!type->setauthsize)
> goto unlock;
> err = type->setauthsize(ask->private, optlen);
> + break;
> + case ALG_SET_DRBG_ENTROPY:
> + if (sock->state == SS_CONNECTED)
> + goto unlock;
> + if (!type->setentropy)
> + goto unlock;
> +
> + err = type->setentropy(ask->private, optval, optlen);
> }
>
> unlock:
> diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
> index 087c0ad09d38..8a3b0eb45a85 100644
> --- a/crypto/algif_rng.c
> +++ b/crypto/algif_rng.c
> @@ -38,6 +38,7 @@
> * DAMAGE.
> */
>
> +#include <linux/capability.h>
> #include <linux/module.h>
> #include <crypto/rng.h>
> #include <linux/random.h>
> @@ -53,8 +54,22 @@ struct rng_ctx {
> #define MAXSIZE 128
> unsigned int len;
> struct crypto_rng *drng;
> + u8 *addtl;
> + size_t addtl_len;
> };
>
> +struct rng_parent_ctx {
> + struct crypto_rng *drng;
> + u8 *entropy;
> +};
> +
> +static void rng_reset_addtl(struct rng_ctx *ctx)
> +{
> + kzfree(ctx->addtl);
> + ctx->addtl = NULL;
> + ctx->addtl_len = 0;
> +}
> +
> static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> int flags)
> {
> @@ -65,6 +80,7 @@ static int rng_recvmsg(struct socket *sock, struct msghdr
> *msg, size_t len, int genlen = 0;
> u8 result[MAXSIZE];
>
> + lock_sock(sock->sk);
> if (len == 0)
> return 0;
> if (len > MAXSIZE)
> @@ -82,16 +98,45 @@ static int rng_recvmsg(struct socket *sock, struct
> msghdr *msg, size_t len, * seeding as they automatically seed. The X9.31
> DRNG will return * an error if it was not seeded properly.
> */
> - genlen = crypto_rng_get_bytes(ctx->drng, result, len);
> + genlen = crypto_rng_generate(ctx->drng, ctx->addtl, ctx->addtl_len,
> + result, len);
> if (genlen < 0)
> return genlen;
>
> err = memcpy_to_msg(msg, result, len);
> memzero_explicit(result, len);
> + rng_reset_addtl(ctx);
> + release_sock(sock->sk);
>
> return err ? err : len;
> }
>
> +static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> +{
> + int err;
> + struct alg_sock *ask = alg_sk(sock->sk);
> + struct rng_ctx *ctx = ask->private;
> +
> + lock_sock(sock->sk);
> + if (len > MAXSIZE)
> + len = MAXSIZE;
> +
> + rng_reset_addtl(ctx);
> + ctx->addtl = kmalloc(len, GFP_KERNEL);
> + if (!ctx->addtl)
> + return -ENOMEM;
> +
> + err = memcpy_from_msg(ctx->addtl, msg, len);
> + if (err) {
> + rng_reset_addtl(ctx);
> + return err;
> + }
> + ctx->addtl_len = len;
> + release_sock(sock->sk);
> +
> + return len;
> +}
> +
> static struct proto_ops algif_rng_ops = {
> .family = PF_ALG,
>
> @@ -106,21 +151,41 @@ static struct proto_ops algif_rng_ops = {
> .bind = sock_no_bind,
> .accept = sock_no_accept,
> .setsockopt = sock_no_setsockopt,
> - .sendmsg = sock_no_sendmsg,
> .sendpage = sock_no_sendpage,
>
> .release = af_alg_release,
> .recvmsg = rng_recvmsg,
> + .sendmsg = rng_sendmsg,
> };
>
> static void *rng_bind(const char *name, u32 type, u32 mask)
> {
> - return crypto_alloc_rng(name, type, mask);
> + struct rng_parent_ctx *pctx;
> + struct crypto_rng *rng;
> +
> + pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
> + if (!pctx)
> + return ERR_PTR(-ENOMEM);
> +
> + rng = crypto_alloc_rng(name, type, mask);
> + if (IS_ERR(rng)) {
> + kfree(pctx);
> + return ERR_CAST(rng);
> + }
> +
> + pctx->drng = rng;
> + return pctx;
> }
>
> static void rng_release(void *private)
> {
> - crypto_free_rng(private);
> + struct rng_parent_ctx *pctx = private;
> +
> + if (unlikely(!pctx))
> + return;
> + crypto_free_rng(pctx->drng);
> + kzfree(pctx->entropy);
> + kzfree(pctx);
> }
>
> static void rng_sock_destruct(struct sock *sk)
> @@ -128,6 +193,7 @@ static void rng_sock_destruct(struct sock *sk)
> struct alg_sock *ask = alg_sk(sk);
> struct rng_ctx *ctx = ask->private;
>
> + rng_reset_addtl(ctx);
> sock_kfree_s(sk, ctx, ctx->len);
> af_alg_release_parent(sk);
> }
> @@ -135,6 +201,7 @@ static void rng_sock_destruct(struct sock *sk)
> static int rng_accept_parent(void *private, struct sock *sk)
> {
> struct rng_ctx *ctx;
> + struct rng_parent_ctx *pctx = private;
> struct alg_sock *ask = alg_sk(sk);
> unsigned int len = sizeof(*ctx);
>
> @@ -150,7 +217,9 @@ static int rng_accept_parent(void *private, struct sock
> *sk) * state of the RNG.
> */
>
> - ctx->drng = private;
> + ctx->drng = pctx->drng;
> + ctx->addtl = NULL;
> + ctx->addtl_len = 0;
> ask->private = ctx;
> sk->sk_destruct = rng_sock_destruct;
>
> @@ -159,18 +228,49 @@ static int rng_accept_parent(void *private, struct
> sock *sk)
>
> static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
> {
> + struct rng_parent_ctx *pctx = private;
> /*
> * Check whether seedlen is of sufficient size is done in RNG
> * implementations.
> */
> - return crypto_rng_reset(private, seed, seedlen);
> + return crypto_rng_reset(pctx->drng, seed, seedlen);
> +}
> +
> +#ifdef CONFIG_CRYPTO_CAVS_DRBG
> +static int rng_setentropy(void *private, const u8 *entropy, unsigned int
> len) +{
> + struct rng_parent_ctx *pctx = private;
> + u8 *kentropy = NULL;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (pctx->entropy)
> + return -EINVAL;
> +
> + if (len > MAXSIZE)
> + len = MAXSIZE;
> +
> + if (len) {
> + kentropy = memdup_user(entropy, len);
> + if (IS_ERR(kentropy))
> + return PTR_ERR(kentropy);
> + }
> +
> + crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
> + pctx->entropy = kentropy;

Why do you need to keep kentropy around? For the check above whether entropy
was set, wouldn't a boolean suffice?

> + return len;
> }
> +#endif
>
> static const struct af_alg_type algif_type_rng = {
> .bind = rng_bind,
> .release = rng_release,
> .accept = rng_accept_parent,
> .setkey = rng_setkey,
> +#ifdef CONFIG_CRYPTO_CAVS_DRBG
> + .setentropy = rng_setentropy,
> +#endif
> .ops = &algif_rng_ops,
> .name = "rng",
> .owner = THIS_MODULE
> diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> index 56527c85d122..312fdb3469cf 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -46,6 +46,7 @@ struct af_alg_type {
> void *(*bind)(const char *name, u32 type, u32 mask);
> void (*release)(void *private);
> int (*setkey)(void *private, const u8 *key, unsigned int keylen);
> + int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
> int (*accept)(void *private, struct sock *sk);
> int (*accept_nokey)(void *private, struct sock *sk);
> int (*setauthsize)(void *private, unsigned int authsize);
> @@ -123,7 +124,7 @@ struct af_alg_async_req {
> * @tsgl_list: Link to TX SGL
> * @iv: IV for cipher operation
> * @aead_assoclen: Length of AAD for AEAD cipher operations
> - * @completion: Work queue for synchronous operation
> + * @wait: Wait on completion of async crypto ops
> * @used: TX bytes sent to kernel. This variable is used to
> * ensure that user space cannot cause the kernel
> * to allocate too much memory in sendmsg operation.
> diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> index bc2bcdec377b..60b7c2efd921 100644
> --- a/include/uapi/linux/if_alg.h
> +++ b/include/uapi/linux/if_alg.h
> @@ -35,6 +35,7 @@ struct af_alg_iv {
> #define ALG_SET_OP 3
> #define ALG_SET_AEAD_ASSOCLEN 4
> #define ALG_SET_AEAD_AUTHSIZE 5
> +#define ALG_SET_DRBG_ENTROPY 6
>
> /* Operations */
> #define ALG_OP_DECRYPT 0


Ciao
Stephan


2020-07-20 17:43:18

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface

Am Donnerstag, 16. Juli 2020, 18:40:28 CEST schrieb Elena Petrova:

Hi Elena,

> diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> index 56527c85d122..312fdb3469cf 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -46,6 +46,7 @@ struct af_alg_type {
> void *(*bind)(const char *name, u32 type, u32 mask);
> void (*release)(void *private);
> int (*setkey)(void *private, const u8 *key, unsigned int keylen);
> + int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
> int (*accept)(void *private, struct sock *sk);
> int (*accept_nokey)(void *private, struct sock *sk);
> int (*setauthsize)(void *private, unsigned int authsize);
> @@ -123,7 +124,7 @@ struct af_alg_async_req {
> * @tsgl_list: Link to TX SGL
> * @iv: IV for cipher operation
> * @aead_assoclen: Length of AAD for AEAD cipher operations
> - * @completion: Work queue for synchronous operation
> + * @wait: Wait on completion of async crypto ops

What is this change about? I am not sure it relates to the changes above.

> * @used: TX bytes sent to kernel. This variable is used to
> * ensure that user space cannot cause the kernel
> * to allocate too much memory in sendmsg operation.
> diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h


Ciao
Stephan


2020-07-21 12:55:59

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface

Hi Stephan,

On Mon, 20 Jul 2020 at 18:35, Stephan Mueller <[email protected]> wrote:
>
> Am Donnerstag, 16. Juli 2020, 18:40:28 CEST schrieb Elena Petrova:
>
> Hi Elena,
>
> sorry for the delay in answering.
>
> > Extending the userspace RNG interface:
> > 1. adding ALG_SET_DRBG_ENTROPY setsockopt option for entropy input;
> > 2. using sendmsg syscall for specifying the additional data.
> >
> > Signed-off-by: Elena Petrova <[email protected]>
> > ---
> >
> > libkcapi patch for testing:
> >
> > https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa59
> > 2cb531
>
> If that kernel patch is taken, I would be happy to take the libkcapi patch
> with the following suggested changes:
>
> - add full documentation of kcapi_rng_set_entropy and kcapi_rng_send_addtl to
> kcapi.h
>
> - move test_cavp into either test/kcapi-main.c or its own application inside
> test/ where the caller provides the entropy_input, personalization string,
> additional inputs

Ok, thanks!

> >
> > diff --git a/crypto/Kconfig b/crypto/Kconfig
> > index 091c0a0bbf26..8484617596d1 100644
> > --- a/crypto/Kconfig
> > +++ b/crypto/Kconfig
> > @@ -1896,6 +1896,14 @@ config CRYPTO_STATS
> > config CRYPTO_HASH_INFO
> > bool
> >
> > +config CRYPTO_CAVS_DRBG
>
> CAVS is dead, long live ACVT :-) So, maybe use CAVP or ACVT as abbreviations?

Ok, let's use CAVP then.

> As the config option applies to AF_ALG, wouldn't it be better to use an
> appropriate name here like CRYPTO_USER_API_CAVP_DRBG or similar?
>
> Note, if indeed we would add akcipher or even KPP with CAVP support, maybe we
> need additional config options here. So, I would recommend to use this
> separate name space.

Yes, that makes sense, thanks.

> > + tristate "Enable CAVS testing of DRBG"
>
> Dto: replace CAVS

Ack

> > +#ifdef CONFIG_CRYPTO_CAVS_DRBG
> > +static int rng_setentropy(void *private, const u8 *entropy, unsigned int
> > len) +{
> > + struct rng_parent_ctx *pctx = private;
> > + u8 *kentropy = NULL;
> > +
> > + if (!capable(CAP_SYS_ADMIN))
> > + return -EPERM;
> > +
> > + if (pctx->entropy)
> > + return -EINVAL;
> > +
> > + if (len > MAXSIZE)
> > + len = MAXSIZE;
> > +
> > + if (len) {
> > + kentropy = memdup_user(entropy, len);
> > + if (IS_ERR(kentropy))
> > + return PTR_ERR(kentropy);
> > + }
> > +
> > + crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
> > + pctx->entropy = kentropy;
>
> Why do you need to keep kentropy around? For the check above whether entropy
> was set, wouldn't a boolean suffice?

I need to keep the pointer to free it after use. Unlike the setting of
the key, DRBG saves the entropy pointer in one of its internal
structures, but doesn't do any memory
management. I had only two ideas on how to prevent memory leaks:
either change drbg code to deal with the memory, or save the pointer
somewhere inside the socket. I opted for the latter. But if you know a
better approach I'm happy to rework my code accordingly.

> > diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> > index 56527c85d122..312fdb3469cf 100644
> > --- a/include/crypto/if_alg.h
> > +++ b/include/crypto/if_alg.h
> > @@ -46,6 +46,7 @@ struct af_alg_type {
> > void *(*bind)(const char *name, u32 type, u32 mask);
> > void (*release)(void *private);
> > int (*setkey)(void *private, const u8 *key, unsigned int keylen);
> > + int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
> > int (*accept)(void *private, struct sock *sk);
> > int (*accept_nokey)(void *private, struct sock *sk);
> > int (*setauthsize)(void *private, unsigned int authsize);
> > @@ -123,7 +124,7 @@ struct af_alg_async_req {
> > * @tsgl_list: Link to TX SGL
> > * @iv: IV for cipher operation
> > * @aead_assoclen: Length of AAD for AEAD cipher operations
> > - * @completion: Work queue for synchronous operation
> > + * @wait: Wait on completion of async crypto ops
>
> What is this change about? I am not sure it relates to the changes above.

I noticed that the documentation for the function differs from the
function declaration, so I thought I'd fix that, since I touched the
header. But yeah, it doesn't relate to the changes.

> Ciao
> Stephan

Thanks,
Elena

2020-07-21 13:19:36

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface

Am Dienstag, 21. Juli 2020, 14:55:14 CEST schrieb Elena Petrova:

Hi Elena,

> > > +#ifdef CONFIG_CRYPTO_CAVS_DRBG
> > > +static int rng_setentropy(void *private, const u8 *entropy, unsigned
> > > int
> > > len) +{
> > > + struct rng_parent_ctx *pctx = private;
> > > + u8 *kentropy = NULL;
> > > +
> > > + if (!capable(CAP_SYS_ADMIN))
> > > + return -EPERM;
> > > +
> > > + if (pctx->entropy)
> > > + return -EINVAL;
> > > +
> > > + if (len > MAXSIZE)
> > > + len = MAXSIZE;
> > > +
> > > + if (len) {
> > > + kentropy = memdup_user(entropy, len);
> > > + if (IS_ERR(kentropy))
> > > + return PTR_ERR(kentropy);
> > > + }
> > > +
> > > + crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
> > > + pctx->entropy = kentropy;
> >
> > Why do you need to keep kentropy around? For the check above whether
> > entropy was set, wouldn't a boolean suffice?
>
> I need to keep the pointer to free it after use. Unlike the setting of
> the key, DRBG saves the entropy pointer in one of its internal
> structures, but doesn't do any memory
> management. I had only two ideas on how to prevent memory leaks:
> either change drbg code to deal with the memory, or save the pointer
> somewhere inside the socket. I opted for the latter. But if you know a
> better approach I'm happy to rework my code accordingly.

I was thinking of calling crypto_rng_alg(pctx->drng)->seed() directly after
set_ent. This call performs a DRBG instantatiate where the entropy buffer is
used. See crypto_drbg_reset_test for the approach.

But maybe you are right, the test "entropy" buffer inside the DRBG currently
cannot be reset. So, for sanity purposes, you need to keep it around.

Ciao
Stephan


2020-07-22 15:59:41

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface

On Thu, Jul 16, 2020 at 05:40:28PM +0100, Elena Petrova wrote:
> Extending the userspace RNG interface:
> 1. adding ALG_SET_DRBG_ENTROPY setsockopt option for entropy input;
> 2. using sendmsg syscall for specifying the additional data.
>
> Signed-off-by: Elena Petrova <[email protected]>

Can you add more details to the commit message? E.g. why this is needed.

Also please use imperative tense, e.g. "Extend the userspace RNG interface".

> static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> int flags)
> {
> @@ -65,6 +80,7 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> int genlen = 0;
> u8 result[MAXSIZE];
>
> + lock_sock(sock->sk);
> if (len == 0)
> return 0;

This returns without unlocking the socket.

> if (len > MAXSIZE)
> @@ -82,16 +98,45 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> * seeding as they automatically seed. The X9.31 DRNG will return
> * an error if it was not seeded properly.
> */
> - genlen = crypto_rng_get_bytes(ctx->drng, result, len);
> + genlen = crypto_rng_generate(ctx->drng, ctx->addtl, ctx->addtl_len,
> + result, len);
> if (genlen < 0)
> return genlen;

Likewise.

>
> err = memcpy_to_msg(msg, result, len);
> memzero_explicit(result, len);
> + rng_reset_addtl(ctx);
> + release_sock(sock->sk);
>
> return err ? err : len;
> }
>
> +static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> +{
> + int err;
> + struct alg_sock *ask = alg_sk(sock->sk);
> + struct rng_ctx *ctx = ask->private;
> +
> + lock_sock(sock->sk);
> + if (len > MAXSIZE)
> + len = MAXSIZE;
> +
> + rng_reset_addtl(ctx);
> + ctx->addtl = kmalloc(len, GFP_KERNEL);
> + if (!ctx->addtl)
> + return -ENOMEM;

Likewise.

> +
> + err = memcpy_from_msg(ctx->addtl, msg, len);
> + if (err) {
> + rng_reset_addtl(ctx);
> + return err;

Likewise.

2020-07-28 15:54:53

by Elena Petrova

[permalink] [raw]
Subject: [PATCH v3] crypto: af_alg - add extra parameters for DRBG interface

Extend the user-space RNG interface:
1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
2. Add additional data input via sendmsg syscall.

This allows DRBG to be tested with test vectors, for example for the
purpose of CAVP testing, which otherwise isn't possible.

To prevent erroneous use of entropy input, it is hidden under
CRYPTO_USER_API_CAVP_DRBG config option and requires CAP_SYS_ADMIN to
succeed.

Signed-off-by: Elena Petrova <[email protected]>
---

libkcapi patch for testing:
https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531

Updates in v3:
1) More details in commit message;
2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
3) fixed a bug of not releasing socket locks.

Updates in v2:
1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
2) Requiring CAP_SYS_ADMIN for entropy reset.
3) Locking for send and recv.
4) Length checks added for send and setentropy; send and setentropy now return
number of bytes accepted.
5) Minor code style corrections.

Documentation/crypto/userspace-if.rst | 17 +++-
crypto/Kconfig | 8 ++
crypto/af_alg.c | 8 ++
crypto/algif_rng.c | 130 ++++++++++++++++++++++++--
include/crypto/if_alg.h | 1 +
include/uapi/linux/if_alg.h | 1 +
6 files changed, 152 insertions(+), 13 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index ff86befa61e0..ef7132802c2d 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -296,15 +296,23 @@ follows:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
- .salg_type = "rng", /* this selects the symmetric cipher */
- .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
+ .salg_type = "rng", /* this selects the random number generator */
+ .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
};


Depending on the RNG type, the RNG must be seeded. The seed is provided
using the setsockopt interface to set the key. For example, the
ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
-seeded.
+seeded. The seed is also known as a *Personalization String* in DRBG800-90A
+standard.
+
+For the purpose of CAVP testing, the concatenation of *Entropy* and *Nonce*
+can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface. This
+requires a kernel built with CONFIG_CRYPTO_USER_API_CAVP_DRBG, and
+CAP_SYS_ADMIN permission.
+
+*Additional Data* can be provided using the send()/sendmsg() system calls.

Using the read()/recvmsg() system calls, random numbers can be obtained.
The kernel generates at most 128 bytes in one call. If user space
@@ -377,6 +385,9 @@ mentioned optname:
provided ciphertext is assumed to contain an authentication tag of
the given size (see section about AEAD memory layout below).

+- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
+ This option is applicable to RNG cipher type only.
+
User space API example
----------------------

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 091c0a0bbf26..aa2b3085a431 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1896,6 +1896,14 @@ config CRYPTO_STATS
config CRYPTO_HASH_INFO
bool

+config CRYPTO_USER_API_CAVP_DRBG
+ tristate "Enable CAVP testing of DRBG"
+ depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
+ help
+ This option enables the resetting of DRBG entropy via the user-space
+ interface. This should only be enabled for CAVP testing. You should say
+ no unless you know what this is.
+
source "lib/crypto/Kconfig"
source "drivers/crypto/Kconfig"
source "crypto/asymmetric_keys/Kconfig"
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index b1cd3535c525..27d6248ca447 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -260,6 +260,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+ break;
+ case ALG_SET_DRBG_ENTROPY:
+ if (sock->state == SS_CONNECTED)
+ goto unlock;
+ if (!type->setentropy)
+ goto unlock;
+
+ err = type->setentropy(ask->private, optval, optlen);
}

unlock:
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 087c0ad09d38..3a88f4fca2a9 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -38,6 +38,7 @@
* DAMAGE.
*/

+#include <linux/capability.h>
#include <linux/module.h>
#include <crypto/rng.h>
#include <linux/random.h>
@@ -53,20 +54,35 @@ struct rng_ctx {
#define MAXSIZE 128
unsigned int len;
struct crypto_rng *drng;
+ u8 *addtl;
+ size_t addtl_len;
};

+struct rng_parent_ctx {
+ struct crypto_rng *drng;
+ u8 *entropy;
+};
+
+static void rng_reset_addtl(struct rng_ctx *ctx)
+{
+ kzfree(ctx->addtl);
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
+}
+
static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
int flags)
{
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;
- int err;
+ int err = 0;
int genlen = 0;
u8 result[MAXSIZE];

+ lock_sock(sock->sk);
if (len == 0)
- return 0;
+ goto unlock;
if (len > MAXSIZE)
len = MAXSIZE;

@@ -82,16 +98,51 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
* seeding as they automatically seed. The X9.31 DRNG will return
* an error if it was not seeded properly.
*/
- genlen = crypto_rng_get_bytes(ctx->drng, result, len);
- if (genlen < 0)
- return genlen;
+ genlen = crypto_rng_generate(ctx->drng, ctx->addtl, ctx->addtl_len,
+ result, len);
+ if (genlen < 0) {
+ err = genlen;
+ goto unlock;
+ }

err = memcpy_to_msg(msg, result, len);
memzero_explicit(result, len);
+ rng_reset_addtl(ctx);

+unlock:
+ release_sock(sock->sk);
return err ? err : len;
}

+static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sock->sk);
+ struct rng_ctx *ctx = ask->private;
+
+ lock_sock(sock->sk);
+ if (len > MAXSIZE)
+ len = MAXSIZE;
+
+ rng_reset_addtl(ctx);
+ ctx->addtl = kmalloc(len, GFP_KERNEL);
+ if (!ctx->addtl) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ err = memcpy_from_msg(ctx->addtl, msg, len);
+ if (err) {
+ rng_reset_addtl(ctx);
+ goto unlock;
+ }
+ ctx->addtl_len = len;
+
+unlock:
+ release_sock(sock->sk);
+ return len;
+}
+
static struct proto_ops algif_rng_ops = {
.family = PF_ALG,

@@ -106,21 +157,41 @@ static struct proto_ops algif_rng_ops = {
.bind = sock_no_bind,
.accept = sock_no_accept,
.setsockopt = sock_no_setsockopt,
- .sendmsg = sock_no_sendmsg,
.sendpage = sock_no_sendpage,

.release = af_alg_release,
.recvmsg = rng_recvmsg,
+ .sendmsg = rng_sendmsg,
};

static void *rng_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_rng(name, type, mask);
+ struct rng_parent_ctx *pctx;
+ struct crypto_rng *rng;
+
+ pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+ if (!pctx)
+ return ERR_PTR(-ENOMEM);
+
+ rng = crypto_alloc_rng(name, type, mask);
+ if (IS_ERR(rng)) {
+ kfree(pctx);
+ return ERR_CAST(rng);
+ }
+
+ pctx->drng = rng;
+ return pctx;
}

static void rng_release(void *private)
{
- crypto_free_rng(private);
+ struct rng_parent_ctx *pctx = private;
+
+ if (unlikely(!pctx))
+ return;
+ crypto_free_rng(pctx->drng);
+ kzfree(pctx->entropy);
+ kzfree(pctx);
}

static void rng_sock_destruct(struct sock *sk)
@@ -128,6 +199,7 @@ static void rng_sock_destruct(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;

+ rng_reset_addtl(ctx);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -135,6 +207,7 @@ static void rng_sock_destruct(struct sock *sk)
static int rng_accept_parent(void *private, struct sock *sk)
{
struct rng_ctx *ctx;
+ struct rng_parent_ctx *pctx = private;
struct alg_sock *ask = alg_sk(sk);
unsigned int len = sizeof(*ctx);

@@ -150,7 +223,9 @@ static int rng_accept_parent(void *private, struct sock *sk)
* state of the RNG.
*/

- ctx->drng = private;
+ ctx->drng = pctx->drng;
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
ask->private = ctx;
sk->sk_destruct = rng_sock_destruct;

@@ -159,18 +234,53 @@ static int rng_accept_parent(void *private, struct sock *sk)

static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
{
+ struct rng_parent_ctx *pctx = private;
/*
* Check whether seedlen is of sufficient size is done in RNG
* implementations.
*/
- return crypto_rng_reset(private, seed, seedlen);
+ return crypto_rng_reset(pctx->drng, seed, seedlen);
+}
+
+#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
+static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
+{
+ struct rng_parent_ctx *pctx = private;
+ u8 *kentropy = NULL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (pctx->entropy)
+ return -EINVAL;
+
+ if (len > MAXSIZE)
+ len = MAXSIZE;
+
+ if (len) {
+ kentropy = memdup_user(entropy, len);
+ if (IS_ERR(kentropy))
+ return PTR_ERR(kentropy);
+ }
+
+ crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
+ /*
+ * Since rng doesn't perform any memory management for the entropy
+ * buffer, save kentropy pointer to pctx now to free it after use.
+ */
+ pctx->entropy = kentropy;
+ return len;
}
+#endif

static const struct af_alg_type algif_type_rng = {
.bind = rng_bind,
.release = rng_release,
.accept = rng_accept_parent,
.setkey = rng_setkey,
+#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
+ .setentropy = rng_setentropy,
+#endif
.ops = &algif_rng_ops,
.name = "rng",
.owner = THIS_MODULE
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 56527c85d122..9e5c8ac53c59 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -46,6 +46,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..60b7c2efd921 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_DRBG_ENTROPY 6

/* Operations */
#define ALG_OP_DECRYPT 0
--
2.28.0.rc0.142.g3c755180ce-goog

2020-07-28 16:17:31

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface

Hi Stephan,

On Tue, 21 Jul 2020 at 14:19, Stephan Mueller <[email protected]> wrote:
>
> Am Dienstag, 21. Juli 2020, 14:55:14 CEST schrieb Elena Petrova:
>
> Hi Elena,
>
> > > > +#ifdef CONFIG_CRYPTO_CAVS_DRBG
> > > > +static int rng_setentropy(void *private, const u8 *entropy, unsigned
> > > > int
> > > > len) +{
> > > > + struct rng_parent_ctx *pctx = private;
> > > > + u8 *kentropy = NULL;
> > > > +
> > > > + if (!capable(CAP_SYS_ADMIN))
> > > > + return -EPERM;
> > > > +
> > > > + if (pctx->entropy)
> > > > + return -EINVAL;
> > > > +
> > > > + if (len > MAXSIZE)
> > > > + len = MAXSIZE;
> > > > +
> > > > + if (len) {
> > > > + kentropy = memdup_user(entropy, len);
> > > > + if (IS_ERR(kentropy))
> > > > + return PTR_ERR(kentropy);
> > > > + }
> > > > +
> > > > + crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
> > > > + pctx->entropy = kentropy;
> > >
> > > Why do you need to keep kentropy around? For the check above whether
> > > entropy was set, wouldn't a boolean suffice?
> >
> > I need to keep the pointer to free it after use. Unlike the setting of
> > the key, DRBG saves the entropy pointer in one of its internal
> > structures, but doesn't do any memory
> > management. I had only two ideas on how to prevent memory leaks:
> > either change drbg code to deal with the memory, or save the pointer
> > somewhere inside the socket. I opted for the latter. But if you know a
> > better approach I'm happy to rework my code accordingly.
>
> I was thinking of calling crypto_rng_alg(pctx->drng)->seed() directly after
> set_ent. This call performs a DRBG instantatiate where the entropy buffer is
> used. See crypto_drbg_reset_test for the approach.
>
> But maybe you are right, the test "entropy" buffer inside the DRBG currently
> cannot be reset. So, for sanity purposes, you need to keep it around.

I looked into this, and afaik `->seed()` needs the seed buffer (a.k.a.
key); and seed() is also invoked on ALG_SET_KEY setsockopt. So we
would need both entropy and seed values at the same time. To avoid
complicating the matters, I decided to leave the code as is. I added a
comment in v3 [https://lore.kernel.org/linux-crypto/[email protected]/]
explaining why the `kentropy` pointer is saved.

> Ciao
> Stephan

Regards,
Elena

2020-07-28 17:36:32

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH v3] crypto: af_alg - add extra parameters for DRBG interface

On Tue, Jul 28, 2020 at 04:51:59PM +0100, Elena Petrova wrote:
> +static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> +{
> + int err;
> + struct alg_sock *ask = alg_sk(sock->sk);
> + struct rng_ctx *ctx = ask->private;
> +
> + lock_sock(sock->sk);
> + if (len > MAXSIZE)
> + len = MAXSIZE;
> +
> + rng_reset_addtl(ctx);
> + ctx->addtl = kmalloc(len, GFP_KERNEL);
> + if (!ctx->addtl) {
> + err = -ENOMEM;
> + goto unlock;

This error code isn't actually returned.

> + }
> +
> + err = memcpy_from_msg(ctx->addtl, msg, len);
> + if (err) {
> + rng_reset_addtl(ctx);
> + goto unlock;

Likewise.

> +#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
> +static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
> +{
> + struct rng_parent_ctx *pctx = private;
> + u8 *kentropy = NULL;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;

This should be EACCES, not EPERM. EACCES means the operation will succeed if
you acquire the needed privileges. EPERM means it will never succeed.

> + if (len > MAXSIZE)
> + len = MAXSIZE;

Truncating the length is error prone. Shouldn't this instead return an error
(EMSGSIZE?) if the length is too long, and 0 on success? Remember this is
setsockopt(), not write().

- Eric

2020-07-29 15:47:05

by Elena Petrova

[permalink] [raw]
Subject: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface

Extend the user-space RNG interface:
1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
2. Add additional data input via sendmsg syscall.

This allows DRBG to be tested with test vectors, for example for the
purpose of CAVP testing, which otherwise isn't possible.

To prevent erroneous use of entropy input, it is hidden under
CRYPTO_USER_API_CAVP_DRBG config option and requires CAP_SYS_ADMIN to
succeed.

Signed-off-by: Elena Petrova <[email protected]>
---

Updates in v4:
1) setentropy returns 0 or error code (used to return length);
2) bigfixes suggested by Eric.

Updates in v3:
1) More details in commit message;
2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
3) fixed a bug of not releasing socket locks.

Updates in v2:
1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
2) Requiring CAP_SYS_ADMIN for entropy reset.
3) Locking for send and recv.
4) Length checks added for send and setentropy; send and setentropy now return
number of bytes accepted.
5) Minor code style corrections.

libkcapi patch for testing:
https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531

Documentation/crypto/userspace-if.rst | 17 +++-
crypto/Kconfig | 8 ++
crypto/af_alg.c | 8 ++
crypto/algif_rng.c | 130 ++++++++++++++++++++++++--
include/crypto/if_alg.h | 1 +
include/uapi/linux/if_alg.h | 1 +
6 files changed, 152 insertions(+), 13 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index ff86befa61e0..ef7132802c2d 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -296,15 +296,23 @@ follows:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
- .salg_type = "rng", /* this selects the symmetric cipher */
- .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
+ .salg_type = "rng", /* this selects the random number generator */
+ .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
};


Depending on the RNG type, the RNG must be seeded. The seed is provided
using the setsockopt interface to set the key. For example, the
ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
-seeded.
+seeded. The seed is also known as a *Personalization String* in DRBG800-90A
+standard.
+
+For the purpose of CAVP testing, the concatenation of *Entropy* and *Nonce*
+can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface. This
+requires a kernel built with CONFIG_CRYPTO_USER_API_CAVP_DRBG, and
+CAP_SYS_ADMIN permission.
+
+*Additional Data* can be provided using the send()/sendmsg() system calls.

Using the read()/recvmsg() system calls, random numbers can be obtained.
The kernel generates at most 128 bytes in one call. If user space
@@ -377,6 +385,9 @@ mentioned optname:
provided ciphertext is assumed to contain an authentication tag of
the given size (see section about AEAD memory layout below).

+- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
+ This option is applicable to RNG cipher type only.
+
User space API example
----------------------

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 091c0a0bbf26..aa2b3085a431 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1896,6 +1896,14 @@ config CRYPTO_STATS
config CRYPTO_HASH_INFO
bool

+config CRYPTO_USER_API_CAVP_DRBG
+ tristate "Enable CAVP testing of DRBG"
+ depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
+ help
+ This option enables the resetting of DRBG entropy via the user-space
+ interface. This should only be enabled for CAVP testing. You should say
+ no unless you know what this is.
+
source "lib/crypto/Kconfig"
source "drivers/crypto/Kconfig"
source "crypto/asymmetric_keys/Kconfig"
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index b1cd3535c525..27d6248ca447 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -260,6 +260,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+ break;
+ case ALG_SET_DRBG_ENTROPY:
+ if (sock->state == SS_CONNECTED)
+ goto unlock;
+ if (!type->setentropy)
+ goto unlock;
+
+ err = type->setentropy(ask->private, optval, optlen);
}

unlock:
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 087c0ad09d38..21e8201844bf 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -38,6 +38,7 @@
* DAMAGE.
*/

+#include <linux/capability.h>
#include <linux/module.h>
#include <crypto/rng.h>
#include <linux/random.h>
@@ -53,20 +54,35 @@ struct rng_ctx {
#define MAXSIZE 128
unsigned int len;
struct crypto_rng *drng;
+ u8 *addtl;
+ size_t addtl_len;
};

+struct rng_parent_ctx {
+ struct crypto_rng *drng;
+ u8 *entropy;
+};
+
+static void rng_reset_addtl(struct rng_ctx *ctx)
+{
+ kzfree(ctx->addtl);
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
+}
+
static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
int flags)
{
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;
- int err;
+ int err = 0;
int genlen = 0;
u8 result[MAXSIZE];

+ lock_sock(sock->sk);
if (len == 0)
- return 0;
+ goto unlock;
if (len > MAXSIZE)
len = MAXSIZE;

@@ -82,13 +98,48 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
* seeding as they automatically seed. The X9.31 DRNG will return
* an error if it was not seeded properly.
*/
- genlen = crypto_rng_get_bytes(ctx->drng, result, len);
- if (genlen < 0)
- return genlen;
+ genlen = crypto_rng_generate(ctx->drng, ctx->addtl, ctx->addtl_len,
+ result, len);
+ if (genlen < 0) {
+ err = genlen;
+ goto unlock;
+ }

err = memcpy_to_msg(msg, result, len);
memzero_explicit(result, len);
+ rng_reset_addtl(ctx);

+unlock:
+ release_sock(sock->sk);
+ return err ? err : len;
+}
+
+static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sock->sk);
+ struct rng_ctx *ctx = ask->private;
+
+ lock_sock(sock->sk);
+ if (len > MAXSIZE)
+ len = MAXSIZE;
+
+ rng_reset_addtl(ctx);
+ ctx->addtl = kmalloc(len, GFP_KERNEL);
+ if (!ctx->addtl) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ err = memcpy_from_msg(ctx->addtl, msg, len);
+ if (err) {
+ rng_reset_addtl(ctx);
+ goto unlock;
+ }
+ ctx->addtl_len = len;
+
+unlock:
+ release_sock(sock->sk);
return err ? err : len;
}

@@ -106,21 +157,41 @@ static struct proto_ops algif_rng_ops = {
.bind = sock_no_bind,
.accept = sock_no_accept,
.setsockopt = sock_no_setsockopt,
- .sendmsg = sock_no_sendmsg,
.sendpage = sock_no_sendpage,

.release = af_alg_release,
.recvmsg = rng_recvmsg,
+ .sendmsg = rng_sendmsg,
};

static void *rng_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_rng(name, type, mask);
+ struct rng_parent_ctx *pctx;
+ struct crypto_rng *rng;
+
+ pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+ if (!pctx)
+ return ERR_PTR(-ENOMEM);
+
+ rng = crypto_alloc_rng(name, type, mask);
+ if (IS_ERR(rng)) {
+ kfree(pctx);
+ return ERR_CAST(rng);
+ }
+
+ pctx->drng = rng;
+ return pctx;
}

static void rng_release(void *private)
{
- crypto_free_rng(private);
+ struct rng_parent_ctx *pctx = private;
+
+ if (unlikely(!pctx))
+ return;
+ crypto_free_rng(pctx->drng);
+ kzfree(pctx->entropy);
+ kzfree(pctx);
}

static void rng_sock_destruct(struct sock *sk)
@@ -128,6 +199,7 @@ static void rng_sock_destruct(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;

+ rng_reset_addtl(ctx);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -135,6 +207,7 @@ static void rng_sock_destruct(struct sock *sk)
static int rng_accept_parent(void *private, struct sock *sk)
{
struct rng_ctx *ctx;
+ struct rng_parent_ctx *pctx = private;
struct alg_sock *ask = alg_sk(sk);
unsigned int len = sizeof(*ctx);

@@ -150,7 +223,9 @@ static int rng_accept_parent(void *private, struct sock *sk)
* state of the RNG.
*/

- ctx->drng = private;
+ ctx->drng = pctx->drng;
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
ask->private = ctx;
sk->sk_destruct = rng_sock_destruct;

@@ -159,18 +234,53 @@ static int rng_accept_parent(void *private, struct sock *sk)

static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
{
+ struct rng_parent_ctx *pctx = private;
/*
* Check whether seedlen is of sufficient size is done in RNG
* implementations.
*/
- return crypto_rng_reset(private, seed, seedlen);
+ return crypto_rng_reset(pctx->drng, seed, seedlen);
+}
+
+#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
+static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
+{
+ struct rng_parent_ctx *pctx = private;
+ u8 *kentropy = NULL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ if (pctx->entropy)
+ return -EINVAL;
+
+ if (len > MAXSIZE)
+ return -EMSGSIZE;
+
+ if (len) {
+ kentropy = memdup_user(entropy, len);
+ if (IS_ERR(kentropy))
+ return PTR_ERR(kentropy);
+ }
+
+ crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
+ /*
+ * Since rng doesn't perform any memory management for the entropy
+ * buffer, save kentropy pointer to pctx now to free it after use.
+ */
+ pctx->entropy = kentropy;
+ return 0;
}
+#endif

static const struct af_alg_type algif_type_rng = {
.bind = rng_bind,
.release = rng_release,
.accept = rng_accept_parent,
.setkey = rng_setkey,
+#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
+ .setentropy = rng_setentropy,
+#endif
.ops = &algif_rng_ops,
.name = "rng",
.owner = THIS_MODULE
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 56527c85d122..9e5c8ac53c59 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -46,6 +46,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..60b7c2efd921 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_DRBG_ENTROPY 6

/* Operations */
#define ALG_OP_DECRYPT 0
--
2.28.0.rc0.142.g3c755180ce-goog

2020-07-29 19:27:03

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface

Am Mittwoch, 29. Juli 2020, 17:45:01 CEST schrieb Elena Petrova:

Hi Elena,

> Extend the user-space RNG interface:
> 1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
> 2. Add additional data input via sendmsg syscall.
>
> This allows DRBG to be tested with test vectors, for example for the
> purpose of CAVP testing, which otherwise isn't possible.
>
> To prevent erroneous use of entropy input, it is hidden under
> CRYPTO_USER_API_CAVP_DRBG config option and requires CAP_SYS_ADMIN to
> succeed.
>
> Signed-off-by: Elena Petrova <[email protected]>

Acked-by: Stephan M?ller <[email protected]>

Ciao
Stephan


2020-07-31 07:24:14

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface

Elena Petrova <[email protected]> wrote:
>
> +#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
> +static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)

Please use __maybe_unused instead of the ifdef.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-07-31 07:27:24

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface

Eric Biggers <[email protected]> wrote:
>
> lock_sock() would solve the former. I'm not sure what should be done about
> rng_recvmsg(). It apparently relies on the crypto_rng doing its own locking,
> but maybe it should just use lock_sock() too.

The lock_sock is only needed if you're doing testing. What I'd
prefer is to have a completely different code-path for testing.

How about you fork the code in rng_accept_parent so that you have
a separate proto_ops for the test path that is used only if
setentropy has been called? That way all of this code could
magically go away if the CONFIG option wasn't set.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-08-03 14:55:42

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface

Hi Herbert,

> > +#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
> > +static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
>
> Please use __maybe_unused instead of the ifdef.

Ack

On Fri, 31 Jul 2020 at 08:27, Herbert Xu <[email protected]> wrote:
>
> Eric Biggers <[email protected]> wrote:
> >
> > lock_sock() would solve the former. I'm not sure what should be done about
> > rng_recvmsg(). It apparently relies on the crypto_rng doing its own locking,
> > but maybe it should just use lock_sock() too.
>
> The lock_sock is only needed if you're doing testing. What I'd
> prefer is to have a completely different code-path for testing.

sendmsg is used for "Additional Data" input, and unlike entropy, it
could be useful outside of testing. But if you confirm it's not
useful, then yes, I can decouple the testing parts.

> How about you fork the code in rng_accept_parent so that you have
> a separate proto_ops for the test path that is used only if
> setentropy has been called? That way all of this code could
> magically go away if the CONFIG option wasn't set.

Depends on the comment above, but otherwise, my only concern is that
the testing variant of rng_recvmsg would be largely copy-pasted from
the normal rng_recvmsg, apart from a few lines of lock/release and
crypto_rng_generate/crypto_rng_get_bytes.


Thanks!
Elena

2020-08-03 15:10:42

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface

Am Montag, 3. August 2020, 16:48:02 CEST schrieb Elena Petrova:

Hi Elena,

> On Fri, 31 Jul 2020 at 08:27, Herbert Xu <[email protected]>
wrote:
> > Eric Biggers <[email protected]> wrote:
> > > lock_sock() would solve the former. I'm not sure what should be done
> > > about
> > > rng_recvmsg(). It apparently relies on the crypto_rng doing its own
> > > locking, but maybe it should just use lock_sock() too.
> >
> > The lock_sock is only needed if you're doing testing. What I'd
> > prefer is to have a completely different code-path for testing.
>
> sendmsg is used for "Additional Data" input, and unlike entropy, it
> could be useful outside of testing. But if you confirm it's not
> useful, then yes, I can decouple the testing parts.

Nobody has requested it for now - so why not only compiling it when the DRBG
test config value is set? If for some reason there is a request to allow
setting the additional data from user space, we may simply take the ifdef
away.

My approach is to have only interfaces into the kernel that are truly
requested and needed.

Ciao
Stephan


2020-08-03 15:31:13

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface

Hi Stephan,

On Mon, 3 Aug 2020 at 16:10, Stephan Mueller <[email protected]> wrote:
>
> Am Montag, 3. August 2020, 16:48:02 CEST schrieb Elena Petrova:
>
> Hi Elena,
>
> > On Fri, 31 Jul 2020 at 08:27, Herbert Xu <[email protected]>
> wrote:
> > > Eric Biggers <[email protected]> wrote:
> > > > lock_sock() would solve the former. I'm not sure what should be done
> > > > about
> > > > rng_recvmsg(). It apparently relies on the crypto_rng doing its own
> > > > locking, but maybe it should just use lock_sock() too.
> > >
> > > The lock_sock is only needed if you're doing testing. What I'd
> > > prefer is to have a completely different code-path for testing.
> >
> > sendmsg is used for "Additional Data" input, and unlike entropy, it
> > could be useful outside of testing. But if you confirm it's not
> > useful, then yes, I can decouple the testing parts.
>
> Nobody has requested it for now - so why not only compiling it when the DRBG
> test config value is set? If for some reason there is a request to allow
> setting the additional data from user space, we may simply take the ifdef
> away.
>
> My approach is to have only interfaces into the kernel that are truly
> requested and needed.

Ok, makes sense, thanks!

> Ciao
> Stephan
>
>

2020-08-04 02:21:06

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface

On Mon, Aug 03, 2020 at 03:48:02PM +0100, Elena Petrova wrote:
>
> sendmsg is used for "Additional Data" input, and unlike entropy, it
> could be useful outside of testing. But if you confirm it's not
> useful, then yes, I can decouple the testing parts.

Unless there is someone asking for it then I'd rather not export
it to user-space.

> Depends on the comment above, but otherwise, my only concern is that
> the testing variant of rng_recvmsg would be largely copy-pasted from
> the normal rng_recvmsg, apart from a few lines of lock/release and
> crypto_rng_generate/crypto_rng_get_bytes.

They could certainly share code through the use of functions.

Chers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-08-13 16:01:10

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface

Hi Herbert,

On Fri, 31 Jul 2020 at 08:27, Herbert Xu <[email protected]> wrote:
>
> How about you fork the code in rng_accept_parent so that you have
> a separate proto_ops for the test path that is used only if
> setentropy has been called? That way all of this code could
> magically go away if the CONFIG option wasn't set.

I tried doing `ask->type->ops = &algif_rng_test_ops;` in
accept_parent, to which the compiler complained "error: assignment of
member ‘ops’ in read-only object". So I'm setting `.ops` accordingly
at compile-time instead, together with `.setentropy`, in v5.

2020-08-13 16:02:47

by Elena Petrova

[permalink] [raw]
Subject: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface

Extend the user-space RNG interface:
1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
2. Add additional data input via sendmsg syscall.

This allows DRBG to be tested with test vectors, for example for the
purpose of CAVP testing, which otherwise isn't possible.

To prevent erroneous use of entropy input, it is hidden under
CRYPTO_USER_API_CAVP_DRBG config option and requires CAP_SYS_ADMIN to
succeed.

Signed-off-by: Elena Petrova <[email protected]>
---

Updates in v4:
1) setentropy returns 0 or error code (used to return length);
2) bigfixes suggested by Eric.

Updates in v3:
1) More details in commit message;
2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
3) fixed a bug of not releasing socket locks.

Updates in v2:
1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
2) Requiring CAP_SYS_ADMIN for entropy reset.
3) Locking for send and recv.
4) Length checks added for send and setentropy; send and setentropy now return
number of bytes accepted.
5) Minor code style corrections.

libkcapi patch for testing:
https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531

Documentation/crypto/userspace-if.rst | 17 +++-
crypto/Kconfig | 8 ++
crypto/af_alg.c | 8 ++
crypto/algif_rng.c | 130 ++++++++++++++++++++++++--
include/crypto/if_alg.h | 1 +
include/uapi/linux/if_alg.h | 1 +
6 files changed, 152 insertions(+), 13 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index ff86befa61e0..ef7132802c2d 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -296,15 +296,23 @@ follows:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
- .salg_type = "rng", /* this selects the symmetric cipher */
- .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
+ .salg_type = "rng", /* this selects the random number generator */
+ .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
};


Depending on the RNG type, the RNG must be seeded. The seed is provided
using the setsockopt interface to set the key. For example, the
ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
-seeded.
+seeded. The seed is also known as a *Personalization String* in DRBG800-90A
+standard.
+
+For the purpose of CAVP testing, the concatenation of *Entropy* and *Nonce*
+can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface. This
+requires a kernel built with CONFIG_CRYPTO_USER_API_CAVP_DRBG, and
+CAP_SYS_ADMIN permission.
+
+*Additional Data* can be provided using the send()/sendmsg() system calls.

Using the read()/recvmsg() system calls, random numbers can be obtained.
The kernel generates at most 128 bytes in one call. If user space
@@ -377,6 +385,9 @@ mentioned optname:
provided ciphertext is assumed to contain an authentication tag of
the given size (see section about AEAD memory layout below).

+- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
+ This option is applicable to RNG cipher type only.
+
User space API example
----------------------

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 091c0a0bbf26..aa2b3085a431 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1896,6 +1896,14 @@ config CRYPTO_STATS
config CRYPTO_HASH_INFO
bool

+config CRYPTO_USER_API_CAVP_DRBG
+ tristate "Enable CAVP testing of DRBG"
+ depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
+ help
+ This option enables the resetting of DRBG entropy via the user-space
+ interface. This should only be enabled for CAVP testing. You should say
+ no unless you know what this is.
+
source "lib/crypto/Kconfig"
source "drivers/crypto/Kconfig"
source "crypto/asymmetric_keys/Kconfig"
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index b1cd3535c525..27d6248ca447 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -260,6 +260,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+ break;
+ case ALG_SET_DRBG_ENTROPY:
+ if (sock->state == SS_CONNECTED)
+ goto unlock;
+ if (!type->setentropy)
+ goto unlock;
+
+ err = type->setentropy(ask->private, optval, optlen);
}

unlock:
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 087c0ad09d38..21e8201844bf 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -38,6 +38,7 @@
* DAMAGE.
*/

+#include <linux/capability.h>
#include <linux/module.h>
#include <crypto/rng.h>
#include <linux/random.h>
@@ -53,20 +54,35 @@ struct rng_ctx {
#define MAXSIZE 128
unsigned int len;
struct crypto_rng *drng;
+ u8 *addtl;
+ size_t addtl_len;
};

+struct rng_parent_ctx {
+ struct crypto_rng *drng;
+ u8 *entropy;
+};
+
+static void rng_reset_addtl(struct rng_ctx *ctx)
+{
+ kzfree(ctx->addtl);
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
+}
+
static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
int flags)
{
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;
- int err;
+ int err = 0;
int genlen = 0;
u8 result[MAXSIZE];

+ lock_sock(sock->sk);
if (len == 0)
- return 0;
+ goto unlock;
if (len > MAXSIZE)
len = MAXSIZE;

@@ -82,13 +98,48 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
* seeding as they automatically seed. The X9.31 DRNG will return
* an error if it was not seeded properly.
*/
- genlen = crypto_rng_get_bytes(ctx->drng, result, len);
- if (genlen < 0)
- return genlen;
+ genlen = crypto_rng_generate(ctx->drng, ctx->addtl, ctx->addtl_len,
+ result, len);
+ if (genlen < 0) {
+ err = genlen;
+ goto unlock;
+ }

err = memcpy_to_msg(msg, result, len);
memzero_explicit(result, len);
+ rng_reset_addtl(ctx);

+unlock:
+ release_sock(sock->sk);
+ return err ? err : len;
+}
+
+static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sock->sk);
+ struct rng_ctx *ctx = ask->private;
+
+ lock_sock(sock->sk);
+ if (len > MAXSIZE)
+ len = MAXSIZE;
+
+ rng_reset_addtl(ctx);
+ ctx->addtl = kmalloc(len, GFP_KERNEL);
+ if (!ctx->addtl) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ err = memcpy_from_msg(ctx->addtl, msg, len);
+ if (err) {
+ rng_reset_addtl(ctx);
+ goto unlock;
+ }
+ ctx->addtl_len = len;
+
+unlock:
+ release_sock(sock->sk);
return err ? err : len;
}

@@ -106,21 +157,41 @@ static struct proto_ops algif_rng_ops = {
.bind = sock_no_bind,
.accept = sock_no_accept,
.setsockopt = sock_no_setsockopt,
- .sendmsg = sock_no_sendmsg,
.sendpage = sock_no_sendpage,

.release = af_alg_release,
.recvmsg = rng_recvmsg,
+ .sendmsg = rng_sendmsg,
};

static void *rng_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_rng(name, type, mask);
+ struct rng_parent_ctx *pctx;
+ struct crypto_rng *rng;
+
+ pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+ if (!pctx)
+ return ERR_PTR(-ENOMEM);
+
+ rng = crypto_alloc_rng(name, type, mask);
+ if (IS_ERR(rng)) {
+ kfree(pctx);
+ return ERR_CAST(rng);
+ }
+
+ pctx->drng = rng;
+ return pctx;
}

static void rng_release(void *private)
{
- crypto_free_rng(private);
+ struct rng_parent_ctx *pctx = private;
+
+ if (unlikely(!pctx))
+ return;
+ crypto_free_rng(pctx->drng);
+ kzfree(pctx->entropy);
+ kzfree(pctx);
}

static void rng_sock_destruct(struct sock *sk)
@@ -128,6 +199,7 @@ static void rng_sock_destruct(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;

+ rng_reset_addtl(ctx);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -135,6 +207,7 @@ static void rng_sock_destruct(struct sock *sk)
static int rng_accept_parent(void *private, struct sock *sk)
{
struct rng_ctx *ctx;
+ struct rng_parent_ctx *pctx = private;
struct alg_sock *ask = alg_sk(sk);
unsigned int len = sizeof(*ctx);

@@ -150,7 +223,9 @@ static int rng_accept_parent(void *private, struct sock *sk)
* state of the RNG.
*/

- ctx->drng = private;
+ ctx->drng = pctx->drng;
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
ask->private = ctx;
sk->sk_destruct = rng_sock_destruct;

@@ -159,18 +234,53 @@ static int rng_accept_parent(void *private, struct sock *sk)

static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
{
+ struct rng_parent_ctx *pctx = private;
/*
* Check whether seedlen is of sufficient size is done in RNG
* implementations.
*/
- return crypto_rng_reset(private, seed, seedlen);
+ return crypto_rng_reset(pctx->drng, seed, seedlen);
+}
+
+#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
+static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
+{
+ struct rng_parent_ctx *pctx = private;
+ u8 *kentropy = NULL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ if (pctx->entropy)
+ return -EINVAL;
+
+ if (len > MAXSIZE)
+ return -EMSGSIZE;
+
+ if (len) {
+ kentropy = memdup_user(entropy, len);
+ if (IS_ERR(kentropy))
+ return PTR_ERR(kentropy);
+ }
+
+ crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
+ /*
+ * Since rng doesn't perform any memory management for the entropy
+ * buffer, save kentropy pointer to pctx now to free it after use.
+ */
+ pctx->entropy = kentropy;
+ return 0;
}
+#endif

static const struct af_alg_type algif_type_rng = {
.bind = rng_bind,
.release = rng_release,
.accept = rng_accept_parent,
.setkey = rng_setkey,
+#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
+ .setentropy = rng_setentropy,
+#endif
.ops = &algif_rng_ops,
.name = "rng",
.owner = THIS_MODULE
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 56527c85d122..9e5c8ac53c59 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -46,6 +46,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..60b7c2efd921 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_DRBG_ENTROPY 6

/* Operations */
#define ALG_OP_DECRYPT 0
--
2.28.0.rc0.142.g3c755180ce-goog

2020-08-13 16:05:25

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface

Err, I sent the old patch file, sorry. Real v5 coming shortly...

On Thu, 13 Aug 2020 at 17:02, Elena Petrova <[email protected]> wrote:
>
> Extend the user-space RNG interface:
> 1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
> 2. Add additional data input via sendmsg syscall.
>
> This allows DRBG to be tested with test vectors, for example for the
> purpose of CAVP testing, which otherwise isn't possible.
>
> To prevent erroneous use of entropy input, it is hidden under
> CRYPTO_USER_API_CAVP_DRBG config option and requires CAP_SYS_ADMIN to
> succeed.
>
> Signed-off-by: Elena Petrova <[email protected]>
> ---
>
> Updates in v4:
> 1) setentropy returns 0 or error code (used to return length);
> 2) bigfixes suggested by Eric.
>
> Updates in v3:
> 1) More details in commit message;
> 2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
> 3) fixed a bug of not releasing socket locks.
>
> Updates in v2:
> 1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
> 2) Requiring CAP_SYS_ADMIN for entropy reset.
> 3) Locking for send and recv.
> 4) Length checks added for send and setentropy; send and setentropy now return
> number of bytes accepted.
> 5) Minor code style corrections.
>
> libkcapi patch for testing:
> https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531
>
> Documentation/crypto/userspace-if.rst | 17 +++-
> crypto/Kconfig | 8 ++
> crypto/af_alg.c | 8 ++
> crypto/algif_rng.c | 130 ++++++++++++++++++++++++--
> include/crypto/if_alg.h | 1 +
> include/uapi/linux/if_alg.h | 1 +
> 6 files changed, 152 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
> index ff86befa61e0..ef7132802c2d 100644
> --- a/Documentation/crypto/userspace-if.rst
> +++ b/Documentation/crypto/userspace-if.rst
> @@ -296,15 +296,23 @@ follows:
>
> struct sockaddr_alg sa = {
> .salg_family = AF_ALG,
> - .salg_type = "rng", /* this selects the symmetric cipher */
> - .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
> + .salg_type = "rng", /* this selects the random number generator */
> + .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
> };
>
>
> Depending on the RNG type, the RNG must be seeded. The seed is provided
> using the setsockopt interface to set the key. For example, the
> ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
> -seeded.
> +seeded. The seed is also known as a *Personalization String* in DRBG800-90A
> +standard.
> +
> +For the purpose of CAVP testing, the concatenation of *Entropy* and *Nonce*
> +can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface. This
> +requires a kernel built with CONFIG_CRYPTO_USER_API_CAVP_DRBG, and
> +CAP_SYS_ADMIN permission.
> +
> +*Additional Data* can be provided using the send()/sendmsg() system calls.
>
> Using the read()/recvmsg() system calls, random numbers can be obtained.
> The kernel generates at most 128 bytes in one call. If user space
> @@ -377,6 +385,9 @@ mentioned optname:
> provided ciphertext is assumed to contain an authentication tag of
> the given size (see section about AEAD memory layout below).
>
> +- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
> + This option is applicable to RNG cipher type only.
> +
> User space API example
> ----------------------
>
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 091c0a0bbf26..aa2b3085a431 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1896,6 +1896,14 @@ config CRYPTO_STATS
> config CRYPTO_HASH_INFO
> bool
>
> +config CRYPTO_USER_API_CAVP_DRBG
> + tristate "Enable CAVP testing of DRBG"
> + depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
> + help
> + This option enables the resetting of DRBG entropy via the user-space
> + interface. This should only be enabled for CAVP testing. You should say
> + no unless you know what this is.
> +
> source "lib/crypto/Kconfig"
> source "drivers/crypto/Kconfig"
> source "crypto/asymmetric_keys/Kconfig"
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index b1cd3535c525..27d6248ca447 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -260,6 +260,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
> if (!type->setauthsize)
> goto unlock;
> err = type->setauthsize(ask->private, optlen);
> + break;
> + case ALG_SET_DRBG_ENTROPY:
> + if (sock->state == SS_CONNECTED)
> + goto unlock;
> + if (!type->setentropy)
> + goto unlock;
> +
> + err = type->setentropy(ask->private, optval, optlen);
> }
>
> unlock:
> diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
> index 087c0ad09d38..21e8201844bf 100644
> --- a/crypto/algif_rng.c
> +++ b/crypto/algif_rng.c
> @@ -38,6 +38,7 @@
> * DAMAGE.
> */
>
> +#include <linux/capability.h>
> #include <linux/module.h>
> #include <crypto/rng.h>
> #include <linux/random.h>
> @@ -53,20 +54,35 @@ struct rng_ctx {
> #define MAXSIZE 128
> unsigned int len;
> struct crypto_rng *drng;
> + u8 *addtl;
> + size_t addtl_len;
> };
>
> +struct rng_parent_ctx {
> + struct crypto_rng *drng;
> + u8 *entropy;
> +};
> +
> +static void rng_reset_addtl(struct rng_ctx *ctx)
> +{
> + kzfree(ctx->addtl);
> + ctx->addtl = NULL;
> + ctx->addtl_len = 0;
> +}
> +
> static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> int flags)
> {
> struct sock *sk = sock->sk;
> struct alg_sock *ask = alg_sk(sk);
> struct rng_ctx *ctx = ask->private;
> - int err;
> + int err = 0;
> int genlen = 0;
> u8 result[MAXSIZE];
>
> + lock_sock(sock->sk);
> if (len == 0)
> - return 0;
> + goto unlock;
> if (len > MAXSIZE)
> len = MAXSIZE;
>
> @@ -82,13 +98,48 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> * seeding as they automatically seed. The X9.31 DRNG will return
> * an error if it was not seeded properly.
> */
> - genlen = crypto_rng_get_bytes(ctx->drng, result, len);
> - if (genlen < 0)
> - return genlen;
> + genlen = crypto_rng_generate(ctx->drng, ctx->addtl, ctx->addtl_len,
> + result, len);
> + if (genlen < 0) {
> + err = genlen;
> + goto unlock;
> + }
>
> err = memcpy_to_msg(msg, result, len);
> memzero_explicit(result, len);
> + rng_reset_addtl(ctx);
>
> +unlock:
> + release_sock(sock->sk);
> + return err ? err : len;
> +}
> +
> +static int rng_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> +{
> + int err;
> + struct alg_sock *ask = alg_sk(sock->sk);
> + struct rng_ctx *ctx = ask->private;
> +
> + lock_sock(sock->sk);
> + if (len > MAXSIZE)
> + len = MAXSIZE;
> +
> + rng_reset_addtl(ctx);
> + ctx->addtl = kmalloc(len, GFP_KERNEL);
> + if (!ctx->addtl) {
> + err = -ENOMEM;
> + goto unlock;
> + }
> +
> + err = memcpy_from_msg(ctx->addtl, msg, len);
> + if (err) {
> + rng_reset_addtl(ctx);
> + goto unlock;
> + }
> + ctx->addtl_len = len;
> +
> +unlock:
> + release_sock(sock->sk);
> return err ? err : len;
> }
>
> @@ -106,21 +157,41 @@ static struct proto_ops algif_rng_ops = {
> .bind = sock_no_bind,
> .accept = sock_no_accept,
> .setsockopt = sock_no_setsockopt,
> - .sendmsg = sock_no_sendmsg,
> .sendpage = sock_no_sendpage,
>
> .release = af_alg_release,
> .recvmsg = rng_recvmsg,
> + .sendmsg = rng_sendmsg,
> };
>
> static void *rng_bind(const char *name, u32 type, u32 mask)
> {
> - return crypto_alloc_rng(name, type, mask);
> + struct rng_parent_ctx *pctx;
> + struct crypto_rng *rng;
> +
> + pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
> + if (!pctx)
> + return ERR_PTR(-ENOMEM);
> +
> + rng = crypto_alloc_rng(name, type, mask);
> + if (IS_ERR(rng)) {
> + kfree(pctx);
> + return ERR_CAST(rng);
> + }
> +
> + pctx->drng = rng;
> + return pctx;
> }
>
> static void rng_release(void *private)
> {
> - crypto_free_rng(private);
> + struct rng_parent_ctx *pctx = private;
> +
> + if (unlikely(!pctx))
> + return;
> + crypto_free_rng(pctx->drng);
> + kzfree(pctx->entropy);
> + kzfree(pctx);
> }
>
> static void rng_sock_destruct(struct sock *sk)
> @@ -128,6 +199,7 @@ static void rng_sock_destruct(struct sock *sk)
> struct alg_sock *ask = alg_sk(sk);
> struct rng_ctx *ctx = ask->private;
>
> + rng_reset_addtl(ctx);
> sock_kfree_s(sk, ctx, ctx->len);
> af_alg_release_parent(sk);
> }
> @@ -135,6 +207,7 @@ static void rng_sock_destruct(struct sock *sk)
> static int rng_accept_parent(void *private, struct sock *sk)
> {
> struct rng_ctx *ctx;
> + struct rng_parent_ctx *pctx = private;
> struct alg_sock *ask = alg_sk(sk);
> unsigned int len = sizeof(*ctx);
>
> @@ -150,7 +223,9 @@ static int rng_accept_parent(void *private, struct sock *sk)
> * state of the RNG.
> */
>
> - ctx->drng = private;
> + ctx->drng = pctx->drng;
> + ctx->addtl = NULL;
> + ctx->addtl_len = 0;
> ask->private = ctx;
> sk->sk_destruct = rng_sock_destruct;
>
> @@ -159,18 +234,53 @@ static int rng_accept_parent(void *private, struct sock *sk)
>
> static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
> {
> + struct rng_parent_ctx *pctx = private;
> /*
> * Check whether seedlen is of sufficient size is done in RNG
> * implementations.
> */
> - return crypto_rng_reset(private, seed, seedlen);
> + return crypto_rng_reset(pctx->drng, seed, seedlen);
> +}
> +
> +#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
> +static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
> +{
> + struct rng_parent_ctx *pctx = private;
> + u8 *kentropy = NULL;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
> +
> + if (pctx->entropy)
> + return -EINVAL;
> +
> + if (len > MAXSIZE)
> + return -EMSGSIZE;
> +
> + if (len) {
> + kentropy = memdup_user(entropy, len);
> + if (IS_ERR(kentropy))
> + return PTR_ERR(kentropy);
> + }
> +
> + crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
> + /*
> + * Since rng doesn't perform any memory management for the entropy
> + * buffer, save kentropy pointer to pctx now to free it after use.
> + */
> + pctx->entropy = kentropy;
> + return 0;
> }
> +#endif
>
> static const struct af_alg_type algif_type_rng = {
> .bind = rng_bind,
> .release = rng_release,
> .accept = rng_accept_parent,
> .setkey = rng_setkey,
> +#ifdef CONFIG_CRYPTO_USER_API_CAVP_DRBG
> + .setentropy = rng_setentropy,
> +#endif
> .ops = &algif_rng_ops,
> .name = "rng",
> .owner = THIS_MODULE
> diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> index 56527c85d122..9e5c8ac53c59 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -46,6 +46,7 @@ struct af_alg_type {
> void *(*bind)(const char *name, u32 type, u32 mask);
> void (*release)(void *private);
> int (*setkey)(void *private, const u8 *key, unsigned int keylen);
> + int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
> int (*accept)(void *private, struct sock *sk);
> int (*accept_nokey)(void *private, struct sock *sk);
> int (*setauthsize)(void *private, unsigned int authsize);
> diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> index bc2bcdec377b..60b7c2efd921 100644
> --- a/include/uapi/linux/if_alg.h
> +++ b/include/uapi/linux/if_alg.h
> @@ -35,6 +35,7 @@ struct af_alg_iv {
> #define ALG_SET_OP 3
> #define ALG_SET_AEAD_ASSOCLEN 4
> #define ALG_SET_AEAD_AUTHSIZE 5
> +#define ALG_SET_DRBG_ENTROPY 6
>
> /* Operations */
> #define ALG_OP_DECRYPT 0
> --
> 2.28.0.rc0.142.g3c755180ce-goog
>

2020-08-13 16:08:48

by Elena Petrova

[permalink] [raw]
Subject: [PATCH v5] crypto: af_alg - add extra parameters for DRBG interface

Extend the user-space RNG interface:
1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
2. Add additional data input via sendmsg syscall.

This allows DRBG to be tested with test vectors, for example for the
purpose of CAVP testing, which otherwise isn't possible.

To prevent erroneous use of entropy input, it is hidden under
CRYPTO_USER_API_CAVP_DRBG config option and requires CAP_SYS_ADMIN to
succeed.

Signed-off-by: Elena Petrova <[email protected]>
Acked-by: Stephan Müller <[email protected]>
---

Updates in v5:
1) use __maybe_unused instead of #ifdef;
2) separate code path for a testing mode;
3) only allow Additional Data input in a testing mode.

Updates in v4:
1) setentropy returns 0 or error code (used to return length);
2) bigfixes suggested by Eric.

Updates in v3:
1) More details in commit message;
2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
3) fixed a bug of not releasing socket locks.

Updates in v2:
1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
2) Requiring CAP_SYS_ADMIN for entropy reset.
3) Locking for send and recv.
4) Length checks added for send and setentropy; send and setentropy now return
number of bytes accepted.
5) Minor code style corrections.

libkcapi patch for testing:
https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531

Documentation/crypto/userspace-if.rst | 17 ++-
crypto/Kconfig | 9 ++
crypto/af_alg.c | 8 ++
crypto/algif_rng.c | 172 ++++++++++++++++++++++++--
include/crypto/if_alg.h | 1 +
include/uapi/linux/if_alg.h | 1 +
6 files changed, 193 insertions(+), 15 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index ff86befa61e0..ef7132802c2d 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -296,15 +296,23 @@ follows:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
- .salg_type = "rng", /* this selects the symmetric cipher */
- .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
+ .salg_type = "rng", /* this selects the random number generator */
+ .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
};


Depending on the RNG type, the RNG must be seeded. The seed is provided
using the setsockopt interface to set the key. For example, the
ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
-seeded.
+seeded. The seed is also known as a *Personalization String* in DRBG800-90A
+standard.
+
+For the purpose of CAVP testing, the concatenation of *Entropy* and *Nonce*
+can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface. This
+requires a kernel built with CONFIG_CRYPTO_USER_API_CAVP_DRBG, and
+CAP_SYS_ADMIN permission.
+
+*Additional Data* can be provided using the send()/sendmsg() system calls.

Using the read()/recvmsg() system calls, random numbers can be obtained.
The kernel generates at most 128 bytes in one call. If user space
@@ -377,6 +385,9 @@ mentioned optname:
provided ciphertext is assumed to contain an authentication tag of
the given size (see section about AEAD memory layout below).

+- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
+ This option is applicable to RNG cipher type only.
+
User space API example
----------------------

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 091c0a0bbf26..7c8736f71681 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1896,6 +1896,15 @@ config CRYPTO_STATS
config CRYPTO_HASH_INFO
bool

+config CRYPTO_USER_API_CAVP_DRBG
+ tristate "Enable CAVP testing of DRBG"
+ depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
+ help
+ This option enables extra API for CAVP testing via the user-space
+ interface: resetting of DRBG entropy, and providing Additional Data.
+ This should only be enabled for CAVP testing. You should say
+ no unless you know what this is.
+
source "lib/crypto/Kconfig"
source "drivers/crypto/Kconfig"
source "crypto/asymmetric_keys/Kconfig"
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index b1cd3535c525..27d6248ca447 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -260,6 +260,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+ break;
+ case ALG_SET_DRBG_ENTROPY:
+ if (sock->state == SS_CONNECTED)
+ goto unlock;
+ if (!type->setentropy)
+ goto unlock;
+
+ err = type->setentropy(ask->private, optval, optlen);
}

unlock:
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 087c0ad09d38..6ec78c444206 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -38,6 +38,7 @@
* DAMAGE.
*/

+#include <linux/capability.h>
#include <linux/module.h>
#include <crypto/rng.h>
#include <linux/random.h>
@@ -53,15 +54,26 @@ struct rng_ctx {
#define MAXSIZE 128
unsigned int len;
struct crypto_rng *drng;
+ u8 *addtl;
+ size_t addtl_len;
};

-static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
- int flags)
+struct rng_parent_ctx {
+ struct crypto_rng *drng;
+ u8 *entropy;
+};
+
+static void rng_reset_addtl(struct rng_ctx *ctx)
{
- struct sock *sk = sock->sk;
- struct alg_sock *ask = alg_sk(sk);
- struct rng_ctx *ctx = ask->private;
- int err;
+ kzfree(ctx->addtl);
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
+}
+
+static int _rng_recvmsg(struct crypto_rng *drng, struct msghdr *msg, size_t len,
+ u8 *addtl, size_t addtl_len)
+{
+ int err = 0;
int genlen = 0;
u8 result[MAXSIZE];

@@ -82,7 +94,7 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
* seeding as they automatically seed. The X9.31 DRNG will return
* an error if it was not seeded properly.
*/
- genlen = crypto_rng_get_bytes(ctx->drng, result, len);
+ genlen = crypto_rng_generate(drng, addtl, addtl_len, result, len);
if (genlen < 0)
return genlen;

@@ -92,7 +104,62 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
return err ? err : len;
}

-static struct proto_ops algif_rng_ops = {
+static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+
+ return _rng_recvmsg(ctx->drng, msg, len, NULL, 0);
+}
+
+static int rng_test_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+ int err;
+
+ lock_sock(sock->sk);
+ err = _rng_recvmsg(ctx->drng, msg, len, ctx->addtl, ctx->addtl_len);
+ rng_reset_addtl(ctx);
+ release_sock(sock->sk);
+
+ return err ? err : len;
+}
+
+static int rng_test_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sock->sk);
+ struct rng_ctx *ctx = ask->private;
+
+ lock_sock(sock->sk);
+ if (len > MAXSIZE)
+ len = MAXSIZE;
+
+ rng_reset_addtl(ctx);
+ ctx->addtl = kmalloc(len, GFP_KERNEL);
+ if (!ctx->addtl) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ err = memcpy_from_msg(ctx->addtl, msg, len);
+ if (err) {
+ rng_reset_addtl(ctx);
+ goto unlock;
+ }
+ ctx->addtl_len = len;
+
+unlock:
+ release_sock(sock->sk);
+ return err ? err : len;
+}
+
+static struct proto_ops __maybe_unused algif_rng_ops = {
.family = PF_ALG,

.connect = sock_no_connect,
@@ -113,14 +180,55 @@ static struct proto_ops algif_rng_ops = {
.recvmsg = rng_recvmsg,
};

+static struct proto_ops __maybe_unused algif_rng_test_ops = {
+ .family = PF_ALG,
+
+ .connect = sock_no_connect,
+ .socketpair = sock_no_socketpair,
+ .getname = sock_no_getname,
+ .ioctl = sock_no_ioctl,
+ .listen = sock_no_listen,
+ .shutdown = sock_no_shutdown,
+ .getsockopt = sock_no_getsockopt,
+ .mmap = sock_no_mmap,
+ .bind = sock_no_bind,
+ .accept = sock_no_accept,
+ .setsockopt = sock_no_setsockopt,
+ .sendpage = sock_no_sendpage,
+
+ .release = af_alg_release,
+ .recvmsg = rng_test_recvmsg,
+ .sendmsg = rng_test_sendmsg,
+};
+
static void *rng_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_rng(name, type, mask);
+ struct rng_parent_ctx *pctx;
+ struct crypto_rng *rng;
+
+ pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+ if (!pctx)
+ return ERR_PTR(-ENOMEM);
+
+ rng = crypto_alloc_rng(name, type, mask);
+ if (IS_ERR(rng)) {
+ kfree(pctx);
+ return ERR_CAST(rng);
+ }
+
+ pctx->drng = rng;
+ return pctx;
}

static void rng_release(void *private)
{
- crypto_free_rng(private);
+ struct rng_parent_ctx *pctx = private;
+
+ if (unlikely(!pctx))
+ return;
+ crypto_free_rng(pctx->drng);
+ kzfree(pctx->entropy);
+ kzfree(pctx);
}

static void rng_sock_destruct(struct sock *sk)
@@ -128,6 +236,7 @@ static void rng_sock_destruct(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;

+ rng_reset_addtl(ctx);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -135,6 +244,7 @@ static void rng_sock_destruct(struct sock *sk)
static int rng_accept_parent(void *private, struct sock *sk)
{
struct rng_ctx *ctx;
+ struct rng_parent_ctx *pctx = private;
struct alg_sock *ask = alg_sk(sk);
unsigned int len = sizeof(*ctx);

@@ -143,6 +253,8 @@ static int rng_accept_parent(void *private, struct sock *sk)
return -ENOMEM;

ctx->len = len;
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;

/*
* No seeding done at that point -- if multiple accepts are
@@ -150,7 +262,7 @@ static int rng_accept_parent(void *private, struct sock *sk)
* state of the RNG.
*/

- ctx->drng = private;
+ ctx->drng = pctx->drng;
ask->private = ctx;
sk->sk_destruct = rng_sock_destruct;

@@ -159,11 +271,42 @@ static int rng_accept_parent(void *private, struct sock *sk)

static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
{
+ struct rng_parent_ctx *pctx = private;
/*
* Check whether seedlen is of sufficient size is done in RNG
* implementations.
*/
- return crypto_rng_reset(private, seed, seedlen);
+ return crypto_rng_reset(pctx->drng, seed, seedlen);
+}
+
+static int __maybe_unused rng_setentropy(void *private, const u8 *entropy,
+ unsigned int len)
+{
+ struct rng_parent_ctx *pctx = private;
+ u8 *kentropy = NULL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ if (pctx->entropy)
+ return -EINVAL;
+
+ if (len > MAXSIZE)
+ return -EMSGSIZE;
+
+ if (len) {
+ kentropy = memdup_user(entropy, len);
+ if (IS_ERR(kentropy))
+ return PTR_ERR(kentropy);
+ }
+
+ crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
+ /*
+ * Since rng doesn't perform any memory management for the entropy
+ * buffer, save kentropy pointer to pctx now to free it after use.
+ */
+ pctx->entropy = kentropy;
+ return 0;
}

static const struct af_alg_type algif_type_rng = {
@@ -171,7 +314,12 @@ static const struct af_alg_type algif_type_rng = {
.release = rng_release,
.accept = rng_accept_parent,
.setkey = rng_setkey,
+#if IS_ENABLED(CONFIG_CRYPTO_USER_API_CAVP_DRBG)
+ .setentropy = rng_setentropy,
+ .ops = &algif_rng_test_ops,
+#else
.ops = &algif_rng_ops,
+#endif
.name = "rng",
.owner = THIS_MODULE
};
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 56527c85d122..9e5c8ac53c59 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -46,6 +46,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..60b7c2efd921 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_DRBG_ENTROPY 6

/* Operations */
#define ALG_OP_DECRYPT 0
--
2.28.0.220.ged08abb693-goog

2020-08-13 19:33:20

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH v5] crypto: af_alg - add extra parameters for DRBG interface

On Thu, Aug 13, 2020 at 05:08:11PM +0100, Elena Petrova wrote:
> Extend the user-space RNG interface:
> 1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
> 2. Add additional data input via sendmsg syscall.
>
> This allows DRBG to be tested with test vectors, for example for the
> purpose of CAVP testing, which otherwise isn't possible.
>
> To prevent erroneous use of entropy input, it is hidden under
> CRYPTO_USER_API_CAVP_DRBG config option and requires CAP_SYS_ADMIN to
> succeed.
>
> Signed-off-by: Elena Petrova <[email protected]>
> Acked-by: Stephan M?ller <[email protected]>
> ---
>
> Updates in v5:
> 1) use __maybe_unused instead of #ifdef;
> 2) separate code path for a testing mode;
> 3) only allow Additional Data input in a testing mode.
>
> Updates in v4:
> 1) setentropy returns 0 or error code (used to return length);
> 2) bigfixes suggested by Eric.
>
> Updates in v3:
> 1) More details in commit message;
> 2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
> 3) fixed a bug of not releasing socket locks.
>
> Updates in v2:
> 1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
> 2) Requiring CAP_SYS_ADMIN for entropy reset.
> 3) Locking for send and recv.
> 4) Length checks added for send and setentropy; send and setentropy now return
> number of bytes accepted.
> 5) Minor code style corrections.
>
> libkcapi patch for testing:
> https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531
>
> Documentation/crypto/userspace-if.rst | 17 ++-
> crypto/Kconfig | 9 ++
> crypto/af_alg.c | 8 ++
> crypto/algif_rng.c | 172 ++++++++++++++++++++++++--
> include/crypto/if_alg.h | 1 +
> include/uapi/linux/if_alg.h | 1 +
> 6 files changed, 193 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
> index ff86befa61e0..ef7132802c2d 100644
> --- a/Documentation/crypto/userspace-if.rst
> +++ b/Documentation/crypto/userspace-if.rst
> @@ -296,15 +296,23 @@ follows:
>
> struct sockaddr_alg sa = {
> .salg_family = AF_ALG,
> - .salg_type = "rng", /* this selects the symmetric cipher */
> - .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
> + .salg_type = "rng", /* this selects the random number generator */
> + .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
> };
>
>
> Depending on the RNG type, the RNG must be seeded. The seed is provided
> using the setsockopt interface to set the key. For example, the
> ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
> -seeded.
> +seeded. The seed is also known as a *Personalization String* in DRBG800-90A
> +standard.

Isn't the standard called "NIST SP 800-90A"?
"DRBG800-90A" doesn't return many hits on Google.

> +For the purpose of CAVP testing, the concatenation of *Entropy* and *Nonce*
> +can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface. This
> +requires a kernel built with CONFIG_CRYPTO_USER_API_CAVP_DRBG, and
> +CAP_SYS_ADMIN permission.
> +
> +*Additional Data* can be provided using the send()/sendmsg() system calls.

This doesn't make it clear whether the support for "Additional Data" is
dependent on CONFIG_CRYPTO_USER_API_CAVP_DRBG or not.

> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 091c0a0bbf26..7c8736f71681 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1896,6 +1896,15 @@ config CRYPTO_STATS
> config CRYPTO_HASH_INFO
> bool
>
> +config CRYPTO_USER_API_CAVP_DRBG
> + tristate "Enable CAVP testing of DRBG"
> + depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
> + help
> + This option enables extra API for CAVP testing via the user-space
> + interface: resetting of DRBG entropy, and providing Additional Data.
> + This should only be enabled for CAVP testing. You should say
> + no unless you know what this is.

Using "tristate" here is incorrect because this option is not a module itself.
It's an option *for* a module. So it needs to be "bool" instead.

Also, since this is an option to refine CRYPTO_USER_API_RNG, how about renaming
it to "CRYPTO_USER_API_RNG_CAVP", and moving it to just below the definition of
"CRYPTO_USER_API_RNG" so that they show up adjacent to each other?

> +static int rng_test_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> + int flags)
> +{
> + struct sock *sk = sock->sk;
> + struct alg_sock *ask = alg_sk(sk);
> + struct rng_ctx *ctx = ask->private;
> + int err;
> +
> + lock_sock(sock->sk);
> + err = _rng_recvmsg(ctx->drng, msg, len, ctx->addtl, ctx->addtl_len);
> + rng_reset_addtl(ctx);
> + release_sock(sock->sk);
> +
> + return err ? err : len;

Shouldn't this just return the value that _rng_recvmsg() returned?

Also 'err' is conventionally just for 0 or -errno codes. Use 'ret' if the
variable can also hold a length.

> +static struct proto_ops __maybe_unused algif_rng_test_ops = {
> + .family = PF_ALG,
> +
> + .connect = sock_no_connect,
> + .socketpair = sock_no_socketpair,
> + .getname = sock_no_getname,
> + .ioctl = sock_no_ioctl,
> + .listen = sock_no_listen,
> + .shutdown = sock_no_shutdown,
> + .getsockopt = sock_no_getsockopt,
> + .mmap = sock_no_mmap,
> + .bind = sock_no_bind,
> + .accept = sock_no_accept,
> + .setsockopt = sock_no_setsockopt,
> + .sendpage = sock_no_sendpage,
> +
> + .release = af_alg_release,
> + .recvmsg = rng_test_recvmsg,
> + .sendmsg = rng_test_sendmsg,
> +};
[...]
> static const struct af_alg_type algif_type_rng = {
> @@ -171,7 +314,12 @@ static const struct af_alg_type algif_type_rng = {
> .release = rng_release,
> .accept = rng_accept_parent,
> .setkey = rng_setkey,
> +#if IS_ENABLED(CONFIG_CRYPTO_USER_API_CAVP_DRBG)
> + .setentropy = rng_setentropy,
> + .ops = &algif_rng_test_ops,
> +#else
> .ops = &algif_rng_ops,
> +#endif
> .name = "rng",
> .owner = THIS_MODULE
> };

I think that switching to the separate proto_ops structs made the patch worse.
Now there's duplicated code.

Since proto_ops are almost identical, and only one is used in a given kernel
build, why not just do:

static struct proto_ops algif_rng_ops = {
...
#ifdef CONFIG_CRYPTO_USER_API_RNG_CAVP
.sendmsg = rng_sendmsg,
#else
.sendmsg = sock_no_sendmsg,
#endif
...
};

Similarly for .recvmsg(), although I don't understand what's wrong with just
adding the lock_sock() instead... The RNG algorithms do locking anyway, so it's
not like that would regress the ability to recvmsg() in parallel. Also,
conditional locking depending on the kernel config makes it more difficult to
find kernel bugs like deadlocks.

- Eric

2020-08-21 04:25:27

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v5] crypto: af_alg - add extra parameters for DRBG interface

Eric Biggers <[email protected]> wrote:
>
> Since proto_ops are almost identical, and only one is used in a given kernel
> build, why not just do:
>
> static struct proto_ops algif_rng_ops = {
> ...
> #ifdef CONFIG_CRYPTO_USER_API_RNG_CAVP
> .sendmsg = rng_sendmsg,
> #else
> .sendmsg = sock_no_sendmsg,
> #endif
> ...
> };
>
> Similarly for .recvmsg(), although I don't understand what's wrong with just
> adding the lock_sock() instead... The RNG algorithms do locking anyway, so it's
> not like that would regress the ability to recvmsg() in parallel. Also,
> conditional locking depending on the kernel config makes it more difficult to
> find kernel bugs like deadlocks.

I want this to have minimal impact on anyone who's not using it.
After all, this is something that only Google is asking for.

Anyway, I wasn't looking for a compile-time ops switch, but a
run-time one.

I think what we can do is move the new newsock->ops assignment
in af_alg_accept up above the type->accept call which would then
allow it to be overridden by the accept call.

After that you could just change newsock->ops depending on whether
pctx->entropy is NULL or not in rng_accept_parent.

As for the proto_ops duplication I don't think it's that big a
deal, but if you're really bothered just create a macro for the
identical bits in the struct.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-09-08 17:06:17

by Elena Petrova

[permalink] [raw]
Subject: [PATCH v6] crypto: af_alg - add extra parameters for DRBG interface

Extend the user-space RNG interface:
1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
2. Add additional data input via sendmsg syscall.

This allows DRBG to be tested with test vectors, for example for the
purpose of CAVP testing, which otherwise isn't possible.

To prevent erroneous use of entropy input, it is hidden under
CRYPTO_USER_API_RNG_CAVP config option and requires CAP_SYS_ADMIN to
succeed.

Signed-off-by: Elena Petrova <[email protected]>
Acked-by: Stephan Müller <[email protected]>
---

Updates in v6:
1) Kconfig option renamed to CRYPTO_USER_API_RNG_CAVP and is now bool instead
of tristate;
2) run-time switch of proto_ops depending on whether the entropy was set;
3) corrected the NIST standard name;
4) rebased onto the tip of the tree;
5) documentation clarified;

Updates in v5:
1) use __maybe_unused instead of #ifdef;
2) separate code path for a testing mode;
3) only allow Additional Data input in a testing mode.

Updates in v4:
1) setentropy returns 0 or error code (used to return length);
2) bigfixes suggested by Eric.

Updates in v3:
1) More details in commit message;
2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
3) fixed a bug of not releasing socket locks.

Updates in v2:
1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
2) Requiring CAP_SYS_ADMIN for entropy reset.
3) Locking for send and recv.
4) Length checks added for send and setentropy; send and setentropy now return
number of bytes accepted.
5) Minor code style corrections.

libkcapi patch for testing:
https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531

Documentation/crypto/userspace-if.rst | 19 ++-
crypto/Kconfig | 9 ++
crypto/af_alg.c | 14 ++-
crypto/algif_rng.c | 175 ++++++++++++++++++++++++--
include/crypto/if_alg.h | 1 +
include/uapi/linux/if_alg.h | 1 +
6 files changed, 204 insertions(+), 15 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index 52019e905900..6a542b0cf22c 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -296,15 +296,16 @@ follows:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
- .salg_type = "rng", /* this selects the symmetric cipher */
- .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
+ .salg_type = "rng", /* this selects the random number generator */
+ .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
};


Depending on the RNG type, the RNG must be seeded. The seed is provided
using the setsockopt interface to set the key. For example, the
ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
-seeded.
+seeded. The seed is also known as a *Personalization String* in NIST SP 800-90A
+standard.

Using the read()/recvmsg() system calls, random numbers can be obtained.
The kernel generates at most 128 bytes in one call. If user space
@@ -314,6 +315,15 @@ WARNING: The user space caller may invoke the initially mentioned accept
system call multiple times. In this case, the returned file descriptors
have the same state.

+Following CAVP testing interfaces are enabled when kernel is built with
+CRYPTO_USER_API_RNG_CAVP option:
+
+- the concatenation of *Entropy* and *Nonce* can be provided to the RNG via
+ ALG_SET_DRBG_ENTROPY setsockopt interface. Setting the entropy requires
+ CAP_SYS_ADMIN permission.
+
+- *Additional Data* can be provided using the send()/sendmsg() system calls.
+
Zero-Copy Interface
-------------------

@@ -377,6 +387,9 @@ mentioned optname:
provided ciphertext is assumed to contain an authentication tag of
the given size (see section about AEAD memory layout below).

+- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
+ This option is applicable to RNG cipher type only.
+
User space API example
----------------------

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 1b57419fa2e7..070a88ec1ba8 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1870,6 +1870,15 @@ config CRYPTO_USER_API_RNG
This option enables the user-spaces interface for random
number generator algorithms.

+config CRYPTO_USER_API_RNG_CAVP
+ bool "Enable CAVP testing of DRBG"
+ depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
+ help
+ This option enables extra API for CAVP testing via the user-space
+ interface: resetting of DRBG entropy, and providing Additional Data.
+ This should only be enabled for CAVP testing. You should say
+ no unless you know what this is.
+
config CRYPTO_USER_API_AEAD
tristate "User-space interface for AEAD cipher algorithms"
depends on NET
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 8be8bec07cdd..d11db80d24cd 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -254,6 +254,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+ break;
+ case ALG_SET_DRBG_ENTROPY:
+ if (sock->state == SS_CONNECTED)
+ goto unlock;
+ if (!type->setentropy)
+ goto unlock;
+
+ err = type->setentropy(ask->private, optval, optlen);
}

unlock:
@@ -286,6 +294,11 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
security_sock_graft(sk2, newsock);
security_sk_clone(sk, sk2);

+ /*
+ * newsock->ops assigned here to allow type->accept call to override
+ * them when required.
+ */
+ newsock->ops = type->ops;
err = type->accept(ask->private, sk2);

nokey = err == -ENOKEY;
@@ -304,7 +317,6 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
alg_sk(sk2)->parent = sk;
alg_sk(sk2)->type = type;

- newsock->ops = type->ops;
newsock->state = SS_CONNECTED;

if (nokey)
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 6300e0566dc5..6b1f01ca8e31 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -38,6 +38,7 @@
* DAMAGE.
*/

+#include <linux/capability.h>
#include <linux/module.h>
#include <crypto/rng.h>
#include <linux/random.h>
@@ -53,15 +54,26 @@ struct rng_ctx {
#define MAXSIZE 128
unsigned int len;
struct crypto_rng *drng;
+ u8 *addtl;
+ size_t addtl_len;
};

-static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
- int flags)
+struct rng_parent_ctx {
+ struct crypto_rng *drng;
+ u8 *entropy;
+};
+
+static void rng_reset_addtl(struct rng_ctx *ctx)
{
- struct sock *sk = sock->sk;
- struct alg_sock *ask = alg_sk(sk);
- struct rng_ctx *ctx = ask->private;
- int err;
+ kzfree(ctx->addtl);
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
+}
+
+static int _rng_recvmsg(struct crypto_rng *drng, struct msghdr *msg, size_t len,
+ u8 *addtl, size_t addtl_len)
+{
+ int err = 0;
int genlen = 0;
u8 result[MAXSIZE];

@@ -82,7 +94,7 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
* seeding as they automatically seed. The X9.31 DRNG will return
* an error if it was not seeded properly.
*/
- genlen = crypto_rng_get_bytes(ctx->drng, result, len);
+ genlen = crypto_rng_generate(drng, addtl, addtl_len, result, len);
if (genlen < 0)
return genlen;

@@ -92,6 +104,61 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
return err ? err : len;
}

+static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+
+ return _rng_recvmsg(ctx->drng, msg, len, NULL, 0);
+}
+
+static int rng_test_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+ int ret;
+
+ lock_sock(sock->sk);
+ ret = _rng_recvmsg(ctx->drng, msg, len, ctx->addtl, ctx->addtl_len);
+ rng_reset_addtl(ctx);
+ release_sock(sock->sk);
+
+ return ret;
+}
+
+static int rng_test_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sock->sk);
+ struct rng_ctx *ctx = ask->private;
+
+ lock_sock(sock->sk);
+ if (len > MAXSIZE)
+ len = MAXSIZE;
+
+ rng_reset_addtl(ctx);
+ ctx->addtl = kmalloc(len, GFP_KERNEL);
+ if (!ctx->addtl) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ err = memcpy_from_msg(ctx->addtl, msg, len);
+ if (err) {
+ rng_reset_addtl(ctx);
+ goto unlock;
+ }
+ ctx->addtl_len = len;
+
+unlock:
+ release_sock(sock->sk);
+ return err ? err : len;
+}
+
static struct proto_ops algif_rng_ops = {
.family = PF_ALG,

@@ -111,14 +178,55 @@ static struct proto_ops algif_rng_ops = {
.recvmsg = rng_recvmsg,
};

+static struct proto_ops __maybe_unused algif_rng_test_ops = {
+ .family = PF_ALG,
+
+ .connect = sock_no_connect,
+ .socketpair = sock_no_socketpair,
+ .getname = sock_no_getname,
+ .ioctl = sock_no_ioctl,
+ .listen = sock_no_listen,
+ .shutdown = sock_no_shutdown,
+ .getsockopt = sock_no_getsockopt,
+ .mmap = sock_no_mmap,
+ .bind = sock_no_bind,
+ .accept = sock_no_accept,
+ .setsockopt = sock_no_setsockopt,
+ .sendpage = sock_no_sendpage,
+
+ .release = af_alg_release,
+ .recvmsg = rng_test_recvmsg,
+ .sendmsg = rng_test_sendmsg,
+};
+
static void *rng_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_rng(name, type, mask);
+ struct rng_parent_ctx *pctx;
+ struct crypto_rng *rng;
+
+ pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+ if (!pctx)
+ return ERR_PTR(-ENOMEM);
+
+ rng = crypto_alloc_rng(name, type, mask);
+ if (IS_ERR(rng)) {
+ kfree(pctx);
+ return ERR_CAST(rng);
+ }
+
+ pctx->drng = rng;
+ return pctx;
}

static void rng_release(void *private)
{
- crypto_free_rng(private);
+ struct rng_parent_ctx *pctx = private;
+
+ if (unlikely(!pctx))
+ return;
+ crypto_free_rng(pctx->drng);
+ kzfree(pctx->entropy);
+ kzfree(pctx);
}

static void rng_sock_destruct(struct sock *sk)
@@ -126,6 +234,7 @@ static void rng_sock_destruct(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;

+ rng_reset_addtl(ctx);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -133,6 +242,7 @@ static void rng_sock_destruct(struct sock *sk)
static int rng_accept_parent(void *private, struct sock *sk)
{
struct rng_ctx *ctx;
+ struct rng_parent_ctx *pctx = private;
struct alg_sock *ask = alg_sk(sk);
unsigned int len = sizeof(*ctx);

@@ -141,6 +251,8 @@ static int rng_accept_parent(void *private, struct sock *sk)
return -ENOMEM;

ctx->len = len;
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;

/*
* No seeding done at that point -- if multiple accepts are
@@ -148,20 +260,58 @@ static int rng_accept_parent(void *private, struct sock *sk)
* state of the RNG.
*/

- ctx->drng = private;
+ ctx->drng = pctx->drng;
ask->private = ctx;
sk->sk_destruct = rng_sock_destruct;

+ /*
+ * Non NULL pctx->entropy means that CAVP test has been initiated on
+ * this socket, replace proto_ops algif_rng_ops with algif_rng_test_ops.
+ */
+ if (pctx->entropy)
+ sk->sk_socket->ops = algif_rng_test_ops;
+
return 0;
}

static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
{
+ struct rng_parent_ctx *pctx = private;
/*
* Check whether seedlen is of sufficient size is done in RNG
* implementations.
*/
- return crypto_rng_reset(private, seed, seedlen);
+ return crypto_rng_reset(pctx->drng, seed, seedlen);
+}
+
+static int __maybe_unused rng_setentropy(void *private, const u8 *entropy,
+ unsigned int len)
+{
+ struct rng_parent_ctx *pctx = private;
+ u8 *kentropy = NULL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ if (pctx->entropy)
+ return -EINVAL;
+
+ if (len > MAXSIZE)
+ return -EMSGSIZE;
+
+ if (len) {
+ kentropy = memdup_user(entropy, len);
+ if (IS_ERR(kentropy))
+ return PTR_ERR(kentropy);
+ }
+
+ crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
+ /*
+ * Since rng doesn't perform any memory management for the entropy
+ * buffer, save kentropy pointer to pctx now to free it after use.
+ */
+ pctx->entropy = kentropy;
+ return 0;
}

static const struct af_alg_type algif_type_rng = {
@@ -169,6 +319,9 @@ static const struct af_alg_type algif_type_rng = {
.release = rng_release,
.accept = rng_accept_parent,
.setkey = rng_setkey,
+#if IS_ENABLED(CONFIG_CRYPTO_USER_API_RNG_CAVP)
+ .setentropy = rng_setentropy,
+#endif
.ops = &algif_rng_ops,
.name = "rng",
.owner = THIS_MODULE
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index ee6412314f8f..e6f4a342417d 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -46,6 +46,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..60b7c2efd921 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_DRBG_ENTROPY 6

/* Operations */
#define ALG_OP_DECRYPT 0
--
2.28.0.526.ge36021eeef-goog

2020-09-08 17:19:40

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH v5] crypto: af_alg - add extra parameters for DRBG interface

On Thu, 13 Aug 2020 at 20:32, Eric Biggers <[email protected]> wrote:

> > Depending on the RNG type, the RNG must be seeded. The seed is provided
> > using the setsockopt interface to set the key. For example, the
> > ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
> > -seeded.
> > +seeded. The seed is also known as a *Personalization String* in DRBG800-90A
> > +standard.
>
> Isn't the standard called "NIST SP 800-90A"?
> "DRBG800-90A" doesn't return many hits on Google.

Fixed, thanks!

> > +For the purpose of CAVP testing, the concatenation of *Entropy* and *Nonce*
> > +can be provided to the RNG via ALG_SET_DRBG_ENTROPY setsockopt interface. This
> > +requires a kernel built with CONFIG_CRYPTO_USER_API_CAVP_DRBG, and
> > +CAP_SYS_ADMIN permission.
> > +
> > +*Additional Data* can be provided using the send()/sendmsg() system calls.
>
> This doesn't make it clear whether the support for "Additional Data" is
> dependent on CONFIG_CRYPTO_USER_API_CAVP_DRBG or not.

Fixed.

> > +config CRYPTO_USER_API_CAVP_DRBG
> > + tristate "Enable CAVP testing of DRBG"
> > + depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
> > + help
> > + This option enables extra API for CAVP testing via the user-space
> > + interface: resetting of DRBG entropy, and providing Additional Data.
> > + This should only be enabled for CAVP testing. You should say
> > + no unless you know what this is.
>
> Using "tristate" here is incorrect because this option is not a module itself.
> It's an option *for* a module. So it needs to be "bool" instead.

Done.

> Also, since this is an option to refine CRYPTO_USER_API_RNG, how about renaming
> it to "CRYPTO_USER_API_RNG_CAVP", and moving it to just below the definition of
> "CRYPTO_USER_API_RNG" so that they show up adjacent to each other?

Sounds good, done.

> > +static int rng_test_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> > + int flags)
> > +{
> > + struct sock *sk = sock->sk;
> > + struct alg_sock *ask = alg_sk(sk);
> > + struct rng_ctx *ctx = ask->private;
> > + int err;
> > +
> > + lock_sock(sock->sk);
> > + err = _rng_recvmsg(ctx->drng, msg, len, ctx->addtl, ctx->addtl_len);
> > + rng_reset_addtl(ctx);
> > + release_sock(sock->sk);
> > +
> > + return err ? err : len;
>
> Shouldn't this just return the value that _rng_recvmsg() returned?
>
> Also 'err' is conventionally just for 0 or -errno codes. Use 'ret' if the
> variable can also hold a length.

Done.

2020-09-08 17:24:33

by Elena Petrova

[permalink] [raw]
Subject: Re: [PATCH v5] crypto: af_alg - add extra parameters for DRBG interface

On Fri, 21 Aug 2020 at 05:24, Herbert Xu <[email protected]> wrote:
>
> Eric Biggers <[email protected]> wrote:
> >
> > Since proto_ops are almost identical, and only one is used in a given kernel
> > build, why not just do:
> >
> > static struct proto_ops algif_rng_ops = {
> > ...
> > #ifdef CONFIG_CRYPTO_USER_API_RNG_CAVP
> > .sendmsg = rng_sendmsg,
> > #else
> > .sendmsg = sock_no_sendmsg,
> > #endif
> > ...
> > };
> >
> > Similarly for .recvmsg(), although I don't understand what's wrong with just
> > adding the lock_sock() instead... The RNG algorithms do locking anyway, so it's
> > not like that would regress the ability to recvmsg() in parallel. Also,
> > conditional locking depending on the kernel config makes it more difficult to
> > find kernel bugs like deadlocks.
>
> I want this to have minimal impact on anyone who's not using it.
> After all, this is something that only Google is asking for.
>
> Anyway, I wasn't looking for a compile-time ops switch, but a
> run-time one.
>
> I think what we can do is move the new newsock->ops assignment
> in af_alg_accept up above the type->accept call which would then
> allow it to be overridden by the accept call.
>
> After that you could just change newsock->ops depending on whether
> pctx->entropy is NULL or not in rng_accept_parent.

Ack, done in v6.

> As for the proto_ops duplication I don't think it's that big a
> deal, but if you're really bothered just create a macro for the
> identical bits in the struct.

I didn't create a macro to avoid complicating the code.

Thanks,
Elena

2020-09-09 04:39:04

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH v6] crypto: af_alg - add extra parameters for DRBG interface

On Tue, Sep 08, 2020 at 06:04:03PM +0100, Elena Petrova wrote:
> Extend the user-space RNG interface:
> 1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
> 2. Add additional data input via sendmsg syscall.
>
> This allows DRBG to be tested with test vectors, for example for the
> purpose of CAVP testing, which otherwise isn't possible.
>
> To prevent erroneous use of entropy input, it is hidden under
> CRYPTO_USER_API_RNG_CAVP config option and requires CAP_SYS_ADMIN to
> succeed.
>
> Signed-off-by: Elena Petrova <[email protected]>
> Acked-by: Stephan M?ller <[email protected]>

This doesn't compile for me. Can you rebase this onto the latest
"master" branch from
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git?

> +static void rng_reset_addtl(struct rng_ctx *ctx)
> {
> - struct sock *sk = sock->sk;
> - struct alg_sock *ask = alg_sk(sk);
> - struct rng_ctx *ctx = ask->private;
> - int err;
> + kzfree(ctx->addtl);
> + ctx->addtl = NULL;
> + ctx->addtl_len = 0;
> +}

kzfree() has been renamed to kfree_sensitive(); see commit 453431a54934
("mm, treewide: rename kzfree() to kfree_sensitive()").
So please use kfree_sensitive() rather than kzfree(), in all three places.

Note, kzfree() won't actually cause a compilation error since it's still
#define'd to kfree_sensitive(). But that #define probably will go away soon.

> +static int rng_test_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> +{
> + int err;
> + struct alg_sock *ask = alg_sk(sock->sk);
> + struct rng_ctx *ctx = ask->private;
> +
> + lock_sock(sock->sk);
> + if (len > MAXSIZE)
> + len = MAXSIZE;

Since this function only supports providing the additional data all at once, not
incrementally, shouldn't it return an error code if the length is too long,
rather than truncate the length?

> + /*
> + * Non NULL pctx->entropy means that CAVP test has been initiated on
> + * this socket, replace proto_ops algif_rng_ops with algif_rng_test_ops.
> + */
> + if (pctx->entropy)
> + sk->sk_socket->ops = algif_rng_test_ops;

This means that providing additional data on a "request socket" via sendmsg will
only work if ALG_SET_DRBG_ENTROPY was used on the "algorithm socket" earlier.
If that's intentional, it needs to be mentioned in the documentation.

> static const struct af_alg_type algif_type_rng = {
> @@ -169,6 +319,9 @@ static const struct af_alg_type algif_type_rng = {
> .release = rng_release,
> .accept = rng_accept_parent,
> .setkey = rng_setkey,
> +#if IS_ENABLED(CONFIG_CRYPTO_USER_API_RNG_CAVP)
> + .setentropy = rng_setentropy,
> +#endif

Since CRYPTO_USER_API_RNG_CAVP is now a bool rather than a tristate, this should
use '#ifdef CONFIG_CRYPTO_USER_API_RNG_CAVP' instead of
'IS_ENABLED(CONFIG_CRYPTO_USER_API_RNG_CAVP)'.

- Eric

2020-09-09 18:30:38

by Elena Petrova

[permalink] [raw]
Subject: [PATCH v7] crypto: af_alg - add extra parameters for DRBG interface

Extend the user-space RNG interface:
1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
2. Add additional data input via sendmsg syscall.

This allows DRBG to be tested with test vectors, for example for the
purpose of CAVP testing, which otherwise isn't possible.

To prevent erroneous use of entropy input, it is hidden under
CRYPTO_USER_API_RNG_CAVP config option and requires CAP_SYS_ADMIN to
succeed.

Signed-off-by: Elena Petrova <[email protected]>
Acked-by: Stephan Müller <[email protected]>
---

Updates in v7:
1) rebased onto the latest at cryptodev-2.6.git, fixed compiler errors;
2) replaced kzfree with kfree_sensitive;
3) changed rng_test_sendmsg to return an error if len > MAXSIZE;
4) updated documentation to say when can Additional Data be provided.

Updates in v6:
1) Kconfig option renamed to CRYPTO_USER_API_RNG_CAVP and is now bool instead
of tristate;
2) run-time switch of proto_ops depending on whether the entropy was set;
3) corrected the NIST standard name;
4) rebased onto the tip of the tree;
5) documentation clarified;

Updates in v5:
1) use __maybe_unused instead of #ifdef;
2) separate code path for a testing mode;
3) only allow Additional Data input in a testing mode.

Updates in v4:
1) setentropy returns 0 or error code (used to return length);
2) bigfixes suggested by Eric.

Updates in v3:
1) More details in commit message;
2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
3) fixed a bug of not releasing socket locks.

Updates in v2:
1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
2) Requiring CAP_SYS_ADMIN for entropy reset.
3) Locking for send and recv.
4) Length checks added for send and setentropy; send and setentropy now return
number of bytes accepted.
5) Minor code style corrections.

libkcapi patch for testing:
https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531

Documentation/crypto/userspace-if.rst | 20 ++-
crypto/Kconfig | 9 ++
crypto/af_alg.c | 14 ++-
crypto/algif_rng.c | 175 ++++++++++++++++++++++++--
include/crypto/if_alg.h | 1 +
include/uapi/linux/if_alg.h | 1 +
6 files changed, 205 insertions(+), 15 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index 52019e905900..b45dabbf69d6 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -296,15 +296,16 @@ follows:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
- .salg_type = "rng", /* this selects the symmetric cipher */
- .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
+ .salg_type = "rng", /* this selects the random number generator */
+ .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
};


Depending on the RNG type, the RNG must be seeded. The seed is provided
using the setsockopt interface to set the key. For example, the
ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
-seeded.
+seeded. The seed is also known as a *Personalization String* in NIST SP 800-90A
+standard.

Using the read()/recvmsg() system calls, random numbers can be obtained.
The kernel generates at most 128 bytes in one call. If user space
@@ -314,6 +315,16 @@ WARNING: The user space caller may invoke the initially mentioned accept
system call multiple times. In this case, the returned file descriptors
have the same state.

+Following CAVP testing interfaces are enabled when kernel is built with
+CRYPTO_USER_API_RNG_CAVP option:
+
+- the concatenation of *Entropy* and *Nonce* can be provided to the RNG via
+ ALG_SET_DRBG_ENTROPY setsockopt interface. Setting the entropy requires
+ CAP_SYS_ADMIN permission.
+
+- *Additional Data* can be provided using the send()/sendmsg() system calls,
+ but only after the entropy has been set.
+
Zero-Copy Interface
-------------------

@@ -377,6 +388,9 @@ mentioned optname:
provided ciphertext is assumed to contain an authentication tag of
the given size (see section about AEAD memory layout below).

+- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
+ This option is applicable to RNG cipher type only.
+
User space API example
----------------------

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 1b57419fa2e7..070a88ec1ba8 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1870,6 +1870,15 @@ config CRYPTO_USER_API_RNG
This option enables the user-spaces interface for random
number generator algorithms.

+config CRYPTO_USER_API_RNG_CAVP
+ bool "Enable CAVP testing of DRBG"
+ depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
+ help
+ This option enables extra API for CAVP testing via the user-space
+ interface: resetting of DRBG entropy, and providing Additional Data.
+ This should only be enabled for CAVP testing. You should say
+ no unless you know what this is.
+
config CRYPTO_USER_API_AEAD
tristate "User-space interface for AEAD cipher algorithms"
depends on NET
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index a6f581ab200c..8535cb03b484 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -253,6 +253,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+ break;
+ case ALG_SET_DRBG_ENTROPY:
+ if (sock->state == SS_CONNECTED)
+ goto unlock;
+ if (!type->setentropy)
+ goto unlock;
+
+ err = type->setentropy(ask->private, optval, optlen);
}

unlock:
@@ -285,6 +293,11 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
security_sock_graft(sk2, newsock);
security_sk_clone(sk, sk2);

+ /*
+ * newsock->ops assigned here to allow type->accept call to override
+ * them when required.
+ */
+ newsock->ops = type->ops;
err = type->accept(ask->private, sk2);

nokey = err == -ENOKEY;
@@ -303,7 +316,6 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
alg_sk(sk2)->parent = sk;
alg_sk(sk2)->type = type;

- newsock->ops = type->ops;
newsock->state = SS_CONNECTED;

if (nokey)
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 6300e0566dc5..3ca571b10b08 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -38,6 +38,7 @@
* DAMAGE.
*/

+#include <linux/capability.h>
#include <linux/module.h>
#include <crypto/rng.h>
#include <linux/random.h>
@@ -53,15 +54,26 @@ struct rng_ctx {
#define MAXSIZE 128
unsigned int len;
struct crypto_rng *drng;
+ u8 *addtl;
+ size_t addtl_len;
};

-static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
- int flags)
+struct rng_parent_ctx {
+ struct crypto_rng *drng;
+ u8 *entropy;
+};
+
+static void rng_reset_addtl(struct rng_ctx *ctx)
{
- struct sock *sk = sock->sk;
- struct alg_sock *ask = alg_sk(sk);
- struct rng_ctx *ctx = ask->private;
- int err;
+ kfree_sensitive(ctx->addtl);
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
+}
+
+static int _rng_recvmsg(struct crypto_rng *drng, struct msghdr *msg, size_t len,
+ u8 *addtl, size_t addtl_len)
+{
+ int err = 0;
int genlen = 0;
u8 result[MAXSIZE];

@@ -82,7 +94,7 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
* seeding as they automatically seed. The X9.31 DRNG will return
* an error if it was not seeded properly.
*/
- genlen = crypto_rng_get_bytes(ctx->drng, result, len);
+ genlen = crypto_rng_generate(drng, addtl, addtl_len, result, len);
if (genlen < 0)
return genlen;

@@ -92,6 +104,63 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
return err ? err : len;
}

+static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+
+ return _rng_recvmsg(ctx->drng, msg, len, NULL, 0);
+}
+
+static int rng_test_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+ int ret;
+
+ lock_sock(sock->sk);
+ ret = _rng_recvmsg(ctx->drng, msg, len, ctx->addtl, ctx->addtl_len);
+ rng_reset_addtl(ctx);
+ release_sock(sock->sk);
+
+ return ret;
+}
+
+static int rng_test_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sock->sk);
+ struct rng_ctx *ctx = ask->private;
+
+ lock_sock(sock->sk);
+ if (len > MAXSIZE) {
+ err = -EMSGSIZE;
+ goto unlock;
+ }
+
+ rng_reset_addtl(ctx);
+ ctx->addtl = kmalloc(len, GFP_KERNEL);
+ if (!ctx->addtl) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ err = memcpy_from_msg(ctx->addtl, msg, len);
+ if (err) {
+ rng_reset_addtl(ctx);
+ goto unlock;
+ }
+ ctx->addtl_len = len;
+
+unlock:
+ release_sock(sock->sk);
+ return err ? err : len;
+}
+
static struct proto_ops algif_rng_ops = {
.family = PF_ALG,

@@ -111,14 +180,53 @@ static struct proto_ops algif_rng_ops = {
.recvmsg = rng_recvmsg,
};

+static struct proto_ops __maybe_unused algif_rng_test_ops = {
+ .family = PF_ALG,
+
+ .connect = sock_no_connect,
+ .socketpair = sock_no_socketpair,
+ .getname = sock_no_getname,
+ .ioctl = sock_no_ioctl,
+ .listen = sock_no_listen,
+ .shutdown = sock_no_shutdown,
+ .mmap = sock_no_mmap,
+ .bind = sock_no_bind,
+ .accept = sock_no_accept,
+ .sendpage = sock_no_sendpage,
+
+ .release = af_alg_release,
+ .recvmsg = rng_test_recvmsg,
+ .sendmsg = rng_test_sendmsg,
+};
+
static void *rng_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_rng(name, type, mask);
+ struct rng_parent_ctx *pctx;
+ struct crypto_rng *rng;
+
+ pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+ if (!pctx)
+ return ERR_PTR(-ENOMEM);
+
+ rng = crypto_alloc_rng(name, type, mask);
+ if (IS_ERR(rng)) {
+ kfree(pctx);
+ return ERR_CAST(rng);
+ }
+
+ pctx->drng = rng;
+ return pctx;
}

static void rng_release(void *private)
{
- crypto_free_rng(private);
+ struct rng_parent_ctx *pctx = private;
+
+ if (unlikely(!pctx))
+ return;
+ crypto_free_rng(pctx->drng);
+ kfree_sensitive(pctx->entropy);
+ kfree_sensitive(pctx);
}

static void rng_sock_destruct(struct sock *sk)
@@ -126,6 +234,7 @@ static void rng_sock_destruct(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;

+ rng_reset_addtl(ctx);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -133,6 +242,7 @@ static void rng_sock_destruct(struct sock *sk)
static int rng_accept_parent(void *private, struct sock *sk)
{
struct rng_ctx *ctx;
+ struct rng_parent_ctx *pctx = private;
struct alg_sock *ask = alg_sk(sk);
unsigned int len = sizeof(*ctx);

@@ -141,6 +251,8 @@ static int rng_accept_parent(void *private, struct sock *sk)
return -ENOMEM;

ctx->len = len;
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;

/*
* No seeding done at that point -- if multiple accepts are
@@ -148,20 +260,58 @@ static int rng_accept_parent(void *private, struct sock *sk)
* state of the RNG.
*/

- ctx->drng = private;
+ ctx->drng = pctx->drng;
ask->private = ctx;
sk->sk_destruct = rng_sock_destruct;

+ /*
+ * Non NULL pctx->entropy means that CAVP test has been initiated on
+ * this socket, replace proto_ops algif_rng_ops with algif_rng_test_ops.
+ */
+ if (pctx->entropy)
+ sk->sk_socket->ops = &algif_rng_test_ops;
+
return 0;
}

static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
{
+ struct rng_parent_ctx *pctx = private;
/*
* Check whether seedlen is of sufficient size is done in RNG
* implementations.
*/
- return crypto_rng_reset(private, seed, seedlen);
+ return crypto_rng_reset(pctx->drng, seed, seedlen);
+}
+
+static int __maybe_unused rng_setentropy(void *private, sockptr_t entropy,
+ unsigned int len)
+{
+ struct rng_parent_ctx *pctx = private;
+ u8 *kentropy = NULL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ if (pctx->entropy)
+ return -EINVAL;
+
+ if (len > MAXSIZE)
+ return -EMSGSIZE;
+
+ if (len) {
+ kentropy = memdup_sockptr(entropy, len);
+ if (IS_ERR(kentropy))
+ return PTR_ERR(kentropy);
+ }
+
+ crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
+ /*
+ * Since rng doesn't perform any memory management for the entropy
+ * buffer, save kentropy pointer to pctx now to free it after use.
+ */
+ pctx->entropy = kentropy;
+ return 0;
}

static const struct af_alg_type algif_type_rng = {
@@ -169,6 +319,9 @@ static const struct af_alg_type algif_type_rng = {
.release = rng_release,
.accept = rng_accept_parent,
.setkey = rng_setkey,
+#ifdef CONFIG_CRYPTO_USER_API_RNG_CAVP
+ .setentropy = rng_setentropy,
+#endif
.ops = &algif_rng_ops,
.name = "rng",
.owner = THIS_MODULE
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index ee6412314f8f..a5db86670bdf 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -46,6 +46,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setentropy)(void *private, sockptr_t entropy, unsigned int len);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..60b7c2efd921 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_DRBG_ENTROPY 6

/* Operations */
#define ALG_OP_DECRYPT 0
--
2.28.0.526.ge36021eeef-goog

2020-09-09 21:00:46

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH v7] crypto: af_alg - add extra parameters for DRBG interface

On Wed, Sep 09, 2020 at 07:29:47PM +0100, Elena Petrova wrote:
> Extend the user-space RNG interface:
> 1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
> 2. Add additional data input via sendmsg syscall.
>
> This allows DRBG to be tested with test vectors, for example for the
> purpose of CAVP testing, which otherwise isn't possible.
>
> To prevent erroneous use of entropy input, it is hidden under
> CRYPTO_USER_API_RNG_CAVP config option and requires CAP_SYS_ADMIN to
> succeed.
>
> Signed-off-by: Elena Petrova <[email protected]>
> Acked-by: Stephan M?ller <[email protected]>

Reviewed-by: Eric Biggers <[email protected]>

2020-09-16 16:30:57

by Elena Petrova

[permalink] [raw]
Subject: [PATCH v8] crypto: af_alg - add extra parameters for DRBG interface

Extend the user-space RNG interface:
1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
2. Add additional data input via sendmsg syscall.

This allows DRBG to be tested with test vectors, for example for the
purpose of CAVP testing, which otherwise isn't possible.

To prevent erroneous use of entropy input, it is hidden under
CRYPTO_USER_API_RNG_CAVP config option and requires CAP_SYS_ADMIN to
succeed.

Signed-off-by: Elena Petrova <[email protected]>
Acked-by: Stephan Müller <[email protected]>
Reviewed-by: Eric Biggers <[email protected]>
---

Herbert, can you please include this for 5.10?

Updates in v8:
Added Reviewed-by tag to the description.

Updates in v7:
1) rebased onto the latest at cryptodev-2.6.git, fixed compiler errors;
2) replaced kzfree with kfree_sensitive;
3) changed rng_test_sendmsg to return an error if len > MAXSIZE;
4) updated documentation to say when can Additional Data be provided.

Updates in v6:
1) Kconfig option renamed to CRYPTO_USER_API_RNG_CAVP and is now bool instead
of tristate;
2) run-time switch of proto_ops depending on whether the entropy was set;
3) corrected the NIST standard name;
4) rebased onto the tip of the tree;
5) documentation clarified;

Updates in v5:
1) use __maybe_unused instead of #ifdef;
2) separate code path for a testing mode;
3) only allow Additional Data input in a testing mode.

Updates in v4:
1) setentropy returns 0 or error code (used to return length);
2) bigfixes suggested by Eric.

Updates in v3:
1) More details in commit message;
2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
3) fixed a bug of not releasing socket locks.

Updates in v2:
1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
2) Requiring CAP_SYS_ADMIN for entropy reset.
3) Locking for send and recv.
4) Length checks added for send and setentropy; send and setentropy now return
number of bytes accepted.
5) Minor code style corrections.

libkcapi patch for testing:
https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531

Documentation/crypto/userspace-if.rst | 20 ++-
crypto/Kconfig | 9 ++
crypto/af_alg.c | 14 ++-
crypto/algif_rng.c | 175 ++++++++++++++++++++++++--
include/crypto/if_alg.h | 1 +
include/uapi/linux/if_alg.h | 1 +
6 files changed, 205 insertions(+), 15 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index 52019e905900..b45dabbf69d6 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -296,15 +296,16 @@ follows:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
- .salg_type = "rng", /* this selects the symmetric cipher */
- .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
+ .salg_type = "rng", /* this selects the random number generator */
+ .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
};


Depending on the RNG type, the RNG must be seeded. The seed is provided
using the setsockopt interface to set the key. For example, the
ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
-seeded.
+seeded. The seed is also known as a *Personalization String* in NIST SP 800-90A
+standard.

Using the read()/recvmsg() system calls, random numbers can be obtained.
The kernel generates at most 128 bytes in one call. If user space
@@ -314,6 +315,16 @@ WARNING: The user space caller may invoke the initially mentioned accept
system call multiple times. In this case, the returned file descriptors
have the same state.

+Following CAVP testing interfaces are enabled when kernel is built with
+CRYPTO_USER_API_RNG_CAVP option:
+
+- the concatenation of *Entropy* and *Nonce* can be provided to the RNG via
+ ALG_SET_DRBG_ENTROPY setsockopt interface. Setting the entropy requires
+ CAP_SYS_ADMIN permission.
+
+- *Additional Data* can be provided using the send()/sendmsg() system calls,
+ but only after the entropy has been set.
+
Zero-Copy Interface
-------------------

@@ -377,6 +388,9 @@ mentioned optname:
provided ciphertext is assumed to contain an authentication tag of
the given size (see section about AEAD memory layout below).

+- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
+ This option is applicable to RNG cipher type only.
+
User space API example
----------------------

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 1b57419fa2e7..070a88ec1ba8 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1870,6 +1870,15 @@ config CRYPTO_USER_API_RNG
This option enables the user-spaces interface for random
number generator algorithms.

+config CRYPTO_USER_API_RNG_CAVP
+ bool "Enable CAVP testing of DRBG"
+ depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
+ help
+ This option enables extra API for CAVP testing via the user-space
+ interface: resetting of DRBG entropy, and providing Additional Data.
+ This should only be enabled for CAVP testing. You should say
+ no unless you know what this is.
+
config CRYPTO_USER_API_AEAD
tristate "User-space interface for AEAD cipher algorithms"
depends on NET
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index a6f581ab200c..8535cb03b484 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -253,6 +253,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+ break;
+ case ALG_SET_DRBG_ENTROPY:
+ if (sock->state == SS_CONNECTED)
+ goto unlock;
+ if (!type->setentropy)
+ goto unlock;
+
+ err = type->setentropy(ask->private, optval, optlen);
}

unlock:
@@ -285,6 +293,11 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
security_sock_graft(sk2, newsock);
security_sk_clone(sk, sk2);

+ /*
+ * newsock->ops assigned here to allow type->accept call to override
+ * them when required.
+ */
+ newsock->ops = type->ops;
err = type->accept(ask->private, sk2);

nokey = err == -ENOKEY;
@@ -303,7 +316,6 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
alg_sk(sk2)->parent = sk;
alg_sk(sk2)->type = type;

- newsock->ops = type->ops;
newsock->state = SS_CONNECTED;

if (nokey)
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 6300e0566dc5..3ca571b10b08 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -38,6 +38,7 @@
* DAMAGE.
*/

+#include <linux/capability.h>
#include <linux/module.h>
#include <crypto/rng.h>
#include <linux/random.h>
@@ -53,15 +54,26 @@ struct rng_ctx {
#define MAXSIZE 128
unsigned int len;
struct crypto_rng *drng;
+ u8 *addtl;
+ size_t addtl_len;
};

-static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
- int flags)
+struct rng_parent_ctx {
+ struct crypto_rng *drng;
+ u8 *entropy;
+};
+
+static void rng_reset_addtl(struct rng_ctx *ctx)
{
- struct sock *sk = sock->sk;
- struct alg_sock *ask = alg_sk(sk);
- struct rng_ctx *ctx = ask->private;
- int err;
+ kfree_sensitive(ctx->addtl);
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
+}
+
+static int _rng_recvmsg(struct crypto_rng *drng, struct msghdr *msg, size_t len,
+ u8 *addtl, size_t addtl_len)
+{
+ int err = 0;
int genlen = 0;
u8 result[MAXSIZE];

@@ -82,7 +94,7 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
* seeding as they automatically seed. The X9.31 DRNG will return
* an error if it was not seeded properly.
*/
- genlen = crypto_rng_get_bytes(ctx->drng, result, len);
+ genlen = crypto_rng_generate(drng, addtl, addtl_len, result, len);
if (genlen < 0)
return genlen;

@@ -92,6 +104,63 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
return err ? err : len;
}

+static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+
+ return _rng_recvmsg(ctx->drng, msg, len, NULL, 0);
+}
+
+static int rng_test_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+ int ret;
+
+ lock_sock(sock->sk);
+ ret = _rng_recvmsg(ctx->drng, msg, len, ctx->addtl, ctx->addtl_len);
+ rng_reset_addtl(ctx);
+ release_sock(sock->sk);
+
+ return ret;
+}
+
+static int rng_test_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sock->sk);
+ struct rng_ctx *ctx = ask->private;
+
+ lock_sock(sock->sk);
+ if (len > MAXSIZE) {
+ err = -EMSGSIZE;
+ goto unlock;
+ }
+
+ rng_reset_addtl(ctx);
+ ctx->addtl = kmalloc(len, GFP_KERNEL);
+ if (!ctx->addtl) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ err = memcpy_from_msg(ctx->addtl, msg, len);
+ if (err) {
+ rng_reset_addtl(ctx);
+ goto unlock;
+ }
+ ctx->addtl_len = len;
+
+unlock:
+ release_sock(sock->sk);
+ return err ? err : len;
+}
+
static struct proto_ops algif_rng_ops = {
.family = PF_ALG,

@@ -111,14 +180,53 @@ static struct proto_ops algif_rng_ops = {
.recvmsg = rng_recvmsg,
};

+static struct proto_ops __maybe_unused algif_rng_test_ops = {
+ .family = PF_ALG,
+
+ .connect = sock_no_connect,
+ .socketpair = sock_no_socketpair,
+ .getname = sock_no_getname,
+ .ioctl = sock_no_ioctl,
+ .listen = sock_no_listen,
+ .shutdown = sock_no_shutdown,
+ .mmap = sock_no_mmap,
+ .bind = sock_no_bind,
+ .accept = sock_no_accept,
+ .sendpage = sock_no_sendpage,
+
+ .release = af_alg_release,
+ .recvmsg = rng_test_recvmsg,
+ .sendmsg = rng_test_sendmsg,
+};
+
static void *rng_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_rng(name, type, mask);
+ struct rng_parent_ctx *pctx;
+ struct crypto_rng *rng;
+
+ pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+ if (!pctx)
+ return ERR_PTR(-ENOMEM);
+
+ rng = crypto_alloc_rng(name, type, mask);
+ if (IS_ERR(rng)) {
+ kfree(pctx);
+ return ERR_CAST(rng);
+ }
+
+ pctx->drng = rng;
+ return pctx;
}

static void rng_release(void *private)
{
- crypto_free_rng(private);
+ struct rng_parent_ctx *pctx = private;
+
+ if (unlikely(!pctx))
+ return;
+ crypto_free_rng(pctx->drng);
+ kfree_sensitive(pctx->entropy);
+ kfree_sensitive(pctx);
}

static void rng_sock_destruct(struct sock *sk)
@@ -126,6 +234,7 @@ static void rng_sock_destruct(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;

+ rng_reset_addtl(ctx);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -133,6 +242,7 @@ static void rng_sock_destruct(struct sock *sk)
static int rng_accept_parent(void *private, struct sock *sk)
{
struct rng_ctx *ctx;
+ struct rng_parent_ctx *pctx = private;
struct alg_sock *ask = alg_sk(sk);
unsigned int len = sizeof(*ctx);

@@ -141,6 +251,8 @@ static int rng_accept_parent(void *private, struct sock *sk)
return -ENOMEM;

ctx->len = len;
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;

/*
* No seeding done at that point -- if multiple accepts are
@@ -148,20 +260,58 @@ static int rng_accept_parent(void *private, struct sock *sk)
* state of the RNG.
*/

- ctx->drng = private;
+ ctx->drng = pctx->drng;
ask->private = ctx;
sk->sk_destruct = rng_sock_destruct;

+ /*
+ * Non NULL pctx->entropy means that CAVP test has been initiated on
+ * this socket, replace proto_ops algif_rng_ops with algif_rng_test_ops.
+ */
+ if (pctx->entropy)
+ sk->sk_socket->ops = &algif_rng_test_ops;
+
return 0;
}

static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
{
+ struct rng_parent_ctx *pctx = private;
/*
* Check whether seedlen is of sufficient size is done in RNG
* implementations.
*/
- return crypto_rng_reset(private, seed, seedlen);
+ return crypto_rng_reset(pctx->drng, seed, seedlen);
+}
+
+static int __maybe_unused rng_setentropy(void *private, sockptr_t entropy,
+ unsigned int len)
+{
+ struct rng_parent_ctx *pctx = private;
+ u8 *kentropy = NULL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ if (pctx->entropy)
+ return -EINVAL;
+
+ if (len > MAXSIZE)
+ return -EMSGSIZE;
+
+ if (len) {
+ kentropy = memdup_sockptr(entropy, len);
+ if (IS_ERR(kentropy))
+ return PTR_ERR(kentropy);
+ }
+
+ crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
+ /*
+ * Since rng doesn't perform any memory management for the entropy
+ * buffer, save kentropy pointer to pctx now to free it after use.
+ */
+ pctx->entropy = kentropy;
+ return 0;
}

static const struct af_alg_type algif_type_rng = {
@@ -169,6 +319,9 @@ static const struct af_alg_type algif_type_rng = {
.release = rng_release,
.accept = rng_accept_parent,
.setkey = rng_setkey,
+#ifdef CONFIG_CRYPTO_USER_API_RNG_CAVP
+ .setentropy = rng_setentropy,
+#endif
.ops = &algif_rng_ops,
.name = "rng",
.owner = THIS_MODULE
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index ee6412314f8f..a5db86670bdf 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -46,6 +46,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setentropy)(void *private, sockptr_t entropy, unsigned int len);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..60b7c2efd921 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_DRBG_ENTROPY 6

/* Operations */
#define ALG_OP_DECRYPT 0
--
2.28.0.526.ge36021eeef-goog

2020-09-18 06:45:01

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v8] crypto: af_alg - add extra parameters for DRBG interface

On Wed, Sep 16, 2020 at 12:07:31PM +0100, Elena Petrova wrote:
>
> @@ -148,20 +260,58 @@ static int rng_accept_parent(void *private, struct sock *sk)
> * state of the RNG.
> */
>
> - ctx->drng = private;
> + ctx->drng = pctx->drng;
> ask->private = ctx;
> sk->sk_destruct = rng_sock_destruct;
>
> + /*
> + * Non NULL pctx->entropy means that CAVP test has been initiated on
> + * this socket, replace proto_ops algif_rng_ops with algif_rng_test_ops.
> + */
> + if (pctx->entropy)
> + sk->sk_socket->ops = &algif_rng_test_ops;
> +

Please make that

if (IS_ENABLED(CONFIG_CRYPTO_USER_API_RNG_CAVP) && pctx->entropy)

so that this and the rest of the new code simply disappears when
the Kconfig option is off.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-09-18 15:43:18

by Elena Petrova

[permalink] [raw]
Subject: [PATCH v9] crypto: af_alg - add extra parameters for DRBG interface

Extend the user-space RNG interface:
1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
2. Add additional data input via sendmsg syscall.

This allows DRBG to be tested with test vectors, for example for the
purpose of CAVP testing, which otherwise isn't possible.

To prevent erroneous use of entropy input, it is hidden under
CRYPTO_USER_API_RNG_CAVP config option and requires CAP_SYS_ADMIN to
succeed.

Signed-off-by: Elena Petrova <[email protected]>
Acked-by: Stephan Müller <[email protected]>
Reviewed-by: Eric Biggers <[email protected]>
---

Updates in v9:
Add IS_ENABLED(CONFIG_CRYPTO_USER_API_RNG_CAVP) condition for replacing
proto_ops.

Updates in v8:
Added Reviewed-by tag to the description.

Updates in v7:
1) rebased onto the latest at cryptodev-2.6.git, fixed compiler errors;
2) replaced kzfree with kfree_sensitive;
3) changed rng_test_sendmsg to return an error if len > MAXSIZE;
4) updated documentation to say when can Additional Data be provided.

Updates in v6:
1) Kconfig option renamed to CRYPTO_USER_API_RNG_CAVP and is now bool instead
of tristate;
2) run-time switch of proto_ops depending on whether the entropy was set;
3) corrected the NIST standard name;
4) rebased onto the tip of the tree;
5) documentation clarified;

Updates in v5:
1) use __maybe_unused instead of #ifdef;
2) separate code path for a testing mode;
3) only allow Additional Data input in a testing mode.

Updates in v4:
1) setentropy returns 0 or error code (used to return length);
2) bigfixes suggested by Eric.

Updates in v3:
1) More details in commit message;
2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
3) fixed a bug of not releasing socket locks.

Updates in v2:
1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
2) Requiring CAP_SYS_ADMIN for entropy reset.
3) Locking for send and recv.
4) Length checks added for send and setentropy; send and setentropy now return
number of bytes accepted.
5) Minor code style corrections.

libkcapi patch for testing:
https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531

Documentation/crypto/userspace-if.rst | 20 ++-
crypto/Kconfig | 9 ++
crypto/af_alg.c | 14 ++-
crypto/algif_rng.c | 175 ++++++++++++++++++++++++--
include/crypto/if_alg.h | 1 +
include/uapi/linux/if_alg.h | 1 +
6 files changed, 205 insertions(+), 15 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index 52019e905900..b45dabbf69d6 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -296,15 +296,16 @@ follows:

struct sockaddr_alg sa = {
.salg_family = AF_ALG,
- .salg_type = "rng", /* this selects the symmetric cipher */
- .salg_name = "drbg_nopr_sha256" /* this is the cipher name */
+ .salg_type = "rng", /* this selects the random number generator */
+ .salg_name = "drbg_nopr_sha256" /* this is the RNG name */
};


Depending on the RNG type, the RNG must be seeded. The seed is provided
using the setsockopt interface to set the key. For example, the
ansi_cprng requires a seed. The DRBGs do not require a seed, but may be
-seeded.
+seeded. The seed is also known as a *Personalization String* in NIST SP 800-90A
+standard.

Using the read()/recvmsg() system calls, random numbers can be obtained.
The kernel generates at most 128 bytes in one call. If user space
@@ -314,6 +315,16 @@ WARNING: The user space caller may invoke the initially mentioned accept
system call multiple times. In this case, the returned file descriptors
have the same state.

+Following CAVP testing interfaces are enabled when kernel is built with
+CRYPTO_USER_API_RNG_CAVP option:
+
+- the concatenation of *Entropy* and *Nonce* can be provided to the RNG via
+ ALG_SET_DRBG_ENTROPY setsockopt interface. Setting the entropy requires
+ CAP_SYS_ADMIN permission.
+
+- *Additional Data* can be provided using the send()/sendmsg() system calls,
+ but only after the entropy has been set.
+
Zero-Copy Interface
-------------------

@@ -377,6 +388,9 @@ mentioned optname:
provided ciphertext is assumed to contain an authentication tag of
the given size (see section about AEAD memory layout below).

+- ALG_SET_DRBG_ENTROPY -- Setting the entropy of the random number generator.
+ This option is applicable to RNG cipher type only.
+
User space API example
----------------------

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 1b57419fa2e7..070a88ec1ba8 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1870,6 +1870,15 @@ config CRYPTO_USER_API_RNG
This option enables the user-spaces interface for random
number generator algorithms.

+config CRYPTO_USER_API_RNG_CAVP
+ bool "Enable CAVP testing of DRBG"
+ depends on CRYPTO_USER_API_RNG && CRYPTO_DRBG
+ help
+ This option enables extra API for CAVP testing via the user-space
+ interface: resetting of DRBG entropy, and providing Additional Data.
+ This should only be enabled for CAVP testing. You should say
+ no unless you know what this is.
+
config CRYPTO_USER_API_AEAD
tristate "User-space interface for AEAD cipher algorithms"
depends on NET
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index a6f581ab200c..8535cb03b484 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -253,6 +253,14 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+ break;
+ case ALG_SET_DRBG_ENTROPY:
+ if (sock->state == SS_CONNECTED)
+ goto unlock;
+ if (!type->setentropy)
+ goto unlock;
+
+ err = type->setentropy(ask->private, optval, optlen);
}

unlock:
@@ -285,6 +293,11 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
security_sock_graft(sk2, newsock);
security_sk_clone(sk, sk2);

+ /*
+ * newsock->ops assigned here to allow type->accept call to override
+ * them when required.
+ */
+ newsock->ops = type->ops;
err = type->accept(ask->private, sk2);

nokey = err == -ENOKEY;
@@ -303,7 +316,6 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern)
alg_sk(sk2)->parent = sk;
alg_sk(sk2)->type = type;

- newsock->ops = type->ops;
newsock->state = SS_CONNECTED;

if (nokey)
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 6300e0566dc5..407408c43730 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -38,6 +38,7 @@
* DAMAGE.
*/

+#include <linux/capability.h>
#include <linux/module.h>
#include <crypto/rng.h>
#include <linux/random.h>
@@ -53,15 +54,26 @@ struct rng_ctx {
#define MAXSIZE 128
unsigned int len;
struct crypto_rng *drng;
+ u8 *addtl;
+ size_t addtl_len;
};

-static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
- int flags)
+struct rng_parent_ctx {
+ struct crypto_rng *drng;
+ u8 *entropy;
+};
+
+static void rng_reset_addtl(struct rng_ctx *ctx)
{
- struct sock *sk = sock->sk;
- struct alg_sock *ask = alg_sk(sk);
- struct rng_ctx *ctx = ask->private;
- int err;
+ kfree_sensitive(ctx->addtl);
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;
+}
+
+static int _rng_recvmsg(struct crypto_rng *drng, struct msghdr *msg, size_t len,
+ u8 *addtl, size_t addtl_len)
+{
+ int err = 0;
int genlen = 0;
u8 result[MAXSIZE];

@@ -82,7 +94,7 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
* seeding as they automatically seed. The X9.31 DRNG will return
* an error if it was not seeded properly.
*/
- genlen = crypto_rng_get_bytes(ctx->drng, result, len);
+ genlen = crypto_rng_generate(drng, addtl, addtl_len, result, len);
if (genlen < 0)
return genlen;

@@ -92,6 +104,63 @@ static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
return err ? err : len;
}

+static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+
+ return _rng_recvmsg(ctx->drng, msg, len, NULL, 0);
+}
+
+static int rng_test_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+ int flags)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+ int ret;
+
+ lock_sock(sock->sk);
+ ret = _rng_recvmsg(ctx->drng, msg, len, ctx->addtl, ctx->addtl_len);
+ rng_reset_addtl(ctx);
+ release_sock(sock->sk);
+
+ return ret;
+}
+
+static int rng_test_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sock->sk);
+ struct rng_ctx *ctx = ask->private;
+
+ lock_sock(sock->sk);
+ if (len > MAXSIZE) {
+ err = -EMSGSIZE;
+ goto unlock;
+ }
+
+ rng_reset_addtl(ctx);
+ ctx->addtl = kmalloc(len, GFP_KERNEL);
+ if (!ctx->addtl) {
+ err = -ENOMEM;
+ goto unlock;
+ }
+
+ err = memcpy_from_msg(ctx->addtl, msg, len);
+ if (err) {
+ rng_reset_addtl(ctx);
+ goto unlock;
+ }
+ ctx->addtl_len = len;
+
+unlock:
+ release_sock(sock->sk);
+ return err ? err : len;
+}
+
static struct proto_ops algif_rng_ops = {
.family = PF_ALG,

@@ -111,14 +180,53 @@ static struct proto_ops algif_rng_ops = {
.recvmsg = rng_recvmsg,
};

+static struct proto_ops __maybe_unused algif_rng_test_ops = {
+ .family = PF_ALG,
+
+ .connect = sock_no_connect,
+ .socketpair = sock_no_socketpair,
+ .getname = sock_no_getname,
+ .ioctl = sock_no_ioctl,
+ .listen = sock_no_listen,
+ .shutdown = sock_no_shutdown,
+ .mmap = sock_no_mmap,
+ .bind = sock_no_bind,
+ .accept = sock_no_accept,
+ .sendpage = sock_no_sendpage,
+
+ .release = af_alg_release,
+ .recvmsg = rng_test_recvmsg,
+ .sendmsg = rng_test_sendmsg,
+};
+
static void *rng_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_rng(name, type, mask);
+ struct rng_parent_ctx *pctx;
+ struct crypto_rng *rng;
+
+ pctx = kzalloc(sizeof(*pctx), GFP_KERNEL);
+ if (!pctx)
+ return ERR_PTR(-ENOMEM);
+
+ rng = crypto_alloc_rng(name, type, mask);
+ if (IS_ERR(rng)) {
+ kfree(pctx);
+ return ERR_CAST(rng);
+ }
+
+ pctx->drng = rng;
+ return pctx;
}

static void rng_release(void *private)
{
- crypto_free_rng(private);
+ struct rng_parent_ctx *pctx = private;
+
+ if (unlikely(!pctx))
+ return;
+ crypto_free_rng(pctx->drng);
+ kfree_sensitive(pctx->entropy);
+ kfree_sensitive(pctx);
}

static void rng_sock_destruct(struct sock *sk)
@@ -126,6 +234,7 @@ static void rng_sock_destruct(struct sock *sk)
struct alg_sock *ask = alg_sk(sk);
struct rng_ctx *ctx = ask->private;

+ rng_reset_addtl(ctx);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -133,6 +242,7 @@ static void rng_sock_destruct(struct sock *sk)
static int rng_accept_parent(void *private, struct sock *sk)
{
struct rng_ctx *ctx;
+ struct rng_parent_ctx *pctx = private;
struct alg_sock *ask = alg_sk(sk);
unsigned int len = sizeof(*ctx);

@@ -141,6 +251,8 @@ static int rng_accept_parent(void *private, struct sock *sk)
return -ENOMEM;

ctx->len = len;
+ ctx->addtl = NULL;
+ ctx->addtl_len = 0;

/*
* No seeding done at that point -- if multiple accepts are
@@ -148,20 +260,58 @@ static int rng_accept_parent(void *private, struct sock *sk)
* state of the RNG.
*/

- ctx->drng = private;
+ ctx->drng = pctx->drng;
ask->private = ctx;
sk->sk_destruct = rng_sock_destruct;

+ /*
+ * Non NULL pctx->entropy means that CAVP test has been initiated on
+ * this socket, replace proto_ops algif_rng_ops with algif_rng_test_ops.
+ */
+ if (IS_ENABLED(CONFIG_CRYPTO_USER_API_RNG_CAVP) && pctx->entropy)
+ sk->sk_socket->ops = &algif_rng_test_ops;
+
return 0;
}

static int rng_setkey(void *private, const u8 *seed, unsigned int seedlen)
{
+ struct rng_parent_ctx *pctx = private;
/*
* Check whether seedlen is of sufficient size is done in RNG
* implementations.
*/
- return crypto_rng_reset(private, seed, seedlen);
+ return crypto_rng_reset(pctx->drng, seed, seedlen);
+}
+
+static int __maybe_unused rng_setentropy(void *private, sockptr_t entropy,
+ unsigned int len)
+{
+ struct rng_parent_ctx *pctx = private;
+ u8 *kentropy = NULL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ if (pctx->entropy)
+ return -EINVAL;
+
+ if (len > MAXSIZE)
+ return -EMSGSIZE;
+
+ if (len) {
+ kentropy = memdup_sockptr(entropy, len);
+ if (IS_ERR(kentropy))
+ return PTR_ERR(kentropy);
+ }
+
+ crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
+ /*
+ * Since rng doesn't perform any memory management for the entropy
+ * buffer, save kentropy pointer to pctx now to free it after use.
+ */
+ pctx->entropy = kentropy;
+ return 0;
}

static const struct af_alg_type algif_type_rng = {
@@ -169,6 +319,9 @@ static const struct af_alg_type algif_type_rng = {
.release = rng_release,
.accept = rng_accept_parent,
.setkey = rng_setkey,
+#ifdef CONFIG_CRYPTO_USER_API_RNG_CAVP
+ .setentropy = rng_setentropy,
+#endif
.ops = &algif_rng_ops,
.name = "rng",
.owner = THIS_MODULE
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index ee6412314f8f..a5db86670bdf 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -46,6 +46,7 @@ struct af_alg_type {
void *(*bind)(const char *name, u32 type, u32 mask);
void (*release)(void *private);
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
+ int (*setentropy)(void *private, sockptr_t entropy, unsigned int len);
int (*accept)(void *private, struct sock *sk);
int (*accept_nokey)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index bc2bcdec377b..60b7c2efd921 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,7 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_DRBG_ENTROPY 6

/* Operations */
#define ALG_OP_DECRYPT 0
--
2.28.0.681.g6f77f65b4e-goog

2020-09-25 08:18:21

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH v9] crypto: af_alg - add extra parameters for DRBG interface

On Fri, Sep 18, 2020 at 04:42:16PM +0100, Elena Petrova wrote:
> Extend the user-space RNG interface:
> 1. Add entropy input via ALG_SET_DRBG_ENTROPY setsockopt option;
> 2. Add additional data input via sendmsg syscall.
>
> This allows DRBG to be tested with test vectors, for example for the
> purpose of CAVP testing, which otherwise isn't possible.
>
> To prevent erroneous use of entropy input, it is hidden under
> CRYPTO_USER_API_RNG_CAVP config option and requires CAP_SYS_ADMIN to
> succeed.
>
> Signed-off-by: Elena Petrova <[email protected]>
> Acked-by: Stephan M?ller <[email protected]>
> Reviewed-by: Eric Biggers <[email protected]>
> ---
>
> Updates in v9:
> Add IS_ENABLED(CONFIG_CRYPTO_USER_API_RNG_CAVP) condition for replacing
> proto_ops.
>
> Updates in v8:
> Added Reviewed-by tag to the description.
>
> Updates in v7:
> 1) rebased onto the latest at cryptodev-2.6.git, fixed compiler errors;
> 2) replaced kzfree with kfree_sensitive;
> 3) changed rng_test_sendmsg to return an error if len > MAXSIZE;
> 4) updated documentation to say when can Additional Data be provided.
>
> Updates in v6:
> 1) Kconfig option renamed to CRYPTO_USER_API_RNG_CAVP and is now bool instead
> of tristate;
> 2) run-time switch of proto_ops depending on whether the entropy was set;
> 3) corrected the NIST standard name;
> 4) rebased onto the tip of the tree;
> 5) documentation clarified;
>
> Updates in v5:
> 1) use __maybe_unused instead of #ifdef;
> 2) separate code path for a testing mode;
> 3) only allow Additional Data input in a testing mode.
>
> Updates in v4:
> 1) setentropy returns 0 or error code (used to return length);
> 2) bigfixes suggested by Eric.
>
> Updates in v3:
> 1) More details in commit message;
> 2) config option name is now CRYPTO_USER_API_CAVP_DRBG;
> 3) fixed a bug of not releasing socket locks.
>
> Updates in v2:
> 1) Adding CONFIG_CRYPTO_CAVS_DRBG around setentropy.
> 2) Requiring CAP_SYS_ADMIN for entropy reset.
> 3) Locking for send and recv.
> 4) Length checks added for send and setentropy; send and setentropy now return
> number of bytes accepted.
> 5) Minor code style corrections.
>
> libkcapi patch for testing:
> https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa592cb531
>
> Documentation/crypto/userspace-if.rst | 20 ++-
> crypto/Kconfig | 9 ++
> crypto/af_alg.c | 14 ++-
> crypto/algif_rng.c | 175 ++++++++++++++++++++++++--
> include/crypto/if_alg.h | 1 +
> include/uapi/linux/if_alg.h | 1 +
> 6 files changed, 205 insertions(+), 15 deletions(-)

Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt