Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1300067 > unrolled thread

Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Milan Broz <gmazyland@gmail.com> - 2016-01-02 13:00 +0100
    Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Milan Broz <gmazyland@gmail.com> - 2016-01-02 15:50 +0100
      Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2) Stephan Mueller <smueller@chronox.de> - 2016-01-02 21:10 +0100
        Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Milan Broz <gmazyland@gmail.com> - 2016-01-02 21:20 +0100
          Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-03 02:40 +0100
            Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Milan Broz <gmazyland@gmail.com> - 2016-01-03 10:50 +0100
              [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-04 05:40 +0100
                [PATCH 2/2] crypto: algif_skcipher - Add nokey compatibility path Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-04 05:40 +0100
                Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Milan Broz <gmazyland@gmail.com> - 2016-01-04 13:40 +0100
                  Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-08 13:50 +0100
                    Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Milan Broz <gmazyland@gmail.com> - 2016-01-08 19:30 +0100
                      Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-09 06:50 +0100
                        Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Milan Broz <gmazyland@gmail.com> - 2016-01-09 11:20 +0100
                          [PATCH 1/2] crypto: skcipher - Add crypto_skcipher_has_setkey Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-11 14:30 +0100
                            [PATCH 2/2] crypto: algif_skcipher - Add key check exception for  cipher_null Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-11 14:40 +0100
                              Re: [PATCH 2/2] crypto: algif_skcipher - Add key check exception for  cipher_null Milan Broz <gmazyland@gmail.com> - 2016-01-11 16:00 +0100
                  [PATCH 1/2] crypto: hash - Add crypto_ahash_has_setkey Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-08 14:30 +0100
                    [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2) Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-08 14:40 +0100
                      Re: [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2) kbuild test robot <lkp@intel.com> - 2016-01-08 15:00 +0100
                      Re: [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2) Milan Broz <gmazyland@gmail.com> - 2016-01-09 11:20 +0100

#1300067 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

On 12/25/2015 08:40 AM, Herbert Xu wrote:
> Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> I am testing with your two patches:
>> crypto: algif_skcipher - Use new skcipher interface
>> crypto: algif_skcipher - Require setkey before accept(2)
>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
> 
> You sent the email to everyone on the original CC list except me.
> Please don't do that.
> 
>> Now the following program causes a bunch of use-after-frees and them
>> kills kernel:
> 
> Yes there is an obvious bug in the patch that Julia Lawall has
> responded to in another thread.  Here is a fixed version.
> 
> ---8<--
> Some cipher implementations will crash if you try to use them
> without calling setkey first.  This patch adds a check so that
> the accept(2) call will fail with -ENOKEY if setkey hasn't been
> done on the socket yet.


Hi Herbert,

this patch breaks userspace in cryptsetup...

We use algif_skcipher in cryptsetup (for years, even before
there was Stephan's library) and with this patch applied
I see fail in ALG_SET_IV call (patch from your git).

I can fix it upstream, but for thousands of installations it will
be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
it will be unusable. Also people who configured kernel crypto API as default
backend will have non-working cryptsetup).

Is it really thing for stable branch?

Milan

> 
> Cc: stable@vger.kernel.org
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 5c756b3..f4431bc 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -31,6 +31,11 @@ struct skcipher_sg_list {
>  	struct scatterlist sg[0];
>  };
>  
> +struct skcipher_tfm {
> +	struct crypto_skcipher *skcipher;
> +	bool has_key;
> +};
> +
>  struct skcipher_ctx {
>  	struct list_head tsgl;
>  	struct af_alg_sgl rsgl;
> @@ -750,17 +755,41 @@ static struct proto_ops algif_skcipher_ops = {
>  
>  static void *skcipher_bind(const char *name, u32 type, u32 mask)
>  {
> -	return crypto_alloc_skcipher(name, type, mask);
> +	struct skcipher_tfm *tfm;
> +	struct crypto_skcipher *skcipher;
> +
> +	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
> +	if (!tfm)
> +		return ERR_PTR(-ENOMEM);
> +
> +	skcipher = crypto_alloc_skcipher(name, type, mask);
> +	if (IS_ERR(skcipher)) {
> +		kfree(tfm);
> +		return ERR_CAST(skcipher);
> +	}
> +
> +	tfm->skcipher = skcipher;
> +
> +	return tfm;
>  }
>  
>  static void skcipher_release(void *private)
>  {
> -	crypto_free_skcipher(private);
> +	struct skcipher_tfm *tfm = private;
> +
> +	crypto_free_skcipher(tfm->skcipher);
> +	kfree(tfm);
>  }
>  
>  static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
>  {
> -	return crypto_skcipher_setkey(private, key, keylen);
> +	struct skcipher_tfm *tfm = private;
> +	int err;
> +
> +	err = crypto_skcipher_setkey(tfm->skcipher, key, keylen);
> +	tfm->has_key = !err;
> +
> +	return err;
>  }
>  
>  static void skcipher_wait(struct sock *sk)
> @@ -792,20 +821,25 @@ 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_skcipher_reqsize(private);
> +	struct skcipher_tfm *tfm = private;
> +	struct crypto_skcipher *skcipher = tfm->skcipher;
> +	unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(skcipher);
> +
> +	if (!tfm->has_key)
> +		return -ENOKEY;
>  
>  	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
>  	if (!ctx)
>  		return -ENOMEM;
>  
> -	ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(private),
> +	ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(skcipher),
>  			       GFP_KERNEL);
>  	if (!ctx->iv) {
>  		sock_kfree_s(sk, ctx, len);
>  		return -ENOMEM;
>  	}
>  
> -	memset(ctx->iv, 0, crypto_skcipher_ivsize(private));
> +	memset(ctx->iv, 0, crypto_skcipher_ivsize(skcipher));
>  
>  	INIT_LIST_HEAD(&ctx->tsgl);
>  	ctx->len = len;
> @@ -818,7 +852,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
>  
>  	ask->private = ctx;
>  
> -	skcipher_request_set_tfm(&ctx->req, private);
> +	skcipher_request_set_tfm(&ctx->req, skcipher);
>  	skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>  				      af_alg_complete, &ctx->completion);
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1300079

On 01/02/2016 12:52 PM, Milan Broz wrote:
> On 12/25/2015 08:40 AM, Herbert Xu wrote:
>> Dmitry Vyukov <dvyukov@google.com> wrote:
>>>
>>> I am testing with your two patches:
>>> crypto: algif_skcipher - Use new skcipher interface
>>> crypto: algif_skcipher - Require setkey before accept(2)
>>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
>>
>> You sent the email to everyone on the original CC list except me.
>> Please don't do that.
>>
>>> Now the following program causes a bunch of use-after-frees and them
>>> kills kernel:
>>
>> Yes there is an obvious bug in the patch that Julia Lawall has
>> responded to in another thread.  Here is a fixed version.
>>
>> ---8<--
>> Some cipher implementations will crash if you try to use them
>> without calling setkey first.  This patch adds a check so that
>> the accept(2) call will fail with -ENOKEY if setkey hasn't been
>> done on the socket yet.
> 
> 
> Hi Herbert,
> 
> this patch breaks userspace in cryptsetup...
> 
> We use algif_skcipher in cryptsetup (for years, even before
> there was Stephan's library) and with this patch applied
> I see fail in ALG_SET_IV call (patch from your git).

(Obviously this was because of failing accept() call here, not set_iv.)

> 
> I can fix it upstream, but for thousands of installations it will
> be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
> it will be unusable. Also people who configured kernel crypto API as default
> backend will have non-working cryptsetup).
> 
> Is it really thing for stable branch?

Also how it is supposed to work for cipher_null, where there is no key?
Why it should call set_key if it is noop? (and set key length 0 is not possible).

(We are using cipher_null for testing and for offline re-encryption tool
to create temporary "fake" header for not-yet encrypted device...)

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300113 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

Am Samstag, 2. Januar 2016, 15:41:34 schrieb Milan Broz:

Hi Milan,

> On 01/02/2016 12:52 PM, Milan Broz wrote:
> > On 12/25/2015 08:40 AM, Herbert Xu wrote:
> >> Dmitry Vyukov <dvyukov@google.com> wrote:
> >>> I am testing with your two patches:
> >>> crypto: algif_skcipher - Use new skcipher interface
> >>> crypto: algif_skcipher - Require setkey before accept(2)
> >>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
> >> 
> >> You sent the email to everyone on the original CC list except me.
> >> Please don't do that.
> >> 
> >>> Now the following program causes a bunch of use-after-frees and them
> >> 
> >>> kills kernel:
> >> Yes there is an obvious bug in the patch that Julia Lawall has
> >> responded to in another thread.  Here is a fixed version.
> >> 
> >> ---8<--
> >> Some cipher implementations will crash if you try to use them
> >> without calling setkey first.  This patch adds a check so that
> >> the accept(2) call will fail with -ENOKEY if setkey hasn't been
> >> done on the socket yet.
> > 
> > Hi Herbert,
> > 
> > this patch breaks userspace in cryptsetup...
> > 
> > We use algif_skcipher in cryptsetup (for years, even before
> > there was Stephan's library) and with this patch applied
> > I see fail in ALG_SET_IV call (patch from your git).
> 
> (Obviously this was because of failing accept() call here, not set_iv.)
> 
> > I can fix it upstream, but for thousands of installations it will
> > be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
> > it will be unusable. Also people who configured kernel crypto API as
> > default backend will have non-working cryptsetup).
> > 
> > Is it really thing for stable branch?
> 
> Also how it is supposed to work for cipher_null, where there is no key?
> Why it should call set_key if it is noop? (and set key length 0 is not
> possible).
> 
> (We are using cipher_null for testing and for offline re-encryption tool
> to create temporary "fake" header for not-yet encrypted device...)

The change implies that any setkey or set IV operations (i.e. any operations 
on the tfmfd) are done before the opfd(s) are created with one or more accept 
calls.

Thus, after a bind that returns the tfmfd, the setkey and setiv operations 
shall be called. This is followed by accept. If you change the order of 
invocations in your code, it should work.
> 
> Milan


-- 
Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300116

On 01/02/2016 09:03 PM, Stephan Mueller wrote:
> Am Samstag, 2. Januar 2016, 15:41:34 schrieb Milan Broz:
> 
> Hi Milan,
> 

...
>>> Hi Herbert,
>>>
>>> this patch breaks userspace in cryptsetup...
>>>
>>> We use algif_skcipher in cryptsetup (for years, even before
>>> there was Stephan's library) and with this patch applied
>>> I see fail in ALG_SET_IV call (patch from your git).
>>
>> (Obviously this was because of failing accept() call here, not set_iv.)
>>
>>> I can fix it upstream, but for thousands of installations it will
>>> be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
>>> it will be unusable. Also people who configured kernel crypto API as
>>> default backend will have non-working cryptsetup).
>>>
>>> Is it really thing for stable branch?
>>
>> Also how it is supposed to work for cipher_null, where there is no key?
>> Why it should call set_key if it is noop? (and set key length 0 is not
>> possible).
>>
>> (We are using cipher_null for testing and for offline re-encryption tool
>> to create temporary "fake" header for not-yet encrypted device...)
> 
> The change implies that any setkey or set IV operations (i.e. any operations 
> on the tfmfd) are done before the opfd(s) are created with one or more accept 
> calls.
> 
> Thus, after a bind that returns the tfmfd, the setkey and setiv operations 
> shall be called. This is followed by accept. If you change the order of 
> invocations in your code, it should work.

Hi,

I already changed it in cryptsetup upstream this way.

But I cannot change thousands of cryptsetup installations that are actively using that code.
This is clear userspace breakage which should not happen this way.

(Moreover it still doesn't work for cipher_null that has min/max key size 0.)

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300166

On Sat, Jan 02, 2016 at 09:18:30PM +0100, Milan Broz wrote:
>
> But I cannot change thousands of cryptsetup installations that are actively using that code.
> This is clear userspace breakage which should not happen this way.

I'll try to add some compatibility code for your case, assuming
your modus operandi is accept(2) followed by a single setkey before
proceeding to encryption/decryption.

> (Moreover it still doesn't work for cipher_null that has min/max key size 0.)

Setkey works just fine on cipher_null.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300282

On 01/03/2016 02:31 AM, Herbert Xu wrote:
> On Sat, Jan 02, 2016 at 09:18:30PM +0100, Milan Broz wrote:
>>
>> But I cannot change thousands of cryptsetup installations that are actively using that code.
>> This is clear userspace breakage which should not happen this way.
> 
> I'll try to add some compatibility code for your case, assuming
> your modus operandi is accept(2) followed by a single setkey before
> proceeding to encryption/decryption.

Hi,

yes, basically it prepares socket()/bind()/accept() and then it calls setkey once.
(I'll try to fix in next releases to call setkey first though.)

I am doing exactly the same for AF_ALG HMAC (hmac(<hash>) key,
does this requirement for order if accept/setkey applies there as well?
(It is not enforced yet.)

Anyway, you can easily simulate that skcipher API call just with running "cryptsetup benchmark"
(with accept() patch it will print N/A for all ciphers while without patch it measures some
more-or-less magic performance numbers :)

> 
>> (Moreover it still doesn't work for cipher_null that has min/max key size 0.)
> 
> Setkey works just fine on cipher_null.

Yes, it works if ALG_SET_KEY is set to zero-length key.
I just re-introduced old bug to code, sorry.

Thanks!
Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300479 — [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

On Sun, Jan 03, 2016 at 10:42:28AM +0100, Milan Broz wrote:
> 
> yes, basically it prepares socket()/bind()/accept() and then it calls setkey once.
> (I'll try to fix in next releases to call setkey first though.)

OK please try these two patches (warning, totally untested).

---8<---
This patch adds a compatibility path to support old applications
that do acept(2) before setkey.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index eaf98e2..6566d2e 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -76,6 +76,8 @@ int af_alg_register_type(const struct af_alg_type *type)
 		goto unlock;
 
 	type->ops->owner = THIS_MODULE;
+	if (type->ops_nokey)
+		type->ops_nokey->owner = THIS_MODULE;
 	node->type = type;
 	list_add(&node->list, &alg_types);
 	err = 0;
@@ -267,6 +269,7 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)
 	const struct af_alg_type *type;
 	struct sock *sk2;
 	int err;
+	bool nokey;
 
 	lock_sock(sk);
 	type = ask->type;
@@ -285,12 +288,17 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)
 	security_sk_clone(sk, sk2);
 
 	err = type->accept(ask->private, sk2);
+
+	nokey = err == -ENOKEY;
+	if (nokey && type->accept_nokey)
+		err = type->accept_nokey(ask->private, sk2);
+
 	if (err)
 		goto unlock;
 
 	sk2->sk_family = PF_ALG;
 
-	if (!ask->refcnt++)
+	if (nokey || !ask->refcnt++)
 		sock_hold(sk);
 	alg_sk(sk2)->parent = sk;
 	alg_sk(sk2)->type = type;
@@ -298,6 +306,9 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)
 	newsock->ops = type->ops;
 	newsock->state = SS_CONNECTED;
 
+	if (nokey)
+		newsock->ops = type->ops_nokey;
+
 	err = 0;
 
 unlock:
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 589716f..df82844 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -52,9 +52,11 @@ struct af_alg_type {
 	void (*release)(void *private);
 	int (*setkey)(void *private, const u8 *key, unsigned int keylen);
 	int (*accept)(void *private, struct sock *sk);
+	int (*accept_nokey)(void *private, struct sock *sk);
 	int (*setauthsize)(void *private, unsigned int authsize);
 
 	struct proto_ops *ops;
+	struct proto_ops *ops_nokey;
 	struct module *owner;
 	char name[14];
 };
-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300482 — [PATCH 2/2] crypto: algif_skcipher - Add nokey compatibility path

This patch adds a compatibility path to support old applications
that do acept(2) before setkey.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index f4431bc..110bab4 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -753,6 +753,99 @@ static struct proto_ops algif_skcipher_ops = {
 	.poll		=	skcipher_poll,
 };
 
+static int skcipher_check_key(struct socket *sock)
+{
+	int err;
+	struct sock *psk;
+	struct alg_sock *pask;
+	struct skcipher_tfm *tfm;
+	struct sock *sk = sock->sk;
+	struct alg_sock *ask = alg_sk(sk);
+
+	if (ask->refcnt)
+		return 0;
+
+	psk = ask->parent;
+	pask = alg_sk(ask->parent);
+	tfm = pask->private;
+
+	err = -ENOKEY;
+	lock_sock(psk);
+	if (!tfm->has_key)
+		goto unlock;
+
+	if (!pask->refcnt++)
+		sock_hold(psk);
+
+	ask->refcnt = 1;
+	sock_put(psk);
+
+	err = 0;
+
+unlock:
+	release_sock(psk);
+
+	return err;
+}
+
+static int skcipher_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
+				  size_t size)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_sendmsg(sock, msg, size);
+}
+
+static ssize_t skcipher_sendpage_nokey(struct socket *sock, struct page *page,
+				       int offset, size_t size, int flags)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_sendpage(sock, page, offset, size, flags);
+}
+
+static int skcipher_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
+				  size_t ignored, int flags)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_recvmsg(sock, msg, ignored, flags);
+}
+
+static struct proto_ops algif_skcipher_ops_nokey = {
+	.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,
+
+	.release	=	af_alg_release,
+	.sendmsg	=	skcipher_sendmsg_nokey,
+	.sendpage	=	skcipher_sendpage_nokey,
+	.recvmsg	=	skcipher_recvmsg_nokey,
+	.poll		=	skcipher_poll,
+};
+
 static void *skcipher_bind(const char *name, u32 type, u32 mask)
 {
 	struct skcipher_tfm *tfm;
@@ -802,7 +895,7 @@ static void skcipher_wait(struct sock *sk)
 		msleep(100);
 }
 
