[PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

[PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

[Kees Cook]

Some protocols do not correctly wipe the contents of the on-stack
struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
kernel stack contents to userspace. This wipes it unconditionally before
per-protocol handlers run.

Note that leaks like this are mitigated by building with
CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y

Reported-by: Alexander Potapenko <glider@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/socket.c b/net/socket.c
index c729625eb5d3..34183f4fbdf8 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
 	struct sockaddr __user *uaddr;
 	int __user *uaddr_len = COMPAT_NAMELEN(msg);
 
+	memset(&addr, 0, sizeof(addr));
 	msg_sys->msg_name = &addr;
 
 	if (MSG_CMSG_COMPAT & flags)
-- 
2.7.4


-- 
Kees Cook
Pixel Security

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

[Eric Dumazet]

On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
> Some protocols do not correctly wipe the contents of the on-stack
> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
> kernel stack contents to userspace. This wipes it unconditionally before
> per-protocol handlers run.
> 
> Note that leaks like this are mitigated by building with
> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
> 
> Reported-by: Alexander Potapenko <glider@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  net/socket.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/socket.c b/net/socket.c
> index c729625eb5d3..34183f4fbdf8 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>  	struct sockaddr __user *uaddr;
>  	int __user *uaddr_len = COMPAT_NAMELEN(msg);
>  
> +	memset(&addr, 0, sizeof(addr));
>  	msg_sys->msg_name = &addr;
>  

This kind of patch comes every year.

Standard answer is : We fix the buggy protocol, we do not make
everything slower just because we are lazy.

struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.

Also memset() is using long word stores, so next 4-byte or 2-byte stores
on same location hit a performance problem on x86.

By adding all these defensive programming, we would give strong
incentives to bypass the kernel for networking. That would be bad.

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]

On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
> Some protocols do not correctly wipe the contents of the on-stack
> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
> kernel stack contents to userspace. This wipes it unconditionally before
> per-protocol handlers run.
> 
> Note that leaks like this are mitigated by building with
> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
> 
> Reported-by: Alexander Potapenko <glider@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  net/socket.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/socket.c b/net/socket.c
> index c729625eb5d3..34183f4fbdf8 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>  	struct sockaddr __user *uaddr;
>  	int __user *uaddr_len = COMPAT_NAMELEN(msg);
>  
> +	memset(&addr, 0, sizeof(addr));

That's a fairly large structure (128 bytes), most of which won't
normally be used.  We already initialise msg_namelen to 0 before
calling the per-protocol handler, which means by default nothing leaks.
 Only cases where msg_namelen is set but msg_name[] is not initialised
up to that length are a problem.  I would have thought they were not
too hard to find and fix.

Ben.

>  	msg_sys->msg_name = &addr;
>  
>  	if (MSG_CMSG_COMPAT & flags)
> -- 
> 2.7.4
> 
> 
-- 
Ben Hutchings
It is a miracle that curiosity survives formal education. - Albert
Einstein


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

[Willy Tarreau]

On Tue, Oct 31, 2017 at 09:14:45AM -0700, Kees Cook wrote:
> diff --git a/net/socket.c b/net/socket.c
> index c729625eb5d3..34183f4fbdf8 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>  	struct sockaddr __user *uaddr;
>  	int __user *uaddr_len = COMPAT_NAMELEN(msg);
>  
> +	memset(&addr, 0, sizeof(addr));
>  	msg_sys->msg_name = &addr;

Isn't this going to cause a performance hit in the fast path ? Just
checking, I have not read the whole code with the patch in its context.

Willy

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

[Eric W. Biederman]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
>> Some protocols do not correctly wipe the contents of the on-stack
>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
>> kernel stack contents to userspace. This wipes it unconditionally before
>> per-protocol handlers run.
>> 
>> Note that leaks like this are mitigated by building with
>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>> 
>> Reported-by: Alexander Potapenko <glider@google.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  net/socket.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/net/socket.c b/net/socket.c
>> index c729625eb5d3..34183f4fbdf8 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>>  	struct sockaddr __user *uaddr;
>>  	int __user *uaddr_len = COMPAT_NAMELEN(msg);
>>  
>> +	memset(&addr, 0, sizeof(addr));
>>  	msg_sys->msg_name = &addr;
>>  
>
> This kind of patch comes every year.
>
> Standard answer is : We fix the buggy protocol, we do not make
> everything slower just because we are lazy.
>
> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
>
> Also memset() is using long word stores, so next 4-byte or 2-byte stores
> on same location hit a performance problem on x86.
>
> By adding all these defensive programming, we would give strong
> incentives to bypass the kernel for networking. That would be bad.

In this case it looks like the root cause is something in sctp
not filling in the ipv6 sin6_scope_id.

Which is not only a leak but a correctness bug.

I ran the reproducer test program and while none of the leak checkers
are telling me anything I have gotten as far as seeing that the returned
length is correct and sometimes nonsense.

Hmm.

At a quick look it looks like all that is necessary is to do this:

diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 51c488769590..6301913d0516 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
 		addr->v6.sin6_flowinfo = 0;
 		addr->v6.sin6_port = sh->source;
 		addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
-		if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+		if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
 			addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
-		}
+		else
+			addr->v6.sin6_scope_id = 0;
 	}
 
 	*addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);

