Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964865AbdCYASr (ORCPT ); Fri, 24 Mar 2017 20:18:47 -0400 Received: from mother.openwall.net ([195.42.179.200]:60349 "HELO mother.openwall.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752284AbdCYASi (ORCPT ); Fri, 24 Mar 2017 20:18:38 -0400 Date: Sat, 25 Mar 2017 01:10:57 +0100 From: Solar Designer To: oss-security@lists.openwall.com Cc: Eric Dumazet , Andrey Konovalov , "David S. Miller" , Alexey Kuznetsov , James Morris , Hideaki YOSHIFUJI , Patrick McHardy , netdev , LKML , Vasily Kulikov Subject: Re: [oss-security] Linux kernel ping socket / AF_LLC connect() sin_family race Message-ID: <20170325001057.GA31046@openwall.com> References: <20170324202714.GA29241@openwall.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2450 Lines: 67 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