[PATCH 0/1][SRU][T] CVE-2018-9568 - Networking socket memory corruption

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/1][SRU][T] CVE-2018-9568 - Networking socket memory corruption

Tyler Hicks-2
https://people.canonical.com/~ubuntu-security/cve/2018/CVE-2018-9568.html

  Memory corruption due to incorrect socket cloning

I've tested this change with the test program from the patch's commit
message. Clean cherry pick to Trusty.

Tyler

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/1] net: Set sk_prot_creator when cloning sockets to the right proto

Tyler Hicks-2
From: Christoph Paasch <[hidden email]>

sk->sk_prot and sk->sk_prot_creator can differ when the app uses
IPV6_ADDRFORM (transforming an IPv6-socket to an IPv4-one).
Which is why sk_prot_creator is there to make sure that sk_prot_free()
does the kmem_cache_free() on the right kmem_cache slab.

Now, if such a socket gets transformed back to a listening socket (using
connect() with AF_UNSPEC) we will allocate an IPv4 tcp_sock through
sk_clone_lock() when a new connection comes in. But sk_prot_creator will
still point to the IPv6 kmem_cache (as everything got copied in
sk_clone_lock()). When freeing, we will thus put this
memory back into the IPv6 kmem_cache although it was allocated in the
IPv4 cache. I have seen memory corruption happening because of this.

With slub-debugging and MEMCG_KMEM enabled this gives the warning
        "cache_from_obj: Wrong slab cache. TCPv6 but object is from TCP"

A C-program to trigger this:

void main(void)
{
        int fd = socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP);
        int new_fd, newest_fd, client_fd;
        struct sockaddr_in6 bind_addr;
        struct sockaddr_in bind_addr4, client_addr1, client_addr2;
        struct sockaddr unsp;
        int val;

        memset(&bind_addr, 0, sizeof(bind_addr));
        bind_addr.sin6_family = AF_INET6;
        bind_addr.sin6_port = ntohs(42424);

        memset(&client_addr1, 0, sizeof(client_addr1));
        client_addr1.sin_family = AF_INET;
        client_addr1.sin_port = ntohs(42424);
        client_addr1.sin_addr.s_addr = inet_addr("127.0.0.1");

        memset(&client_addr2, 0, sizeof(client_addr2));
        client_addr2.sin_family = AF_INET;
        client_addr2.sin_port = ntohs(42421);
        client_addr2.sin_addr.s_addr = inet_addr("127.0.0.1");

        memset(&unsp, 0, sizeof(unsp));
        unsp.sa_family = AF_UNSPEC;

        bind(fd, (struct sockaddr *)&bind_addr, sizeof(bind_addr));

        listen(fd, 5);

        client_fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
        connect(client_fd, (struct sockaddr *)&client_addr1, sizeof(client_addr1));
        new_fd = accept(fd, NULL, NULL);
        close(fd);

        val = AF_INET;
        setsockopt(new_fd, SOL_IPV6, IPV6_ADDRFORM, &val, sizeof(val));

        connect(new_fd, &unsp, sizeof(unsp));

        memset(&bind_addr4, 0, sizeof(bind_addr4));
        bind_addr4.sin_family = AF_INET;
        bind_addr4.sin_port = ntohs(42421);
        bind(new_fd, (struct sockaddr *)&bind_addr4, sizeof(bind_addr4));

        listen(new_fd, 5);

        client_fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
        connect(client_fd, (struct sockaddr *)&client_addr2, sizeof(client_addr2));

        newest_fd = accept(new_fd, NULL, NULL);
        close(new_fd);

        close(client_fd);
        close(new_fd);
}

As far as I can see, this bug has been there since the beginning of the
git-days.

Signed-off-by: Christoph Paasch <[hidden email]>
Reviewed-by: Eric Dumazet <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>

CVE-2018-9568