Eric

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

[Kees Cook]

On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>
>> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
>>> Some protocols do not correctly wipe the contents of the on-stack
>>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
>>> kernel stack contents to userspace. This wipes it unconditionally before
>>> per-protocol handlers run.
>>>
>>> Note that leaks like this are mitigated by building with
>>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>>>
>>> Reported-by: Alexander Potapenko <glider@google.com>
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: netdev@vger.kernel.org
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>>  net/socket.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/net/socket.c b/net/socket.c
>>> index c729625eb5d3..34183f4fbdf8 100644
>>> --- a/net/socket.c
>>> +++ b/net/socket.c
>>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>>>      struct sockaddr __user *uaddr;
>>>      int __user *uaddr_len = COMPAT_NAMELEN(msg);
>>>
>>> +    memset(&addr, 0, sizeof(addr));
>>>      msg_sys->msg_name = &addr;
>>>
>>
>> This kind of patch comes every year.
>>
>> Standard answer is : We fix the buggy protocol, we do not make
>> everything slower just because we are lazy.
>>
>> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
>>
>> Also memset() is using long word stores, so next 4-byte or 2-byte stores
>> on same location hit a performance problem on x86.
>>
>> By adding all these defensive programming, we would give strong
>> incentives to bypass the kernel for networking. That would be bad.
>
> In this case it looks like the root cause is something in sctp
> not filling in the ipv6 sin6_scope_id.
>
> Which is not only a leak but a correctness bug.
>
> I ran the reproducer test program and while none of the leak checkers
> are telling me anything I have gotten as far as seeing that the returned
> length is correct and sometimes nonsense.
>
> Hmm.
>
> At a quick look it looks like all that is necessary is to do this:
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 51c488769590..6301913d0516 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
>                 addr->v6.sin6_flowinfo = 0;
>                 addr->v6.sin6_port = sh->source;
>                 addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
> -               if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
> +               if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
>                         addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
> -               }
> +               else
> +                       addr->v6.sin6_scope_id = 0;
>         }
>
>         *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
>
> Eric
>

Thanks for digging into this Eric! Alexander, can you confirm this
fixes it for you when CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is not
enabled?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

[Kees Cook]

