Re: UBSAN: array-index-out-of-bounds in alg_bind

Re: UBSAN: array-index-out-of-bounds in alg_bind

[Jann Horn]

+linux-api because this is about fixing UAPI without breaking userspace

On Sat, Oct 17, 2020 at 8:02 AM Kees Cook <keescook@chromium.org> wrote:
> On Fri, Oct 16, 2020 at 01:12:24AM -0700, syzbot wrote:
> > dashboard link: https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e
> > [...]
> > Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com
> > [...]
> > UBSAN: array-index-out-of-bounds in crypto/af_alg.c:166:2
> > index 91 is out of range for type '__u8 [64]'
>
> This seems to be an "as intended", if very odd. false positive (the actual
> memory area is backed by the on-stack _K_SS_MAXSIZE-sized sockaddr_storage
> "address" variable in __sys_bind. But yes, af_alg's salg_name member
> size here doesn't make sense. The origin appears to be that 3f69cc60768b
> ("crypto: af_alg - Allow arbitrarily long algorithm names") intentionally
> didn't extend the kernel structure (which is actually just using the UAPI
> structure). I don't see a reason the UAPI couldn't have been extended:
> it's a sockaddr implementation, so the size is always passed in as part
> of the existing API.

If you e.g. recompiled the wrong parts of the "btcheck" project with
such changed UAPI headers, I think you'd get OOB writes, because they
have this in a header
(https://sources.debian.org/src/btcheck/2.1-4/src/kernelcryptoapi.h/?hl=29#L29):

typedef struct {
  struct sockaddr_alg sa;
  int safd;
  int fd;
} lkca_hash_ctx;

so if you rebuilt e.g. kernelcryptoapi.o (which uses the struct)
without also rebuilding hash.o (which allocates the struct), code in
kernelcryptoapi.o would write beyond the end of lkca_hash_ctx, I
think.

Sure, there aren't many places that do this kind of thing for this
struct. But at least in theory, you can't change the size of UAPI
structs because someone might be passing instances of that struct
around between object files.

> At the very least the kernel needs to switch to using a correctly-sized
> structure: I expected UBSAN_BOUNDS to be enabled globally by default at
> some point in the future (with the minimal runtime -- the
> instrumentation is tiny and catches real issues).

Yeah, the kernel should probably use a struct that looks different
from the userspace one. :/ I guess we'll probably end up with some
ugly hack with "#ifdef __KERNEL__", where the same struct has
different sizes between kernel and userspace? Or am I being too
puritan about UAPI consistency?

> Reproduction:
>
> struct sockaddr_alg sa = {
>     .salg_family = AF_ALG,
>     .salg_type = "skcipher",
>     .salg_name = "cbc(aes)"
> };
> fd = socket(AF_ALG, SOCK_SEQPACKET, 0);
> bind(fd, (void *)&sa, sizeof(sa));
>
> Replace "sizeof(sa)" with x where 64<x<=128.

I think you mean 88<x<=128 ?

Re: UBSAN: array-index-out-of-bounds in alg_bind

[Jann Horn]

On Sat, Oct 17, 2020 at 12:50 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> On Sat, Oct 17, 2020 at 5:49 AM Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Oct 16, 2020 at 01:12:24AM -0700, syzbot wrote:
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e
> > > [...]
> > > Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com
> > > [...]
> > > UBSAN: array-index-out-of-bounds in crypto/af_alg.c:166:2
> > > index 91 is out of range for type '__u8 [64]'
> >
> > This seems to be an "as intended", if very odd. false positive (the actual
> > memory area is backed by the on-stack _K_SS_MAXSIZE-sized sockaddr_storage
> > "address" variable in __sys_bind. But yes, af_alg's salg_name member
> > size here doesn't make sense.
>
> As Vegard noted elsewhere, compilers can start making assumptions
> based on absence of UB and compile code in surprising ways as the
> result leading to very serious and very real bugs.
>
> One example would be a compiler generating jump table for common sizes
> during PGO and leaving size > 64 as wild jump.
>
> Another example would be a compiler assuming that copy size <= 64.
> Then if there is another copy into a 64-byte buffer with a proper size
> check, the compiler can now drop that size check (since it now knows
> size <= 64) and we get real stack smash (for a copy that does have a
> proper size check before!).

FWIW, the kernel currently still has a bunch of places that use
C89-style length-1 arrays (which were in the past used to work around
C89's lack of proper flexible arrays). Gustavo A. R. Silva has a bunch
of patches pending to change those places now, but those are not
marked for stable backporting; so in all currently released kernels,
we'll probably keep having length-1 arrays at the ends of C structs
that are used as if they were flexible arrays. (Unless someone makes
the case that these patches are not just cleanups but actually fix
some sort of real bug, and therefore need to be backported.)

The code in this example looks just like one of those C89-style
length-1 arrays to me (except that the length isn't 1).

Of course I do agree that this should be cleaned up, and that having
bogus array lengths in the source code is a bad idea.

> And we do want compilers to be that smart today. Because of all levels
> of abstractions/macros/inlining we actually have lots of
> redundant/nonsensical code in the end after all inlining and
> expansions, and we do want compilers to infer things, remove redundant
> checks, etc so that we can have both nice abstract source code and
> efficient machine code at the same time.

I guess that kinda leads to the question: Do we just need to fix the
kernel code here (which is comparatively easy), or do you think that
this is a sufficiently big problem that we need to go and somehow
change the actual UAPI headers here (e.g. by deprecating the existing
UAPI struct and making a new one with a different name)?

Re: UBSAN: array-index-out-of-bounds in alg_bind

[Dmitry Vyukov]

On Sat, Oct 17, 2020 at 1:02 PM Jann Horn <jannh@google.com> wrote:
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e
> > > > [...]
> > > > Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com
> > > > [...]
> > > > UBSAN: array-index-out-of-bounds in crypto/af_alg.c:166:2
> > > > index 91 is out of range for type '__u8 [64]'
> > >
> > > This seems to be an "as intended", if very odd. false positive (the actual
> > > memory area is backed by the on-stack _K_SS_MAXSIZE-sized sockaddr_storage
> > > "address" variable in __sys_bind. But yes, af_alg's salg_name member
> > > size here doesn't make sense.
> >
> > As Vegard noted elsewhere, compilers can start making assumptions
> > based on absence of UB and compile code in surprising ways as the
> > result leading to very serious and very real bugs.
> >
> > One example would be a compiler generating jump table for common sizes
> > during PGO and leaving size > 64 as wild jump.
> >
> > Another example would be a compiler assuming that copy size <= 64.
> > Then if there is another copy into a 64-byte buffer with a proper size
> > check, the compiler can now drop that size check (since it now knows
> > size <= 64) and we get real stack smash (for a copy that does have a
> > proper size check before!).
>
> FWIW, the kernel currently still has a bunch of places that use
> C89-style length-1 arrays (which were in the past used to work around
> C89's lack of proper flexible arrays). Gustavo A. R. Silva has a bunch
> of patches pending to change those places now, but those are not
> marked for stable backporting; so in all currently released kernels,
> we'll probably keep having length-1 arrays at the ends of C structs
> that are used as if they were flexible arrays. (Unless someone makes
> the case that these patches are not just cleanups but actually fix
> some sort of real bug, and therefore need to be backported.)
>
> The code in this example looks just like one of those C89-style
> length-1 arrays to me (except that the length isn't 1).
>
> Of course I do agree that this should be cleaned up, and that having
> bogus array lengths in the source code is a bad idea.
>
> > And we do want compilers to be that smart today. Because of all levels
> > of abstractions/macros/inlining we actually have lots of
> > redundant/nonsensical code in the end after all inlining and
> > expansions, and we do want compilers to infer things, remove redundant
> > checks, etc so that we can have both nice abstract source code and
> > efficient machine code at the same time.
>
> I guess that kinda leads to the question: Do we just need to fix the
> kernel code here (which is comparatively easy), or do you think that
> this is a sufficiently big problem that we need to go and somehow
> change the actual UAPI headers here (e.g. by deprecating the existing
> UAPI struct and making a new one with a different name)?

Good question. What I wrote is not based on some concrete
miscompilation at hand. I just meant that there are more things
involved that may appear at first glance.

Re proactively fixing UAPI, I would say if somebody is up to doing it
now, I would say it's good and a right change. Otherwise delaying
fixing it is also a reasonable strategy because (1) there are probably
more such cases, (2) any work on enabling more optimizations, global
optimizations, etc is only feasible if there is a tool that helps to
identify all places that need to be fixed. So whoever/whenever will be
fixing this, one more or one less case probably does not matter much.
It's a different story if there is already a tool/compiler warning
that traps on some code and that code harms deployment of the tool.

[PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm
names") made the kernel start accepting arbitrarily long algorithm names
in sockaddr_alg.  However, the actual length of the salg_name field
stayed at the original 64 bytes.

This is broken because the kernel can access indices >= 64 in salg_name,
which is undefined behavior -- even though the memory that is accessed
is still located within the sockaddr structure.  It would only be
defined behavior if the array were properly marked as arbitrary-length
(either by making it a flexible array, which is the recommended way
these days, or by making it an array of length 0 or 1).

We can't simply change salg_name into a flexible array, since that would
break source compatibility with userspace programs that embed
sockaddr_alg into another struct, or (more commonly) declare a
sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'.

One solution would be to change salg_name into a flexible array only
when '#ifdef __KERNEL__'.  However, that would keep userspace without an
easy way to actually use the longer algorithm names.

Instead, add a new structure 'sockaddr_alg_new' that has the flexible
array field, and expose it to both userspace and the kernel.
Make the kernel use it correctly in alg_bind().

This addresses the syzbot report
"UBSAN: array-index-out-of-bounds in alg_bind"
(https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e).

Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com
Fixes: 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm names")
Cc: <stable@vger.kernel.org> # v4.12+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/af_alg.c             | 10 +++++++---
 include/uapi/linux/if_alg.h | 16 ++++++++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index d11db80d24cd1..9acb9d2c4bcf9 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	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;
+	struct sockaddr_alg_new *sa = (void *)uaddr;
 	const struct af_alg_type *type;
 	void *private;
 	int err;
@@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	if (sock->state == SS_CONNECTED)
 		return -EINVAL;
 
-	if (addr_len < sizeof(*sa))
+	BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) !=
+		     offsetof(struct sockaddr_alg, salg_name));
+	BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa));
+
+	if (addr_len < sizeof(*sa) + 1)
 		return -EINVAL;
 
 	/* If caller uses non-allowed flag, return error. */
@@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 		return -EINVAL;
 
 	sa->salg_type[sizeof(sa->salg_type) - 1] = 0;
-	sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0;
+	sa->salg_name[addr_len - sizeof(*sa) - 1] = 0;
 
 	type = alg_get_type(sa->salg_type);
 	if (PTR_ERR(type) == -ENOENT) {
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index 60b7c2efd921c..dc52a11ba6d15 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -24,6 +24,22 @@ struct sockaddr_alg {
 	__u8	salg_name[64];
 };
 
+/*
+ * Linux v4.12 and later removed the 64-byte limit on salg_name[]; it's now an
+ * arbitrary-length field.  We had to keep the original struct above for source
+ * compatibility with existing userspace programs, though.  Use the new struct
+ * below if support for very long algorithm names is needed.  To do this,
+ * allocate 'sizeof(struct sockaddr_alg_new) + strlen(algname) + 1' bytes, and
+ * copy algname (including the null terminator) into salg_name.
+ */
+struct sockaddr_alg_new {
+	__u16	salg_family;
+	__u8	salg_type[14];
+	__u32	salg_feat;
+	__u32	salg_mask;
+	__u8	salg_name[];
+};
+
 struct af_alg_iv {
 	__u32	ivlen;
 	__u8	iv[0];

base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec
-- 
2.29.1

Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name

[Gustavo A. R. Silva]

Hi,

On Mon, Oct 26, 2020 at 01:07:15PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm
> names") made the kernel start accepting arbitrarily long algorithm names
> in sockaddr_alg.  However, the actual length of the salg_name field
> stayed at the original 64 bytes.
> 
> This is broken because the kernel can access indices >= 64 in salg_name,
> which is undefined behavior -- even though the memory that is accessed
> is still located within the sockaddr structure.  It would only be
> defined behavior if the array were properly marked as arbitrary-length
> (either by making it a flexible array, which is the recommended way
> these days, or by making it an array of length 0 or 1).
> 
> We can't simply change salg_name into a flexible array, since that would
> break source compatibility with userspace programs that embed
> sockaddr_alg into another struct, or (more commonly) declare a
> sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'.
> 
> One solution would be to change salg_name into a flexible array only
> when '#ifdef __KERNEL__'.  However, that would keep userspace without an
> easy way to actually use the longer algorithm names.
> 
> Instead, add a new structure 'sockaddr_alg_new' that has the flexible
> array field, and expose it to both userspace and the kernel.
> Make the kernel use it correctly in alg_bind().
> 
> This addresses the syzbot report
> "UBSAN: array-index-out-of-bounds in alg_bind"
> (https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e).
> 
> Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com
> Fixes: 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm names")
> Cc: <stable@vger.kernel.org> # v4.12+
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  crypto/af_alg.c             | 10 +++++++---
>  include/uapi/linux/if_alg.h | 16 ++++++++++++++++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index d11db80d24cd1..9acb9d2c4bcf9 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  	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;
> +	struct sockaddr_alg_new *sa = (void *)uaddr;
>  	const struct af_alg_type *type;
>  	void *private;
>  	int err;
> @@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  	if (sock->state == SS_CONNECTED)
>  		return -EINVAL;
>  
> -	if (addr_len < sizeof(*sa))
> +	BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) !=
> +		     offsetof(struct sockaddr_alg, salg_name));
> +	BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa));
> +
> +	if (addr_len < sizeof(*sa) + 1)
>  		return -EINVAL;
>  
>  	/* If caller uses non-allowed flag, return error. */
> @@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  		return -EINVAL;
>  
>  	sa->salg_type[sizeof(sa->salg_type) - 1] = 0;
> -	sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0;
> +	sa->salg_name[addr_len - sizeof(*sa) - 1] = 0;
>  
>  	type = alg_get_type(sa->salg_type);
>  	if (PTR_ERR(type) == -ENOENT) {
> diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> index 60b7c2efd921c..dc52a11ba6d15 100644
> --- a/include/uapi/linux/if_alg.h
> +++ b/include/uapi/linux/if_alg.h
> @@ -24,6 +24,22 @@ struct sockaddr_alg {
>  	__u8	salg_name[64];
>  };
>  
> +/*
> + * Linux v4.12 and later removed the 64-byte limit on salg_name[]; it's now an
> + * arbitrary-length field.  We had to keep the original struct above for source
> + * compatibility with existing userspace programs, though.  Use the new struct
> + * below if support for very long algorithm names is needed.  To do this,
> + * allocate 'sizeof(struct sockaddr_alg_new) + strlen(algname) + 1' bytes, and
> + * copy algname (including the null terminator) into salg_name.
> + */
> +struct sockaddr_alg_new {
> +	__u16	salg_family;
> +	__u8	salg_type[14];
> +	__u32	salg_feat;
> +	__u32	salg_mask;
> +	__u8	salg_name[];
> +};
> +

How something like this, instead:

 struct sockaddr_alg {
-	__u16	salg_family;
-	__u8	salg_type[14];
-	__u32	salg_feat;
-	__u32	salg_mask;
-	__u8	salg_name[64];
+	union {
+		struct {
+			__u16	salg_v1_family;
+			__u8	salg_v1_type[14];
+			__u32	salg_v1_feat;
+			__u32	salg_v1_mask;
+			__u8	salg_name[64];
+		};
+		struct {
+			__u16	salg_family;
+			__u8	salg_type[14];
+			__u32	salg_feat;
+			__u32	salg_mask;
+			__u8	salg_name_new[];
+		};
+	};
 };


--
Gustavo

Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name

[Gustavo A. R. Silva]

On Mon, Oct 26, 2020 at 04:21:48PM -0500, Gustavo A. R. Silva wrote:
> > +/*
> > + * Linux v4.12 and later removed the 64-byte limit on salg_name[]; it's now an
> > + * arbitrary-length field.  We had to keep the original struct above for source
> > + * compatibility with existing userspace programs, though.  Use the new struct
> > + * below if support for very long algorithm names is needed.  To do this,
> > + * allocate 'sizeof(struct sockaddr_alg_new) + strlen(algname) + 1' bytes, and
> > + * copy algname (including the null terminator) into salg_name.
> > + */
> > +struct sockaddr_alg_new {
> > +	__u16	salg_family;
> > +	__u8	salg_type[14];
> > +	__u32	salg_feat;
> > +	__u32	salg_mask;
> > +	__u8	salg_name[];
> > +};
> > +
> 
> How something like this, instead:
> 
>  struct sockaddr_alg {
> -	__u16	salg_family;
> -	__u8	salg_type[14];
> -	__u32	salg_feat;
> -	__u32	salg_mask;
> -	__u8	salg_name[64];
> +	union {
> +		struct {
> +			__u16	salg_v1_family;
> +			__u8	salg_v1_type[14];
> +			__u32	salg_v1_feat;
> +			__u32	salg_v1_mask;
> +			__u8	salg_name[64];
> +		};
> +		struct {
> +			__u16	salg_family;
> +			__u8	salg_type[14];
> +			__u32	salg_feat;
> +			__u32	salg_mask;
> +			__u8	salg_name_new[];
> +		};
> +	};
>  };
> 

Something similar to the following approach might work:

https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=testing/uapi/gntalloc&id=db46c8aba41c436edb0b4ef2941bd7390b0e5d61

--
Gustavo

Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name

[Eric Biggers]

On Mon, Oct 26, 2020 at 06:10:59PM -0500, Gustavo A. R. Silva wrote:
> On Mon, Oct 26, 2020 at 04:21:48PM -0500, Gustavo A. R. Silva wrote:
> > > +/*
> > > + * Linux v4.12 and later removed the 64-byte limit on salg_name[]; it's now an
> > > + * arbitrary-length field.  We had to keep the original struct above for source
> > > + * compatibility with existing userspace programs, though.  Use the new struct
> > > + * below if support for very long algorithm names is needed.  To do this,
> > > + * allocate 'sizeof(struct sockaddr_alg_new) + strlen(algname) + 1' bytes, and
> > > + * copy algname (including the null terminator) into salg_name.
> > > + */
> > > +struct sockaddr_alg_new {
> > > +	__u16	salg_family;
> > > +	__u8	salg_type[14];
> > > +	__u32	salg_feat;
> > > +	__u32	salg_mask;
> > > +	__u8	salg_name[];
> > > +};
> > > +
> > 
> > How something like this, instead:
> > 
> >  struct sockaddr_alg {
> > -	__u16	salg_family;
> > -	__u8	salg_type[14];
> > -	__u32	salg_feat;
> > -	__u32	salg_mask;
> > -	__u8	salg_name[64];
> > +	union {
> > +		struct {
> > +			__u16	salg_v1_family;
> > +			__u8	salg_v1_type[14];
> > +			__u32	salg_v1_feat;
> > +			__u32	salg_v1_mask;
> > +			__u8	salg_name[64];
> > +		};
> > +		struct {
> > +			__u16	salg_family;
> > +			__u8	salg_type[14];
> > +			__u32	salg_feat;
> > +			__u32	salg_mask;
> > +			__u8	salg_name_new[];
> > +		};
> > +	};
> >  };
> > 
> 
> Something similar to the following approach might work:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=testing/uapi/gntalloc&id=db46c8aba41c436edb0b4ef2941bd7390b0e5d61
> 

I suppose so.  It's very confusing to see a union like that at first glance,
though.  It definitely needs an explanatory comment...

- Eric

Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name

[Jann Horn]

On Mon, Oct 26, 2020 at 9:08 PM Eric Biggers <ebiggers@kernel.org> wrote:
> Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm
> names") made the kernel start accepting arbitrarily long algorithm names
> in sockaddr_alg.

That's not true; it's still limited by the size of struct
sockaddr_storage (128 bytes total for the entire address). If you make
it longer, __copy_msghdr_from_user() will silently truncate the size.

> This is broken because the kernel can access indices >= 64 in salg_name,
> which is undefined behavior -- even though the memory that is accessed
> is still located within the sockaddr structure.  It would only be
> defined behavior if the array were properly marked as arbitrary-length
> (either by making it a flexible array, which is the recommended way
> these days, or by making it an array of length 0 or 1).
>
> We can't simply change salg_name into a flexible array, since that would
> break source compatibility with userspace programs that embed
> sockaddr_alg into another struct, or (more commonly) declare a
> sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'.
>
> One solution would be to change salg_name into a flexible array only
> when '#ifdef __KERNEL__'.  However, that would keep userspace without an
> easy way to actually use the longer algorithm names.
>
> Instead, add a new structure 'sockaddr_alg_new' that has the flexible
> array field, and expose it to both userspace and the kernel.
> Make the kernel use it correctly in alg_bind().
[...]
> @@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>         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;
> +       struct sockaddr_alg_new *sa = (void *)uaddr;
>         const struct af_alg_type *type;
>         void *private;
>         int err;
> @@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>         if (sock->state == SS_CONNECTED)
>                 return -EINVAL;
>
> -       if (addr_len < sizeof(*sa))
> +       BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) !=
> +                    offsetof(struct sockaddr_alg, salg_name));
> +       BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa));
> +
> +       if (addr_len < sizeof(*sa) + 1)
>                 return -EINVAL;
>
>         /* If caller uses non-allowed flag, return error. */
> @@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>                 return -EINVAL;
>
>         sa->salg_type[sizeof(sa->salg_type) - 1] = 0;
> -       sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0;
> +       sa->salg_name[addr_len - sizeof(*sa) - 1] = 0;