(cherry picked from commit 9d538fa60bad4f7b23193c89e843797a1cf71ef3)
Signed-off-by: Tyler Hicks <[hidden email]>
---
 net/core/sock.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/sock.c b/net/core/sock.c
index a2702d5d03bf..871e98eb8dd1 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1511,6 +1511,8 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
 
  sock_copy(newsk, sk);
 
+ newsk->sk_prot_creator = sk->sk_prot;
+
  /* SANITY */
  get_net(sock_net(newsk));
  sk_node_init(&newsk->sk_node);
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 1/1] net: Set sk_prot_creator when cloning sockets to the right proto

Kleber Souza
On 12/6/18 10:59 PM, Tyler Hicks wrote:

> From: Christoph Paasch <[hidden email]>
>
> sk->sk_prot and sk->sk_prot_creator can differ when the app uses
> IPV6_ADDRFORM (transforming an IPv6-socket to an IPv4-one).
> Which is why sk_prot_creator is there to make sure that sk_prot_free()
> does the kmem_cache_free() on the right kmem_cache slab.
>
> Now, if such a socket gets transformed back to a listening socket (using
> connect() with AF_UNSPEC) we will allocate an IPv4 tcp_sock through
> sk_clone_lock() when a new connection comes in. But sk_prot_creator will
> still point to the IPv6 kmem_cache (as everything got copied in
> sk_clone_lock()). When freeing, we will thus put this
> memory back into the IPv6 kmem_cache although it was allocated in the
> IPv4 cache. I have seen memory corruption happening because of this.
>
> With slub-debugging and MEMCG_KMEM enabled this gives the warning
> "cache_from_obj: Wrong slab cache. TCPv6 but object is from TCP"
>
> A C-program to trigger this:
>
> void main(void)
> {
>         int fd = socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP);
>         int new_fd, newest_fd, client_fd;
>         struct sockaddr_in6 bind_addr;
>         struct sockaddr_in bind_addr4, client_addr1, client_addr2;
>         struct sockaddr unsp;
>         int val;
>
>         memset(&bind_addr, 0, sizeof(bind_addr));
>         bind_addr.sin6_family = AF_INET6;
>         bind_addr.sin6_port = ntohs(42424);
>
>         memset(&client_addr1, 0, sizeof(client_addr1));
>         client_addr1.sin_family = AF_INET;
>         client_addr1.sin_port = ntohs(42424);
>         client_addr1.sin_addr.s_addr = inet_addr("127.0.0.1");
>
>         memset(&client_addr2, 0, sizeof(client_addr2));
>         client_addr2.sin_family = AF_INET;
>         client_addr2.sin_port = ntohs(42421);
>         client_addr2.sin_addr.s_addr = inet_addr("127.0.0.1");
>
>         memset(&unsp, 0, sizeof(unsp));
>         unsp.sa_family = AF_UNSPEC;
>
>         bind(fd, (struct sockaddr *)&bind_addr, sizeof(bind_addr));
>
>         listen(fd, 5);
>
>         client_fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
>         connect(client_fd, (struct sockaddr *)&client_addr1, sizeof(client_addr1));
>         new_fd = accept(fd, NULL, NULL);
>         close(fd);
>
>         val = AF_INET;
>         setsockopt(new_fd, SOL_IPV6, IPV6_ADDRFORM, &val, sizeof(val));
>
>         connect(new_fd, &unsp, sizeof(unsp));
>
>         memset(&bind_addr4, 0, sizeof(bind_addr4));
>         bind_addr4.sin_family = AF_INET;
>         bind_addr4.sin_port = ntohs(42421);
>         bind(new_fd, (struct sockaddr *)&bind_addr4, sizeof(bind_addr4));
>
>         listen(new_fd, 5);
>
>         client_fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
>         connect(client_fd, (struct sockaddr *)&client_addr2, sizeof(client_addr2));
>
>         newest_fd = accept(new_fd, NULL, NULL);
>         close(new_fd);
>
>         close(client_fd);
>         close(new_fd);
> }
>
> As far as I can see, this bug has been there since the beginning of the
> git-days.
>
> Signed-off-by: Christoph Paasch <[hidden email]>
> Reviewed-by: Eric Dumazet <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
>
> CVE-2018-9568
>
> (cherry picked from commit 9d538fa60bad4f7b23193c89e843797a1cf71ef3)
> Signed-off-by: Tyler Hicks <[hidden email]>
Acked-by: Kleber Sacilotto de Souza <[hidden email]>

