Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name
On Mon, Oct 26, 2020 at 01:07:15PM -0700, Eric Biggers wrote: > From: Eric Biggers > > 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+92ead4eb8e26a26d4...@syzkaller.appspotmail.com > Fixes: 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm > names") > Cc: # v4.12+ > Signed-off-by: Eric Biggers > --- > 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 Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name
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; > > > + __u8salg_type[14]; > > > + __u32 salg_feat; > > > + __u32 salg_mask; > > > + __u8salg_name[]; > > > +}; > > > + > > > > How something like this, instead: > > > > struct sockaddr_alg { > > - __u16 salg_family; > > - __u8salg_type[14]; > > - __u32 salg_feat; > > - __u32 salg_mask; > > - __u8salg_name[64]; > > + union { > > + struct { > > + __u16 salg_v1_family; > > + __u8salg_v1_type[14]; > > + __u32 salg_v1_feat; > > + __u32 salg_v1_mask; > > + __u8salg_name[64]; > > + }; > > + struct { > > + __u16 salg_family; > > + __u8salg_type[14]; > > + __u32 salg_feat; > > + __u32 salg_mask; > > + __u8salg_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=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
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; > > + __u8salg_type[14]; > > + __u32 salg_feat; > > + __u32 salg_mask; > > + __u8salg_name[]; > > +}; > > + > > How something like this, instead: > > struct sockaddr_alg { > - __u16 salg_family; > - __u8salg_type[14]; > - __u32 salg_feat; > - __u32 salg_mask; > - __u8salg_name[64]; > + union { > + struct { > + __u16 salg_v1_family; > + __u8salg_v1_type[14]; > + __u32 salg_v1_feat; > + __u32 salg_v1_mask; > + __u8salg_name[64]; > + }; > + struct { > + __u16 salg_family; > + __u8salg_type[14]; > + __u32 salg_feat; > + __u32 salg_mask; > + __u8salg_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=db46c8aba41c436edb0b4ef2941bd7390b0e5d61 -- Gustavo
Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name
On Mon, Oct 26, 2020 at 10:57 PM Eric Biggers 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 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
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 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
On Mon, Oct 26, 2020 at 9:08 PM Eric Biggers 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
Hi, On Mon, Oct 26, 2020 at 01:07:15PM -0700, Eric Biggers wrote: > From: Eric Biggers > > 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+92ead4eb8e26a26d4...@syzkaller.appspotmail.com > Fixes: 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm > names") > Cc: # v4.12+ > Signed-off-by: Eric Biggers > --- > 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 { > __u8salg_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; > + __u8salg_type[14]; > + __u32 salg_feat; > + __u32 salg_mask; > + __u8salg_name[]; > +}; > + How something like this, instead: struct sockaddr_alg { - __u16 salg_family; - __u8salg_type[14]; - __u32 salg_feat; - __u32 salg_mask; - __u8salg_name[64]; + union { + struct { + __u16 salg_v1_family; + __u8salg_v1_type[14]; + __u32 salg_v1_feat; + __u32 salg_v1_mask; + __u8salg_name[64]; + }; + struct { +