This looks like an out-of-bounds write in the case `addr_len ==
sizeof(struct sockaddr_storage)`.

Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name

[Eric Biggers]

On Mon, Oct 26, 2020 at 10:23:35PM +0100, 'Jann Horn' via syzkaller-bugs wrote:
> On Mon, Oct 26, 2020 at 9:08 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm
> > names") made the kernel start accepting arbitrarily long algorithm names
> > in sockaddr_alg.
> 
> That's not true; it's still limited by the size of struct
> sockaddr_storage (128 bytes total for the entire address).

Interesting, so the actual limit is 104 bytes.  It seems like the intent of that
commit was to make it unlimited, though...

> If you make it longer, __copy_msghdr_from_user() will silently truncate the
> size.

That's used for sys_sendmsg(), which AFAICT isn't relevant here.  sockaddr_alg
is used with sys_bind(), which fails with EINVAL if the address is longer than
sizeof(struct sockaddr_storage).

However, since sys_sendmsg() is truncating overly-long addresses, it's probably
the case that sizeof(struct sockaddr_storage) can never be increased in the
future...

> 
> > This is broken because the kernel can access indices >= 64 in salg_name,
> > which is undefined behavior -- even though the memory that is accessed
> > is still located within the sockaddr structure.  It would only be
> > defined behavior if the array were properly marked as arbitrary-length
> > (either by making it a flexible array, which is the recommended way
> > these days, or by making it an array of length 0 or 1).
> >
> > We can't simply change salg_name into a flexible array, since that would
> > break source compatibility with userspace programs that embed
> > sockaddr_alg into another struct, or (more commonly) declare a
> > sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'.
> >
> > One solution would be to change salg_name into a flexible array only
> > when '#ifdef __KERNEL__'.  However, that would keep userspace without an
> > easy way to actually use the longer algorithm names.
> >
> > Instead, add a new structure 'sockaddr_alg_new' that has the flexible
> > array field, and expose it to both userspace and the kernel.
> > Make the kernel use it correctly in alg_bind().
> [...]
> > @@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> >         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;
> > +       struct sockaddr_alg_new *sa = (void *)uaddr;
> >         const struct af_alg_type *type;
> >         void *private;
> >         int err;
> > @@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> >         if (sock->state == SS_CONNECTED)
> >                 return -EINVAL;
> >
> > -       if (addr_len < sizeof(*sa))
> > +       BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) !=
> > +                    offsetof(struct sockaddr_alg, salg_name));
> > +       BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa));
> > +
> > +       if (addr_len < sizeof(*sa) + 1)
> >                 return -EINVAL;
> >
> >         /* If caller uses non-allowed flag, return error. */
> > @@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> >                 return -EINVAL;
> >
> >         sa->salg_type[sizeof(sa->salg_type) - 1] = 0;
> > -       sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0;
> > +       sa->salg_name[addr_len - sizeof(*sa) - 1] = 0;
> 
> This looks like an out-of-bounds write in the case `addr_len ==
> sizeof(struct sockaddr_storage)`.

