From patchwork Mon Jul 17 09:35:54 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Herrmann X-Patchwork-Id: 789340 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3x9yr3282kz9t1t for ; Mon, 17 Jul 2017 19:37:23 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="PJ0UJW2P"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751317AbdGQJhL (ORCPT ); Mon, 17 Jul 2017 05:37:11 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35454 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751298AbdGQJhI (ORCPT ); Mon, 17 Jul 2017 05:37:08 -0400 Received: by mail-wm0-f68.google.com with SMTP id u23so21993855wma.2; Mon, 17 Jul 2017 02:37:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=6vB8aHh2Bww1NogJ85/EtU5B4T4FNSXP0bFiPLnQoN8=; b=PJ0UJW2Pp069lPsOLXIjOslTw+T97hhqCinv580z0CgzvoXASbnS1Wi0PSvMR9vXJU EtbI8Q6r6ciR8HFldilfpgoTA+6xMb1fC+Ajb0Ncjl/Z0m0DOWkaonYBPbEkYDJnU0Xo QjP8yUy0LYguYvG0b6atF1u+IyNFRcvTG08BR7SyR3DPbyNcJKxqAiTJh5iGXy365eUO DhNftvHZPBpwQDDlWalr/hczepctaXVIirTwyxCHe7wVv0mnVISpSReGTN1sJn1R8v7b l+WaP9INPndJQp/UYer6lr8Nxtt9QmsCvNSfQLSirlT4IYNqAQHcii0AChlQJbHupDxo l6LQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=6vB8aHh2Bww1NogJ85/EtU5B4T4FNSXP0bFiPLnQoN8=; b=WhM7e2lVl7EOBek6C+KjBYUcBiRfKIJhDea/Jxa1fftG4hJfs1CTOOUabgWiPcLZZA 9CoEFVeoFI+qwICIChy8BCjR2GqVGuTUH/ZkHAzVit82h7kHULJOoUpAQFNP7Lov2DXa nIYXaOD1YIjOfDswElatvmvOQCZiB6TrSBxpLYSqapEyr42mmALfxPWf5YAQxZoS5QcE MS29A/oZi9Jd2Msi2sMPu8MXT5aNhQvyoy5SMOeOOHhTen8oXitCYPNmrOh6sEqEEnwb S2o4OMS627Eh33/7XUqJFK6ccdLo+e3AunrvTDFlrrEfz4UKtXQD2X3edaj1krZEWAtR FuiA== X-Gm-Message-State: AIVw113I4xH+Og98xyvUAEqejpANLh/k1T/f6oTz1Ryj7Qhm49OMsGLa XBhOFoUGDy6dAI8y X-Received: by 10.28.87.82 with SMTP id l79mr2573850wmb.73.1500284227060; Mon, 17 Jul 2017 02:37:07 -0700 (PDT) Received: from localhost.localdomain (p200300C2A3C9CC0073D1015CDDF3BEE7.dip0.t-ipconnect.de. [2003:c2:a3c9:cc00:73d1:15c:ddf3:bee7]) by smtp.gmail.com with ESMTPSA id 1sm9757324wmn.32.2017.07.17.02.37.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 17 Jul 2017 02:37:06 -0700 (PDT) From: David Herrmann To: netdev@vger.kernel.org Cc: Tom Gundersen , davem@davemloft.net, Eric Dumazet , Hannes Frederic Sowa , linux-kernel@vger.kernel.org, David Herrmann , Alban Crequy , Simon McVittie Subject: [PATCH] net/unix: drop obsolete fd-recursion limits Date: Mon, 17 Jul 2017 11:35:54 +0200 Message-Id: <20170717093554.16459-1-dh.herrmann@gmail.com> X-Mailer: git-send-email 2.13.2 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org All unix sockets now account inflight FDs to the respective sender. This was introduced in: commit 712f4aad406bb1ed67f3f98d04c044191f0ff593 Author: willy tarreau Date: Sun Jan 10 07:54:56 2016 +0100 unix: properly account for FDs passed over unix sockets and further refined in: commit 415e3d3e90ce9e18727e8843ae343eda5a58fad6 Author: Hannes Frederic Sowa Date: Wed Feb 3 02:11:03 2016 +0100 unix: correctly track in-flight fds in sending process user_struct Hence, regardless of the stacking depth of FDs, the total number of inflight FDs is limited, and accounted. There is no known way for a local user to exceed those limits or exploit the accounting. Furthermore, the GC logic is independent of the recursion/stacking depth as well. It solely depends on the total number of inflight FDs, regardless of their layout. Lastly, the current `recursion_level' suffers a TOCTOU race, since it checks and inherits depths only at queue time. If we consider `A<-B' to mean `queue-B-on-A', the following sequence circumvents the recursion level easily: A<-B B<-C C<-D ... Y<-Z resulting in: A<-B<-C<-...<-Z With all of this in mind, lets drop the recursion limit. It has no additional security value, anymore. On the contrary, it randomly confuses message brokers that try to forward file-descriptors, since any sendmsg(2) call can fail spuriously with ETOOMANYREFS if a client maliciously modifies the FD while inflight. Cc: Alban Crequy Cc: Simon McVittie Signed-off-by: David Herrmann Reviewed-by: Tom Gundersen --- include/net/af_unix.h | 1 - net/unix/af_unix.c | 24 +----------------------- 2 files changed, 1 insertion(+), 24 deletions(-) diff --git a/include/net/af_unix.h b/include/net/af_unix.h index 678e4d6fa317..3b3194b2fc65 100644 --- a/include/net/af_unix.h +++ b/include/net/af_unix.h @@ -58,7 +58,6 @@ struct unix_sock { struct list_head link; atomic_long_t inflight; spinlock_t lock; - unsigned char recursion_level; unsigned long gc_flags; #define UNIX_GC_CANDIDATE 0 #define UNIX_GC_MAYBE_CYCLE 1 diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 7b52a380d710..5c53f22d62e8 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1528,26 +1528,13 @@ static inline bool too_many_unix_fds(struct task_struct *p) return false; } -#define MAX_RECURSION_LEVEL 4 - static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb) { int i; - unsigned char max_level = 0; if (too_many_unix_fds(current)) return -ETOOMANYREFS; - for (i = scm->fp->count - 1; i >= 0; i--) { - struct sock *sk = unix_get_socket(scm->fp->fp[i]); - - if (sk) - max_level = max(max_level, - unix_sk(sk)->recursion_level); - } - if (unlikely(max_level > MAX_RECURSION_LEVEL)) - return -ETOOMANYREFS; - /* * Need to duplicate file references for the sake of garbage * collection. Otherwise a socket in the fps might become a @@ -1559,7 +1546,7 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb) for (i = scm->fp->count - 1; i >= 0; i--) unix_inflight(scm->fp->user, scm->fp->fp[i]); - return max_level; + return 0; } static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool send_fds) @@ -1649,7 +1636,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, struct sk_buff *skb; long timeo; struct scm_cookie scm; - int max_level; int data_len = 0; int sk_locked; @@ -1701,7 +1687,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, err = unix_scm_to_skb(&scm, skb, true); if (err < 0) goto out_free; - max_level = err + 1; skb_put(skb, len - data_len); skb->data_len = data_len; @@ -1819,8 +1804,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, __net_timestamp(skb); maybe_add_creds(skb, sock, other); skb_queue_tail(&other->sk_receive_queue, skb); - if (max_level > unix_sk(other)->recursion_level) - unix_sk(other)->recursion_level = max_level; unix_state_unlock(other); other->sk_data_ready(other); sock_put(other); @@ -1855,7 +1838,6 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg, int sent = 0; struct scm_cookie scm; bool fds_sent = false; - int max_level; int data_len; wait_for_unix_gc(); @@ -1905,7 +1887,6 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg, kfree_skb(skb); goto out_err; } - max_level = err + 1; fds_sent = true; skb_put(skb, size - data_len); @@ -1925,8 +1906,6 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg, maybe_add_creds(skb, sock, other); skb_queue_tail(&other->sk_receive_queue, skb); - if (max_level > unix_sk(other)->recursion_level) - unix_sk(other)->recursion_level = max_level; unix_state_unlock(other); other->sk_data_ready(other); sent += size; @@ -2324,7 +2303,6 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state, last_len = last ? last->len : 0; again: if (skb == NULL) { - unix_sk(sk)->recursion_level = 0; if (copied >= target) goto unlock;