-static void skcipher_sock_destruct(struct sock *sk)
+static void skcipher_sock_destruct_common(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct skcipher_ctx *ctx = ask->private;
@@ -814,10 +907,33 @@ static void skcipher_sock_destruct(struct sock *sk)
 	skcipher_free_sgl(sk);
 	sock_kzfree_s(sk, ctx->iv, crypto_skcipher_ivsize(tfm));
 	sock_kfree_s(sk, ctx, ctx->len);
+}
+
+static void skcipher_sock_destruct(struct sock *sk)
+{
+	skcipher_sock_destruct_common(sk);
 	af_alg_release_parent(sk);
 }
 
-static int skcipher_accept_parent(void *private, struct sock *sk)
+static void skcipher_release_parent_nokey(struct sock *sk)
+{
+	struct alg_sock *ask = alg_sk(sk);
+
+	if (!ask->refcnt) {
+		sock_put(ask->parent);
+		return;
+	}
+
+	af_alg_release_parent(sk);
+}
+
+static void skcipher_sock_destruct_nokey(struct sock *sk)
+{
+	skcipher_sock_destruct_common(sk);
+	skcipher_release_parent_nokey(sk);
+}
+
+static int skcipher_accept_parent_common(void *private, struct sock *sk)
 {
 	struct skcipher_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
@@ -825,9 +941,6 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 	struct crypto_skcipher *skcipher = tfm->skcipher;
 	unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(skcipher);
 
-	if (!tfm->has_key)
-		return -ENOKEY;
-
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
@@ -861,12 +974,38 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 	return 0;
 }
 