I think you mean addr_len == sizeof(*sa)?  That's what the
'if (addr_len < sizeof(*sa) + 1) return -EINVAL' above is for.

- Eric

Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name

[Jann Horn]

On Mon, Oct 26, 2020 at 10:57 PM Eric Biggers <ebiggers@kernel.org> wrote:
> On Mon, Oct 26, 2020 at 10:23:35PM +0100, 'Jann Horn' via syzkaller-bugs wrote:
> > On Mon, Oct 26, 2020 at 9:08 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm
> > > names") made the kernel start accepting arbitrarily long algorithm names
> > > in sockaddr_alg.
> >
> > That's not true; it's still limited by the size of struct
> > sockaddr_storage (128 bytes total for the entire address).
>
> Interesting, so the actual limit is 104 bytes.  It seems like the intent of that
> commit was to make it unlimited, though...
>
> > If you make it longer, __copy_msghdr_from_user() will silently truncate the
> > size.
>
> That's used for sys_sendmsg(), which AFAICT isn't relevant here.  sockaddr_alg
> is used with sys_bind(), which fails with EINVAL if the address is longer than
> sizeof(struct sockaddr_storage).

Ugh, of course you're right, sorry.

> However, since sys_sendmsg() is truncating overly-long addresses, it's probably
> the case that sizeof(struct sockaddr_storage) can never be increased in the
> future...

