general protection fault in crypto_remove_spawns

[syzbot]

Hello,

syzkaller hit the following crash on
1ea8d039f9edcfefb20d8ddfe136930f6e551529
git://git.cmpxchg.org/linux-mmots.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.
C reproducer is attached
syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers


kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
(ftrace buffer empty)
Modules linked in:
CPU: 0 PID: 25985 Comm: cryptomgr_test Not tainted 4.14.0-mm1+ #25
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
task: ffff8801c4562180 task.stack: ffff8801d5b70000
RIP: 0010:crypto_remove_spawns+0x58c/0x1260 crypto/algapi.c:159
RSP: 0018:ffff8801d5b779e8 EFLAGS: 00010206
RAX: 0000000000000003 RBX: dffffc0000000000 RCX: ffffffff82258aab
RDX: 0000000000000000 RSI: 1ffff1003ab6efa6 RDI: 0000000000000018
RBP: ffff8801d5b77dd8 R08: ffff8801d5b77d70 R09: 0000000000000004
R10: 0000000000000000 R11: ffffffff8747dda0 R12: 0000000000000000
R13: ffff8801c505bb60 R14: ffffed003ab6ef4e R15: ffff8801d5b77db0
FS: 0000000000000000(0000) GS:ffff8801db400000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fff1d3ffcac CR3: 00000001cf825000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
crypto_alg_tested+0x514/0x6f0 crypto/algapi.c:311
cryptomgr_test+0x17/0x30 crypto/algboss.c:226
kthread+0x37a/0x440 kernel/kthread.c:238
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:437
Code: 84 e3 01 00 00 e8 35 94 4a ff 4c 89 e8 48 c1 e8 03 80 3c 18 00 0f 85
d8 09 00 00 4d 8b 65 00 49 8d 7c 24 18 48 89 f8 48 c1 e8 03 <80> 3c 18 00
0f 85 b4 09 00 00 4d 8b 6c 24 18 4c 3b ad 50 fc ff
RIP: crypto_remove_spawns+0x58c/0x1260 crypto/algapi.c:159 RSP:
ffff8801d5b779e8
---[ end trace 14ce8f86fe2873b1 ]---
Kernel panic - not syncing: Fatal exception
Dumping ftrace buffer:
(ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 86400 seconds..


---
This bug is generated by a dumb bot. It may contain errors.
See https://goo.gl/tpsmEJ for details.
Direct all questions to syzk...@googlegroups.com.
Please credit me with: Reported-by: syzbot <syzk...@googlegroups.com>

syzbot will keep track of this bug report.
Once a fix for this bug is committed, please reply to this email with:
#syz fix: exact-commit-title
If you want to test a patch for this bug, please reply with:
#syz test: git://repo/address.git branch
and provide the patch inline or as an attachment.
To mark this as a duplicate of another syzbot report, please reply with:
#syz dup: exact-subject-of-another-report
If it's a one-off invalid bug report, please reply with:
#syz invalid
Note: if the crash happens again, it will cause creation of a new bug
report.
Note: all commands must start from beginning of the line in the email body.

[Stephan Müller]

Am Montag, 27. November 2017, 19:56:46 CET schrieb syzbot:

Hi Herbert,

The issue seems to trigger a bug whose results we have seen before. When
starting the reproducer and stopping it shortly thereafter, I see the numerous
identical entries in /proc/crypto:

name : cmac(des3_ede)
driver : cmac(des3_ede-asm)
module : kernel
priority : 200
refcnt : 1
selftest : passed
internal : no
type : shash
blocksize : 8
digestsize : 8

name : cmac(des3_ede)
driver : cmac(des3_ede-asm)
module : kernel
priority : 200
refcnt : 1
selftest : passed
internal : no
type : shash
blocksize : 8
digestsize : 8

name : cmac(des3_ede)
driver : cmac(des3_ede-asm)
module : kernel
priority : 200
refcnt : 1
selftest : passed
internal : no
type : shash
blocksize : 8
digestsize : 8

...

And this list keeps on growing without end:

# ./repro

# less /proc/crypto | wc
9559 26456 188754

# ./repro

# less /proc/crypto | wc
11440 31586 226032

At one point in time I think the system simply has too many entries.

Ciao
Stephan

[Stephan Müller]

Hi Herbert,

you see the reported problem by simply using

sa.salg_mask = 0xffffffff;

Note, I am not fully sure about whether CRYPTO_AF_ALG_ALLOWED_MASK and
CRYPTO_AF_ALG_ALLOWED_TYPE have the correct value. But I think that all
that user space should reach is potentially the ASYNC flag and the
cipher types flags.

---8<---

The user space interface allows specifying the type and the mask field
used to allocate the cipher. Only a subset of the type and mask is
considered relevant to be set by user space if needed at all.

This fixes a bug where user space is able to cause one cipher to be
registered multiple times potentially exhausting kernel memory.

Reported-by: syzbot <syzk...@googlegroups.com>
Cc: <sta...@vger.kernel.org>
Signed-off-by: Stephan Mueller <smue...@chronox.de>
---
crypto/af_alg.c | 7 +++++++
crypto/algif_aead.c | 2 ++
crypto/algif_hash.c | 2 ++
crypto/algif_rng.c | 2 ++
crypto/algif_skcipher.c | 2 ++
include/crypto/if_alg.h | 1 +
include/linux/crypto.h | 3 +++
7 files changed, 19 insertions(+)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 1e5353f62067..16cfbde64048 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -1172,6 +1172,13 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, int flags,
}
EXPORT_SYMBOL_GPL(af_alg_get_rsgl);

