net: recvmsg: Unconditionally zero struct sockaddr_storage

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 <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

net/socket.c +0 -0

index c729625eb5d3..34183f4fbdf8 100644

Quoted 21 lines · 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 <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

net/socket.c +0 -0

index c729625eb5d3..34183f4fbdf8 100644

net/socket.c static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, · line 2193

Quoted 6 lines

net/socket.c +1 -0

@@ -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.

Quoted 34 lines · Eric Dumazet

Quoted 21 lines · 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 <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

net/socket.c +0 -0

index c729625eb5d3..34183f4fbdf8 100644

net/socket.c static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, · line 2193

Quoted 6 lines

net/socket.c +1 -0

@@ -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.

Quoted 7 lines

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:

net/sctp/ipv6.c +3 -2

index 51c488769590..6301913d0516 100644
@@ -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

Quoted 73 lines · Eric W. Biederman

Quoted 34 lines · Eric Dumazet

Quoted 21 lines · 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 <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

net/socket.c +0 -0

index c729625eb5d3..34183f4fbdf8 100644

net/socket.c static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, · line 2193

Quoted 6 lines

net/socket.c +1 -0

@@ -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.

Quoted 7 lines

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:

net/sctp/ipv6.c +3 -2

index 51c488769590..6301913d0516 100644
@@ -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;
        }
 

Quoted 4 lines

net/sctp/ipv6.c +0 -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

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 <[email protected]>

Tested-by Alexander Potapenko <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

Quoted 34 lines · Eric Dumazet

Quoted 21 lines · 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 <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

net/socket.c +0 -0

index c729625eb5d3..34183f4fbdf8 100644

net/socket.c static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, · line 2193

Quoted 6 lines

net/socket.c +1 -0

@@ -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.

Quoted 7 lines

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:

net/sctp/ipv6.c +0 -0

index 51c488769590..6301913d0516 100644
@@ -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;

Quoted 73 lines · Eric W. Biederman

Quoted 34 lines · Eric Dumazet

Quoted 21 lines · 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 <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

net/socket.c +0 -0

index c729625eb5d3..34183f4fbdf8 100644

net/socket.c static int ___sys_recvmsg(struct socket *sock, struct user_msghdr __user *msg, · line 2193

Quoted 6 lines

net/socket.c +1 -0

@@ -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.

Quoted 7 lines

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:

net/sctp/ipv6.c +3 -2

index 51c488769590..6301913d0516 100644
@@ -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;
        }
 

net/sctp/ipv6.c static void sctp_inet6_skb_msgname(struct sk_buff *skb, char *msgname, · line 816

Quoted 2 lines

net/sctp/ipv6.c +0 -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?

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 <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

net/socket.c +0 -0

index c729625eb5d3..34183f4fbdf8 100644

net/socket.c +0 -0

 	msg_sys->msg_name = &addr;
 
 	if (MSG_CMSG_COMPAT & flags)

Signature · 1 line

2.7.4