Hi,
This patch set adds AEAD and RNG support to the AF_ALG interface
exported by the kernel crypto API. By extending AF_ALG with AEAD and RNG
support, all cipher types the kernel crypto API allows access to are
now accessible from userspace.
The RNG support is stand-alone.
The AEAD implementation is added to algif_skcipher.c to prevent
re-implementation of the memory moving logic.
The extension for the AEAD support can be summarized with the following
types of changes:
* select the correct crypto API functions (either the ablkcipher
or the aead functions)
* apply the additional data needed for AEAD at the right time
(associated data, authentication tag) -- this includes the addition
of user space interfaces to allow setting this data.
* add the calculation for the memory size needed for encryption and
decryption.
In addition, the patch set adds a getsockopt implementation to skcipher to
allow user space to inquire about properties of the ciphers (IV size,
block size, authentication data size). This extension would be needed for a
generic user space usage of these ciphers.
The new AEAD and RNG interfaces are fully tested with the test application
provided at [1]. That test application exercises all newly added user space
interfaces.
The patch set was tested on x86_64 and i386.
[1] http://www.chronox.de/libkcapi.html
Stephan Mueller (8):
crypto: AF_ALG: add user space interface for AEAD
crypto: AF_ALG: user space interface for cipher info
crypto: AF_ALG: extend data structuers for AEAD
crypto: AF_ALG: crypto API calls to inline functions
crypto: AF_ALG: add AEAD support
crypto: AF_ALG: make setkey optional
crypto: AF_ALG: add random number generator support
crypto: AF_ALG: enable RNG interface compilation
crypto/Kconfig | 9 ++
crypto/Makefile | 1 +
crypto/af_alg.c | 20 +++
crypto/algif_rng.c | 186 +++++++++++++++++++++++
crypto/algif_skcipher.c | 350 ++++++++++++++++++++++++++++++++++++++++----
include/crypto/if_alg.h | 2 +
include/uapi/linux/if_alg.h | 10 ++
7 files changed, 550 insertions(+), 28 deletions(-)
create mode 100644 crypto/algif_rng.c
--
2.1.0
Enable compilation of the RNG AF_ALG support and provide a Kconfig
option to compile the RNG AF_ALG support.
Signed-off-by: Stephan Mueller <[email protected]>
---
crypto/Kconfig | 9 +++++++++
crypto/Makefile | 1 +
2 files changed, 10 insertions(+)
diff --git a/crypto/Kconfig b/crypto/Kconfig
index 87bbc9c..e127323 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1505,6 +1505,15 @@ config CRYPTO_USER_API_SKCIPHER
This option enables the user-spaces interface for symmetric
key cipher algorithms.
+config CRYPTO_USER_API_RNG
+ tristate "User-space interface for random number generator algorithms"
+ depends on NET
+ select CRYPTO_RNG
+ select CRYPTO_USER_API
+ help
+ This option enables the user-spaces interface for random
+ number generator algorithms.
+
config CRYPTO_HASH_INFO
bool
diff --git a/crypto/Makefile b/crypto/Makefile
index 1445b91..ba19465 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -99,6 +99,7 @@ obj-$(CONFIG_CRYPTO_GHASH) += ghash-generic.o
obj-$(CONFIG_CRYPTO_USER_API) += af_alg.o
obj-$(CONFIG_CRYPTO_USER_API_HASH) += algif_hash.o
obj-$(CONFIG_CRYPTO_USER_API_SKCIPHER) += algif_skcipher.o
+obj-$(CONFIG_CRYPTO_USER_API_RNG) += algif_rng.o
#
# generic algorithms and the async_tx api
--
2.1.0
The current AF_ALG implementation requires that a userspace interface
implementation must provide a callback for setkey. Such a call is not
appliable to random number generators.
To prepare AF_ALG for the addition of a random number generator user
space interface, this function callback invocation is made optional.
Signed-off-by: Stephan Mueller <[email protected]>
---
crypto/af_alg.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 635140b..47a199c 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -177,6 +177,9 @@ static int alg_setkey(struct sock *sk, char __user *ukey,
u8 *key;
int err;
+ if (!type->setkey)
+ return -EOPNOTSUPP;
+
key = sock_kmalloc(sk, keylen, GFP_KERNEL);
if (!key)
return -ENOMEM;
--
2.1.0
This patch adds the AEAD support for AF_ALG.
The AEAD implementation uses the entire memory handling and
infrastructure of the existing skcipher implementation.
To use AEAD, the user space consumer has to use the salg_type named
"aead". The AEAD extension only uses the bind callback as the key
differentiator. The previously added functions that select whether to
use AEAD or ablkcipher crypto API functions depend on the TFM type
allocated during the bind() call.
The addition of AEAD brings a bit of overhead to calculate the size of
the ciphertext, because the AEAD implementation of the kernel crypto API
makes implied assumption on the location of the authentication tag. When
performing an encryption, the tag will be added to the created
ciphertext (note, the tag is placed adjacent to the ciphertext). For
decryption, the caller must hand in the ciphertext with the tag appended
to the ciphertext. Therefore, the selection of the used memory
needs to add/subtract the tag size from the source/destination buffers
depending on the encryption type. The code is provided with comments
explainint when and how that operation is performed.
Note: The AF_ALG interface does not support zero length plaintext or
zero length ciphertext. Such zero length input data may be used if one
wants to access the hash implementation of an AEAD directly (e.g. the
GHASH of GCM or CMAC for CCM). However, this is a use case that is not
of interest. GHASH or CMAC is directly available via the hash AF_ALG
interface and we therefore do not need to take precautions for this
use case.
A fully working example using all aspects of AEAD is provided at
http://www.chronox.de/libkcapi.html
Signed-off-by: Stephan Mueller <[email protected]>
---
crypto/algif_skcipher.c | 153 +++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 144 insertions(+), 9 deletions(-)
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index fb8efc8..1e2763d 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -387,6 +387,17 @@ static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
if (con.iv && con.iv->ivlen != ivsize)
return -EINVAL;
+
+ /*
+ * AEAD associated data is limited to a sensible size
+ * Size limit is set to some arbitrary length to avoid
+ * user space eating up memory
+ */
+ if (ctx->aead &&
+ (con.aead_assoc->aead_assoclen > MAX_AEAD_ASSOCLEN ||
+ !con.aead_assoc->aead_assoclen ||
+ !con.aead_assoc || !con.aead_authsize))
+ return -EINVAL;
}
err = -EINVAL;
@@ -399,6 +410,25 @@ static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
ctx->enc = enc;
if (con.iv)
memcpy(ctx->iv, con.iv->iv, ivsize);
+ /* AEAD authentication data handling */
+ if (ctx->aead) {
+ if (con.aead_authsize)
+ err = crypto_aead_setauthsize(
+ crypto_aead_reqtfm(&ctx->u.aead_req),
+ con.aead_authsize);
+ if (err)
+ goto unlock;
+ /* set associated data */
+ memcpy(ctx->aead_assoc,
+ con.aead_assoc->aead_assoc,
+ con.aead_assoc->aead_assoclen);
+ sg_init_one(&ctx->sg_aead_assoc,
+ ctx->aead_assoc,
+ con.aead_assoc->aead_assoclen);
+ aead_request_set_assoc(&ctx->u.aead_req,
+ &ctx->sg_aead_assoc,
+ con.aead_assoc->aead_assoclen);
+ }
}
while (size) {
@@ -547,10 +577,41 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
int err = -EAGAIN;
int used;
long copied = 0;
+ unsigned int aead_authsize_enc = 0;
+ unsigned int aead_authsize_dec = 0;
lock_sock(sk);
+ /*
+ * AEAD memory structure: For encryption, the tag is appended to the
+ * ciphertext which implies that the memory allocated for the ciphertext
+ * must be increased by the tag length. For decryption, the tag
+ * is expected to be concatenated to the ciphertext. The plaintext
+ * therefore has a memory size of the ciphertext minus the tag length.
+ *
+ * Note: this memory calculation only works because we require the
+ * user space caller to:
+ * * perform encryption by invoking the recv function with a buffer
+ * length of ciphertext + tag size -- the send function can be
+ * invoked normally with just the plaintext.
+ * * perform a decryption by invoking the the write function with
+ * a buffer holding the ciphertext + tag (and setting the
+ * buffer size accordingly) -- the recv function can be invoked
+ * normally with just the space needed for the ciphertext.
+ * Though, the caller should check for EBADMSG to catch integiry
+ * violations.
+ */
+ if (ctx->aead) {
+ if (ctx->enc)
+ aead_authsize_enc = crypto_aead_authsize(
+ crypto_aead_reqtfm(&ctx->u.aead_req));
+ else
+ aead_authsize_dec = crypto_aead_authsize(
+ crypto_aead_reqtfm(&ctx->u.aead_req));
+ }
+
for (iov = msg->msg_iov, iovlen = msg->msg_iovlen; iovlen > 0;
iovlen--, iov++) {
+ /* size of the output data memory */
unsigned long seglen = iov->iov_len;
char __user *from = iov->iov_base;
@@ -562,6 +623,7 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
while (!sg->length)
sg++;
+ /* size of the input data memory */
used = ctx->used;
if (!used) {
err = skcipher_wait_for_data(sk, flags);
@@ -569,7 +631,28 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
goto unlock;
}
- used = min_t(unsigned long, used, seglen);
+ used = min_t(unsigned long,
+ /*
+ * In case of encryption, add
+ * the memory needed for the tag
+ * to the input data length to
+ * give the cipher the necessary
+ * space to add the tag.
+ */
+ used + aead_authsize_enc,
+ /*
+ * In case of decryption, add the
+ * memory needed for the tag
+ * calculations to the output
+ * buffer.
+ */
+ seglen + aead_authsize_dec);
+
+ if (used < aead_authsize_enc ||
+ seglen < aead_authsize_dec) {
+ err = -ENOMEM;
+ goto unlock;
+ }
used = af_alg_make_sg(&ctx->rsgl, from, used, 1);
err = used;
@@ -583,9 +666,16 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
if (!used)
goto free;
- ablkcipher_request_set_crypt(&ctx->req, sg,
- ctx->rsgl.sg, used,
- ctx->iv);
+ /*
+ * See API specification of the AEAD API: for
+ * encryption, we need to tell the encrypt function
+ * what the size of the plaintext is. But we have
+ * ensured that we have sufficient memory allocated for
+ * the operation.
+ */
+ skcipher_crypto_set_crypt(ctx, sg, ctx->rsgl.sg,
+ used - aead_authsize_enc,
+ ctx->iv);
err = af_alg_wait_for_completion(
ctx->enc ?
@@ -599,10 +689,19 @@ free:
if (err)
goto unlock;
- copied += used;
- from += used;
- seglen -= used;
- skcipher_pull_sgl(sk, used);
+ /*
+ * Adjust the output buffer counters by only the size
+ * needed for the plaintext in case of a decryption
+ */
+ copied += (used - aead_authsize_dec);
+ from += (used - aead_authsize_dec);
+ seglen -= (used - aead_authsize_dec);
+ /*
+ * Adjust the input buffer by how much we have encrypted
+ * or decrypted. In case of encryption, we only credit
+ * the memory of the plaintext.
+ */
+ skcipher_pull_sgl(sk, used - aead_authsize_enc);
}
}
@@ -724,6 +823,10 @@ static void skcipher_sock_destruct(struct sock *sk)
unsigned int ivlen = skcipher_crypto_ivsize_ctx(ctx);
skcipher_free_sgl(sk);
+ if (ctx->aead) {
+ memset(ctx->aead_assoc, 0, MAX_AEAD_ASSOCLEN);
+ sock_kfree_s(sk, ctx->aead_assoc, MAX_AEAD_ASSOCLEN);
+ }
sock_kfree_s(sk, ctx->iv, ivlen);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
@@ -748,6 +851,17 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
memset(ctx->iv, 0, ivlen);
+ if (skcipher_is_aead(private)) {
+ ctx->aead_assoc = sock_kmalloc(sk, MAX_AEAD_ASSOCLEN,
+ GFP_KERNEL);
+ if (!ctx->aead_assoc) {
+ sock_kfree_s(sk, ctx->iv, ivlen);
+ sock_kfree_s(sk, ctx, len);
+ return -ENOMEM;
+ }
+ memset(ctx->aead_assoc, 0, MAX_AEAD_ASSOCLEN);
+ }
+
INIT_LIST_HEAD(&ctx->tsgl);
ctx->len = len;
ctx->used = 0;
@@ -778,15 +892,36 @@ static const struct af_alg_type algif_type_skcipher = {
.owner = THIS_MODULE
};
+static void *aead_bind(const char *name, u32 type, u32 mask)
+{
+ return crypto_alloc_aead(name, type, mask);
+}
+
+static const struct af_alg_type algif_type_aead = {
+ .bind = aead_bind,
+ .release = skcipher_release,
+ .setkey = skcipher_setkey,
+ .accept = skcipher_accept_parent,
+ .ops = &algif_skcipher_ops,
+ .name = "aead",
+ .owner = THIS_MODULE
+};
+
static int __init algif_skcipher_init(void)
{
- return af_alg_register_type(&algif_type_skcipher);
+ int ret = af_alg_register_type(&algif_type_skcipher);
+
+ if (ret)
+ return ret;
+ return af_alg_register_type(&algif_type_aead);
}
static void __exit algif_skcipher_exit(void)
{
int err = af_alg_unregister_type(&algif_type_skcipher);
BUG_ON(err);
+ err = af_alg_unregister_type(&algif_type_aead);
+ BUG_ON(err);
}
module_init(algif_skcipher_init);
--
2.1.0
The data structure holding the state of an ongoing symmetric cipher
operation is extended by the data variables needed for AEAD.
The request data structures are encapsulated by a union as the symmetric
cipher implementation is either exclusively used for "normal" symmetric
ciphers or for AEAD ciphers.
The define MAX_AEAD_ASSOCLEN restricts the size of the associated
authentication data. The kernel must allocate memory for this data to be
stored for the cipher operation. To prevent an excessive use of memory,
it is limited to 128 bytes, which is considered to be a sensible size.
Signed-off-by: Stephan Mueller <[email protected]>
---
crypto/algif_skcipher.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 7a1656b..9286cfc 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -46,7 +46,15 @@ struct skcipher_ctx {
bool merge;
bool enc;
- struct ablkcipher_request req;
+ bool aead;
+ void *aead_assoc;
+ /* define arbitrary maximum length of associated data */
+ #define MAX_AEAD_ASSOCLEN 128
+ struct scatterlist sg_aead_assoc;
+ union {
+ struct ablkcipher_request ablkcipher_req;
+ struct aead_request aead_req;
+ } u;
};
#define MAX_SGL_ENTS ((4096 - sizeof(struct skcipher_sg_list)) / \
--
2.1.0
To avoid excessive branches and cluttering the code, all kernel crypto
API calls are extracted into separate inline functions. These functions
invoke either the ablkcipher or the aead crypto API function calls, as
necessary.
Signed-off-by: Stephan Mueller <[email protected]>
---
crypto/algif_skcipher.c | 141 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 124 insertions(+), 17 deletions(-)
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 9286cfc..fb8efc8 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -247,14 +247,121 @@ static void skcipher_data_wakeup(struct sock *sk)
rcu_read_unlock();
}
+static inline bool skcipher_is_aead(struct crypto_tfm *tfm)
+{
+ return ((crypto_tfm_alg_type(tfm) & CRYPTO_ALG_TYPE_MASK) ==
+ CRYPTO_ALG_TYPE_AEAD);
+}
+
+static inline unsigned int skcipher_crypto_ivsize(void *private)
+{
+ if (skcipher_is_aead(private))
+ return crypto_aead_ivsize(private);
+ else
+ return crypto_ablkcipher_ivsize(private);
+}
+
+static inline unsigned int skcipher_crypto_ivsize_ctx(struct skcipher_ctx *ctx)
+{
+ if (ctx->aead)
+ return crypto_aead_ivsize(crypto_aead_reqtfm(&ctx->u.aead_req));
+ else
+ return crypto_ablkcipher_ivsize(
+ crypto_ablkcipher_reqtfm(&ctx->u.ablkcipher_req));
+}
+
+static inline unsigned int skcipher_crypto_blocksize(struct skcipher_ctx *ctx)
+{
+ if (ctx->aead)
+ return crypto_aead_blocksize(
+ crypto_aead_reqtfm(&ctx->u.aead_req));
+ else
+ return crypto_ablkcipher_blocksize(
+ crypto_ablkcipher_reqtfm(&ctx->u.ablkcipher_req));
+}
+
+static inline unsigned int skcipher_crypto_reqsize(void *private)
+{
+ if (skcipher_is_aead(private))
+ return crypto_aead_reqsize(private);
+ else
+ return crypto_ablkcipher_reqsize(private);
+}
+
+static inline unsigned int skcipher_crypto_setkey(void *private, const u8 *key,
+ unsigned int keylen)
+{
+ if (skcipher_is_aead(private))
+ return crypto_aead_setkey(private, key, keylen);
+ else
+ return crypto_ablkcipher_setkey(private, key, keylen);
+}
+
+static inline void skcipher_crypto_free(void *private)
+{
+ if (skcipher_is_aead(private))
+ crypto_free_aead(private);
+ else
+ crypto_free_ablkcipher(private);
+}
+
+static inline void skcipher_request_set_tfm(struct skcipher_ctx *ctx, void *tfm)
+{
+ if (ctx->aead)
+ aead_request_set_tfm(&ctx->u.aead_req, tfm);
+ else
+ ablkcipher_request_set_tfm(&ctx->u.ablkcipher_req, tfm);
+}
+
+static inline int skcipher_crypto_encrypt(struct skcipher_ctx *ctx)
+{
+ if (ctx->aead)
+ return crypto_aead_encrypt(&ctx->u.aead_req);
+ else
+ return crypto_ablkcipher_encrypt(&ctx->u.ablkcipher_req);
+}
+
+static inline int skcipher_crypto_decrypt(struct skcipher_ctx *ctx)
+{
+ if (ctx->aead)
+ return crypto_aead_decrypt(&ctx->u.aead_req);
+ else
+ return crypto_ablkcipher_decrypt(&ctx->u.ablkcipher_req);
+}
+
+static inline void skcipher_crypto_set_crypt(struct skcipher_ctx *ctx,
+ struct scatterlist *src,
+ struct scatterlist *dst,
+ unsigned int cryptlen, u8 *iv)
+{
+ if (ctx->aead)
+ return aead_request_set_crypt(&ctx->u.aead_req, src, dst,
+ cryptlen, iv);
+ else
+ return ablkcipher_request_set_crypt(&ctx->u.ablkcipher_req, src,
+ dst, cryptlen, iv);
+}
+
+static inline void skcipher_request_set_callback(struct skcipher_ctx *ctx,
+ u32 flags,
+ crypto_completion_t complete,
+ void *data)
+{
+ if (ctx->aead)
+ aead_request_set_callback(&ctx->u.aead_req, flags, complete,
+ data);
+ else
+ ablkcipher_request_set_callback(&ctx->u.ablkcipher_req, flags,
+ complete, data);
+}
+
static int skcipher_sendmsg(struct kiocb *unused, struct socket *sock,
struct msghdr *msg, size_t size)
{
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct skcipher_ctx *ctx = ask->private;
- struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(&ctx->req);
- unsigned ivsize = crypto_ablkcipher_ivsize(tfm);
+ unsigned ivsize = skcipher_crypto_ivsize_ctx(ctx);
struct skcipher_sg_list *sgl;
struct af_alg_control con = {};
long copied = 0;
@@ -432,8 +539,7 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct skcipher_ctx *ctx = ask->private;
- unsigned bs = crypto_ablkcipher_blocksize(crypto_ablkcipher_reqtfm(
- &ctx->req));
+ unsigned bs = skcipher_crypto_blocksize(ctx);
struct skcipher_sg_list *sgl;
struct scatterlist *sg;
unsigned long iovlen;
@@ -483,8 +589,8 @@ static int skcipher_recvmsg(struct kiocb *unused, struct socket *sock,
err = af_alg_wait_for_completion(
ctx->enc ?
- crypto_ablkcipher_encrypt(&ctx->req) :
- crypto_ablkcipher_decrypt(&ctx->req),
+ skcipher_crypto_encrypt(ctx) :
+ skcipher_crypto_decrypt(ctx),
&ctx->completion);
free:
@@ -603,22 +709,22 @@ static void *skcipher_bind(const char *name, u32 type, u32 mask)
static void skcipher_release(void *private)
{
- crypto_free_ablkcipher(private);
+ skcipher_crypto_free(private);
}
static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
{
- return crypto_ablkcipher_setkey(private, key, keylen);
+ return skcipher_crypto_setkey(private, key, keylen);
}
static void skcipher_sock_destruct(struct sock *sk)
{
struct alg_sock *ask = alg_sk(sk);
struct skcipher_ctx *ctx = ask->private;
- struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(&ctx->req);
+ unsigned int ivlen = skcipher_crypto_ivsize_ctx(ctx);
skcipher_free_sgl(sk);
- sock_kfree_s(sk, ctx->iv, crypto_ablkcipher_ivsize(tfm));
+ sock_kfree_s(sk, ctx->iv, ivlen);
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -627,20 +733,20 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
{
struct skcipher_ctx *ctx;
struct alg_sock *ask = alg_sk(sk);
- unsigned int len = sizeof(*ctx) + crypto_ablkcipher_reqsize(private);
+ unsigned int len = sizeof(*ctx) + skcipher_crypto_reqsize(private);
+ unsigned int ivlen = skcipher_crypto_ivsize(private);
ctx = sock_kmalloc(sk, len, GFP_KERNEL);
if (!ctx)
return -ENOMEM;
- ctx->iv = sock_kmalloc(sk, crypto_ablkcipher_ivsize(private),
- GFP_KERNEL);
+ ctx->iv = sock_kmalloc(sk, ivlen, GFP_KERNEL);
if (!ctx->iv) {
sock_kfree_s(sk, ctx, len);
return -ENOMEM;
}
- memset(ctx->iv, 0, crypto_ablkcipher_ivsize(private));
+ memset(ctx->iv, 0, ivlen);
INIT_LIST_HEAD(&ctx->tsgl);
ctx->len = len;
@@ -648,13 +754,14 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
ctx->more = 0;
ctx->merge = 0;
ctx->enc = 0;
+ ctx->aead = skcipher_is_aead(private);
af_alg_init_completion(&ctx->completion);
ask->private = ctx;
- ablkcipher_request_set_tfm(&ctx->req, private);
- ablkcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
- af_alg_complete, &ctx->completion);
+ skcipher_request_set_tfm(ctx, private);
+ skcipher_request_set_callback(ctx, CRYPTO_TFM_REQ_MAY_BACKLOG,
+ af_alg_complete, &ctx->completion);
sk->sk_destruct = skcipher_sock_destruct;
--
2.1.0
AEAD requires the following data in addition to normal symmetric
ciphers:
* Associated authentication data of arbitrary length
* Authentication tag for decryption
* Length of authentication tag for encryption
The authentication tag data is communicated as part of the actual
ciphertext as mandated by the kernel crypto API. Therefore we only need
to provide a user space interface for the associated authentication data
as well as for the authentication tag length.
This patch adds both as a setsockopt interface that is identical to the
AF_ALG interface for setting an IV and for selecting the cipher
operation type (encrypt or decrypt).
Signed-off-by: Stephan Mueller <[email protected]>
---
crypto/af_alg.c | 17 +++++++++++++++++
include/crypto/if_alg.h | 2 ++
include/uapi/linux/if_alg.h | 7 +++++++
3 files changed, 26 insertions(+)
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 6a3ad80..635140b 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -421,6 +421,23 @@ int af_alg_cmsg_send(struct msghdr *msg, struct af_alg_control *con)
con->op = *(u32 *)CMSG_DATA(cmsg);
break;
+
+ case ALG_SET_AEAD_AUTHSIZE:
+ if (cmsg->cmsg_len < CMSG_LEN(sizeof(u32)))
+ return -EINVAL;
+ con->aead_authsize = *(u32 *)CMSG_DATA(cmsg);
+ break;
+
+ case ALG_SET_AEAD_ASSOC:
+ if (cmsg->cmsg_len < CMSG_LEN(sizeof(*con->aead_assoc)))
+ return -EINVAL;
+ con->aead_assoc = (void *)CMSG_DATA(cmsg);
+ if (cmsg->cmsg_len <
+ CMSG_LEN(con->aead_assoc->aead_assoclen +
+ sizeof(*con->aead_assoc)))
+ return -EINVAL;
+ break;
+
default:
return -EINVAL;
}
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index d61c111..c741483 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -41,7 +41,9 @@ struct af_alg_completion {
struct af_alg_control {
struct af_alg_iv *iv;
+ struct af_alg_aead_assoc *aead_assoc;
int op;
+ unsigned int aead_authsize;
};
struct af_alg_type {
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index 0f9acce..64e7008 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -28,10 +28,17 @@ struct af_alg_iv {
__u8 iv[0];
};
+struct af_alg_aead_assoc {
+ __u32 aead_assoclen;
+ __u8 aead_assoc[0];
+};
+
/* Socket options */
#define ALG_SET_KEY 1
#define ALG_SET_IV 2
#define ALG_SET_OP 3
+#define ALG_SET_AEAD_ASSOC 4
+#define ALG_SET_AEAD_AUTHSIZE 5
/* Operations */
#define ALG_OP_DECRYPT 0
--
2.1.0
The AF_ALG interface allows normal cipher (hash, encrypt, decrypt).
However, it does not allow user space to obtain the following generic
information about the currently active cipher:
* block size of the cipher
* IV size of the cipher
* for AEAD, the maximum authentication tag size
The patch adds a getsockopt interface for the symmetric ciphers to
answer such information requests from user space.
The kernel crypto API function calls are used to obtain the real data.
As all data are simple integer values, the getsockopt handler function
uses put_user() to return the integer value to user space in the
*optval parameter of getsockopt.
Signed-off-by: Stephan Mueller <[email protected]>
---
crypto/algif_skcipher.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-
include/uapi/linux/if_alg.h | 3 +++
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 83187f4..7a1656b 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -522,6 +522,50 @@ static unsigned int skcipher_poll(struct file *file, struct socket *sock,
return mask;
}
+static int skcipher_getsockopt(struct socket *sock, int level, int optname,
+ char __user *optval, int __user *optlen)
+{
+ struct sock *sk = sock->sk;
+ struct alg_sock *ask = alg_sk(sk);
+ struct skcipher_ctx *ctx = ask->private;
+ const struct af_alg_type *type;
+ int len = 0;
+ int err = -EOPNOTSUPP;
+
+ lock_sock(sk);
+ type = ask->type;
+
+ if (level != SOL_ALG || !type)
+ goto unlock;
+
+ switch (optname) {
+ case ALG_GET_BLOCKSIZE:
+ len = skcipher_crypto_blocksize(ctx);
+ err = 0;
+ break;
+ case ALG_GET_IVSIZE:
+ len = skcipher_crypto_ivsize_ctx(ctx);
+ err = 0;
+ break;
+ case ALG_GET_AEAD_AUTHSIZE:
+ if (ctx->aead) {
+ len = crypto_aead_authsize(crypto_aead_reqtfm(
+ &ctx->u.aead_req));
+ err = 0;
+ }
+ break;
+ default:
+ break;
+ }
+
+unlock:
+ release_sock(sk);
+ if (err >= 0)
+ err = put_user(len, optlen);
+
+ return err;
+}
+
static struct proto_ops algif_skcipher_ops = {
.family = PF_ALG,
@@ -531,7 +575,6 @@ static struct proto_ops algif_skcipher_ops = {
.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,
@@ -542,6 +585,7 @@ static struct proto_ops algif_skcipher_ops = {
.sendpage = skcipher_sendpage,
.recvmsg = skcipher_recvmsg,
.poll = skcipher_poll,
+ .getsockopt = skcipher_getsockopt,
};
static void *skcipher_bind(const char *name, u32 type, u32 mask)
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index 64e7008..267fd73 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -39,6 +39,9 @@ struct af_alg_aead_assoc {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOC 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_GET_BLOCKSIZE 6
+#define ALG_GET_IVSIZE 7
+#define ALG_GET_AEAD_AUTHSIZE 8
/* Operations */
#define ALG_OP_DECRYPT 0
--
2.1.0
This patch adds the random number generator support for AF_ALG.
A random number generator's purpose is to generate data without
requiring the caller to provide any data. Therefore, the AF_ALG
interface handler for RNGs only implements a callback handler for
recvmsg.
The following parameters provided with a recvmsg are processed by the
RNG callback handler:
* sock - to resolve the RNG context data structure accessing the
RNG instance private to the socket
* len - this parameter allows userspace callers to specify how
many random bytes the RNG shall produce and return. As the
kernel context for the RNG allocates a buffer of 128 bytes to
store random numbers before copying them to userspace, the len
parameter is checked that it is not larger than 128. If a
caller wants more random numbers, a new request for recvmsg
shall be made.
The size of 128 bytes is chose because of the following considerations:
* to increase the memory footprint of the kernel too much (note,
that would be 128 bytes per open socket)
* 128 is divisible by any typical cryptographic block size an
RNG may have
* A request for random numbers typically only shall supply small
amount of data like for keys or IVs that should only require
one invocation of the recvmsg function.
Note, during instantiation of the RNG, the code checks whether the RNG
implementation requires seeding. If so, the RNG is seeded with output
from get_random_bytes.
A fully working example using all aspects of the RNG interface is
provided at http://www.chronox.de/libkcapi.html
Signed-off-by: Stephan Mueller <[email protected]>
---
crypto/algif_rng.c | 186 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 186 insertions(+)
create mode 100644 crypto/algif_rng.c
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
new file mode 100644
index 0000000..d1904d7
--- /dev/null
+++ b/crypto/algif_rng.c
@@ -0,0 +1,186 @@
+/*
+ * algif_rng: User-space interface for random number generators
+ *
+ * This file provides the user-space API for random number generators.
+ *
+ * Copyright (C) 2014, Stephan Mueller <[email protected]>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, and the entire permission notice in its entirety,
+ * including the disclaimer of warranties.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * 3. The name of the author may not be used to endorse or promote
+ * products derived from this software without specific prior
+ * written permission.
+ *
+ * ALTERNATIVELY, this product may be distributed under the terms of
+ * the GNU General Public License, in which case the provisions of the GPL2 are
+ * required INSTEAD OF the above restrictions. (This clause is
+ * necessary due to a potential bad interaction between the GPL and
+ * the restrictions contained in a BSD-style copyright.)
+ *
+ * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
+ * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE, ALL OF
+ * WHICH ARE HEREBY DISCLAIMED. IN NO EVENT SHALL THE AUTHOR BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT
+ * OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
+ * USE OF THIS SOFTWARE, EVEN IF NOT ADVISED OF THE POSSIBILITY OF SUCH
+ * DAMAGE.
+ */
+
+#include <linux/module.h>
+#include <crypto/rng.h>
+#include <linux/random.h>
+#include <crypto/if_alg.h>
+#include <linux/net.h>
+#include <net/sock.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Stephan Mueller <[email protected]>");
+MODULE_DESCRIPTION("User-space interface for random number generators");
+
+struct rng_ctx {
+#define MAXSIZE 128
+ u8 result[MAXSIZE];
+ unsigned int len;
+ struct crypto_rng *drng;
+};
+
+static int rng_recvmsg(struct kiocb *unused, 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 = -EFAULT;
+
+ if (0 == len)
+ return 0;
+ if (MAXSIZE < len)
+ len = MAXSIZE;
+
+ lock_sock(sk);
+ len = crypto_rng_get_bytes(ctx->drng, ctx->result, len);
+ if (0 > len)
+ goto unlock;
+
+ err = memcpy_toiovec(msg->msg_iov, ctx->result, len);
+ memset(ctx->result, 0, err);
+
+unlock:
+ release_sock(sk);
+
+ return err ? err : len;
+}
+
+static struct proto_ops algif_rng_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,
+ .poll = sock_no_poll,
+ .sendmsg = sock_no_sendmsg,
+ .sendpage = sock_no_sendpage,
+
+ .release = af_alg_release,
+ .recvmsg = rng_recvmsg,
+};
+
+static void *rng_bind(const char *name, u32 type, u32 mask)
+{
+ return crypto_alloc_rng(name, type, mask);
+}
+
+static void rng_release(void *private)
+{
+ crypto_free_rng(private);
+}
+
+static void rng_sock_destruct(struct sock *sk)
+{
+ struct alg_sock *ask = alg_sk(sk);
+ struct rng_ctx *ctx = ask->private;
+
+ memset(ctx->result, 0, MAXSIZE);
+ sock_kfree_s(sk, ctx, ctx->len);
+ af_alg_release_parent(sk);
+}
+
+static int rng_accept_parent(void *private, struct sock *sk)
+{
+ struct rng_ctx *ctx;
+ struct alg_sock *ask = alg_sk(sk);
+ unsigned int len = sizeof(*ctx);
+ int seedsize = crypto_rng_seedsize(private);
+ int ret = -ENOMEM;
+
+ ctx = sock_kmalloc(sk, len, GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+ memset(ctx->result, 0, MAXSIZE);
+
+ ctx->len = len;
+
+ if (seedsize) {
+ u8 *buf = kmalloc(seedsize, GFP_KERNEL);
+ if (!buf)
+ goto err;
+ get_random_bytes(buf, seedsize);
+ ret = crypto_rng_reset(private, buf, len);
+ kzfree(buf);
+ if (ret)
+ goto err;
+ }
+
+ ctx->drng = private;
+ ask->private = ctx;
+ sk->sk_destruct = rng_sock_destruct;
+
+ return 0;
+
+err:
+ sock_kfree_s(sk, ctx, len);
+ return ret;
+}
+
+static const struct af_alg_type algif_type_rng = {
+ .bind = rng_bind,
+ .release = rng_release,
+ .accept = rng_accept_parent,
+ .ops = &algif_rng_ops,
+ .name = "rng",
+ .owner = THIS_MODULE
+};
+
+static int __init rng_init(void)
+{
+ return af_alg_register_type(&algif_type_rng);
+}
+
+void __exit rng_exit(void)
+{
+ int err = af_alg_unregister_type(&algif_type_rng);
+ BUG_ON(err);
+}
+
+module_init(rng_init);
+module_exit(rng_exit);
--
2.1.0
On 11/12/2014 08:05 AM, Stephan Mueller wrote:
> This patch adds the random number generator support for AF_ALG.
>
> A random number generator's purpose is to generate data without
> requiring the caller to provide any data. Therefore, the AF_ALG
> interface handler for RNGs only implements a callback handler for
> recvmsg.
...
> +static int rng_recvmsg(struct kiocb *unused, 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 = -EFAULT;
> +
> + if (0 == len)
if (len == 0)
...
[And also other places.]
We don't use Yoda condition style in the kernel.
> + return 0;
> + if (MAXSIZE < len)
> + len = MAXSIZE;
> +
> + lock_sock(sk);
> + len = crypto_rng_get_bytes(ctx->drng, ctx->result, len);
> + if (0 > len)
> + goto unlock;
> +
> + err = memcpy_toiovec(msg->msg_iov, ctx->result, len);
> + memset(ctx->result, 0, err);
> +
This looks buggy.
If copy_to_user() fails from within memcpy_toiovec(), we call memset()
with a negative return value which is interpreted as size_t and thus
causes a buffer overflow writing beyond ctx->result, no?
If it succeeds, we call memset(ctx->result, 0, 0) .....
> +unlock:
> + release_sock(sk);
> +
> + return err ? err : len;
> +}
> +
> +static struct proto_ops algif_rng_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,
> + .poll = sock_no_poll,
> + .sendmsg = sock_no_sendmsg,
> + .sendpage = sock_no_sendpage,
> +
> + .release = af_alg_release,
> + .recvmsg = rng_recvmsg,
> +};
> +
> +static void *rng_bind(const char *name, u32 type, u32 mask)
> +{
> + return crypto_alloc_rng(name, type, mask);
> +}
> +
> +static void rng_release(void *private)
> +{
> + crypto_free_rng(private);
> +}
> +
> +static void rng_sock_destruct(struct sock *sk)
> +{
> + struct alg_sock *ask = alg_sk(sk);
> + struct rng_ctx *ctx = ask->private;
> +
> + memset(ctx->result, 0, MAXSIZE);
memset(ctx->result, 0, sizeof(ctx->result));
> + sock_kfree_s(sk, ctx, ctx->len);
> + af_alg_release_parent(sk);
> +}
> +
> +static int rng_accept_parent(void *private, struct sock *sk)
> +{
> + struct rng_ctx *ctx;
> + struct alg_sock *ask = alg_sk(sk);
> + unsigned int len = sizeof(*ctx);
> + int seedsize = crypto_rng_seedsize(private);
> + int ret = -ENOMEM;
> +
> + ctx = sock_kmalloc(sk, len, GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> + memset(ctx->result, 0, MAXSIZE);
Ditto...
> + ctx->len = len;
> +
> + if (seedsize) {
> + u8 *buf = kmalloc(seedsize, GFP_KERNEL);
> + if (!buf)
> + goto err;
> + get_random_bytes(buf, seedsize);
> + ret = crypto_rng_reset(private, buf, len);
> + kzfree(buf);
Am Mittwoch, 12. November 2014, 17:15:52 schrieb Daniel Borkmann:
Hi Daniel,
thanks for the comments.
> On 11/12/2014 08:05 AM, Stephan Mueller wrote:
> > This patch adds the random number generator support for AF_ALG.
> >
> > A random number generator's purpose is to generate data without
> > requiring the caller to provide any data. Therefore, the AF_ALG
> > interface handler for RNGs only implements a callback handler for
> > recvmsg.
>
> ...
>
> > +static int rng_recvmsg(struct kiocb *unused, 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 = -EFAULT;
> > +
> > + if (0 == len)
>
> if (len == 0)
> ...
>
> [And also other places.]
>
> We don't use Yoda condition style in the kernel.
Well, there is a very good reason for using the approach I have: we all have
done the error of forgetting the second = sign.
In my case, the compiler will complain and we fix the error right away.
In your case, nobody is complaining but we introduced a nasty, potentially
hard to debug error. Thus, I very much like to keep my version just to be on
the safe side.
Note, there was even a backdoor I have seen where the missing 2nd equal sign
introduced a privilege escalation.
Therefore, my standard coding practice is to have a fixed value on the left
side and the variable on the right side of any comparison.
>
> > + return 0;
> > + if (MAXSIZE < len)
> > + len = MAXSIZE;
> > +
> > + lock_sock(sk);
> > + len = crypto_rng_get_bytes(ctx->drng, ctx->result, len);
> > + if (0 > len)
> > + goto unlock;
> > +
> > + err = memcpy_toiovec(msg->msg_iov, ctx->result, len);
> > + memset(ctx->result, 0, err);
> > +
>
> This looks buggy.
>
> If copy_to_user() fails from within memcpy_toiovec(), we call memset()
> with a negative return value which is interpreted as size_t and thus
> causes a buffer overflow writing beyond ctx->result, no?
>
> If it succeeds, we call memset(ctx->result, 0, 0) .....
Right, good catch, I have to add a catch for negative error here.
>
> > +unlock:
> > + release_sock(sk);
> > +
> > + return err ? err : len;
> > +}
> > +
> > +static struct proto_ops algif_rng_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,
> > + .poll = sock_no_poll,
> > + .sendmsg = sock_no_sendmsg,
> > + .sendpage = sock_no_sendpage,
> > +
> > + .release = af_alg_release,
> > + .recvmsg = rng_recvmsg,
> > +};
> > +
> > +static void *rng_bind(const char *name, u32 type, u32 mask)
> > +{
> > + return crypto_alloc_rng(name, type, mask);
> > +}
> > +
> > +static void rng_release(void *private)
> > +{
> > + crypto_free_rng(private);
> > +}
> > +
> > +static void rng_sock_destruct(struct sock *sk)
> > +{
> > + struct alg_sock *ask = alg_sk(sk);
> > + struct rng_ctx *ctx = ask->private;
> > +
> > + memset(ctx->result, 0, MAXSIZE);
>
> memset(ctx->result, 0, sizeof(ctx->result));
Ok, if this is desired, fine with me.
>
> > + sock_kfree_s(sk, ctx, ctx->len);
> > + af_alg_release_parent(sk);
> > +}
> > +
> > +static int rng_accept_parent(void *private, struct sock *sk)
> > +{
> > + struct rng_ctx *ctx;
> > + struct alg_sock *ask = alg_sk(sk);
> > + unsigned int len = sizeof(*ctx);
> > + int seedsize = crypto_rng_seedsize(private);
> > + int ret = -ENOMEM;
> > +
> > + ctx = sock_kmalloc(sk, len, GFP_KERNEL);
> > + if (!ctx)
> > + return -ENOMEM;
> > + memset(ctx->result, 0, MAXSIZE);
>
> Ditto...
Will do.
>
> > + ctx->len = len;
> > +
> > + if (seedsize) {
> > + u8 *buf = kmalloc(seedsize, GFP_KERNEL);
> > + if (!buf)
> > + goto err;
> > + get_random_bytes(buf, seedsize);
> > + ret = crypto_rng_reset(private, buf, len);
> > + kzfree(buf);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Ciao
Stephan
On 11/12/2014 05:54 PM, Stephan Mueller wrote:
> Am Mittwoch, 12. November 2014, 17:15:52 schrieb Daniel Borkmann:
>> On 11/12/2014 08:05 AM, Stephan Mueller wrote:
>>> This patch adds the random number generator support for AF_ALG.
>>>
>>> A random number generator's purpose is to generate data without
>>> requiring the caller to provide any data. Therefore, the AF_ALG
>>> interface handler for RNGs only implements a callback handler for
>>> recvmsg.
>>
>> ...
>>
>>> +static int rng_recvmsg(struct kiocb *unused, 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 = -EFAULT;
>>> +
>>> + if (0 == len)
>>
>> if (len == 0)
>> ...
>>
>> [And also other places.]
>>
>> We don't use Yoda condition style in the kernel.
>
> Well, there is a very good reason for using the approach I have: we all have
> done the error of forgetting the second = sign.
>
> In my case, the compiler will complain and we fix the error right away.
>
> In your case, nobody is complaining but we introduced a nasty, potentially
> hard to debug error. Thus, I very much like to keep my version just to be on
> the safe side.
>
> Note, there was even a backdoor I have seen where the missing 2nd equal sign
> introduced a privilege escalation.
>
> Therefore, my standard coding practice is to have a fixed value on the left
> side and the variable on the right side of any comparison.
I understand, but then please add this proposal first into ...
Documentation/CodingStyle
The problem is that while the rest of the kernel does not follow
this coding style, it's also much harder to read and/or program
this way for people not being used to. So the danger of bugs
slipping in this way is at least equally high. Besides that, this
argument would also only account for '==' checks.
>>> + return 0;
>>> + if (MAXSIZE < len)
>>> + len = MAXSIZE;
>>> +
>>> + lock_sock(sk);
>>> + len = crypto_rng_get_bytes(ctx->drng, ctx->result, len);
>>> + if (0 > len)
>>> + goto unlock;
>>> +
>>> + err = memcpy_toiovec(msg->msg_iov, ctx->result, len);
>>> + memset(ctx->result, 0, err);
>>> +
>>
>> This looks buggy.
>>
>> If copy_to_user() fails from within memcpy_toiovec(), we call memset()
>> with a negative return value which is interpreted as size_t and thus
>> causes a buffer overflow writing beyond ctx->result, no?
>>
>> If it succeeds, we call memset(ctx->result, 0, 0) .....
>
> Right, good catch, I have to add a catch for negative error here.
Hm? Don't you rather mean to say to unconditionally do something like ...
memzero_explicit(ctx->result, len);
...
>>> + memset(ctx->result, 0, MAXSIZE);
>>
>> memset(ctx->result, 0, sizeof(ctx->result));
>
> Ok, if this is desired, fine with me.
Yes, please.
Am Mittwoch, 12. November 2014, 18:23:27 schrieb Daniel Borkmann:
Hi Daniel,
>On 11/12/2014 05:54 PM, Stephan Mueller wrote:
>> Am Mittwoch, 12. November 2014, 17:15:52 schrieb Daniel Borkmann:
>>> On 11/12/2014 08:05 AM, Stephan Mueller wrote:
>>>> This patch adds the random number generator support for AF_ALG.
>>>>
>>>> A random number generator's purpose is to generate data without
>>>> requiring the caller to provide any data. Therefore, the AF_ALG
>>>> interface handler for RNGs only implements a callback handler for
>>>> recvmsg.
>>>
>>> ...
>>>
>>>> +static int rng_recvmsg(struct kiocb *unused, 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 = -EFAULT;
>>>> +
>>>> + if (0 == len)
>>>
>>> if (len == 0)
>>>
>>> ...
>>>
>>> [And also other places.]
>>>
>>> We don't use Yoda condition style in the kernel.
>>
>> Well, there is a very good reason for using the approach I have: we
>> all have done the error of forgetting the second = sign.
>>
>> In my case, the compiler will complain and we fix the error right
>> away.
>>
>> In your case, nobody is complaining but we introduced a nasty,
>> potentially hard to debug error. Thus, I very much like to keep my
>> version just to be on the safe side.
>>
>> Note, there was even a backdoor I have seen where the missing 2nd
>> equal sign introduced a privilege escalation.
>>
>> Therefore, my standard coding practice is to have a fixed value on
>> the left side and the variable on the right side of any comparison.
>
>I understand, but then please add this proposal first into ...
>
> Documentation/CodingStyle
>
>The problem is that while the rest of the kernel does not follow
>this coding style, it's also much harder to read and/or program
>this way for people not being used to. So the danger of bugs
>slipping in this way is at least equally high. Besides that, this
>argument would also only account for '==' checks.
Ok, I can change that throughout the code.
>
>>>> + return 0;
>>>> + if (MAXSIZE < len)
>>>> + len = MAXSIZE;
>>>> +
>>>> + lock_sock(sk);
>>>> + len = crypto_rng_get_bytes(ctx->drng, ctx->result, len);
>>>> + if (0 > len)
>>>> + goto unlock;
>>>> +
>>>> + err = memcpy_toiovec(msg->msg_iov, ctx->result, len);
>>>> + memset(ctx->result, 0, err);
>>>> +
>>>
>>> This looks buggy.
>>>
>>> If copy_to_user() fails from within memcpy_toiovec(), we call
>>> memset()
>>> with a negative return value which is interpreted as size_t and thus
>>> causes a buffer overflow writing beyond ctx->result, no?
>>>
>>> If it succeeds, we call memset(ctx->result, 0, 0) .....
>>
>> Right, good catch, I have to add a catch for negative error here.
>
>Hm? Don't you rather mean to say to unconditionally do something like
>...
>
> memzero_explicit(ctx->result, len);
Sorry, I was not clear:
* I need to catch a failing memcpy, but not return an error.
* I unconditionally use the memset after memcpy as you indicated. Once
the cryptodev tree contains the memzero_explicit call, I will start
picking up that function.
Essentially, I throught of the line you suggested.
Ciao
Stephan
On 11/12/2014 06:46 PM, Stephan Mueller wrote:
...
> * I unconditionally use the memset after memcpy as you indicated. Once
> the cryptodev tree contains the memzero_explicit call, I will start
> picking up that function.
Herbert merged it actually in this morning, so it's already part of
the cryptodev tree by now.
> Essentially, I throught of the line you suggested.
Ok, thanks.