+void af_alg_restrict_type_mask(u32 *type, u32 *mask)
+{
+ *type &= CRYPTO_AF_ALG_ALLOWED_TYPE;
+ *mask &= CRYPTO_AF_ALG_ALLOWED_MASK;
+}
+EXPORT_SYMBOL_GPL(af_alg_restrict_type_mask);
+
static int __init af_alg_init(void)
{
int err = proto_register(&alg_proto, 0);
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 9d73be28cf01..5d21db83bdfd 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -463,6 +463,8 @@ static void *aead_bind(const char *name, u32 type, u32 mask)
if (!tfm)
return ERR_PTR(-ENOMEM);

+ af_alg_restrict_type_mask(&type, &mask);
+
aead = crypto_alloc_aead(name, type, mask);
if (IS_ERR(aead)) {
kfree(tfm);
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 76d2e716c792..f7660e80cd05 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -419,6 +419,8 @@ static void *hash_bind(const char *name, u32 type, u32 mask)
if (!tfm)
return ERR_PTR(-ENOMEM);

+ af_alg_restrict_type_mask(&type, &mask);
+
hash = crypto_alloc_ahash(name, type, mask);
if (IS_ERR(hash)) {
kfree(tfm);
diff --git a/crypto/algif_rng.c b/crypto/algif_rng.c
index 150c2b6480ed..33a7064996f2 100644
--- a/crypto/algif_rng.c
+++ b/crypto/algif_rng.c
@@ -116,6 +116,8 @@ static struct proto_ops algif_rng_ops = {

static void *rng_bind(const char *name, u32 type, u32 mask)
{
+ af_alg_restrict_type_mask(&type, &mask);
+
return crypto_alloc_rng(name, type, mask);
}

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 9954b078f0b9..0a4987aa9d5c 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -314,6 +314,8 @@ static void *skcipher_bind(const char *name, u32 type, u32 mask)
if (!tfm)
return ERR_PTR(-ENOMEM);

+ af_alg_restrict_type_mask(&type, &mask);
+
skcipher = crypto_alloc_skcipher(name, type, mask);
if (IS_ERR(skcipher)) {
kfree(tfm);
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 6abf0a3604dc..8ade69d46025 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -250,5 +250,6 @@ struct af_alg_async_req *af_alg_alloc_areq(struct sock *sk,
int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, int flags,
struct af_alg_async_req *areq, size_t maxsize,
size_t *outlen);
+void af_alg_restrict_type_mask(u32 *type, u32 *mask);

#endif /* _CRYPTO_IF_ALG_H */
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 78508ca4b108..0d7694673fff 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -70,6 +70,9 @@
#define CRYPTO_ALG_DYING 0x00000040
#define CRYPTO_ALG_ASYNC 0x00000080

+#define CRYPTO_AF_ALG_ALLOWED_MASK 0x000000ff
+#define CRYPTO_AF_ALG_ALLOWED_TYPE 0x000000ff
+
/*
* Set this bit if and only if the algorithm requires another algorithm of
* the same type to handle corner cases.
--
2.14.3

[Eric Biggers]

Hi Stephan,

On Tue, Dec 12, 2017 at 07:09:08AM +0100, Stephan Müller wrote:
> Hi Herbert,
>
> you see the reported problem by simply using
>
> sa.salg_mask = 0xffffffff;
>
> Note, I am not fully sure about whether CRYPTO_AF_ALG_ALLOWED_MASK and
> CRYPTO_AF_ALG_ALLOWED_TYPE have the correct value. But I think that all
> that user space should reach is potentially the ASYNC flag and the
> cipher types flags.
>
> ---8<---
>
> The user space interface allows specifying the type and the mask field
> used to allocate the cipher. Only a subset of the type and mask is
> considered relevant to be set by user space if needed at all.
>
> This fixes a bug where user space is able to cause one cipher to be
> registered multiple times potentially exhausting kernel memory.
>
> Reported-by: syzbot <syzk...@googlegroups.com>
> Cc: <sta...@vger.kernel.org>
> Signed-off-by: Stephan Mueller <smue...@chronox.de>

The syzkaller reproducer triggered a crash in crypto_remove_spawns(). Is it
possible the bug is still there somewhere, while this patch just makes it
inaccessible through AF_ALG?

Anyway, we definitely should expose as few algorithm flags to AF_ALG as
possible. There are just way too many things that can go wrong with exposing
arbitrary flags.

However, why do the check in every af_alg_type.bind() method instead of just
once in alg_bind()?

If it can be done without breaking users, it also would be nice if we would
actually validate the flags and return -EINVAL if unknown flags are specified.
Otherwise users cannot test for whether specific flags are supported.

Also, note that even after this fix there are still ways to register an
arbitrarily large number of algorithms. There are two classes of problems.

First, it can happen that a template gets instantiated for a request but the
resulting algorithm does not exactly match the original request, so making the
same request again will instantiate the template again. This could happen by
specifically requesting an untested algorithm (type=0, mask=CRYPTO_ALG_TESTED),
which your patch fixes. However this can also happen in cases where neither the
final ->cra_name nor the final ->cra_driver_name matches what was requested.
For example asking for "cryptd(sha1)" results in .cra_name = "sha1" and
.cra_driver_name = "cryptd(sha1-avx2)", or asking for "xts(ecb(aes))" results in
.cra_name = "xts(aes)" and .cra_driver_name = "xts(ecb-aes-aesni)".

Probably the crypto API needs to be taught how to find the instantiated
templates correctly.

Second, you can just keep choosing different combinations of algorithms when
instantiating templates, taking advantage of the fact that templates can be
nested and some take multiple parameters, so the number of possible combinations
grows exponentially. I don't know how to easily solve this. Perhaps
crypto_free_skcipher(), crypto_free_ahash(), etc. should unregister the
algorithm if it was created from a template and nothing else is using it; then
the number of algorithms someone could instantiate via AF_ALG at a given time
would be limited by their number of file descriptors.

Eric

[Stephan Mueller]

Am Dienstag, 12. Dezember 2017, 09:57:37 CET schrieb Eric Biggers:

Hi Eric,

> Hi Stephan,
>
> On Tue, Dec 12, 2017 at 07:09:08AM +0100, Stephan Müller wrote:
> > Hi Herbert,
> >
> > you see the reported problem by simply using
> >
> > sa.salg_mask = 0xffffffff;
> >
> > Note, I am not fully sure about whether CRYPTO_AF_ALG_ALLOWED_MASK and
> > CRYPTO_AF_ALG_ALLOWED_TYPE have the correct value. But I think that all
> > that user space should reach is potentially the ASYNC flag and the
> > cipher types flags.
> >
> > ---8<---
> >
> > The user space interface allows specifying the type and the mask field
> > used to allocate the cipher. Only a subset of the type and mask is
> > considered relevant to be set by user space if needed at all.
> >
> > This fixes a bug where user space is able to cause one cipher to be
> > registered multiple times potentially exhausting kernel memory.
> >
> > Reported-by: syzbot <syzk...@googlegroups.com>
> > Cc: <sta...@vger.kernel.org>
> > Signed-off-by: Stephan Mueller <smue...@chronox.de>
>
> The syzkaller reproducer triggered a crash in crypto_remove_spawns(). Is it
> possible the bug is still there somewhere, while this patch just makes it
> inaccessible through AF_ALG?

I think the issue is that the syzkaller generates a vast amount of registered
ciphers. At one point in time, I would think that some implied limit is
overflown. But I cannot say for sure.

Yet, it is definitely a bug to have more than one instance of the same cipher
implementation registered.

>
> Anyway, we definitely should expose as few algorithm flags to AF_ALG as
> possible. There are just way too many things that can go wrong with
> exposing arbitrary flags.

Absolutely, I would even say that we should not expose any mask/type at all.
I.e. the patch I offered here should be changed to set the mask/type to zero
in all cases.

>
> However, why do the check in every af_alg_type.bind() method instead of just
> once in alg_bind()?

You are quite right, that is the right place to add this code as it contains
already some verification there.

>
> If it can be done without breaking users, it also would be nice if we would
> actually validate the flags and return -EINVAL if unknown flags are
> specified. Otherwise users cannot test for whether specific flags are
> supported.

If we (and we need to hear Herbert) conclude that these values should not be
exposed in the first place, I think we should not return any error but simply
set it to zero.

If Herbert concludes that some flags are necessary, we should build a white-
list and return an error for any flag that is not in the white list.

>
> Also, note that even after this fix there are still ways to register an
> arbitrarily large number of algorithms. There are two classes of problems.
>
> First, it can happen that a template gets instantiated for a request but the
> resulting algorithm does not exactly match the original request, so making
> the same request again will instantiate the template again. This could
> happen by specifically requesting an untested algorithm (type=0,
> mask=CRYPTO_ALG_TESTED), which your patch fixes. However this can also
> happen in cases where neither the final ->cra_name nor the final
> ->cra_driver_name matches what was requested. For example asking for
> "cryptd(sha1)" results in .cra_name = "sha1" and .cra_driver_name =
> "cryptd(sha1-avx2)", or asking for "xts(ecb(aes))" results in .cra_name =
> "xts(aes)" and .cra_driver_name = "xts(ecb-aes-aesni)".
>
> Probably the crypto API needs to be taught how to find the instantiated
> templates correctly.

Maybe a name mangling should be removed. A template/cipher has only two names,
period. Either you use exactly these names or you will not find a cipher.

>
> Second, you can just keep choosing different combinations of algorithms when
> instantiating templates, taking advantage of the fact that templates can be
> nested and some take multiple parameters, so the number of possible
> combinations grows exponentially. I don't know how to easily solve this.
> Perhaps crypto_free_skcipher(), crypto_free_ahash(), etc. should unregister
> the algorithm if it was created from a template and nothing else is using
> it; then the number of algorithms someone could instantiate via AF_ALG at a
> given time would be limited by their number of file descriptors.

There could be a large set of permutations of ciphers, I agree. However, do
you think that in case all of them are registered, we have an issue? The goal
is that if one template/cipher combo is registered once, any subsequent
allocation of that combo should reuse the registered instance.

PS: The cipher allocation function has another long-standing bug which could
be viewed as a DoS via AF_ALG: Assume you do not yet have gcm(aes) allocated.
Now, AF_ALG allocates gcm_base(ctr(aes), ghash), the registered cipher
instance will have *both*, the name and the driver name to be set to
gcm_base(ctr(aes), ghash). Any subsequent allocation of gcm(aes) (e.g. by
IPSEC) will fail with -ENOENT even though the cipher is allocated. Note,
gcm(aes) here is only an example -- this issue is a general problem.


Ciao
Stephan

[Stephan Müller]

The user space interface allows specifying the type and the mask field

used to allocate the cipher. As user space can precisely select the
desired cipher by using either the name or the driver name, additional
selection options for cipher are not considered necessary and relevant
for user space.

This fixes a bug where user space is able to cause one cipher to be
registered multiple times potentially exhausting kernel memory.

Reported-by: syzbot <syzk...@googlegroups.com>
Cc: <sta...@vger.kernel.org>
Signed-off-by: Stephan Mueller <smue...@chronox.de>
---

crypto/af_alg.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 1e5353f62067..4f4cfc5a7ef3 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -150,7 +150,6 @@ EXPORT_SYMBOL_GPL(af_alg_release_parent);

static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
{
- const u32 forbidden = CRYPTO_ALG_INTERNAL;
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct sockaddr_alg *sa = (void *)uaddr;
@@ -176,9 +175,12 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
if (IS_ERR(type))
return PTR_ERR(type);

- private = type->bind(sa->salg_name,
- sa->salg_feat & ~forbidden,
- sa->salg_mask & ~forbidden);
+ /*
+ * The use of the salg_feat and salg_mask are forbidden as they expose
+ * too much of the low-level handling which is not suitable for
+ * hostile code.
+ */
+ private = type->bind(sa->salg_name, 0, 0);
if (IS_ERR(private)) {
module_put(type->owner);
return PTR_ERR(private);
--
2.14.3

[Herbert Xu]

On Tue, Dec 19, 2017 at 07:25:04AM +0100, Stephan Müller wrote:
> The user space interface allows specifying the type and the mask field
> used to allocate the cipher. As user space can precisely select the
> desired cipher by using either the name or the driver name, additional
> selection options for cipher are not considered necessary and relevant
> for user space.
>
> This fixes a bug where user space is able to cause one cipher to be
> registered multiple times potentially exhausting kernel memory.
>
> Reported-by: syzbot <syzk...@googlegroups.com>
> Cc: <sta...@vger.kernel.org>
> Signed-off-by: Stephan Mueller <smue...@chronox.de>

This will break users of CRYPTO_ALG_KERN_DRIVER_ONLY. I think
we should add CRYPTO_ALG_TESTED to the blacklist since there is
no sane reason to use it here.

Most other problems however would be bugs in the template code.
The first thing a template does when it creates an instance is
to check whether the resulting algorithm would fulfil the requested
type/mask using crypto_check_attr_type. So if that's not working
then we should fix it there as it may also be triggered via other
code paths that can create instances.

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

[Stephan Mueller]

Am Freitag, 22. Dezember 2017, 08:36:07 CET schrieb Herbert Xu:

Hi Herbert,

> On Tue, Dec 19, 2017 at 07:25:04AM +0100, Stephan Müller wrote:
> > The user space interface allows specifying the type and the mask field
> > used to allocate the cipher. As user space can precisely select the
> > desired cipher by using either the name or the driver name, additional
> > selection options for cipher are not considered necessary and relevant
> > for user space.
> >
> > This fixes a bug where user space is able to cause one cipher to be
> > registered multiple times potentially exhausting kernel memory.
> >
> > Reported-by: syzbot <syzk...@googlegroups.com>
> > Cc: <sta...@vger.kernel.org>
> > Signed-off-by: Stephan Mueller <smue...@chronox.de>
>
> This will break users of CRYPTO_ALG_KERN_DRIVER_ONLY. I think
> we should add CRYPTO_ALG_TESTED to the blacklist since there is
> no sane reason to use it here.

Shouldn't we then rather use a white list instead of a black list?

>
> Most other problems however would be bugs in the template code.
> The first thing a template does when it creates an instance is
> to check whether the resulting algorithm would fulfil the requested
> type/mask using crypto_check_attr_type. So if that's not working
> then we should fix it there as it may also be triggered via other
> code paths that can create instances.

I think I understand that, but does user space need to utilize this logic?
>
> Thanks,



Ciao
Stephan

[Herbert Xu]

On Fri, Dec 22, 2017 at 08:41:10AM +0100, Stephan Mueller wrote:
>
> Shouldn't we then rather use a white list instead of a black list?
> >
> > Most other problems however would be bugs in the template code.
> > The first thing a template does when it creates an instance is
> > to check whether the resulting algorithm would fulfil the requested
> > type/mask using crypto_check_attr_type. So if that's not working
> > then we should fix it there as it may also be triggered via other
> > code paths that can create instances.
>
> I think I understand that, but does user space need to utilize this logic?

I'm open to a white list as well. But we should certainly fix
the underlying bugs rather than just papering over them as they
could come back via other channels.

Cheers,

[Eric Biggers]

From: Eric Biggers <ebig...@google.com>

syzkaller triggered a NULL pointer dereference in crypto_remove_spawns()
via a program that repeatedly and concurrently requests AEADs
"authenc(cmac(des3_ede-asm),pcbc-aes-aesni)" and hashes "cmac(des3_ede)"
through AF_ALG, where the hashes are requested as "untested"
(CRYPTO_ALG_TESTED is set in ->salg_mask but clear in ->salg_feat; this
causes the template to be instantiated for every request).

Although AF_ALG users really shouldn't be able to request an "untested"
algorithm, the NULL pointer dereference is actually caused by a
longstanding race condition where crypto_remove_spawns() can encounter
an instance which has had spawn(s) "grabbed" but hasn't yet been
registered, resulting in ->cra_users still being NULL.

We probably should properly initialize ->cra_users earlier, but that
would require updating many templates individually. For now just fix
the bug in a simple way that can easily be backported: make
crypto_remove_spawns() treat a NULL ->cra_users list as empty.

Reported-by: syzbot <syzk...@googlegroups.com>
Cc: sta...@vger.kernel.org

Signed-off-by: Eric Biggers <ebig...@google.com>
---
crypto/algapi.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 9895cafcce7e..395b082d03a9 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -166,6 +166,18 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,

spawn->alg = NULL;
spawns = &inst->alg.cra_users;
+
+ /*
+ * We may encounter an unregistered instance here, since
+ * an instance's spawns are set up prior to the instance
+ * being registered. An unregistered instance will have
+ * NULL ->cra_users.next, since ->cra_users isn't
+ * properly initialized until registration. But an
+ * unregistered instance cannot have any users, so treat
+ * it the same as ->cra_users being empty.
+ */
+ if (spawns->next == NULL)
+ break;
}
} while ((spawns = crypto_more_spawns(alg, &stack, &top,
&secondary_spawns)));
--
2.15.1

[Stephan Müller]

The user space interface allows specifying the type and mask field used
to allocate the cipher. Only a subset of the possible flags are intended
for user space. Therefore, white-list the allowed flags.

In case the user space caller uses at least one non-allowed flag, EINVAL
is returned.

Signed-off-by: Stephan Mueller <smue...@chronox.de>
---
crypto/af_alg.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c

index 35d4dcea381f..5231f421ad00 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -150,7 +150,7 @@ EXPORT_SYMBOL_GPL(af_alg_release_parent);

static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
{
- const u32 forbidden = CRYPTO_ALG_INTERNAL;

+ const u32 allowed = CRYPTO_ALG_KERN_DRIVER_ONLY;

struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct sockaddr_alg *sa = (void *)uaddr;

@@ -158,6 +158,10 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
void *private;
int err;

+ /* If caller uses non-allowed flag, return error. */
+ if ((sa->salg_feat & ~allowed) || (sa->salg_mask & ~allowed))
+ return -EINVAL;
+
if (sock->state == SS_CONNECTED)
return -EINVAL;

@@ -176,9 +180,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)

if (IS_ERR(type))
return PTR_ERR(type);

- private = type->bind(sa->salg_name,
- sa->salg_feat & ~forbidden,
- sa->salg_mask & ~forbidden);

+ private = type->bind(sa->salg_name, sa->salg_feat, sa->salg_mask);

[Stephan Müller]

Hi,

sorry, I forgot the right tags.

---8<---

The user space interface allows specifying the type and mask field used
to allocate the cipher. Only a subset of the possible flags are intended
for user space. Therefore, white-list the allowed flags.

In case the user space caller uses at least one non-allowed flag, EINVAL
is returned.

Reported-by: syzbot <syzk...@googlegroups.com>
Cc: <sta...@vger.kernel.org>

[Herbert Xu]

On Fri, Dec 29, 2017 at 02:30:19PM -0600, Eric Biggers wrote:
> From: Eric Biggers <ebig...@google.com>
>
> syzkaller triggered a NULL pointer dereference in crypto_remove_spawns()
> via a program that repeatedly and concurrently requests AEADs
> "authenc(cmac(des3_ede-asm),pcbc-aes-aesni)" and hashes "cmac(des3_ede)"
> through AF_ALG, where the hashes are requested as "untested"
> (CRYPTO_ALG_TESTED is set in ->salg_mask but clear in ->salg_feat; this
> causes the template to be instantiated for every request).
>
> Although AF_ALG users really shouldn't be able to request an "untested"
> algorithm, the NULL pointer dereference is actually caused by a
> longstanding race condition where crypto_remove_spawns() can encounter
> an instance which has had spawn(s) "grabbed" but hasn't yet been
> registered, resulting in ->cra_users still being NULL.
>
> We probably should properly initialize ->cra_users earlier, but that
> would require updating many templates individually. For now just fix
> the bug in a simple way that can easily be backported: make
> crypto_remove_spawns() treat a NULL ->cra_users list as empty.
>
> Reported-by: syzbot <syzk...@googlegroups.com>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Eric Biggers <ebig...@google.com>

Patch applied. Thanks.

[Herbert Xu]

On Tue, Jan 02, 2018 at 08:55:25AM +0100, Stephan Müller wrote:
> Hi,
>
> sorry, I forgot the right tags.
>
> ---8<---
>
> The user space interface allows specifying the type and mask field used
> to allocate the cipher. Only a subset of the possible flags are intended
> for user space. Therefore, white-list the allowed flags.
>
> In case the user space caller uses at least one non-allowed flag, EINVAL
> is returned.
>
> Reported-by: syzbot <syzk...@googlegroups.com>
> Cc: <sta...@vger.kernel.org>
> Signed-off-by: Stephan Mueller <smue...@chronox.de>

Patch applied. Thanks.

[Eric Biggers]

Fix for the actual crash now is in Linus' tree:

#syz fix: crypto: algapi - fix NULL dereference in crypto_remove_spawns()