* [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface @ 2020-07-13 16:48 Elena Petrova 2020-07-13 16:48 ` [PATCH 1/1] " Elena Petrova 2020-07-14 5:17 ` [PATCH 0/1] " Stephan Mueller 0 siblings, 2 replies; 44+ messages in thread From: Elena Petrova @ 2020-07-13 16:48 UTC (permalink / raw) To: linux-crypto; +Cc: Elena Petrova, Eric Biggers, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-13 16:48 [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface Elena Petrova @ 2020-07-13 16:48 ` Elena Petrova 2020-07-13 17:10 ` Eric Biggers 2020-07-13 17:25 ` [PATCH 1/1] " Eric Biggers 2020-07-14 5:17 ` [PATCH 0/1] " Stephan Mueller 1 sibling, 2 replies; 44+ messages in thread From: Elena Petrova @ 2020-07-13 16:48 UTC (permalink / raw) To: linux-crypto; +Cc: Elena Petrova, Eric Biggers, Ard Biesheuvel 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 <lenaptr@google.com> --- 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-13 16:48 ` [PATCH 1/1] " Elena Petrova @ 2020-07-13 17:10 ` Eric Biggers 2020-07-16 14:23 ` Elena Petrova 2020-07-13 17:25 ` [PATCH 1/1] " Eric Biggers 1 sibling, 1 reply; 44+ messages in thread From: Eric Biggers @ 2020-07-13 17:10 UTC (permalink / raw) To: Elena Petrova; +Cc: linux-crypto, Ard Biesheuvel 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 <lenaptr@google.com> 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-13 17:10 ` Eric Biggers @ 2020-07-16 14:23 ` Elena Petrova 2020-07-16 16:40 ` [PATCH v2] " Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-07-16 14:23 UTC (permalink / raw) To: Eric Biggers Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Ard Biesheuvel Thank you for the review, Eric, I'll address your comments in v2. On Mon, 13 Jul 2020 at 18:10, Eric Biggers <ebiggers@kernel.org> 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 <lenaptr@google.com> > > 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface 2020-07-16 14:23 ` Elena Petrova @ 2020-07-16 16:40 ` Elena Petrova 2020-07-20 17:35 ` Stephan Mueller ` (2 more replies) 0 siblings, 3 replies; 44+ messages in thread From: Elena Petrova @ 2020-07-16 16:40 UTC (permalink / raw) To: linux-crypto Cc: Elena Petrova, Eric Biggers, Stephan Müller, Ard Biesheuvel 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 <lenaptr@google.com> --- 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface 2020-07-16 16:40 ` [PATCH v2] " Elena Petrova @ 2020-07-20 17:35 ` Stephan Mueller 2020-07-21 12:55 ` Elena Petrova 2020-07-20 17:42 ` Stephan Müller 2020-07-22 15:59 ` Eric Biggers 2 siblings, 1 reply; 44+ messages in thread From: Stephan Mueller @ 2020-07-20 17:35 UTC (permalink / raw) To: linux-crypto, Elena Petrova; +Cc: Elena Petrova, Eric Biggers, Ard Biesheuvel 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 <lenaptr@google.com> > --- > > 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface 2020-07-20 17:35 ` Stephan Mueller @ 2020-07-21 12:55 ` Elena Petrova 2020-07-21 13:18 ` Stephan Mueller 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-07-21 12:55 UTC (permalink / raw) To: Stephan Mueller Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Ard Biesheuvel, Jeffrey Vander Stoep Hi Stephan, On Mon, 20 Jul 2020 at 18:35, Stephan Mueller <smueller@chronox.de> 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 <lenaptr@google.com> > > --- > > > > 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface 2020-07-21 12:55 ` Elena Petrova @ 2020-07-21 13:18 ` Stephan Mueller 2020-07-28 16:16 ` Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Stephan Mueller @ 2020-07-21 13:18 UTC (permalink / raw) To: Elena Petrova Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Ard Biesheuvel, Jeffrey Vander Stoep 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface 2020-07-21 13:18 ` Stephan Mueller @ 2020-07-28 16:16 ` Elena Petrova 0 siblings, 0 replies; 44+ messages in thread From: Elena Petrova @ 2020-07-28 16:16 UTC (permalink / raw) To: Stephan Mueller Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Ard Biesheuvel, Jeffrey Vander Stoep Hi Stephan, On Tue, 21 Jul 2020 at 14:19, Stephan Mueller <smueller@chronox.de> 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/20200728155159.2156480-1-lenaptr@google.com/] explaining why the `kentropy` pointer is saved. > Ciao > Stephan Regards, Elena ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface 2020-07-16 16:40 ` [PATCH v2] " Elena Petrova 2020-07-20 17:35 ` Stephan Mueller @ 2020-07-20 17:42 ` Stephan Müller 2020-07-22 15:59 ` Eric Biggers 2 siblings, 0 replies; 44+ messages in thread From: Stephan Müller @ 2020-07-20 17:42 UTC (permalink / raw) To: linux-crypto, Elena Petrova; +Cc: Elena Petrova, Eric Biggers, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface 2020-07-16 16:40 ` [PATCH v2] " Elena Petrova 2020-07-20 17:35 ` Stephan Mueller 2020-07-20 17:42 ` Stephan Müller @ 2020-07-22 15:59 ` Eric Biggers 2020-07-28 15:51 ` [PATCH v3] " Elena Petrova 2 siblings, 1 reply; 44+ messages in thread From: Eric Biggers @ 2020-07-22 15:59 UTC (permalink / raw) To: Elena Petrova; +Cc: linux-crypto, Stephan Müller, Ard Biesheuvel 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 <lenaptr@google.com> 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3] crypto: af_alg - add extra parameters for DRBG interface 2020-07-22 15:59 ` Eric Biggers @ 2020-07-28 15:51 ` Elena Petrova 2020-07-28 17:36 ` Eric Biggers 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-07-28 15:51 UTC (permalink / raw) To: linux-crypto Cc: Elena Petrova, Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <lenaptr@google.com> --- 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v3] crypto: af_alg - add extra parameters for DRBG interface 2020-07-28 15:51 ` [PATCH v3] " Elena Petrova @ 2020-07-28 17:36 ` Eric Biggers 2020-07-29 15:45 ` [PATCH v4] " Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Eric Biggers @ 2020-07-28 17:36 UTC (permalink / raw) To: Elena Petrova Cc: linux-crypto, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface 2020-07-28 17:36 ` Eric Biggers @ 2020-07-29 15:45 ` Elena Petrova 2020-07-29 19:26 ` Stephan Müller 2020-07-31 7:23 ` Herbert Xu 0 siblings, 2 replies; 44+ messages in thread From: Elena Petrova @ 2020-07-29 15:45 UTC (permalink / raw) To: linux-crypto Cc: Elena Petrova, Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <lenaptr@google.com> --- 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface 2020-07-29 15:45 ` [PATCH v4] " Elena Petrova @ 2020-07-29 19:26 ` Stephan Müller 2020-07-31 7:23 ` Herbert Xu 1 sibling, 0 replies; 44+ messages in thread From: Stephan Müller @ 2020-07-29 19:26 UTC (permalink / raw) To: linux-crypto, Elena Petrova Cc: Elena Petrova, Eric Biggers, Ard Biesheuvel, Jeffrey Vander Stoep 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 <lenaptr@google.com> Acked-by: Stephan Müller <smueller@chronox.de> Ciao Stephan ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface 2020-07-29 15:45 ` [PATCH v4] " Elena Petrova 2020-07-29 19:26 ` Stephan Müller @ 2020-07-31 7:23 ` Herbert Xu 2020-08-03 14:48 ` Elena Petrova 1 sibling, 1 reply; 44+ messages in thread From: Herbert Xu @ 2020-07-31 7:23 UTC (permalink / raw) To: Elena Petrova; +Cc: linux-crypto, lenaptr, ebiggers, smueller, ardb, jeffv Elena Petrova <lenaptr@google.com> 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 <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface 2020-07-31 7:23 ` Herbert Xu @ 2020-08-03 14:48 ` Elena Petrova 2020-08-03 15:10 ` Stephan Mueller 2020-08-04 2:18 ` Herbert Xu 0 siblings, 2 replies; 44+ messages in thread From: Elena Petrova @ 2020-08-03 14:48 UTC (permalink / raw) To: Herbert Xu Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Stephan Mueller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <herbert@gondor.apana.org.au> wrote: > > Eric Biggers <ebiggers@kernel.org> 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface 2020-08-03 14:48 ` Elena Petrova @ 2020-08-03 15:10 ` Stephan Mueller 2020-08-03 15:30 ` Elena Petrova 2020-08-04 2:18 ` Herbert Xu 1 sibling, 1 reply; 44+ messages in thread From: Stephan Mueller @ 2020-08-03 15:10 UTC (permalink / raw) To: Herbert Xu, Elena Petrova Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Ard Biesheuvel, Jeffrey Vander Stoep Am Montag, 3. August 2020, 16:48:02 CEST schrieb Elena Petrova: Hi Elena, > On Fri, 31 Jul 2020 at 08:27, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Eric Biggers <ebiggers@kernel.org> 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface 2020-08-03 15:10 ` Stephan Mueller @ 2020-08-03 15:30 ` Elena Petrova 0 siblings, 0 replies; 44+ messages in thread From: Elena Petrova @ 2020-08-03 15:30 UTC (permalink / raw) To: Stephan Mueller Cc: Herbert Xu, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Ard Biesheuvel, Jeffrey Vander Stoep Hi Stephan, On Mon, 3 Aug 2020 at 16:10, Stephan Mueller <smueller@chronox.de> 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 <herbert@gondor.apana.org.au> > wrote: > > > Eric Biggers <ebiggers@kernel.org> 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 > > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface 2020-08-03 14:48 ` Elena Petrova 2020-08-03 15:10 ` Stephan Mueller @ 2020-08-04 2:18 ` Herbert Xu 1 sibling, 0 replies; 44+ messages in thread From: Herbert Xu @ 2020-08-04 2:18 UTC (permalink / raw) To: Elena Petrova Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Stephan Mueller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-13 16:48 ` [PATCH 1/1] " Elena Petrova 2020-07-13 17:10 ` Eric Biggers @ 2020-07-13 17:25 ` Eric Biggers 2020-07-31 7:26 ` Herbert Xu 1 sibling, 1 reply; 44+ messages in thread From: Eric Biggers @ 2020-07-13 17:25 UTC (permalink / raw) To: Elena Petrova; +Cc: linux-crypto, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-13 17:25 ` [PATCH 1/1] " Eric Biggers @ 2020-07-31 7:26 ` Herbert Xu 2020-08-13 16:00 ` Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Herbert Xu @ 2020-07-31 7:26 UTC (permalink / raw) To: Eric Biggers; +Cc: lenaptr, linux-crypto, ardb Eric Biggers <ebiggers@kernel.org> 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 <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-31 7:26 ` Herbert Xu @ 2020-08-13 16:00 ` Elena Petrova 2020-08-13 16:01 ` [PATCH v4] " Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-08-13 16:00 UTC (permalink / raw) To: Herbert Xu Cc: Eric Biggers, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Ard Biesheuvel Hi Herbert, On Fri, 31 Jul 2020 at 08:27, Herbert Xu <herbert@gondor.apana.org.au> 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface 2020-08-13 16:00 ` Elena Petrova @ 2020-08-13 16:01 ` Elena Petrova 2020-08-13 16:04 ` Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-08-13 16:01 UTC (permalink / raw) To: linux-crypto Cc: Elena Petrova, Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <lenaptr@google.com> --- 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v4] crypto: af_alg - add extra parameters for DRBG interface 2020-08-13 16:01 ` [PATCH v4] " Elena Petrova @ 2020-08-13 16:04 ` Elena Petrova 2020-08-13 16:08 ` [PATCH v5] " Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-08-13 16:04 UTC (permalink / raw) To: open list:HARDWARE RANDOM NUMBER GENERATOR CORE Cc: Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep Err, I sent the old patch file, sorry. Real v5 coming shortly... On Thu, 13 Aug 2020 at 17:02, Elena Petrova <lenaptr@google.com> 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 <lenaptr@google.com> > --- > > 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 > ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v5] crypto: af_alg - add extra parameters for DRBG interface 2020-08-13 16:04 ` Elena Petrova @ 2020-08-13 16:08 ` Elena Petrova 2020-08-13 19:32 ` Eric Biggers 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-08-13 16:08 UTC (permalink / raw) To: linux-crypto Cc: Elena Petrova, Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <lenaptr@google.com> Acked-by: Stephan Müller <smueller@chronox.de> --- 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v5] crypto: af_alg - add extra parameters for DRBG interface 2020-08-13 16:08 ` [PATCH v5] " Elena Petrova @ 2020-08-13 19:32 ` Eric Biggers 2020-08-21 4:24 ` Herbert Xu 2020-09-08 17:18 ` Elena Petrova 0 siblings, 2 replies; 44+ messages in thread From: Eric Biggers @ 2020-08-13 19:32 UTC (permalink / raw) To: Elena Petrova Cc: linux-crypto, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <lenaptr@google.com> > Acked-by: Stephan Müller <smueller@chronox.de> > --- > > 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v5] crypto: af_alg - add extra parameters for DRBG interface 2020-08-13 19:32 ` Eric Biggers @ 2020-08-21 4:24 ` Herbert Xu 2020-09-08 17:04 ` [PATCH v6] " Elena Petrova 2020-09-08 17:23 ` [PATCH v5] " Elena Petrova 2020-09-08 17:18 ` Elena Petrova 1 sibling, 2 replies; 44+ messages in thread From: Herbert Xu @ 2020-08-21 4:24 UTC (permalink / raw) To: Eric Biggers; +Cc: lenaptr, linux-crypto, smueller, ardb, jeffv Eric Biggers <ebiggers@kernel.org> 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 <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v6] crypto: af_alg - add extra parameters for DRBG interface 2020-08-21 4:24 ` Herbert Xu @ 2020-09-08 17:04 ` Elena Petrova 2020-09-09 4:35 ` Eric Biggers 2020-09-08 17:23 ` [PATCH v5] " Elena Petrova 1 sibling, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-09-08 17:04 UTC (permalink / raw) To: linux-crypto Cc: Elena Petrova, Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <lenaptr@google.com> Acked-by: Stephan Müller <smueller@chronox.de> --- 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v6] crypto: af_alg - add extra parameters for DRBG interface 2020-09-08 17:04 ` [PATCH v6] " Elena Petrova @ 2020-09-09 4:35 ` Eric Biggers 2020-09-09 18:29 ` [PATCH v7] " Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Eric Biggers @ 2020-09-09 4:35 UTC (permalink / raw) To: Elena Petrova Cc: linux-crypto, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <lenaptr@google.com> > Acked-by: Stephan Müller <smueller@chronox.de> 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v7] crypto: af_alg - add extra parameters for DRBG interface 2020-09-09 4:35 ` Eric Biggers @ 2020-09-09 18:29 ` Elena Petrova 2020-09-09 21:00 ` Eric Biggers 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-09-09 18:29 UTC (permalink / raw) To: linux-crypto Cc: Elena Petrova, Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <lenaptr@google.com> Acked-by: Stephan Müller <smueller@chronox.de> --- 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v7] crypto: af_alg - add extra parameters for DRBG interface 2020-09-09 18:29 ` [PATCH v7] " Elena Petrova @ 2020-09-09 21:00 ` Eric Biggers 2020-09-16 11:07 ` [PATCH v8] " Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Eric Biggers @ 2020-09-09 21:00 UTC (permalink / raw) To: Elena Petrova Cc: linux-crypto, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep 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 <lenaptr@google.com> > Acked-by: Stephan Müller <smueller@chronox.de> Reviewed-by: Eric Biggers <ebiggers@google.com> ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v8] crypto: af_alg - add extra parameters for DRBG interface 2020-09-09 21:00 ` Eric Biggers @ 2020-09-16 11:07 ` Elena Petrova 2020-09-18 6:43 ` Herbert Xu 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-09-16 11:07 UTC (permalink / raw) To: herbert Cc: Elena Petrova, linux-crypto, Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep, Eric Biggers 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 <lenaptr@google.com> Acked-by: Stephan Müller <smueller@chronox.de> Reviewed-by: Eric Biggers <ebiggers@google.com> --- 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v8] crypto: af_alg - add extra parameters for DRBG interface 2020-09-16 11:07 ` [PATCH v8] " Elena Petrova @ 2020-09-18 6:43 ` Herbert Xu 2020-09-18 15:42 ` [PATCH v9] " Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Herbert Xu @ 2020-09-18 6:43 UTC (permalink / raw) To: Elena Petrova Cc: linux-crypto, Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep, Eric Biggers 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 <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v9] crypto: af_alg - add extra parameters for DRBG interface 2020-09-18 6:43 ` Herbert Xu @ 2020-09-18 15:42 ` Elena Petrova 2020-09-25 8:16 ` Herbert Xu 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-09-18 15:42 UTC (permalink / raw) To: herbert Cc: Elena Petrova, linux-crypto, Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep, Eric Biggers 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 <lenaptr@google.com> Acked-by: Stephan Müller <smueller@chronox.de> Reviewed-by: Eric Biggers <ebiggers@google.com> --- 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 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v9] crypto: af_alg - add extra parameters for DRBG interface 2020-09-18 15:42 ` [PATCH v9] " Elena Petrova @ 2020-09-25 8:16 ` Herbert Xu 0 siblings, 0 replies; 44+ messages in thread From: Herbert Xu @ 2020-09-25 8:16 UTC (permalink / raw) To: Elena Petrova Cc: linux-crypto, Eric Biggers, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep, Eric Biggers 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 <lenaptr@google.com> > Acked-by: Stephan Müller <smueller@chronox.de> > Reviewed-by: Eric Biggers <ebiggers@google.com> > --- > > 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 <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v5] crypto: af_alg - add extra parameters for DRBG interface 2020-08-21 4:24 ` Herbert Xu 2020-09-08 17:04 ` [PATCH v6] " Elena Petrova @ 2020-09-08 17:23 ` Elena Petrova 1 sibling, 0 replies; 44+ messages in thread From: Elena Petrova @ 2020-09-08 17:23 UTC (permalink / raw) To: Herbert Xu Cc: Eric Biggers, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Stephan Mueller, Ard Biesheuvel, Jeffrey Vander Stoep On Fri, 21 Aug 2020 at 05:24, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Eric Biggers <ebiggers@kernel.org> 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v5] crypto: af_alg - add extra parameters for DRBG interface 2020-08-13 19:32 ` Eric Biggers 2020-08-21 4:24 ` Herbert Xu @ 2020-09-08 17:18 ` Elena Petrova 1 sibling, 0 replies; 44+ messages in thread From: Elena Petrova @ 2020-09-08 17:18 UTC (permalink / raw) To: Eric Biggers Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Stephan Müller, Ard Biesheuvel, Jeffrey Vander Stoep On Thu, 13 Aug 2020 at 20:32, Eric Biggers <ebiggers@kernel.org> 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-13 16:48 [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface Elena Petrova 2020-07-13 16:48 ` [PATCH 1/1] " Elena Petrova @ 2020-07-14 5:17 ` Stephan Mueller 2020-07-14 15:23 ` Elena Petrova 1 sibling, 1 reply; 44+ messages in thread From: Stephan Mueller @ 2020-07-14 5:17 UTC (permalink / raw) To: linux-crypto, Elena Petrova; +Cc: Elena Petrova, Eric Biggers, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-14 5:17 ` [PATCH 0/1] " Stephan Mueller @ 2020-07-14 15:23 ` Elena Petrova 2020-07-14 15:34 ` Stephan Mueller 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-07-14 15:23 UTC (permalink / raw) To: Stephan Mueller Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Ard Biesheuvel Hi Stephan, On Tue, 14 Jul 2020 at 06:17, Stephan Mueller <smueller@chronox.de> 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 <jayalakshmi.bhat@hp.com>, 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-14 15:23 ` Elena Petrova @ 2020-07-14 15:34 ` Stephan Mueller 2020-07-16 14:41 ` Elena Petrova 0 siblings, 1 reply; 44+ messages in thread From: Stephan Mueller @ 2020-07-14 15:34 UTC (permalink / raw) To: Elena Petrova Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Ard Biesheuvel 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 <smueller@chronox.de> 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 <jayalakshmi.bhat@hp.com>, 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-14 15:34 ` Stephan Mueller @ 2020-07-16 14:41 ` Elena Petrova 2020-07-16 14:49 ` Stephan Mueller 0 siblings, 1 reply; 44+ messages in thread From: Elena Petrova @ 2020-07-16 14:41 UTC (permalink / raw) To: Stephan Mueller Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Ard Biesheuvel Hi Stephan, On Tue, 14 Jul 2020 at 16:34, Stephan Mueller <smueller@chronox.de> 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 <smueller@chronox.de> 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 <jayalakshmi.bhat@hp.com>, 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 > > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-16 14:41 ` Elena Petrova @ 2020-07-16 14:49 ` Stephan Mueller 2020-07-16 14:59 ` Stephan Mueller 0 siblings, 1 reply; 44+ messages in thread From: Stephan Mueller @ 2020-07-16 14:49 UTC (permalink / raw) To: Elena Petrova Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Ard Biesheuvel, keyring 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface 2020-07-16 14:49 ` Stephan Mueller @ 2020-07-16 14:59 ` Stephan Mueller 0 siblings, 0 replies; 44+ messages in thread From: Stephan Mueller @ 2020-07-16 14:59 UTC (permalink / raw) To: Elena Petrova, herbert Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Eric Biggers, Ard Biesheuvel, keyrings 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2020-09-25 8:16 UTC | newest] Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-13 16:48 [PATCH 0/1] crypto: af_alg - add extra parameters for DRBG interface Elena Petrova 2020-07-13 16:48 ` [PATCH 1/1] " Elena Petrova 2020-07-13 17:10 ` Eric Biggers 2020-07-16 14:23 ` Elena Petrova 2020-07-16 16:40 ` [PATCH v2] " Elena Petrova 2020-07-20 17:35 ` Stephan Mueller 2020-07-21 12:55 ` Elena Petrova 2020-07-21 13:18 ` Stephan Mueller 2020-07-28 16:16 ` Elena Petrova 2020-07-20 17:42 ` Stephan Müller 2020-07-22 15:59 ` Eric Biggers 2020-07-28 15:51 ` [PATCH v3] " Elena Petrova 2020-07-28 17:36 ` Eric Biggers 2020-07-29 15:45 ` [PATCH v4] " Elena Petrova 2020-07-29 19:26 ` Stephan Müller 2020-07-31 7:23 ` Herbert Xu 2020-08-03 14:48 ` Elena Petrova 2020-08-03 15:10 ` Stephan Mueller 2020-08-03 15:30 ` Elena Petrova 2020-08-04 2:18 ` Herbert Xu 2020-07-13 17:25 ` [PATCH 1/1] " Eric Biggers 2020-07-31 7:26 ` Herbert Xu 2020-08-13 16:00 ` Elena Petrova 2020-08-13 16:01 ` [PATCH v4] " Elena Petrova 2020-08-13 16:04 ` Elena Petrova 2020-08-13 16:08 ` [PATCH v5] " Elena Petrova 2020-08-13 19:32 ` Eric Biggers 2020-08-21 4:24 ` Herbert Xu 2020-09-08 17:04 ` [PATCH v6] " Elena Petrova 2020-09-09 4:35 ` Eric Biggers 2020-09-09 18:29 ` [PATCH v7] " Elena Petrova 2020-09-09 21:00 ` Eric Biggers 2020-09-16 11:07 ` [PATCH v8] " Elena Petrova 2020-09-18 6:43 ` Herbert Xu 2020-09-18 15:42 ` [PATCH v9] " Elena Petrova 2020-09-25 8:16 ` Herbert Xu 2020-09-08 17:23 ` [PATCH v5] " Elena Petrova 2020-09-08 17:18 ` Elena Petrova 2020-07-14 5:17 ` [PATCH 0/1] " Stephan Mueller 2020-07-14 15:23 ` Elena Petrova 2020-07-14 15:34 ` Stephan Mueller 2020-07-16 14:41 ` Elena Petrova 2020-07-16 14:49 ` Stephan Mueller 2020-07-16 14:59 ` Stephan Mueller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).