Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1332025 > unrolled thread

[PATCH 3.12 32/64] unix: properly account for FDs passed over unix sockets

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3.12 32/64] unix: properly account for FDs passed over unix sockets Jiri Slaby <jslaby@suse.cz> - 2016-02-11 15:20 +0100
    Re: [PATCH 3.12 32/64] unix: properly account for FDs passed over unix sockets Willy Tarreau <w@1wt.eu> - 2016-02-11 18:40 +0100
      Re: [PATCH 3.12 32/64] unix: properly account for FDs passed over  unix sockets Jiri Slaby <jslaby@suse.cz> - 2016-02-12 09:00 +0100
        Re: [PATCH 3.12 32/64] unix: properly account for FDs passed over  unix sockets Philipp Hahn <pmhahn@pmhahn.de> - 2016-02-12 09:50 +0100
          Re: [PATCH 3.12 32/64] unix: properly account for FDs passed over unix sockets Willy Tarreau <w@1wt.eu> - 2016-02-12 10:10 +0100

#1332025 — [PATCH 3.12 32/64] unix: properly account for FDs passed over unix sockets

From: willy tarreau <w@1wt.eu>

3.12-stable review patch.  If anyone has any objections, please let me know.

===============

[ Upstream commit 712f4aad406bb1ed67f3f98d04c044191f0ff593 ]

It is possible for a process to allocate and accumulate far more FDs than
the process' limit by sending them over a unix socket then closing them
to keep the process' fd count low.

This change addresses this problem by keeping track of the number of FDs
in flight per user and preventing non-privileged processes from having
more FDs in flight than their configured FD limit.

Reported-by: socketpair@gmail.com
Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Mitigates: CVE-2013-4312 (Linux 2.0+)
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Willy Tarreau <w@1wt.eu>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 include/linux/sched.h |  1 +
 net/unix/af_unix.c    | 24 ++++++++++++++++++++----
 net/unix/garbage.c    | 16 ++++++++++++----
 3 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a4d7d19fc338..3ecea51ea060 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -664,6 +664,7 @@ struct user_struct {
 	unsigned long mq_bytes;	/* How many bytes can be allocated to mqueue? */
 #endif
 	unsigned long locked_shm; /* How many pages of mlocked shm ? */
+	unsigned long unix_inflight;	/* How many files in flight in unix sockets */
 
 #ifdef CONFIG_KEYS
 	struct key *uid_keyring;	/* UID specific keyring */
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 31b88dcb0f01..e6b021327c3a 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1484,6 +1484,21 @@ static void unix_destruct_scm(struct sk_buff *skb)
 	sock_wfree(skb);
 }
 
+/*
+ * The "user->unix_inflight" variable is protected by the garbage
+ * collection lock, and we just read it locklessly here. If you go
+ * over the limit, there might be a tiny race in actually noticing
+ * it across threads. Tough.
+ */
+static inline bool too_many_unix_fds(struct task_struct *p)
+{
+	struct user_struct *user = current_user();
+
+	if (unlikely(user->unix_inflight > task_rlimit(p, RLIMIT_NOFILE)))
+		return !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN);
+	return false;
+}
+
 #define MAX_RECURSION_LEVEL 4
 
 static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
@@ -1492,6 +1507,9 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
 	unsigned char max_level = 0;
 	int unix_sock_count = 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]);
 
@@ -1513,10 +1531,8 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
 	if (!UNIXCB(skb).fp)
 		return -ENOMEM;
 
-	if (unix_sock_count) {
-		for (i = scm->fp->count - 1; i >= 0; i--)
-			unix_inflight(scm->fp->fp[i]);
-	}
+	for (i = scm->fp->count - 1; i >= 0; i--)
+		unix_inflight(scm->fp->fp[i]);
 	return max_level;
 }
 
diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index 9bc73f87f64a..06730fe6ad9d 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -125,9 +125,12 @@ struct sock *unix_get_socket(struct file *filp)
 void unix_inflight(struct file *fp)
 {
 	struct sock *s = unix_get_socket(fp);
+
+	spin_lock(&unix_gc_lock);
+
 	if (s) {
 		struct unix_sock *u = unix_sk(s);
-		spin_lock(&unix_gc_lock);
+
 		if (atomic_long_inc_return(&u->inflight) == 1) {
 			BUG_ON(!list_empty(&u->link));
 			list_add_tail(&u->link, &gc_inflight_list);
@@ -135,22 +138,27 @@ void unix_inflight(struct file *fp)
 			BUG_ON(list_empty(&u->link));
 		}
 		unix_tot_inflight++;
-		spin_unlock(&unix_gc_lock);
 	}
+	fp->f_cred->user->unix_inflight++;
+	spin_unlock(&unix_gc_lock);
 }
 
 void unix_notinflight(struct file *fp)
 {
 	struct sock *s = unix_get_socket(fp);
+
+	spin_lock(&unix_gc_lock);
+
 	if (s) {
 		struct unix_sock *u = unix_sk(s);
-		spin_lock(&unix_gc_lock);
+
 		BUG_ON(list_empty(&u->link));
 		if (atomic_long_dec_and_test(&u->inflight))
 			list_del_init(&u->link);
 		unix_tot_inflight--;
-		spin_unlock(&unix_gc_lock);
 	}
+	fp->f_cred->user->unix_inflight--;
+	spin_unlock(&unix_gc_lock);
 }
 
 static void scan_inflight(struct sock *x, void (*func)(struct unix_sock *),
-- 
2.7.1

[toc] | [next] | [standalone]

#1332263

Hi Jiri,

On Thu, Feb 11, 2016 at 02:59:08PM +0100, Jiri Slaby wrote:
> From: willy tarreau <w@1wt.eu>
> 
> 3.12-stable review patch.  If anyone has any objections, please let me know.
> 
> ===============
> 
> [ Upstream commit 712f4aad406bb1ed67f3f98d04c044191f0ff593 ]
> 
> It is possible for a process to allocate and accumulate far more FDs than
> the process' limit by sending them over a unix socket then closing them
> to keep the process' fd count low.
> 
> This change addresses this problem by keeping track of the number of FDs
> in flight per user and preventing non-privileged processes from having
> more FDs in flight than their configured FD limit.
> 
> Reported-by: socketpair@gmail.com
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Mitigates: CVE-2013-4312 (Linux 2.0+)
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Willy Tarreau <w@1wt.eu>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

A possible issue was reported regarding this patch, and Hannes
implemented a fix that's not yet in mainline. I guess it's
preferable to postpone this patch for now.

Thanks,
Willy

[toc] | [prev] | [next] | [standalone]

#1332557 — Re: [PATCH 3.12 32/64] unix: properly account for FDs passed over unix sockets

On 02/11/2016, 06:32 PM, Willy Tarreau wrote:
> Hi Jiri,
> 
> On Thu, Feb 11, 2016 at 02:59:08PM +0100, Jiri Slaby wrote:
>> From: willy tarreau <w@1wt.eu>
>>
>> 3.12-stable review patch.  If anyone has any objections, please let me know.
>>
>> ===============
>>
>> [ Upstream commit 712f4aad406bb1ed67f3f98d04c044191f0ff593 ]
>>
>> It is possible for a process to allocate and accumulate far more FDs than
>> the process' limit by sending them over a unix socket then closing them
>> to keep the process' fd count low.
>>
>> This change addresses this problem by keeping track of the number of FDs
>> in flight per user and preventing non-privileged processes from having
>> more FDs in flight than their configured FD limit.
>>
>> Reported-by: socketpair@gmail.com
>> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Mitigates: CVE-2013-4312 (Linux 2.0+)
>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> Signed-off-by: Willy Tarreau <w@1wt.eu>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> 
> A possible issue was reported regarding this patch, and Hannes
> implemented a fix that's not yet in mainline. I guess it's
> preferable to postpone this patch for now.

Hi,

yes definitely. Thanks for noting.

For reference:
http://article.gmane.org/gmane.linux.kernel/2142236

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1332587 — Re: [PATCH 3.12 32/64] unix: properly account for FDs passed over unix sockets

Am 12.02.2016 um 08:57 schrieb Jiri Slaby:
> On 02/11/2016, 06:32 PM, Willy Tarreau wrote:
>> On Thu, Feb 11, 2016 at 02:59:08PM +0100, Jiri Slaby wrote:
>>> From: willy tarreau <w@1wt.eu>
>>>
>>> 3.12-stable review patch.  If anyone has any objections, please let me know.
>>>
>>> ===============
>>>
>>> [ Upstream commit 712f4aad406bb1ed67f3f98d04c044191f0ff593 ]
>>>
>>> It is possible for a process to allocate and accumulate far more FDs than
>>> the process' limit by sending them over a unix socket then closing them
>>> to keep the process' fd count low.
>>>
>>> This change addresses this problem by keeping track of the number of FDs
>>> in flight per user and preventing non-privileged processes from having
>>> more FDs in flight than their configured FD limit.
>>>
>>> Reported-by: socketpair@gmail.com
>>> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>> Mitigates: CVE-2013-4312 (Linux 2.0+)
>>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>>> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>> Signed-off-by: Willy Tarreau <w@1wt.eu>
>>> Signed-off-by: David S. Miller <davem@davemloft.net>
>>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>>
>> A possible issue was reported regarding this patch, and Hannes
>> implemented a fix that's not yet in mainline. I guess it's
>> preferable to postpone this patch for now.
> 
> yes definitely. Thanks for noting.

Yes and no: the above mentioned patch looks innocent now after more
bisecting, but there is <https://patchwork.ozlabs.org/patch/577653/> as
a folow-up to the FD-accounting.

> For reference:
> http://article.gmane.org/gmane.linux.kernel/2142236

Better read the full thread:
<http://thread.gmane.org/gmane.linux.kernel/2142236>; the suspected bad
patch is
 unix: avoid use-after-free in ep_remove_wait_queue

Philipp

[toc] | [prev] | [next] | [standalone]

#1332591

On Fri, Feb 12, 2016 at 09:45:22AM +0100, Philipp Hahn wrote:
> Am 12.02.2016 um 08:57 schrieb Jiri Slaby:
> > On 02/11/2016, 06:32 PM, Willy Tarreau wrote:
> >> On Thu, Feb 11, 2016 at 02:59:08PM +0100, Jiri Slaby wrote:
> >>> From: willy tarreau <w@1wt.eu>
> >>>
> >>> 3.12-stable review patch.  If anyone has any objections, please let me know.
> >>>
> >>> ===============
> >>>
> >>> [ Upstream commit 712f4aad406bb1ed67f3f98d04c044191f0ff593 ]
> >>>
> >>> It is possible for a process to allocate and accumulate far more FDs than
> >>> the process' limit by sending them over a unix socket then closing them
> >>> to keep the process' fd count low.
> >>>
> >>> This change addresses this problem by keeping track of the number of FDs
> >>> in flight per user and preventing non-privileged processes from having
> >>> more FDs in flight than their configured FD limit.
> >>>
> >>> Reported-by: socketpair@gmail.com
> >>> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >>> Mitigates: CVE-2013-4312 (Linux 2.0+)
> >>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> >>> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> >>> Signed-off-by: Willy Tarreau <w@1wt.eu>
> >>> Signed-off-by: David S. Miller <davem@davemloft.net>
> >>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> >>
> >> A possible issue was reported regarding this patch, and Hannes
> >> implemented a fix that's not yet in mainline. I guess it's
> >> preferable to postpone this patch for now.
> > 
> > yes definitely. Thanks for noting.
> 
> Yes and no: the above mentioned patch looks innocent now after more
> bisecting, but there is <https://patchwork.ozlabs.org/patch/577653/> as
> a folow-up to the FD-accounting.

It was not the only issue reported, as I remember there is a possibility
that for processes having to pass FDs from one socket to another (eg:
dbus), the wrong user could be credited. I don't remember the exact
detail but since the fix is pending and the current patch fixes an
issue which is as old as kernel 2.0 or so, there's no need to rush it
into stable kernels.

Willy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web