Eh, I think there'd probably be bigger issues with that elsewhere.

> > > This is broken because the kernel can access indices >= 64 in salg_name,
> > > which is undefined behavior -- even though the memory that is accessed
> > > is still located within the sockaddr structure.  It would only be
> > > defined behavior if the array were properly marked as arbitrary-length
> > > (either by making it a flexible array, which is the recommended way
> > > these days, or by making it an array of length 0 or 1).
> > >
> > > We can't simply change salg_name into a flexible array, since that would
> > > break source compatibility with userspace programs that embed
> > > sockaddr_alg into another struct, or (more commonly) declare a
> > > sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'.
> > >
> > > One solution would be to change salg_name into a flexible array only
> > > when '#ifdef __KERNEL__'.  However, that would keep userspace without an
> > > easy way to actually use the longer algorithm names.
> > >
> > > Instead, add a new structure 'sockaddr_alg_new' that has the flexible
> > > array field, and expose it to both userspace and the kernel.
> > > Make the kernel use it correctly in alg_bind().
> > [...]
> > > @@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> > >         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;
> > > +       struct sockaddr_alg_new *sa = (void *)uaddr;
> > >         const struct af_alg_type *type;
> > >         void *private;
> > >         int err;
> > > @@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> > >         if (sock->state == SS_CONNECTED)
> > >                 return -EINVAL;
> > >
> > > -       if (addr_len < sizeof(*sa))
> > > +       BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) !=
> > > +                    offsetof(struct sockaddr_alg, salg_name));
> > > +       BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa));
> > > +
> > > +       if (addr_len < sizeof(*sa) + 1)
> > >                 return -EINVAL;
> > >
> > >         /* If caller uses non-allowed flag, return error. */
> > > @@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> > >                 return -EINVAL;
> > >
> > >         sa->salg_type[sizeof(sa->salg_type) - 1] = 0;
> > > -       sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0;
> > > +       sa->salg_name[addr_len - sizeof(*sa) - 1] = 0;
> >
> > This looks like an out-of-bounds write in the case `addr_len ==
> > sizeof(struct sockaddr_storage)`.