> ---
>  net/core/sock.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index a2702d5d03bf..871e98eb8dd1 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1511,6 +1511,8 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
>  
>   sock_copy(newsk, sk);
>  
> + newsk->sk_prot_creator = sk->sk_prot;
> +
>   /* SANITY */
>   get_net(sock_net(newsk));
>   sk_node_init(&newsk->sk_node);



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 1/1] net: Set sk_prot_creator when cloning sockets to the right proto

Po-Hsu Lin (Sam)
In reply to this post by Tyler Hicks-2
Clean cherry-pick.
Acked-by: Po-Hsu Lin <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: ACK: [PATCH 1/1] net: Set sk_prot_creator when cloning sockets to the right proto

Evgenii Shatokhin
In reply to this post by Kleber Souza
Hi,

On 18.12.2018 20:00, Kleber Souza wrote:

> On 12/6/18 10:59 PM, Tyler Hicks wrote:
>> From: Christoph Paasch <[hidden email]>
>>
>> sk->sk_prot and sk->sk_prot_creator can differ when the app uses
>> IPV6_ADDRFORM (transforming an IPv6-socket to an IPv4-one).
>> Which is why sk_prot_creator is there to make sure that sk_prot_free()
>> does the kmem_cache_free() on the right kmem_cache slab.
>>
>> Now, if such a socket gets transformed back to a listening socket (using
>> connect() with AF_UNSPEC) we will allocate an IPv4 tcp_sock through
>> sk_clone_lock() when a new connection comes in. But sk_prot_creator will
>> still point to the IPv6 kmem_cache (as everything got copied in
>> sk_clone_lock()). When freeing, we will thus put this
>> memory back into the IPv6 kmem_cache although it was allocated in the
>> IPv4 cache. I have seen memory corruption happening because of this.
>>
>> With slub-debugging and MEMCG_KMEM enabled this gives the warning
>> "cache_from_obj: Wrong slab cache. TCPv6 but object is from TCP"
>>
>> A C-program to trigger this:
>>
>> void main(void)
>> {
>>          int fd = socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP);
>>          int new_fd, newest_fd, client_fd;
>>          struct sockaddr_in6 bind_addr;
>>          struct sockaddr_in bind_addr4, client_addr1, client_addr2;
>>          struct sockaddr unsp;
>>          int val;
>>
>>          memset(&bind_addr, 0, sizeof(bind_addr));
>>          bind_addr.sin6_family = AF_INET6;
>>          bind_addr.sin6_port = ntohs(42424);
>>
>>          memset(&client_addr1, 0, sizeof(client_addr1));
>>          client_addr1.sin_family = AF_INET;
>>          client_addr1.sin_port = ntohs(42424);
>>          client_addr1.sin_addr.s_addr = inet_addr("127.0.0.1");
>>
>>          memset(&client_addr2, 0, sizeof(client_addr2));
>>          client_addr2.sin_family = AF_INET;
>>          client_addr2.sin_port = ntohs(42421);
>>          client_addr2.sin_addr.s_addr = inet_addr("127.0.0.1");
>>
>>          memset(&unsp, 0, sizeof(unsp));
>>          unsp.sa_family = AF_UNSPEC;
>>
>>          bind(fd, (struct sockaddr *)&bind_addr, sizeof(bind_addr));
>>
>>          listen(fd, 5);
>>
>>          client_fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
>>          connect(client_fd, (struct sockaddr *)&client_addr1, sizeof(client_addr1));
>>          new_fd = accept(fd, NULL, NULL);
>>          close(fd);
>>
>>          val = AF_INET;
>>          setsockopt(new_fd, SOL_IPV6, IPV6_ADDRFORM, &val, sizeof(val));
>>
>>          connect(new_fd, &unsp, sizeof(unsp));
>>
>>          memset(&bind_addr4, 0, sizeof(bind_addr4));
>>          bind_addr4.sin_family = AF_INET;
>>          bind_addr4.sin_port = ntohs(42421);
>>          bind(new_fd, (struct sockaddr *)&bind_addr4, sizeof(bind_addr4));
>>
>>          listen(new_fd, 5);
>>
>>          client_fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
>>          connect(client_fd, (struct sockaddr *)&client_addr2, sizeof(client_addr2));
>>
>>          newest_fd = accept(new_fd, NULL, NULL);
>>          close(new_fd);
>>
>>          close(client_fd);
>>          close(new_fd);
>> }
>>
>> As far as I can see, this bug has been there since the beginning of the
>> git-days.
>>
>> Signed-off-by: Christoph Paasch <[hidden email]>
>> Reviewed-by: Eric Dumazet <[hidden email]>
>> Signed-off-by: David S. Miller <[hidden email]>
>>
>> CVE-2018-9568
>>
>> (cherry picked from commit 9d538fa60bad4f7b23193c89e843797a1cf71ef3)
>> Signed-off-by: Tyler Hicks <[hidden email]>
> Acked-by: Kleber Sacilotto de Souza <[hidden email]>