+static int skcipher_accept_parent(void *private, struct sock *sk)
+{
+	struct skcipher_tfm *tfm = private;
+
+	if (!tfm->has_key)
+		return -ENOKEY;
+
+	return skcipher_accept_parent_common(private, sk);
+}
+
+static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
+{
+	int err;
+
+	err = skcipher_accept_parent_common(private, sk);
+	if (err)
+		goto out;
+
+	sk->sk_destruct = skcipher_sock_destruct_nokey;
+
+out:
+	return err;
+}
+
 static const struct af_alg_type algif_type_skcipher = {
 	.bind		=	skcipher_bind,
 	.release	=	skcipher_release,
 	.setkey		=	skcipher_setkey,
 	.accept		=	skcipher_accept_parent,
+	.accept_nokey	=	skcipher_accept_parent_nokey,
 	.ops		=	&algif_skcipher_ops,
+	.ops_nokey	=	&algif_skcipher_ops_nokey,
 	.name		=	"skcipher",
 	.owner		=	THIS_MODULE
 };
-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300720 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

On 01/04/2016 05:35 AM, Herbert Xu wrote:
> On Sun, Jan 03, 2016 at 10:42:28AM +0100, Milan Broz wrote:
>>
>> yes, basically it prepares socket()/bind()/accept() and then it calls setkey once.
>> (I'll try to fix in next releases to call setkey first though.)
> 
> OK please try these two patches (warning, totally untested).

Well, it is not much better.

I had to apply another two patches that are not mentioned and are not in your tree yet
before it:
  crypto: af_alg - Disallow bind_setkey_... after accept(2)
  crypto: use-after-free in alg_bind

then it at least compiles correctly.

skcipher works, But I still see two compatibility problems:

- hmac() is now failing the same way (SETKEY after accept())
(I initially tested without two patches above, these are not in linux-next yet.)
This breaks all cryptsetup TrueCrypt support (and moreover all systems if
kernel crypto API is set as a default vcrypto backend - but that's not default).

- cipher_null before worked without setkey, now it requires to set key
(either before or after accept().
This was actually probably bad workaround in cryptsetup, anyway it will now cause
old cryptsetup-reencrypt tool failing with your patches.
(Try "cryptsetup benchmark -c cipher_null-ecb". I am not sure what to do here,
but not requiring setkey for "cipher" that has no key internally seems ok for me...)

As I said, I'll fix it in cryptsetup upstream but we are breaking  a lot of existing systems.
Isn't there better way, how to fix it?

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1304504 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

On Mon, Jan 04, 2016 at 01:33:54PM +0100, Milan Broz wrote:
>
> - hmac() is now failing the same way (SETKEY after accept())
> (I initially tested without two patches above, these are not in linux-next yet.)
> This breaks all cryptsetup TrueCrypt support (and moreover all systems if
> kernel crypto API is set as a default vcrypto backend - but that's not default).

Yes algif_hash would need the same compatibility patch and I'm
working on that.

> - cipher_null before worked without setkey, now it requires to set key
> (either before or after accept().
> This was actually probably bad workaround in cryptsetup, anyway it will now cause
> old cryptsetup-reencrypt tool failing with your patches.
> (Try "cryptsetup benchmark -c cipher_null-ecb". I am not sure what to do here,
> but not requiring setkey for "cipher" that has no key internally seems ok for me...)

Is cipher_null actually used in production or is this just a
benchmark? Using the kernel crypto API to perform no encryption
sounds crazy.

> As I said, I'll fix it in cryptsetup upstream but we are breaking  a lot of existing systems.
> Isn't there better way, how to fix it?

Setting the key after accept has always been wrong.  It's just
that it hasn't been noticed until now.  The main crypto socket
corresponds to the tfm object and is shared by all the child
sockets produced by accept(2).  So once you have child sockets
which may then be used by another thread you must not modify
the parent socket/tfm in any way, and in particular you must
not change the key.

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

[toc] | [prev] | [next] | [standalone]

#1304860 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

On 01/08/2016 01:48 PM, Herbert Xu wrote:
> On Mon, Jan 04, 2016 at 01:33:54PM +0100, Milan Broz wrote:
>>
>> - hmac() is now failing the same way (SETKEY after accept())
>> (I initially tested without two patches above, these are not in linux-next yet.)
>> This breaks all cryptsetup TrueCrypt support (and moreover all systems if
>> kernel crypto API is set as a default vcrypto backend - but that's not default).
> 
> Yes algif_hash would need the same compatibility patch and I'm
> working on that.

Ok, I fixed this already.

> 
>> - cipher_null before worked without setkey, now it requires to set key
>> (either before or after accept().
>> This was actually probably bad workaround in cryptsetup, anyway it will now cause
>> old cryptsetup-reencrypt tool failing with your patches.
>> (Try "cryptsetup benchmark -c cipher_null-ecb". I am not sure what to do here,
>> but not requiring setkey for "cipher" that has no key internally seems ok for me...)
> 
> Is cipher_null actually used in production or is this just a
> benchmark? Using the kernel crypto API to perform no encryption
> sounds crazy.

Except benchmarks (I do not care about this) we use cipher_null in cryptsetup-reencrypt
when adding encryption in-place (plaintext-only device is converted
to LUKS, cipher_null is used during conversion for original plainext device,
then offline converted to some real cipher with key).
(It reuses the same logic as re-encryption, IOW volume key change.)

So it is used in production but just for this specific case.

Thanks,
Milan

[toc] | [prev] | [next] | [standalone]

#1305177 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

On Fri, Jan 08, 2016 at 07:21:12PM +0100, Milan Broz wrote:
>
> > Yes algif_hash would need the same compatibility patch and I'm
> > working on that.
> 
> Ok, I fixed this already.

Can you please test the two patches that I sent for algif_hash?

> So it is used in production but just for this specific case.

OK I will add something similar to the algif_hash has_setkey
logic for cipher_null.

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

[toc] | [prev] | [next] | [standalone]

#1305203 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

On 01/09/2016 06:41 AM, Herbert Xu wrote:
> On Fri, Jan 08, 2016 at 07:21:12PM +0100, Milan Broz wrote:
>>
>>> Yes algif_hash would need the same compatibility patch and I'm
>>> working on that.
>>
>> Ok, I fixed this already.
> 
> Can you please test the two patches that I sent for algif_hash?

I have stack of 7 patches on top of Linus' 4.4.0-rc8 tree, just to be sure
that I am testing the right versions, I created a git branch with
imported series here:
  http://git.kernel.org/cgit/linux/kernel/git/mbroz/linux.git/log/?h=crypto-api-compat

With these patches it works (except that cipher_null setkey issue).
I'll reply with tested-by to the patch thread.

> 
>> So it is used in production but just for this specific case.
> 
> OK I will add something similar to the algif_hash has_setkey
> logic for cipher_null.

Thanks!

Milan

[toc] | [prev] | [next] | [standalone]

#1306198 — [PATCH 1/2] crypto: skcipher - Add crypto_skcipher_has_setkey

This patch adds a way for skcipher users to determine whether a key
is required by a transform.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 7591928..d199c0b 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -118,6 +118,7 @@ static int crypto_init_skcipher_ops_blkcipher(struct crypto_tfm *tfm)
 	skcipher->decrypt = skcipher_decrypt_blkcipher;
 
 	skcipher->ivsize = crypto_blkcipher_ivsize(blkcipher);
+	skcipher->has_setkey = calg->cra_blkcipher.max_keysize;
 
 	return 0;
 }
@@ -210,6 +211,7 @@ static int crypto_init_skcipher_ops_ablkcipher(struct crypto_tfm *tfm)
 	skcipher->ivsize = crypto_ablkcipher_ivsize(ablkcipher);
 	skcipher->reqsize = crypto_ablkcipher_reqsize(ablkcipher) +
 			    sizeof(struct ablkcipher_request);
+	skcipher->has_setkey = calg->cra_ablkcipher.max_keysize;
 
 	return 0;
 }
diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index d8dd41f..fd8742a 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -61,6 +61,8 @@ struct crypto_skcipher {
 	unsigned int ivsize;
 	unsigned int reqsize;
 
+	bool has_setkey;
+
 	struct crypto_tfm base;
 };
 
@@ -305,6 +307,11 @@ static inline int crypto_skcipher_setkey(struct crypto_skcipher *tfm,
 	return tfm->setkey(tfm, key, keylen);
 }
 
+static inline bool crypto_skcipher_has_setkey(struct crypto_skcipher *tfm)
+{
+	return tfm->has_setkey;
+}
+
 /**
  * crypto_skcipher_reqtfm() - obtain cipher handle from request
  * @req: skcipher_request out of which the cipher handle is to be obtained
-- 
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

[toc] | [prev] | [next] | [standalone]

#1306215 — [PATCH 2/2] crypto: algif_skcipher - Add key check exception for cipher_null

This patch adds an exception to the key check so that cipher_null
users may continue to use algif_skcipher without setting a key.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 110bab4..4a5bdb6 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -978,7 +978,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 {
 	struct skcipher_tfm *tfm = private;
 
-	if (!tfm->has_key)
+	if (!tfm->has_key && crypto_skcipher_has_setkey(tfm->skcipher))
 		return -ENOKEY;
 
 	return skcipher_accept_parent_common(private, sk);
-- 
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

[toc] | [prev] | [next] | [standalone]

#1306358 — Re: [PATCH 2/2] crypto: algif_skcipher - Add key check exception for cipher_null

On 01/11/2016 02:29 PM, Herbert Xu wrote:
> This patch adds an exception to the key check so that cipher_null
> users may continue to use algif_skcipher without setting a key.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 110bab4..4a5bdb6 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -978,7 +978,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
>  {
>  	struct skcipher_tfm *tfm = private;
>  
> -	if (!tfm->has_key)
> +	if (!tfm->has_key && crypto_skcipher_has_setkey(tfm->skcipher))
>  		return -ENOKEY;
>  
>  	return skcipher_accept_parent_common(private, sk);

Reported-and-tested-by: Milan Broz <gmazyland@gmail.com>

Thanks,
Milan

[toc] | [prev] | [next] | [standalone]

#1304536 — [PATCH 1/2] crypto: hash - Add crypto_ahash_has_setkey

This patch adds a way for ahash users to determine whether a key
is required by a crypto_ahash transform.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 9c1dc8d..d19b523 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -451,6 +451,7 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
 	struct ahash_alg *alg = crypto_ahash_alg(hash);
 
 	hash->setkey = ahash_nosetkey;
+	hash->has_setkey = false;
 	hash->export = ahash_no_export;
 	hash->import = ahash_no_import;
 
@@ -463,8 +464,10 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
 	hash->finup = alg->finup ?: ahash_def_finup;
 	hash->digest = alg->digest;
 
-	if (alg->setkey)
+	if (alg->setkey) {
 		hash->setkey = alg->setkey;
+		hash->has_setkey = true;
+	}
 	if (alg->export)
 		hash->export = alg->export;
 	if (alg->import)
diff --git a/crypto/shash.c b/crypto/shash.c
index ecb1e3d..88a27de 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -355,8 +355,10 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm)
 	crt->finup = shash_async_finup;
 	crt->digest = shash_async_digest;
 
-	if (alg->setkey)
+	if (alg->setkey) {
 		crt->setkey = shash_async_setkey;
+		crt->has_setkey = true;
+	}
 	if (alg->export)
 		crt->export = shash_async_export;
 	if (alg->import)
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 3d69c93..6361892 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -204,6 +204,7 @@ struct crypto_ahash {
 		      unsigned int keylen);
 
 	unsigned int reqsize;
+	bool has_setkey;
 	struct crypto_tfm base;
 };
 
@@ -375,6 +376,11 @@ static inline void *ahash_request_ctx(struct ahash_request *req)
 int crypto_ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
 			unsigned int keylen);
 
+static inline bool crypto_ahash_has_setkey(struct crypto_ahash *tfm)
+{
+	return tfm->has_setkey;
+}
+
 /**
  * crypto_ahash_finup() - update and finalize message digest
  * @req: reference to the ahash_request handle that holds all information
-- 
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

[toc] | [prev] | [next] | [standalone]

#1304548 — [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2)

Hash implementations that require a key may crash if you use
them without setting a key.  This patch adds the necessary checks
so that if you do attempt to use them without a key that we return
-ENOKEY instead of proceeding.

This patch also adds a compatibility path to support old applications
that do acept(2) before setkey.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index b4c24fe..46637be 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -34,6 +34,11 @@ struct hash_ctx {
 	struct ahash_request req;
 };
 
+struct algif_hash_tfm {
+	struct crypto_ahash *hash;
+	bool has_key;
+};
+
 static int hash_sendmsg(struct socket *sock, struct msghdr *msg,
 			size_t ignored)
 {
@@ -235,22 +240,151 @@ static struct proto_ops algif_hash_ops = {
 	.accept		=	hash_accept,
 };
 
+static int hash_check_key(struct socket *sock)
+{
+	int err;
+	struct sock *psk;
+	struct alg_sock *pask;
+	struct algif_hash_tfm *tfm;
+	struct sock *sk = sock->sk;
+	struct alg_sock *ask = alg_sk(sk);
+
+	if (ask->refcnt)
+		return 0;
+
+	psk = ask->parent;
+	pask = alg_sk(ask->parent);
+	tfm = pask->private;
+
+	err = -ENOKEY;
+	lock_sock(psk);
+	if (!tfm->has_key)
+		goto unlock;
+
+	if (!pask->refcnt++)
+		sock_hold(psk);
+
+	ask->refcnt = 1;
+	sock_put(psk);
+
+	err = 0;
+
+unlock:
+	release_sock(psk);
+
+	return err;
+}
+
+static int hash_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
+			      size_t size)
+{
+	int err;
+
+	err = hash_check_key(sock);
+	if (err)
+		return err;
+
+	return hash_sendmsg(sock, msg, size);
+}
+
+static ssize_t hash_sendpage_nokey(struct socket *sock, struct page *page,
+				   int offset, size_t size, int flags)
+{
+	int err;
+
+	err = hash_check_key(sock);
+	if (err)
+		return err;
+
+	return hash_sendpage(sock, page, offset, size, flags);
+}
+
+static int hash_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
+			      size_t ignored, int flags)
+{
+	int err;
+
+	err = hash_check_key(sock);
+	if (err)
+		return err;
+
+	return hash_recvmsg(sock, msg, ignored, flags);
+}
+
+static int hash_accept_nokey(struct socket *sock, struct socket *newsock,
+			     int flags)
+{
+	int err;
+
+	err = hash_check_key(sock);
+	if (err)
+		return err;
+
+	return hash_accept(sock, newsock, flags);
+}
+
+static struct proto_ops algif_hash_ops_nokey = {
+	.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,
+	.setsockopt	=	sock_no_setsockopt,
+	.poll		=	sock_no_poll,
+
+	.release	=	af_alg_release,
+	.sendmsg	=	hash_sendmsg_nokey,
+	.sendpage	=	hash_sendpage_nokey,
+	.recvmsg	=	hash_recvmsg_nokey,
+	.accept		=	hash_accept_nokey,
+};
+
 static void *hash_bind(const char *name, u32 type, u32 mask)
 {
-	return crypto_alloc_ahash(name, type, mask);
+	struct algif_hash_tfm *tfm;
+	struct crypto_ahash *hash;
+
+	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
+	if (!tfm)
+		return ERR_PTR(-ENOMEM);
+
+	hash = crypto_alloc_ahash(name, type, mask);
+	if (IS_ERR(hash)) {
+		kfree(tfm);
+		return ERR_CAST(hash);
+	}
+
+	tfm->hash = hash;
+
+	return tfm;
 }
 
 static void hash_release(void *private)
 {
-	crypto_free_ahash(private);
+	struct algif_hash_tfm *tfm = private;
+
+	crypto_free_ahash(tfm->hash);
+	kfree(tfm);
 }
 
 static int hash_setkey(void *private, const u8 *key, unsigned int keylen)
 {
-	return crypto_ahash_setkey(private, key, keylen);
+	struct algif_hash_tfm *tfm = private;
+	int err;
+
+	err = crypto_ahash_setkey(tfm->hash, key, keylen);
+	tfm->has_key = !err;
+
+	return err;
 }
 
-static void hash_sock_destruct(struct sock *sk)
+static void hash_sock_destruct_common(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct hash_ctx *ctx = ask->private;
@@ -258,15 +392,40 @@ static void hash_sock_destruct(struct sock *sk)
 	sock_kzfree_s(sk, ctx->result,
 		      crypto_ahash_digestsize(crypto_ahash_reqtfm(&ctx->req)));
 	sock_kfree_s(sk, ctx, ctx->len);
+}
+
+static void hash_sock_destruct(struct sock *sk)
+{
+	hash_sock_destruct_common(sk);
 	af_alg_release_parent(sk);
 }
 
-static int hash_accept_parent(void *private, struct sock *sk)
+static void hash_release_parent_nokey(struct sock *sk)
+{
+	struct alg_sock *ask = alg_sk(sk);
+
+	if (!ask->refcnt) {
+		sock_put(ask->parent);
+		return;
+	}
+
+	af_alg_release_parent(sk);
+}
+
+static void hash_sock_destruct_nokey(struct sock *sk)
+{
+	hash_sock_destruct_common(sk);
+	hash_release_parent_nokey(sk);
+}
+
+static int hash_accept_parent_common(void *private, struct sock *sk)
 {
 	struct hash_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
-	unsigned len = sizeof(*ctx) + crypto_ahash_reqsize(private);
-	unsigned ds = crypto_ahash_digestsize(private);
+	struct algif_hash_tfm *tfm = private;
+	struct crypto_ahash *hash = tfm->hash;
+	unsigned len = sizeof(*ctx) + crypto_ahash_reqsize(hash);
+	unsigned ds = crypto_ahash_digestsize(hash);
 
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
@@ -286,7 +445,7 @@ static int hash_accept_parent(void *private, struct sock *sk)
 
 	ask->private = ctx;
 
-	ahash_request_set_tfm(&ctx->req, private);
+	ahash_request_set_tfm(&ctx->req, hash);
 	ahash_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
 				   af_alg_complete, &ctx->completion);
 
@@ -295,12 +454,38 @@ static int hash_accept_parent(void *private, struct sock *sk)
 	return 0;
 }
 
+static int hash_accept_parent(void *private, struct sock *sk)
+{
+	struct algif_hash_tfm *tfm = private;
+
+	if (!tfm->has_key && crypto_ahash_has_setkey(tfm->hash))
+		return -ENOKEY;
+
+	return hash_accept_parent_common(private, sk);
+}
+
+static int hash_accept_parent_nokey(void *private, struct sock *sk)
+{
+	int err;
+
+	err = hash_accept_parent_common(private, sk);
+	if (err)
+		goto out;
+
+	sk->sk_destruct = hash_sock_destruct_nokey;
+
+out:
+	return err;
+}
+
 static const struct af_alg_type algif_type_hash = {
 	.bind		=	hash_bind,
 	.release	=	hash_release,
 	.setkey		=	hash_setkey,
 	.accept		=	hash_accept_parent,
+	.accept_nokey	=	hash_accept_parent_nokey,
 	.ops		=	&algif_hash_ops,
+	.ops_nokey	=	&algif_hash_ops_nokey,
 	.name		=	"hash",
 	.owner		=	THIS_MODULE
 };
-- 
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

[toc] | [prev] | [next] | [standalone]

#1304565 — Re: [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2)

[Multipart message — attachments visible in raw view] — view raw

Hi Herbert,

[auto build test ERROR on crypto/master]
[also build test ERROR on v4.4-rc8 next-20160108]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Herbert-Xu/crypto-hash-Add-crypto_ahash_has_setkey/20160108-213436
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git master
config: x86_64-randconfig-i0-201601 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   crypto/algif_hash.c: In function 'hash_check_key':
>> crypto/algif_hash.c:252:9: error: 'struct alg_sock' has no member named 'refcnt'
     if (ask->refcnt)
            ^
   crypto/algif_hash.c:264:11: error: 'struct alg_sock' has no member named 'refcnt'
     if (!pask->refcnt++)
              ^
   crypto/algif_hash.c:267:5: error: 'struct alg_sock' has no member named 'refcnt'
     ask->refcnt = 1;
        ^
   crypto/algif_hash.c: In function 'hash_release_parent_nokey':
   crypto/algif_hash.c:407:10: error: 'struct alg_sock' has no member named 'refcnt'
     if (!ask->refcnt) {
             ^
   crypto/algif_hash.c: At top level:
>> crypto/algif_hash.c:486:2: error: unknown field 'accept_nokey' specified in initializer
     .accept_nokey = hash_accept_parent_nokey,
     ^
>> crypto/algif_hash.c:486:18: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     .accept_nokey = hash_accept_parent_nokey,
                     ^
   crypto/algif_hash.c:486:18: note: (near initialization for 'algif_type_hash.setauthsize')
>> crypto/algif_hash.c:488:2: error: unknown field 'ops_nokey' specified in initializer
     .ops_nokey = &algif_hash_ops_nokey,
     ^
   crypto/algif_hash.c:488:15: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     .ops_nokey = &algif_hash_ops_nokey,
                  ^
   crypto/algif_hash.c:488:15: note: (near initialization for 'algif_type_hash.owner')

vim +252 crypto/algif_hash.c

   246		struct sock *psk;
   247		struct alg_sock *pask;
   248		struct algif_hash_tfm *tfm;
   249		struct sock *sk = sock->sk;
   250		struct alg_sock *ask = alg_sk(sk);
   251	
 > 252		if (ask->refcnt)
   253			return 0;
   254	
   255		psk = ask->parent;
   256		pask = alg_sk(ask->parent);
   257		tfm = pask->private;
   258	
   259		err = -ENOKEY;
   260		lock_sock(psk);
   261		if (!tfm->has_key)
   262			goto unlock;
   263	
   264		if (!pask->refcnt++)
   265			sock_hold(psk);
   266	
 > 267		ask->refcnt = 1;
   268		sock_put(psk);
   269	
   270		err = 0;
   271	
   272	unlock:
   273		release_sock(psk);
   274	
   275		return err;
   276	}
   277	
   278	static int hash_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
   279				      size_t size)
   280	{
   281		int err;
   282	
   283		err = hash_check_key(sock);
   284		if (err)
   285			return err;
   286	
   287		return hash_sendmsg(sock, msg, size);
   288	}
   289	
   290	static ssize_t hash_sendpage_nokey(struct socket *sock, struct page *page,
   291					   int offset, size_t size, int flags)
   292	{
   293		int err;
   294	
   295		err = hash_check_key(sock);
   296		if (err)
   297			return err;
   298	
   299		return hash_sendpage(sock, page, offset, size, flags);
   300	}
   301	
   302	static int hash_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
   303				      size_t ignored, int flags)
   304	{
   305		int err;
   306	
   307		err = hash_check_key(sock);
   308		if (err)
   309			return err;
   310	
   311		return hash_recvmsg(sock, msg, ignored, flags);
   312	}
   313	
   314	static int hash_accept_nokey(struct socket *sock, struct socket *newsock,
   315				     int flags)
   316	{
   317		int err;
   318	
   319		err = hash_check_key(sock);
   320		if (err)
   321			return err;
   322	
   323		return hash_accept(sock, newsock, flags);
   324	}
   325	
   326	static struct proto_ops algif_hash_ops_nokey = {
   327		.family		=	PF_ALG,
   328	
   329		.connect	=	sock_no_connect,
   330		.socketpair	=	sock_no_socketpair,
   331		.getname	=	sock_no_getname,
   332		.ioctl		=	sock_no_ioctl,
   333		.listen		=	sock_no_listen,
   334		.shutdown	=	sock_no_shutdown,
   335		.getsockopt	=	sock_no_getsockopt,
   336		.mmap		=	sock_no_mmap,
   337		.bind		=	sock_no_bind,
   338		.setsockopt	=	sock_no_setsockopt,
   339		.poll		=	sock_no_poll,
   340	
   341		.release	=	af_alg_release,
   342		.sendmsg	=	hash_sendmsg_nokey,
   343		.sendpage	=	hash_sendpage_nokey,
   344		.recvmsg	=	hash_recvmsg_nokey,
   345		.accept		=	hash_accept_nokey,
   346	};
   347	
   348	static void *hash_bind(const char *name, u32 type, u32 mask)
   349	{
   350		struct algif_hash_tfm *tfm;
   351		struct crypto_ahash *hash;
   352	
   353		tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
   354		if (!tfm)
   355			return ERR_PTR(-ENOMEM);
   356	
   357		hash = crypto_alloc_ahash(name, type, mask);
   358		if (IS_ERR(hash)) {
   359			kfree(tfm);
   360			return ERR_CAST(hash);
   361		}
   362	
   363		tfm->hash = hash;
   364	
   365		return tfm;
   366	}
   367	
   368	static void hash_release(void *private)
   369	{
   370		struct algif_hash_tfm *tfm = private;
   371	
   372		crypto_free_ahash(tfm->hash);
   373		kfree(tfm);
   374	}
   375	
   376	static int hash_setkey(void *private, const u8 *key, unsigned int keylen)
   377	{
   378		struct algif_hash_tfm *tfm = private;
   379		int err;
   380	
   381		err = crypto_ahash_setkey(tfm->hash, key, keylen);
   382		tfm->has_key = !err;
   383	
   384		return err;
   385	}
   386	
   387	static void hash_sock_destruct_common(struct sock *sk)
   388	{
   389		struct alg_sock *ask = alg_sk(sk);
   390		struct hash_ctx *ctx = ask->private;
   391	
   392		sock_kzfree_s(sk, ctx->result,
   393			      crypto_ahash_digestsize(crypto_ahash_reqtfm(&ctx->req)));
   394		sock_kfree_s(sk, ctx, ctx->len);
   395	}
   396	
   397	static void hash_sock_destruct(struct sock *sk)
   398	{
   399		hash_sock_destruct_common(sk);
   400		af_alg_release_parent(sk);
   401	}
   402	
   403	static void hash_release_parent_nokey(struct sock *sk)
   404	{
   405		struct alg_sock *ask = alg_sk(sk);
   406	
 > 407		if (!ask->refcnt) {
   408			sock_put(ask->parent);
   409			return;
   410		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1305206 — Re: [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2)

On 01/08/2016 02:31 PM, Herbert Xu wrote:
> Hash implementations that require a key may crash if you use
> them without setting a key.  This patch adds the necessary checks
> so that if you do attempt to use them without a key that we return
> -ENOKEY instead of proceeding.
> 
> This patch also adds a compatibility path to support old applications
> that do acept(2) before setkey.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Reported-and-tested-by: Milan Broz <gmazyland@gmail.com>

(tested both patches, ditto for previous skcipher series)

Thanks,
Milan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web