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]>

crypto/algif_rng.c +5 -0

index 087c0ad09d38..c3d1667db367 100644
@@ -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;

crypto/algif_rng.c +15 -1

+
+	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)

Just some bike-shedding:

On Mon, Jul 13, 2020 at 05:48:57PM +0100, Elena Petrova wrote:

Quoted 5 lines

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.

Quoted 12 lines

crypto/algif_rng.c +2 -0

index 087c0ad09d38..c3d1667db367 100644
@@ -53,8 +53,24 @@ struct rng_ctx {
 #define MAXSIZE 128
      unsigned int len;
      struct crypto_rng *drng;
+     u8 *addtl;
+     size_t addtl_len;
 };
 

crypto/algif_rng.c struct rng_ctx { · line 72

Quoted 13 lines

crypto/algif_rng.c +13 -0

+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.

crypto/algif_rng.c static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, · line 122

Quoted 10 lines

crypto/algif_rng.c +10 -0

+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.

Quoted 19 lines

crypto/algif_rng.c +15 -1

+
+     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)

crypto/algif_rng.c

Quoted 10 lines

crypto/algif_rng.c +9 -0

+             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;
}

crypto/algif_rng.c static struct proto_ops algif_rng_ops = { · line 177

Quoted 6 lines

static void rng_release(void *private)
{

crypto/algif_rng.c +3 -1

-     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/algif_rng.c static struct proto_ops algif_rng_ops = { · line 180

Quoted 3 lines

crypto/algif_rng.c +3 -0

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

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

Quoted 4 lines

crypto/algif_rng.c +4 -0

+static int rng_setentropy(void *private, const u8 *entropy, unsigned int len)
+{
+     struct rng_parent_ctx *pctx = private;
+     u8 *kentropy = NULL;

crypto/algif_rng.c static int rng_accept_parent(void *private, struct sock *sk) · line 240

Quoted 5 lines

crypto/algif_rng.c +5 -0

+
+     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.

crypto/algif_rng.c static int rng_accept_parent(void *private, struct sock *sk) · line 247

Quoted 7 lines

crypto/algif_rng.c +7 -0

+             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/algif_rng.c static int rng_accept_parent(void *private, struct sock *sk) · line 251

Quoted 4 lines

crypto/algif_rng.c +4 -0

+     }
+
+     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.

Quoted 10 lines

crypto/algif_rng.c +10 -0

+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;

crypto/algif_rng.c static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, · line 132

Quoted 10 lines

crypto/algif_rng.c +10 -0

+
+     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

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

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(-)

(quoted diff) +0 -0

b/Documentation/crypto/userspace-if.rst index ff86befa61e0..c3695d2c7e0b 100644

Documentation/crypto/userspace-if.rst +7 -3

@@ -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.

Documentation/crypto/userspace-if.rst +3 -0

+
+*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.

Documentation/crypto/userspace-if.rst +1 -0

+
 User space API example
 ----------------------

crypto/Kconfig +0 -0

index 091c0a0bbf26..8484617596d1 100644

crypto/Kconfig +2 -0

+	  no unless you know what this is.
+
 source "lib/crypto/Kconfig"
 source "drivers/crypto/Kconfig"
 source "crypto/asymmetric_keys/Kconfig"

crypto/af_alg.c +8 -0

index b1cd3535c525..27d6248ca447 100644
@@ -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:

crypto/algif_rng.c +16 -0

index 087c0ad09d38..8a3b0eb45a85 100644
@@ -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.

crypto/algif_rng.c +74 -6

 	 */
-	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)

crypto/algif_rng.c +5 -0

+	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

include/crypto/if_alg.h +2 -1

index 56527c85d122..312fdb3469cf 100644
@@ -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.

include/uapi/linux/if_alg.h +0 -0

index bc2bcdec377b..60b7c2efd921 100644

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

Hi Elena,

sorry for the delay in answering.

Quoted 11 lines

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

Quoted 9 lines

crypto/Kconfig +0 -0

index 091c0a0bbf26..8484617596d1 100644
@@ -1896,6 +1896,14 @@ config CRYPTO_STATS
 config CRYPTO_HASH_INFO
      bool
 

crypto/Kconfig config CRYPTO_STATS · line 1899

Quoted 1 line

crypto/Kconfig +1 -0

+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.

crypto/Kconfig config CRYPTO_STATS · line 1900

Quoted 1 line

crypto/Kconfig +1 -0

+     tristate "Enable CAVS testing of DRBG"

Dto: replace CAVS

Quoted 18 lines

crypto/Kconfig +17 -0

+#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))

crypto/Kconfig

Quoted 5 lines

crypto/Kconfig +5 -0

+                     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?

Quoted 12 lines

include/crypto/if_alg.h +1 -0

index 56527c85d122..312fdb3469cf 100644
@@ -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);

include/crypto/if_alg.h struct af_alg_async_req { · line 127

Quoted 6 lines

include/crypto/if_alg.h +1 -1

@@ -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.

Ciao
Stephan

Quoted 19 lines

Quoted 19 lines

(quoted diff) +2 -0

+#ifdef CONFIG_CRYPTO_CAVS_DRBG
+static int rng_setentropy(void *private, const u8 *entropy, unsigned

int
len) +{

crypto/algif_rng.c +15 -0

+     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))

Quoted 8 lines

crypto/algif_rng.c static int rng_accept_parent(void *private, struct sock *sk) · line 261

Quoted 5 lines

crypto/algif_rng.c +5 -0

+                     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.

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

Hi Elena,

Quoted 28 lines

Quoted 19 lines

Quoted 19 lines

(quoted diff) +2 -0

+#ifdef CONFIG_CRYPTO_CAVS_DRBG
+static int rng_setentropy(void *private, const u8 *entropy, unsigned

int
len) +{

crypto/algif_rng.c +15 -0

+     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))