Sorry, I've been unusually unconcentrated today. Sorry about the
noise, ignore what I said.

Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name

[Herbert Xu]

On Mon, Oct 26, 2020 at 01:07:15PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm
> names") made the kernel start accepting arbitrarily long algorithm names
> in sockaddr_alg.  However, the actual length of the salg_name field
> stayed at the original 64 bytes.
> 
> This is broken because the kernel can access indices >= 64 in salg_name,
> which is undefined behavior -- even though the memory that is accessed
> is still located within the sockaddr structure.  It would only be
> defined behavior if the array were properly marked as arbitrary-length
> (either by making it a flexible array, which is the recommended way
> these days, or by making it an array of length 0 or 1).
> 
> We can't simply change salg_name into a flexible array, since that would
> break source compatibility with userspace programs that embed
> sockaddr_alg into another struct, or (more commonly) declare a
> sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'.
> 
> One solution would be to change salg_name into a flexible array only
> when '#ifdef __KERNEL__'.  However, that would keep userspace without an
> easy way to actually use the longer algorithm names.
> 
> Instead, add a new structure 'sockaddr_alg_new' that has the flexible
> array field, and expose it to both userspace and the kernel.
> Make the kernel use it correctly in alg_bind().
> 
> This addresses the syzbot report
> "UBSAN: array-index-out-of-bounds in alg_bind"
> (https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e).
> 
> Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com
> Fixes: 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm names")
> Cc: <stable@vger.kernel.org> # v4.12+
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  crypto/af_alg.c             | 10 +++++++---
>  include/uapi/linux/if_alg.h | 16 ++++++++++++++++
>  2 files changed, 23 insertions(+), 3 deletions(-)

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