On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>
>> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
>>> Some protocols do not correctly wipe the contents of the on-stack
>>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
>>> kernel stack contents to userspace. This wipes it unconditionally before
>>> per-protocol handlers run.
>>>
>>> Note that leaks like this are mitigated by building with
>>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>>>
>>> Reported-by: Alexander Potapenko <glider@google.com>
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: netdev@vger.kernel.org
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>>  net/socket.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/net/socket.c b/net/socket.c
>>> index c729625eb5d3..34183f4fbdf8 100644
>>> --- a/net/socket.c
>>> +++ b/net/socket.c
>>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>>>      struct sockaddr __user *uaddr;
>>>      int __user *uaddr_len = COMPAT_NAMELEN(msg);
>>>
>>> +    memset(&addr, 0, sizeof(addr));
>>>      msg_sys->msg_name = &addr;
>>>
>>
>> This kind of patch comes every year.
>>
>> Standard answer is : We fix the buggy protocol, we do not make
>> everything slower just because we are lazy.
>>
>> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
>>
>> Also memset() is using long word stores, so next 4-byte or 2-byte stores
>> on same location hit a performance problem on x86.
>>
>> By adding all these defensive programming, we would give strong
>> incentives to bypass the kernel for networking. That would be bad.
>
> In this case it looks like the root cause is something in sctp
> not filling in the ipv6 sin6_scope_id.
>
> Which is not only a leak but a correctness bug.
>
> I ran the reproducer test program and while none of the leak checkers
> are telling me anything I have gotten as far as seeing that the returned
> length is correct and sometimes nonsense.
>
> Hmm.
>
> At a quick look it looks like all that is necessary is to do this:
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 51c488769590..6301913d0516 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
>                 addr->v6.sin6_flowinfo = 0;
>                 addr->v6.sin6_port = sh->source;
>                 addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
> -               if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
> +               if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
>                         addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
> -               }
> +               else
> +                       addr->v6.sin6_scope_id = 0;
>         }
>
>         *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
>

It looks like this never landed anywhere? Eric, are you able to resend
this as a full patch?

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

[Alexander Potapenko]

On Wed, Nov 1, 2017 at 7:23 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>>
>>> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
>>>> Some protocols do not correctly wipe the contents of the on-stack
>>>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
>>>> kernel stack contents to userspace. This wipes it unconditionally before
>>>> per-protocol handlers run.
>>>>
>>>> Note that leaks like this are mitigated by building with
>>>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>>>>
>>>> Reported-by: Alexander Potapenko <glider@google.com>
>>>> Cc: "David S. Miller" <davem@davemloft.net>
>>>> Cc: netdev@vger.kernel.org
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> ---
>>>>  net/socket.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/net/socket.c b/net/socket.c
>>>> index c729625eb5d3..34183f4fbdf8 100644
>>>> --- a/net/socket.c
>>>> +++ b/net/socket.c
>>>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>>>>      struct sockaddr __user *uaddr;
>>>>      int __user *uaddr_len = COMPAT_NAMELEN(msg);
>>>>
>>>> +    memset(&addr, 0, sizeof(addr));
>>>>      msg_sys->msg_name = &addr;
>>>>
>>>
>>> This kind of patch comes every year.
>>>
>>> Standard answer is : We fix the buggy protocol, we do not make
>>> everything slower just because we are lazy.
>>>
>>> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
>>>
>>> Also memset() is using long word stores, so next 4-byte or 2-byte stores
>>> on same location hit a performance problem on x86.
>>>
>>> By adding all these defensive programming, we would give strong
>>> incentives to bypass the kernel for networking. That would be bad.
>>
>> In this case it looks like the root cause is something in sctp
>> not filling in the ipv6 sin6_scope_id.
>>
>> Which is not only a leak but a correctness bug.
>>
>> I ran the reproducer test program and while none of the leak checkers
>> are telling me anything I have gotten as far as seeing that the returned
>> length is correct and sometimes nonsense.
>>
>> Hmm.
>>
>> At a quick look it looks like all that is necessary is to do this:
>>
>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>> index 51c488769590..6301913d0516 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
>>                 addr->v6.sin6_flowinfo = 0;
>>                 addr->v6.sin6_port = sh->source;
>>                 addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
>> -               if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
>> +               if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
>>                         addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
>> -               }
>> +               else
>> +                       addr->v6.sin6_scope_id = 0;
>>         }
>>
>>         *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
>>
>> Eric
>>
>
> Thanks for digging into this Eric! Alexander, can you confirm this
> fixes it for you when CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is not
> enabled?
Sorry, I accidentally missed this patch.
Yes, I confirm it fixes the problem with
CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL disabled.
> -Kees
>
> --
> Kees Cook
> Pixel Security



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> On Wed, Nov 1, 2017 at 5:48 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>>
>>> On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote:
>>>> Some protocols do not correctly wipe the contents of the on-stack
>>>> struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak
>>>> kernel stack contents to userspace. This wipes it unconditionally before
>>>> per-protocol handlers run.
>>>>
>>>> Note that leaks like this are mitigated by building with
>>>> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y
>>>>
>>>> Reported-by: Alexander Potapenko <glider@google.com>
>>>> Cc: "David S. Miller" <davem@davemloft.net>
>>>> Cc: netdev@vger.kernel.org
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> ---
>>>>  net/socket.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/net/socket.c b/net/socket.c
>>>> index c729625eb5d3..34183f4fbdf8 100644
>>>> --- a/net/socket.c
>>>> +++ b/net/socket.c
>>>> @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg,
>>>>      struct sockaddr __user *uaddr;
>>>>      int __user *uaddr_len = COMPAT_NAMELEN(msg);
>>>>
>>>> +    memset(&addr, 0, sizeof(addr));
>>>>      msg_sys->msg_name = &addr;
>>>>
>>>
>>> This kind of patch comes every year.
>>>
>>> Standard answer is : We fix the buggy protocol, we do not make
>>> everything slower just because we are lazy.
>>>
>>> struct sockaddr is 128 bytes, but IPV4 only uses a fraction of it.
>>>
>>> Also memset() is using long word stores, so next 4-byte or 2-byte stores
>>> on same location hit a performance problem on x86.
>>>
>>> By adding all these defensive programming, we would give strong
>>> incentives to bypass the kernel for networking. That would be bad.
>>
>> In this case it looks like the root cause is something in sctp
>> not filling in the ipv6 sin6_scope_id.
>>
>> Which is not only a leak but a correctness bug.
>>
>> I ran the reproducer test program and while none of the leak checkers
>> are telling me anything I have gotten as far as seeing that the returned
>> length is correct and sometimes nonsense.
>>
>> Hmm.
>>
>> At a quick look it looks like all that is necessary is to do this:
>>
>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>> index 51c488769590..6301913d0516 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
>>                 addr->v6.sin6_flowinfo = 0;
>>                 addr->v6.sin6_port = sh->source;
>>                 addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
>> -               if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
>> +               if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
>>                         addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
>> -               }
>> +               else
>> +                       addr->v6.sin6_scope_id = 0;
>>         }
>>
>>         *addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
>>
>
> It looks like this never landed anywhere? Eric, are you able to resend
> this as a full patch?

