Received: by 2002:ac0:bc90:0:0:0:0:0 with SMTP id a16csp579560img; Fri, 22 Mar 2019 04:27:29 -0700 (PDT) X-Google-Smtp-Source: APXvYqyzh/Q0065114GqoVRkH/7Oqk01g4R2OSTkWY5fKmSgzpuQDS1qk6njdoN2P0L3EhpSn8CE X-Received: by 2002:a17:902:6aca:: with SMTP id i10mr8886252plt.43.1553254049254; Fri, 22 Mar 2019 04:27:29 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1553254049; cv=none; d=google.com; s=arc-20160816; b=Zdmo6BVZfZAh9+05RfNCWMRymG0dQHz2JcNQxBtiYUTcc+s/hxiL5nYijZaRtXsB3z pf5oKVYcTzOqVjwkWqk73Y3Zo6h7yVOhH6bvxS9zZhx+hCThf/Xrm458ciE38oFeqHyD ATbaCJyXNdKbQrDuitF+iQjwWWc2RGrtY0xRY3qmqq8/F4WnwVuzOAuSf5fkKpMKmbJC v4TZU2qcC2NNVaGQUUwkB6MMSL06FiCRGlAFh1fcAeS0URDIQn2Bmhv/61bsO+7i3U7B cp87L4raoeaJR1haoIt3o0cE1rOmw/qGQ39717i3eI3K/VP1vC5qXDEpd1PBz1f1yF4t RKZQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:message-id:date:subject:cc:to :from:dkim-signature; bh=qxiyZGAYlrrtcn8EWOtPn4aKS69kPd0C3vQnT1A48MQ=; b=dzYXBctYufQbUycWlvzEJP1rEIpln4gZpK6xh4BVz9hFaeDlGUQa3sTOQOznEMXTj6 n07DNhGzKjvPvy/gieksrZEew4UIG0s0n+dulhrJN5JugonO2q6wD+ChOSsg+C11Cs1J fvfd+Uy4M1FnbdIWsoy9CUi2TXK8+O4EkRJVguzr629T6Vh9g2AoNy/gjc5GGzhOmigP Z/D/Xq3/IGYuyO8TePS75GzdGURoFpo42Lo3cjjcl4Wq2WDFJsYiTcAaBV0dRb2jlWwj +rauOGEVDgc/HvJHwjQYmoTt7HttgBC/jPjstDvzBRyxJjG7TfccZep8J1gO5J575RKk E17w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=E26OgH2a; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id y2si6652018pll.133.2019.03.22.04.27.11; Fri, 22 Mar 2019 04:27:29 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=E26OgH2a; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728709AbfCVLYI (ORCPT + 99 others); Fri, 22 Mar 2019 07:24:08 -0400 Received: from mail.kernel.org ([198.145.29.99]:51126 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728136AbfCVLYG (ORCPT ); Fri, 22 Mar 2019 07:24:06 -0400 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 00BC7218A2; Fri, 22 Mar 2019 11:24:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1553253844; bh=/ZT+H/HjNOUUKjEy/+RlDZ/QuNraiGdh4/dDGOlgeO4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=E26OgH2aGi0sd2uO5AM+ZA4R/g2Csou45iPVYZiqgedWhA1NFi39jKFqXa0U52zcV lTpXpA5P5SUDifmSMa+U0469bPcduZdwtoshQL/8OSO42qqKucm+nCVa3BlUVkD/Nv T+ANE7kyWFlt7jC4IkL9vSbdYI1U1Xu/Fr/MwRkk= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Al Viro , "David S. Miller" , "Paul E. McKenney" Subject: [PATCH 3.18 089/134] missing barriers in some of unix_sock ->addr and ->path accesses Date: Fri, 22 Mar 2019 12:15:02 +0100 Message-Id: <20190322111216.568536816@linuxfoundation.org> X-Mailer: git-send-email 2.21.0 In-Reply-To: <20190322111210.465931067@linuxfoundation.org> References: <20190322111210.465931067@linuxfoundation.org> User-Agent: quilt/0.65 X-stable: review X-Patchwork-Hint: ignore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 3.18-stable review patch. If anyone has any objections, please let me know. ------------------ From: Al Viro [ Upstream commit ae3b564179bfd06f32d051b9e5d72ce4b2a07c37 ] Several u->addr and u->path users are not holding any locks in common with unix_bind(). unix_state_lock() is useless for those purposes. u->addr is assign-once and *(u->addr) is fully set up by the time we set u->addr (all under unix_table_lock). u->path is also set in the same critical area, also before setting u->addr, and any unix_sock with ->path filled will have non-NULL ->addr. So setting ->addr with smp_store_release() is all we need for those "lockless" users - just have them fetch ->addr with smp_load_acquire() and don't even bother looking at ->path if they see NULL ->addr. Users of ->addr and ->path fall into several classes now: 1) ones that do smp_load_acquire(u->addr) and access *(u->addr) and u->path only if smp_load_acquire() has returned non-NULL. 2) places holding unix_table_lock. These are guaranteed that *(u->addr) is seen fully initialized. If unix_sock is in one of the "bound" chains, so's ->path. 3) unix_sock_destructor() using ->addr is safe. All places that set u->addr are guaranteed to have seen all stores *(u->addr) while holding a reference to u and unix_sock_destructor() is called when (atomic) refcount hits zero. 4) unix_release_sock() using ->path is safe. unix_bind() is serialized wrt unix_release() (normally - by struct file refcount), and for the instances that had ->path set by unix_bind() unix_release_sock() comes from unix_release(), so they are fine. Instances that had it set in unix_stream_connect() either end up attached to a socket (in unix_accept()), in which case the call chain to unix_release_sock() and serialization are the same as in the previous case, or they never get accept'ed and unix_release_sock() is called when the listener is shut down and its queue gets purged. In that case the listener's queue lock provides the barriers needed - unix_stream_connect() shoves our unix_sock into listener's queue under that lock right after having set ->path and eventual unix_release_sock() caller picks them from that queue under the same lock right before calling unix_release_sock(). 5) unix_find_other() use of ->path is pointless, but safe - it happens with successful lookup by (abstract) name, so ->path.dentry is guaranteed to be NULL there. earlier-variant-reviewed-by: "Paul E. McKenney" Signed-off-by: Al Viro Signed-off-by: David S. Miller Signed-off-by: Greg Kroah-Hartman --- net/unix/af_unix.c | 48 +++++++++++++++++++++++++++++------------------- net/unix/diag.c | 3 ++- security/lsm_audit.c | 10 ++++++---- 3 files changed, 37 insertions(+), 24 deletions(-) --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -876,7 +876,7 @@ retry: addr->hash ^= sk->sk_type; __unix_remove_socket(sk); - u->addr = addr; + smp_store_release(&u->addr, addr); __unix_insert_socket(&unix_socket_table[addr->hash], sk); spin_unlock(&unix_table_lock); err = 0; @@ -1045,7 +1045,7 @@ static int unix_bind(struct socket *sock err = 0; __unix_remove_socket(sk); - u->addr = addr; + smp_store_release(&u->addr, addr); __unix_insert_socket(list, sk); out_unlock: @@ -1312,15 +1312,29 @@ restart: RCU_INIT_POINTER(newsk->sk_wq, &newu->peer_wq); otheru = unix_sk(other); - /* copy address information from listening to new sock*/ - if (otheru->addr) { - atomic_inc(&otheru->addr->refcnt); - newu->addr = otheru->addr; - } + /* copy address information from listening to new sock + * + * The contents of *(otheru->addr) and otheru->path + * are seen fully set up here, since we have found + * otheru in hash under unix_table_lock. Insertion + * into the hash chain we'd found it in had been done + * in an earlier critical area protected by unix_table_lock, + * the same one where we'd set *(otheru->addr) contents, + * as well as otheru->path and otheru->addr itself. + * + * Using smp_store_release() here to set newu->addr + * is enough to make those stores, as well as stores + * to newu->path visible to anyone who gets newu->addr + * by smp_load_acquire(). IOW, the same warranties + * as for unix_sock instances bound in unix_bind() or + * in unix_autobind(). + */ if (otheru->path.dentry) { path_get(&otheru->path); newu->path = otheru->path; } + atomic_inc(&otheru->addr->refcnt); + smp_store_release(&newu->addr, otheru->addr); /* Set credentials */ copy_peercred(sk, other); @@ -1433,7 +1447,7 @@ out: static int unix_getname(struct socket *sock, struct sockaddr *uaddr, int *uaddr_len, int peer) { struct sock *sk = sock->sk; - struct unix_sock *u; + struct unix_address *addr; DECLARE_SOCKADDR(struct sockaddr_un *, sunaddr, uaddr); int err = 0; @@ -1448,19 +1462,15 @@ static int unix_getname(struct socket *s sock_hold(sk); } - u = unix_sk(sk); - unix_state_lock(sk); - if (!u->addr) { + addr = smp_load_acquire(&unix_sk(sk)->addr); + if (!addr) { sunaddr->sun_family = AF_UNIX; sunaddr->sun_path[0] = 0; *uaddr_len = sizeof(short); } else { - struct unix_address *addr = u->addr; - *uaddr_len = addr->len; memcpy(sunaddr, addr->name, *uaddr_len); } - unix_state_unlock(sk); sock_put(sk); out: return err; @@ -1936,11 +1946,11 @@ static int unix_seqpacket_recvmsg(struct static void unix_copy_addr(struct msghdr *msg, struct sock *sk) { - struct unix_sock *u = unix_sk(sk); + struct unix_address *addr = smp_load_acquire(&unix_sk(sk)->addr); - if (u->addr) { - msg->msg_namelen = u->addr->len; - memcpy(msg->msg_name, u->addr->name, u->addr->len); + if (addr) { + msg->msg_namelen = addr->len; + memcpy(msg->msg_name, addr->name, addr->len); } } @@ -2551,7 +2561,7 @@ static int unix_seq_show(struct seq_file (s->sk_state == TCP_ESTABLISHED ? SS_CONNECTING : SS_DISCONNECTING), sock_i_ino(s)); - if (u->addr) { + if (u->addr) { // under unix_table_lock here int i, len; seq_putc(seq, ' '); --- a/net/unix/diag.c +++ b/net/unix/diag.c @@ -10,7 +10,8 @@ static int sk_diag_dump_name(struct sock *sk, struct sk_buff *nlskb) { - struct unix_address *addr = unix_sk(sk)->addr; + /* might or might not have unix_table_lock */ + struct unix_address *addr = smp_load_acquire(&unix_sk(sk)->addr); if (!addr) return 0; --- a/security/lsm_audit.c +++ b/security/lsm_audit.c @@ -290,6 +290,7 @@ static void dump_common_audit_data(struc if (a->u.net->sk) { struct sock *sk = a->u.net->sk; struct unix_sock *u; + struct unix_address *addr; int len = 0; char *p = NULL; @@ -320,14 +321,15 @@ static void dump_common_audit_data(struc #endif case AF_UNIX: u = unix_sk(sk); + addr = smp_load_acquire(&u->addr); + if (!addr) + break; if (u->path.dentry) { audit_log_d_path(ab, " path=", &u->path); break; } - if (!u->addr) - break; - len = u->addr->len-sizeof(short); - p = &u->addr->name->sun_path[0]; + len = addr->len-sizeof(short); + p = &addr->name->sun_path[0]; audit_log_format(ab, " path="); if (*p) audit_log_untrustedstring(ab, p);