Re: UBSAN: array-index-out-of-bounds in alg_bind

[syzbot]

syzbot has found a reproducer for the following issue on:

HEAD commit:    3cea11cd Linux 5.10-rc2
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1443bf92500000
kernel config:  https://syzkaller.appspot.com/x/.config?x=e791ddf0875adf65
dashboard link: https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e
compiler:       clang version 11.0.0 (https://github.com/llvm/llvm-project.git ca2dcbd030eadbf0aa9b660efe864ff08af6e18b)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=145afc2c500000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=141ad11a500000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com

================================================================================
UBSAN: array-index-out-of-bounds in crypto/af_alg.c:166:2
index 98 is out of range for type '__u8 [64]'
CPU: 1 PID: 8468 Comm: syz-executor983 Not tainted 5.10.0-rc2-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x137/0x1be lib/dump_stack.c:118
 ubsan_epilogue lib/ubsan.c:148 [inline]
 __ubsan_handle_out_of_bounds+0xdb/0x130 lib/ubsan.c:356
 alg_bind+0x738/0x740 crypto/af_alg.c:166
 __sys_bind+0x283/0x360 net/socket.c:1656
 __do_sys_bind net/socket.c:1667 [inline]
 __se_sys_bind net/socket.c:1665 [inline]
 __x64_sys_bind+0x76/0x80 net/socket.c:1665
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x4402c9
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffe05301528 EFLAGS: 00000246 ORIG_RAX: 0000000000000031
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 00000000004402c9
RDX: 000000000000007b RSI: 00000000200000c0 RDI: 0000000000000003
RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000401ad0
R13: 0000000000401b60 R14: 0000000000000000 R15: 0000000000000000
================================================================================
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 8468 Comm: syz-executor983 Not tainted 5.10.0-rc2-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x137/0x1be lib/dump_stack.c:118
 panic+0x291/0x800 kernel/panic.c:231
 ubsan_epilogue lib/ubsan.c:162 [inline]
 __ubsan_handle_out_of_bounds+0x12b/0x130 lib/ubsan.c:356
 alg_bind+0x738/0x740 crypto/af_alg.c:166
 __sys_bind+0x283/0x360 net/socket.c:1656
 __do_sys_bind net/socket.c:1667 [inline]
 __se_sys_bind net/socket.c:1665 [inline]
 __x64_sys_bind+0x76/0x80 net/socket.c:1665
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x4402c9
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffe05301528 EFLAGS: 00000246 ORIG_RAX: 0000000000000031
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 00000000004402c9
RDX: 000000000000007b RSI: 00000000200000c0 RDI: 0000000000000003
RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000401ad0
R13: 0000000000401b60 R14: 0000000000000000 R15: 0000000000000000
Kernel Offset: disabled
Rebooting in 86400 seconds..