Quoted 8 lines

crypto/algif_rng.c static int rng_accept_parent(void *private, struct sock *sk) · line 261

Quoted 5 lines

crypto/algif_rng.c +5 -0

+                     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?

Quoted 7 lines

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

include/crypto/if_alg.h +1 -0

index 56527c85d122..312fdb3469cf 100644
@@ -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);

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]>

crypto/algif_rng.c +6 -0

 	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);

crypto/algif_rng.c +5 -0

+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;

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]>

crypto/algif_rng.c static int rng_accept_parent(void *private, struct sock *sk) · line 246

Quoted 2 lines

crypto/algif_rng.c +2 -0

+#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.

Quoted 4 lines · Eric Biggers

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.

On Fri, 31 Jul 2020 at 08:27, Herbert Xu <[email protected]>

Quoted 6 lines

Quoted 4 lines · Eric Biggers

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.

Quoted 2 lines

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.

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

Hi Elena,

Quoted 1 line

On Fri, 31 Jul 2020 at 08:27, Herbert Xu <[email protected]>

Quoted 6 lines · wrote:

Quoted 6 lines

Quoted 4 lines · Eric Biggers

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.

Quoted 6 lines

Quoted 2 lines

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

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.

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.

crypto/algif_rng.c +10 -0

+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;

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.

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.

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(-)

Documentation/crypto/userspace-if.rst +14 -3

index ff86befa61e0..ef7132802c2d 100644
@@ -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
 ----------------------
 

crypto/Kconfig +8 -0

index 091c0a0bbf26..aa2b3085a431 100644
@@ -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"

crypto/af_alg.c +8 -0

index b1cd3535c525..27d6248ca447 100644
@@ -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:

crypto/algif_rng.c +120 -10

index 087c0ad09d38..21e8201844bf 100644
@@ -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

include/crypto/if_alg.h +1 -0

index 56527c85d122..9e5c8ac53c59 100644
@@ -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);

include/uapi/linux/if_alg.h +1 -1

index bc2bcdec377b..60b7c2efd921 100644
@@ -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

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(-)

Documentation/crypto/userspace-if.rst +1 -2

index ff86befa61e0..ef7132802c2d 100644
@@ -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 */

crypto/Kconfig +0 -0

index 091c0a0bbf26..7c8736f71681 100644
@@ -1896,6 +1896,15 @@ config CRYPTO_STATS
 config CRYPTO_HASH_INFO

crypto/Kconfig +4 -0

+static int rng_test_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+			    int flags)
+{
+	struct sock *sk = sock->sk;

crypto/Kconfig +20 -0

+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 = {

crypto/Kconfig +0 -0

@@ -171,7 +314,12 @@ static const struct af_alg_type algif_type_rng = {
 	.release	=	rng_release,
 	.accept		=	rng_accept_parent,

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.

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]>

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]>

crypto/algif_rng.c +1 -1

@@ -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;

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(-)

Quoted 14 lines · Eric Biggers

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
       ...
};

Quoted 5 lines

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.

Documentation/crypto/userspace-if.rst follows: · line 308

Quoted 6 lines

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

Documentation/crypto/userspace-if.rst +2 -1

-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.

Documentation/crypto/userspace-if.rst follows: · line 315

Quoted 6 lines

Documentation/crypto/userspace-if.rst +6 -0

+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.

crypto/Kconfig config CRYPTO_STATS · line 1906

Quoted 8 lines

crypto/Kconfig +8 -0

+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?

Quoted 4 lines

crypto/algif_rng.c +4 -0

+static int rng_test_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
+                         int flags)
+{
+     struct sock *sk = sock->sk;

crypto/algif_rng.c static int rng_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, · line 130

Quoted 10 lines

crypto/algif_rng.c +10 -0

+     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.

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.

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

Hi Elena,

Quoted 9 lines

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

Hi Stephan,

Quoted 21 lines · Stephan Mueller

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

Hi Elena,

Quoted 9 lines

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.

Quoted 6 lines

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.

Quoted 10 lines

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)

Quoted 8 lines

- 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.

Quoted 5 lines

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.

Quoted 2 lines

Ciao
Stephan

Thanks,
Elena

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

Hi Elena,

Quoted 50 lines

Hi Stephan,

Quoted 21 lines · Stephan Mueller

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

Hi Elena,

Quoted 9 lines

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.

Quoted 6 lines

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...

Quoted 4 lines

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.

Quoted 19 lines

Quoted 10 lines

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)

Quoted 8 lines

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

- RSA

- DH

- ECDH

Quoted 7 lines

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 :-) ).

Quoted 7 lines

Quoted 4 lines

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