I will take a look.  I have not conducted a thorough review to make
certain that is everything.  I was hoping someone else would pick that
change up and run with it.   However the change seems obviously correct
as is, so I don't have any problem sending just this bit.

Eric

[PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname

[Eric W. Biederman]

Alexandar Potapenko while testing the kernel with KMSAN and syzkaller
discovered that in some configurations sctp would leak 4 bytes of
kernel stack.

Working with his reproducer I discovered that those 4 bytes that
are leaked is the scope id of an ipv6 address returned by recvmsg.

With a little code inspection and a shrewd guess I discovered that
sctp_inet6_skb_msgname only initializes the scope_id field for link
local ipv6 addresses to the interface index the link local address
pertains to instead of initializing the scope_id field for all ipv6
addresses.

That is almost reasonable as scope_id's are meaniningful only for link
local addresses.  Set the scope_id in all other cases to 0 which is
not a valid interface index to make it clear there is nothing useful
in the scope_id field.

There should be no danger of breaking userspace as the stack leak
guaranteed that previously meaningless random data was being returned.

Cc: stable@vger.kernel.org
Fixes: 372f525b495c ("SCTP:  Resync with LKSCTP tree.")
History-tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
Reported-by: Alexander Potapenko <glider@google.com>
Tested-by: Alexander Potapenko <glider@google.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 net/sctp/ipv6.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index a6dfa86c0201..3b18085e3b10 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
 		addr->v6.sin6_flowinfo = 0;
 		addr->v6.sin6_port = sh->source;
 		addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
-		if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+		if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
 			addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
-		}
+		else
+			addr->v6.sin6_scope_id = 0;
 	}
 
 	*addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
-- 
2.14.1

[PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname

[Eric W. Biederman]

Alexandar Potapenko while testing the kernel with KMSAN and syzkaller
discovered that in some configurations sctp would leak 4 bytes of
kernel stack.

Working with his reproducer I discovered that those 4 bytes that
are leaked is the scope id of an ipv6 address returned by recvmsg.

With a little code inspection and a shrewd guess I discovered that
sctp_inet6_skb_msgname only initializes the scope_id field for link
local ipv6 addresses to the interface index the link local address
pertains to instead of initializing the scope_id field for all ipv6
addresses.

That is almost reasonable as scope_id's are meaniningful only for link
local addresses.  Set the scope_id in all other cases to 0 which is
not a valid interface index to make it clear there is nothing useful
in the scope_id field.

There should be no danger of breaking userspace as the stack leak
guaranteed that previously meaningless random data was being returned.

Cc: stable@vger.kernel.org
Fixes: 372f525b495c ("SCTP:  Resync with LKSCTP tree.")
History-tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
Reported-by: Alexander Potapenko <glider@google.com>
Tested-by: Alexander Potapenko <glider@google.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 net/sctp/ipv6.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index a6dfa86c0201..3b18085e3b10 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -807,9 +807,10 @@ static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname,
 		addr->v6.sin6_flowinfo = 0;
 		addr->v6.sin6_port = sh->source;
 		addr->v6.sin6_addr = ipv6_hdr(skb)->saddr;
-		if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL) {
+		if (ipv6_addr_type(&addr->v6.sin6_addr) & IPV6_ADDR_LINKLOCAL)
 			addr->v6.sin6_scope_id = sctp_v6_skb_iif(skb);
-		}
+		else
+			addr->v6.sin6_scope_id = 0;
 	}
 
 	*addr_len = sctp_v6_addr_to_user(sctp_sk(skb->sk), addr);
-- 
2.14.1

Re: [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Wed, 15 Nov 2017 22:17:48 -0600

> 
> Alexandar Potapenko while testing the kernel with KMSAN and syzkaller
> discovered that in some configurations sctp would leak 4 bytes of
> kernel stack.
> 
> Working with his reproducer I discovered that those 4 bytes that
> are leaked is the scope id of an ipv6 address returned by recvmsg.
> 
> With a little code inspection and a shrewd guess I discovered that
> sctp_inet6_skb_msgname only initializes the scope_id field for link
> local ipv6 addresses to the interface index the link local address
> pertains to instead of initializing the scope_id field for all ipv6
> addresses.
> 
> That is almost reasonable as scope_id's are meaniningful only for link
> local addresses.  Set the scope_id in all other cases to 0 which is
> not a valid interface index to make it clear there is nothing useful
> in the scope_id field.
> 
> There should be no danger of breaking userspace as the stack leak
> guaranteed that previously meaningless random data was being returned.
> 
> Fixes: 372f525b495c ("SCTP:  Resync with LKSCTP tree.")
> History-tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> Reported-by: Alexander Potapenko <glider@google.com>
> Tested-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Applied and queued up for -stable, thanks Eric.

Re: [PATCH net] net/sctp: Always set scope_id in sctp_inet6_skb_msgname

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Wed, 15 Nov 2017 22:17:48 -0600

> 
> Alexandar Potapenko while testing the kernel with KMSAN and syzkaller
> discovered that in some configurations sctp would leak 4 bytes of
> kernel stack.
> 
> Working with his reproducer I discovered that those 4 bytes that
> are leaked is the scope id of an ipv6 address returned by recvmsg.
> 
> With a little code inspection and a shrewd guess I discovered that
> sctp_inet6_skb_msgname only initializes the scope_id field for link
> local ipv6 addresses to the interface index the link local address
> pertains to instead of initializing the scope_id field for all ipv6
> addresses.
> 
> That is almost reasonable as scope_id's are meaniningful only for link
> local addresses.  Set the scope_id in all other cases to 0 which is
> not a valid interface index to make it clear there is nothing useful
> in the scope_id field.
> 
> There should be no danger of breaking userspace as the stack leak
> guaranteed that previously meaningless random data was being returned.
> 
> Fixes: 372f525b495c ("SCTP:  Resync with LKSCTP tree.")
> History-tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> Reported-by: Alexander Potapenko <glider@google.com>
> Tested-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Applied and queued up for -stable, thanks Eric.