If you plan to prepare a livepatch for Xenial with this fix, please
consider this issue: https://github.com/dynup/kpatch/issues/931. It
might affect livepatches for at least the kernel 4.4.0-x from Xenial as
well.

I haven't checked that kernel yet but if sk_update_clone() is inlined
into sk_clone_lock() there too, the static key condition used in
sk_update_clone() could be broken in the patched code and always
evaluate to false:

        if (mem_cgroup_sockets_enabled && sk->sk_cgrp)
                sock_update_memcg(newsk);

include/net/sock.h:
#define mem_cgroup_sockets_enabled
static_key_false(&memcg_socket_limit_enabled)

This could result in hard-to-debug kernel crashes due to incorrect
socket accounting in memcg when livepatch is used.

I have mentioned the workaround in that issue at Github: just change
that condition in sk_update_clone() too in the livepatch to check the
static key explicitly, smth. like this:

--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1470,7 +1470,7 @@ EXPORT_SYMBOL(sk_release_kernel);

  static void sk_update_clone(const struct sock *sk, struct sock *newsk)
  {
- if (mem_cgroup_sockets_enabled && sk->sk_cgrp)
+ if (static_key_enabled(&memcg_socket_limit_enabled) && sk->sk_cgrp)
  sock_update_memcg(newsk);
  }

It seems, Bionic kernels 4.15.0-x are not affected, but I am not 100% sure.

Regards,
Evgenii

>> ---
>>   net/core/sock.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index a2702d5d03bf..871e98eb8dd1 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -1511,6 +1511,8 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
>>  
>>   sock_copy(newsk, sk);
>>  
>> + newsk->sk_prot_creator = sk->sk_prot;
>> +
>>   /* SANITY */
>>   get_net(sock_net(newsk));
>>   sk_node_init(&newsk->sk_node);
>
>
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [PATCH 0/1][SRU][T] CVE-2018-9568 - Networking socket memory corruption

Khaled Elmously
In reply to this post by Tyler Hicks-2

On 2018-12-06 21:59:03 , Tyler Hicks wrote:

> https://people.canonical.com/~ubuntu-security/cve/2018/CVE-2018-9568.html
>
>   Memory corruption due to incorrect socket cloning
>
> I've tested this change with the test program from the patch's commit
> message. Clean cherry pick to Trusty.
>
> Tyler
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team