[PATCH 0/7] pipe: buffer limits fixes and cleanups

[PATCH 0/7] pipe: buffer limits fixes and cleanups

[Eric Biggers]

This series simplifies the sysctl handler for pipe-max-size and fixes
another set of bugs related to the pipe buffer limits:

- The root user wasn't allowed to exceed the limits when creating new
  pipes.

- There was an off-by-one error when checking the limits, so a limit of
  N was actually treated as N - 1.

- F_SETPIPE_SZ accepted values over UINT_MAX.

- Reading the pipe buffer limits could be racy.

Eric Biggers (7):
  pipe, sysctl: drop 'min' parameter from pipe-max-size converter
  pipe, sysctl: remove pipe_proc_fn()
  pipe: actually allow root to exceed the pipe buffer limits
  pipe: fix off-by-one error when checking buffer limits
  pipe: reject F_SETPIPE_SZ with size over UINT_MAX
  pipe: simplify round_pipe_size()
  pipe: read buffer limits atomically

 fs/pipe.c                 | 58 ++++++++++++++++++++---------------------------
 include/linux/pipe_fs_i.h |  5 ++--
 include/linux/sysctl.h    |  3 ---
 kernel/sysctl.c           | 33 +++++----------------------
 4 files changed, 32 insertions(+), 67 deletions(-)

-- 
2.15.1

[PATCH 1/7] pipe, sysctl: drop 'min' parameter from pipe-max-size converter

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Before validating the given value against pipe_min_size,
do_proc_dopipe_max_size_conv() calls round_pipe_size(), which rounds the
value up to pipe_min_size.  Therefore, the second check against
pipe_min_size is redundant.  Remove it.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/pipe.c                 | 10 +++-------
 include/linux/pipe_fs_i.h |  2 +-
 kernel/sysctl.c           | 15 +--------------
 3 files changed, 5 insertions(+), 22 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 6d98566201ef..a75f5d2ca99c 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -35,11 +35,6 @@
  */
 unsigned int pipe_max_size = 1048576;
 
-/*
- * Minimum pipe size, as required by POSIX
- */
-unsigned int pipe_min_size = PAGE_SIZE;
-
 /* Maximum allocatable pages per user. Hard limit is unset by default, soft
  * matches default values.
  */
@@ -1024,8 +1019,9 @@ unsigned int round_pipe_size(unsigned int size)
 {
 	unsigned long nr_pages;
 
-	if (size < pipe_min_size)
-		size = pipe_min_size;
+	/* Minimum pipe size, as required by POSIX */
+	if (size < PAGE_SIZE)
+		size = PAGE_SIZE;
 
 	nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	if (nr_pages == 0)
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 2dc5e9870fcd..7d9beda14584 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -167,7 +167,7 @@ void pipe_lock(struct pipe_inode_info *);
 void pipe_unlock(struct pipe_inode_info *);
 void pipe_double_lock(struct pipe_inode_info *, struct pipe_inode_info *);
 
-extern unsigned int pipe_max_size, pipe_min_size;
+extern unsigned int pipe_max_size;
 extern unsigned long pipe_user_pages_hard;
 extern unsigned long pipe_user_pages_soft;
 int pipe_proc_fn(struct ctl_table *, int, void __user *, size_t *, loff_t *);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 557d46728577..a71ebb5c5182 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1820,7 +1820,6 @@ static struct ctl_table fs_table[] = {
 		.maxlen		= sizeof(pipe_max_size),
 		.mode		= 0644,
 		.proc_handler	= &pipe_proc_fn,
-		.extra1		= &pipe_min_size,
 	},
 	{
 		.procname	= "pipe-user-pages-hard",
@@ -2622,16 +2621,10 @@ int proc_douintvec_minmax(struct ctl_table *table, int write,
 				 do_proc_douintvec_minmax_conv, &param);
 }
 
-struct do_proc_dopipe_max_size_conv_param {
-	unsigned int *min;
-};
-
 static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
 					unsigned int *valp,
 					int write, void *data)
 {
-	struct do_proc_dopipe_max_size_conv_param *param = data;
-
 	if (write) {
 		unsigned int val;
 
@@ -2642,9 +2635,6 @@ static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
 		if (val == 0)
 			return -EINVAL;
 
-		if (param->min && *param->min > val)
-			return -ERANGE;
-
 		*valp = val;
 	} else {
 		unsigned int val = *valp;
@@ -2657,11 +2647,8 @@ static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
 int proc_dopipe_max_size(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	struct do_proc_dopipe_max_size_conv_param param = {
-		.min = (unsigned int *) table->extra1,
-	};
 	return do_proc_douintvec(table, write, buffer, lenp, ppos,
-				 do_proc_dopipe_max_size_conv, &param);
+				 do_proc_dopipe_max_size_conv, NULL);
 }
 
 static void validate_coredump_safety(void)
-- 
2.15.1

[PATCH 2/7] pipe, sysctl: remove pipe_proc_fn()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

pipe_proc_fn() is no longer needed, as it only calls through to
proc_dopipe_max_size().  Just put proc_dopipe_max_size() in the
ctl_table entry directly, and remove the unneeded EXPORT_SYMBOL() and
the ENOSYS stub for it.

(The reason the ENOSYS stub isn't needed is that the pipe-max-size
ctl_table entry is located directly in 'kern_table' rather than being
registered separately.  Therefore, the entry is already only defined
when the kernel is built with sysctl support.)

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/pipe.c                 | 10 ----------
 include/linux/pipe_fs_i.h |  1 -
 include/linux/sysctl.h    |  3 ---
 kernel/sysctl.c           | 15 +++++----------
 4 files changed, 5 insertions(+), 24 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index a75f5d2ca99c..d0dec5e7ef33 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1120,16 +1120,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 	return ret;
 }
 
-/*
- * This should work even if CONFIG_PROC_FS isn't set, as proc_dopipe_max_size
- * will return an error.
- */
-int pipe_proc_fn(struct ctl_table *table, int write, void __user *buf,
-		 size_t *lenp, loff_t *ppos)
-{
-	return proc_dopipe_max_size(table, write, buf, lenp, ppos);
-}
-
 /*
  * After the inode slimming patch, i_pipe/i_bdev/i_cdev share the same
  * location, so checking ->i_pipe is not enough to verify that this is a
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 7d9beda14584..5028bd4b2c96 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -170,7 +170,6 @@ void pipe_double_lock(struct pipe_inode_info *, struct pipe_inode_info *);
 extern unsigned int pipe_max_size;
 extern unsigned long pipe_user_pages_hard;
 extern unsigned long pipe_user_pages_soft;
-int pipe_proc_fn(struct ctl_table *, int, void __user *, size_t *, loff_t *);
 
 /* Drop the inode semaphore and wait for a pipe event, atomically */
 void pipe_wait(struct pipe_inode_info *pipe);
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 992bc9948232..b769ecfcc3bd 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -51,9 +51,6 @@ extern int proc_dointvec_minmax(struct ctl_table *, int,
 extern int proc_douintvec_minmax(struct ctl_table *table, int write,
 				 void __user *buffer, size_t *lenp,
 				 loff_t *ppos);
-extern int proc_dopipe_max_size(struct ctl_table *table, int write,
-				void __user *buffer, size_t *lenp,
-				loff_t *ppos);
 extern int proc_dointvec_jiffies(struct ctl_table *, int,
 				 void __user *, size_t *, loff_t *);
 extern int proc_dointvec_userhz_jiffies(struct ctl_table *, int,
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index a71ebb5c5182..33e2f0f02000 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -218,6 +218,8 @@ static int proc_dointvec_minmax_coredump(struct ctl_table *table, int write,
 static int proc_dostring_coredump(struct ctl_table *table, int write,
 		void __user *buffer, size_t *lenp, loff_t *ppos);
 #endif
+static int proc_dopipe_max_size(struct ctl_table *table, int write,
+		void __user *buffer, size_t *lenp, loff_t *ppos);
 
 #ifdef CONFIG_MAGIC_SYSRQ
 /* Note: sysrq code uses it's own private copy */
@@ -1819,7 +1821,7 @@ static struct ctl_table fs_table[] = {
 		.data		= &pipe_max_size,
 		.maxlen		= sizeof(pipe_max_size),
 		.mode		= 0644,
-		.proc_handler	= &pipe_proc_fn,
+		.proc_handler	= proc_dopipe_max_size,
 	},
 	{
 		.procname	= "pipe-user-pages-hard",
@@ -2644,8 +2646,8 @@ static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
 	return 0;
 }
 
-int proc_dopipe_max_size(struct ctl_table *table, int write,
-			 void __user *buffer, size_t *lenp, loff_t *ppos)
+static int proc_dopipe_max_size(struct ctl_table *table, int write,
+				void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	return do_proc_douintvec(table, write, buffer, lenp, ppos,
 				 do_proc_dopipe_max_size_conv, NULL);
@@ -3154,12 +3156,6 @@ int proc_douintvec_minmax(struct ctl_table *table, int write,
 	return -ENOSYS;
 }
 
-int proc_dopipe_max_size(struct ctl_table *table, int write,
-			 void __user *buffer, size_t *lenp, loff_t *ppos)
-{
-	return -ENOSYS;
-}
-
 int proc_dointvec_jiffies(struct ctl_table *table, int write,
 		    void __user *buffer, size_t *lenp, loff_t *ppos)
 {
@@ -3203,7 +3199,6 @@ EXPORT_SYMBOL(proc_douintvec);
 EXPORT_SYMBOL(proc_dointvec_jiffies);
 EXPORT_SYMBOL(proc_dointvec_minmax);
 EXPORT_SYMBOL_GPL(proc_douintvec_minmax);
-EXPORT_SYMBOL_GPL(proc_dopipe_max_size);
 EXPORT_SYMBOL(proc_dointvec_userhz_jiffies);
 EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
 EXPORT_SYMBOL(proc_dostring);
-- 
2.15.1

[PATCH 3/7] pipe: actually allow root to exceed the pipe buffer limits

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

pipe-user-pages-hard and pipe-user-pages-soft are only supposed to apply
to unprivileged users, as documented in both Documentation/sysctl/fs.txt
and the pipe(7) man page.

However, the capabilities are actually only checked when increasing a
pipe's size using F_SETPIPE_SZ, not when creating a new pipe.
Therefore, if pipe-user-pages-hard has been set, the root user can run
into it and be unable to create pipes.  Similarly, if
pipe-user-pages-soft has been set, the root user can run into it and
have their pipes limited to 1 page each.

Fix this by allowing the privileged override in both cases.

Fixes: 759c01142a5d ("pipe: limit the per-user amount of pages allocated in pipes")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/pipe.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index d0dec5e7ef33..847ecc388820 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -613,6 +613,11 @@ static bool too_many_pipe_buffers_hard(unsigned long user_bufs)
 	return pipe_user_pages_hard && user_bufs >= pipe_user_pages_hard;
 }
 
+static bool is_unprivileged_user(void)
+{
+	return !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN);
+}
+
 struct pipe_inode_info *alloc_pipe_info(void)
 {
 	struct pipe_inode_info *pipe;
@@ -629,12 +634,12 @@ struct pipe_inode_info *alloc_pipe_info(void)
 
 	user_bufs = account_pipe_buffers(user, 0, pipe_bufs);
 
-	if (too_many_pipe_buffers_soft(user_bufs)) {
+	if (too_many_pipe_buffers_soft(user_bufs) && is_unprivileged_user()) {
 		user_bufs = account_pipe_buffers(user, pipe_bufs, 1);
 		pipe_bufs = 1;
 	}
 
-	if (too_many_pipe_buffers_hard(user_bufs))
+	if (too_many_pipe_buffers_hard(user_bufs) && is_unprivileged_user())
 		goto out_revert_acct;
 
 	pipe->bufs = kcalloc(pipe_bufs, sizeof(struct pipe_buffer),
@@ -1065,7 +1070,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 	if (nr_pages > pipe->buffers &&
 			(too_many_pipe_buffers_hard(user_bufs) ||
 			 too_many_pipe_buffers_soft(user_bufs)) &&
-			!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN)) {
+			is_unprivileged_user()) {
 		ret = -EPERM;
 		goto out_revert_acct;
 	}
-- 
2.15.1

[PATCH 4/7] pipe: fix off-by-one error when checking buffer limits

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

With pipe-user-pages-hard set to 'N', users were actually only allowed
up to 'N - 1' buffers; and likewise for pipe-user-pages-soft.

Fix this to allow up to 'N' buffers, as would be expected.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/pipe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 847ecc388820..9f20e7128578 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -605,12 +605,12 @@ static unsigned long account_pipe_buffers(struct user_struct *user,
 
 static bool too_many_pipe_buffers_soft(unsigned long user_bufs)
 {
-	return pipe_user_pages_soft && user_bufs >= pipe_user_pages_soft;
+	return pipe_user_pages_soft && user_bufs > pipe_user_pages_soft;
 }
 
 static bool too_many_pipe_buffers_hard(unsigned long user_bufs)
 {
-	return pipe_user_pages_hard && user_bufs >= pipe_user_pages_hard;
+	return pipe_user_pages_hard && user_bufs > pipe_user_pages_hard;
 }
 
 static bool is_unprivileged_user(void)
-- 
2.15.1

[PATCH 5/7] pipe: reject F_SETPIPE_SZ with size over UINT_MAX

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

A pipe's size is represented as an 'unsigned int'.  As expected, writing
a value greater than UINT_MAX to /proc/sys/fs/pipe-max-size fails with
EINVAL.  However, the F_SETPIPE_SZ fcntl silently truncates such values
to 32 bits, rather than failing with EINVAL as expected.  (It *does*
fail with EINVAL for values above (1 << 31) but <= UINT_MAX.)

Fix this by moving the check against UINT_MAX into round_pipe_size()
which is called in both cases.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/pipe.c                 | 5 ++++-
 include/linux/pipe_fs_i.h | 2 +-
 kernel/sysctl.c           | 3 ---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 9f20e7128578..f1ee1e599495 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1020,10 +1020,13 @@ const struct file_operations pipefifo_fops = {
  * Currently we rely on the pipe array holding a power-of-2 number
  * of pages. Returns 0 on error.
  */
-unsigned int round_pipe_size(unsigned int size)
+unsigned int round_pipe_size(unsigned long size)
 {
 	unsigned long nr_pages;
 
+	if (size > UINT_MAX)
+		return 0;
+
 	/* Minimum pipe size, as required by POSIX */
 	if (size < PAGE_SIZE)
 		size = PAGE_SIZE;
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 5028bd4b2c96..5a3bb3b7c9ad 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -190,6 +190,6 @@ long pipe_fcntl(struct file *, unsigned int, unsigned long arg);
 struct pipe_inode_info *get_pipe_info(struct file *file);
 
 int create_pipe_files(struct file **, int);
-unsigned int round_pipe_size(unsigned int size);
+unsigned int round_pipe_size(unsigned long size);
 
 #endif
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 33e2f0f02000..31fe10fd745f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2630,9 +2630,6 @@ static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
 	if (write) {
 		unsigned int val;
 
-		if (*lvalp > UINT_MAX)
-			return -EINVAL;
-
 		val = round_pipe_size(*lvalp);
 		if (val == 0)
 			return -EINVAL;
-- 
2.15.1

[PATCH 6/7] pipe: simplify round_pipe_size()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

round_pipe_size() calculates the number of pages the requested size
corresponds to, then rounds the page count up to the next power of 2.

However, it also rounds everything < PAGE_SIZE up to PAGE_SIZE.
Therefore, there's no need to actually translate the size into a page
count; we just need to round the size up to the next power of 2.

We do need to verify that bit 31 isn't set, since on 32-bit systems
roundup_pow_of_two() would be undefined in that case.  But that can just
be combined with the UINT_MAX check which we need anyway now.

Finally, also remove the check for '!nr_pages' in pipe_set_size(), since
round_pipe_size() always returns either 0 or a multiple of PAGE_SIZE.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/pipe.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index f1ee1e599495..774cafd947dc 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1022,20 +1022,14 @@ const struct file_operations pipefifo_fops = {
  */
 unsigned int round_pipe_size(unsigned long size)
 {
-	unsigned long nr_pages;
-
-	if (size > UINT_MAX)
+	if (size > (1U << 31))
 		return 0;
 
 	/* Minimum pipe size, as required by POSIX */
 	if (size < PAGE_SIZE)
-		size = PAGE_SIZE;
-
-	nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	if (nr_pages == 0)
-		return 0;
+		return PAGE_SIZE;
 
-	return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
+	return roundup_pow_of_two(size);
 }
 
 /*
@@ -1054,9 +1048,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 		return -EINVAL;
 	nr_pages = size >> PAGE_SHIFT;
 
-	if (!nr_pages)
-		return -EINVAL;
-
 	/*
 	 * If trying to increase the pipe capacity, check that an
 	 * unprivileged user is not trying to exceed various limits
-- 
2.15.1

[PATCH 7/7] pipe: read buffer limits atomically

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The pipe buffer limits are accessed without any locking, and may be
changed at any time by the sysctl handlers.  In theory this could cause
problems for expressions like the following:

    pipe_user_pages_hard && user_bufs > pipe_user_pages_hard

... since the assembly code might reference the 'pipe_user_pages_hard'
memory location multiple times, and if the admin removes the limit by
setting it to 0, there is a very brief window where processes could
incorrectly observe the limit to be exceeded.

Fix this by loading the limits with READ_ONCE() prior to use.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/pipe.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 774cafd947dc..2e2349602815 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -605,12 +605,16 @@ static unsigned long account_pipe_buffers(struct user_struct *user,
 
 static bool too_many_pipe_buffers_soft(unsigned long user_bufs)
 {
-	return pipe_user_pages_soft && user_bufs > pipe_user_pages_soft;
+	unsigned long soft_limit = READ_ONCE(pipe_user_pages_soft);
+
+	return soft_limit && user_bufs > soft_limit;
 }
 
 static bool too_many_pipe_buffers_hard(unsigned long user_bufs)
 {
-	return pipe_user_pages_hard && user_bufs > pipe_user_pages_hard;
+	unsigned long hard_limit = READ_ONCE(pipe_user_pages_hard);
+
+	return hard_limit && user_bufs > hard_limit;
 }
 
 static bool is_unprivileged_user(void)
@@ -624,13 +628,14 @@ struct pipe_inode_info *alloc_pipe_info(void)
 	unsigned long pipe_bufs = PIPE_DEF_BUFFERS;
 	struct user_struct *user = get_current_user();
 	unsigned long user_bufs;
+	unsigned int max_size = READ_ONCE(pipe_max_size);
 
 	pipe = kzalloc(sizeof(struct pipe_inode_info), GFP_KERNEL_ACCOUNT);
 	if (pipe == NULL)
 		goto out_free_uid;
 
-	if (pipe_bufs * PAGE_SIZE > pipe_max_size && !capable(CAP_SYS_RESOURCE))
-		pipe_bufs = pipe_max_size >> PAGE_SHIFT;
+	if (pipe_bufs * PAGE_SIZE > max_size && !capable(CAP_SYS_RESOURCE))
+		pipe_bufs = max_size >> PAGE_SHIFT;
 
 	user_bufs = account_pipe_buffers(user, 0, pipe_bufs);
 
-- 
2.15.1

Re: [PATCH 4/7] pipe: fix off-by-one error when checking buffer limits

[Willy Tarreau]

On Sun, Jan 07, 2018 at 09:35:39PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> With pipe-user-pages-hard set to 'N', users were actually only allowed
> up to 'N - 1' buffers; and likewise for pipe-user-pages-soft.
> 
> Fix this to allow up to 'N' buffers, as would be expected.

Interesting. I was a bit surprized at first and found that this was
changed by b0b91d1 ("pipe: fix limit checking in pipe_set_size()").
Prior to this fix, only already allocated pipes were counted. After
the fix, an allocation attempt was made before checking the size. So
I think that your fix is needed in stable versions which backported
the commit above.

> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Willy Tarreau <w@1wt.eu>

Thanks,
Willy

Re: [PATCH 1/7] pipe, sysctl: drop 'min' parameter from pipe-max-size converter

[Kees Cook]

On Sun, Jan 7, 2018 at 9:35 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> Before validating the given value against pipe_min_size,
> do_proc_dopipe_max_size_conv() calls round_pipe_size(), which rounds the
> value up to pipe_min_size.  Therefore, the second check against
> pipe_min_size is redundant.  Remove it.

Well, it's not redundant: it provides a hint to anyone trying to tweak
the sysctl about the minimum value. I think this should stay, but that
pipe_min_size should be made const.

-Kees

>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/pipe.c                 | 10 +++-------
>  include/linux/pipe_fs_i.h |  2 +-
>  kernel/sysctl.c           | 15 +--------------
>  3 files changed, 5 insertions(+), 22 deletions(-)
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 6d98566201ef..a75f5d2ca99c 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -35,11 +35,6 @@
>   */
>  unsigned int pipe_max_size = 1048576;
>
> -/*
> - * Minimum pipe size, as required by POSIX
> - */
> -unsigned int pipe_min_size = PAGE_SIZE;
> -
>  /* Maximum allocatable pages per user. Hard limit is unset by default, soft
>   * matches default values.
>   */
> @@ -1024,8 +1019,9 @@ unsigned int round_pipe_size(unsigned int size)
>  {
>         unsigned long nr_pages;
>
> -       if (size < pipe_min_size)
> -               size = pipe_min_size;
> +       /* Minimum pipe size, as required by POSIX */
> +       if (size < PAGE_SIZE)
> +               size = PAGE_SIZE;
>
>         nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>         if (nr_pages == 0)
> diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
> index 2dc5e9870fcd..7d9beda14584 100644
> --- a/include/linux/pipe_fs_i.h
> +++ b/include/linux/pipe_fs_i.h
> @@ -167,7 +167,7 @@ void pipe_lock(struct pipe_inode_info *);
>  void pipe_unlock(struct pipe_inode_info *);
>  void pipe_double_lock(struct pipe_inode_info *, struct pipe_inode_info *);
>
> -extern unsigned int pipe_max_size, pipe_min_size;
> +extern unsigned int pipe_max_size;
>  extern unsigned long pipe_user_pages_hard;
>  extern unsigned long pipe_user_pages_soft;
>  int pipe_proc_fn(struct ctl_table *, int, void __user *, size_t *, loff_t *);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 557d46728577..a71ebb5c5182 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1820,7 +1820,6 @@ static struct ctl_table fs_table[] = {
>                 .maxlen         = sizeof(pipe_max_size),
>                 .mode           = 0644,
>                 .proc_handler   = &pipe_proc_fn,
> -               .extra1         = &pipe_min_size,
>         },
>         {
>                 .procname       = "pipe-user-pages-hard",
> @@ -2622,16 +2621,10 @@ int proc_douintvec_minmax(struct ctl_table *table, int write,
>                                  do_proc_douintvec_minmax_conv, &param);
>  }
>
> -struct do_proc_dopipe_max_size_conv_param {
> -       unsigned int *min;
> -};
> -
>  static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
>                                         unsigned int *valp,
>                                         int write, void *data)
>  {
> -       struct do_proc_dopipe_max_size_conv_param *param = data;
> -
>         if (write) {
>                 unsigned int val;
>
> @@ -2642,9 +2635,6 @@ static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
>                 if (val == 0)
>                         return -EINVAL;
>
> -               if (param->min && *param->min > val)
> -                       return -ERANGE;
> -
>                 *valp = val;
>         } else {
>                 unsigned int val = *valp;
> @@ -2657,11 +2647,8 @@ static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
>  int proc_dopipe_max_size(struct ctl_table *table, int write,
>                          void __user *buffer, size_t *lenp, loff_t *ppos)
>  {
> -       struct do_proc_dopipe_max_size_conv_param param = {
> -               .min = (unsigned int *) table->extra1,
> -       };
>         return do_proc_douintvec(table, write, buffer, lenp, ppos,
> -                                do_proc_dopipe_max_size_conv, &param);
> +                                do_proc_dopipe_max_size_conv, NULL);
>  }
>
>  static void validate_coredump_safety(void)
> --
> 2.15.1
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 3/7] pipe: actually allow root to exceed the pipe buffer limits

[Kees Cook]

On Sun, Jan 7, 2018 at 9:35 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> pipe-user-pages-hard and pipe-user-pages-soft are only supposed to apply
> to unprivileged users, as documented in both Documentation/sysctl/fs.txt
> and the pipe(7) man page.
>
> However, the capabilities are actually only checked when increasing a
> pipe's size using F_SETPIPE_SZ, not when creating a new pipe.
> Therefore, if pipe-user-pages-hard has been set, the root user can run
> into it and be unable to create pipes.  Similarly, if
> pipe-user-pages-soft has been set, the root user can run into it and
> have their pipes limited to 1 page each.
>
> Fix this by allowing the privileged override in both cases.

Should this be controlled per-namespace instead of via init-ns caps?

-Kees

>
> Fixes: 759c01142a5d ("pipe: limit the per-user amount of pages allocated in pipes")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/pipe.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index d0dec5e7ef33..847ecc388820 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -613,6 +613,11 @@ static bool too_many_pipe_buffers_hard(unsigned long user_bufs)
>         return pipe_user_pages_hard && user_bufs >= pipe_user_pages_hard;
>  }
>
> +static bool is_unprivileged_user(void)
> +{
> +       return !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN);
> +}
> +
>  struct pipe_inode_info *alloc_pipe_info(void)
>  {
>         struct pipe_inode_info *pipe;
> @@ -629,12 +634,12 @@ struct pipe_inode_info *alloc_pipe_info(void)
>
>         user_bufs = account_pipe_buffers(user, 0, pipe_bufs);
>
> -       if (too_many_pipe_buffers_soft(user_bufs)) {
> +       if (too_many_pipe_buffers_soft(user_bufs) && is_unprivileged_user()) {
>                 user_bufs = account_pipe_buffers(user, pipe_bufs, 1);
>                 pipe_bufs = 1;
>         }
>
> -       if (too_many_pipe_buffers_hard(user_bufs))
> +       if (too_many_pipe_buffers_hard(user_bufs) && is_unprivileged_user())
>                 goto out_revert_acct;
>
>         pipe->bufs = kcalloc(pipe_bufs, sizeof(struct pipe_buffer),
> @@ -1065,7 +1070,7 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
>         if (nr_pages > pipe->buffers &&
>                         (too_many_pipe_buffers_hard(user_bufs) ||
>                          too_many_pipe_buffers_soft(user_bufs)) &&
> -                       !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN)) {
> +                       is_unprivileged_user()) {
>                 ret = -EPERM;
>                 goto out_revert_acct;
>         }
> --
> 2.15.1
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 5/7] pipe: reject F_SETPIPE_SZ with size over UINT_MAX

[Kees Cook]

On Sun, Jan 7, 2018 at 9:35 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> A pipe's size is represented as an 'unsigned int'.  As expected, writing
> a value greater than UINT_MAX to /proc/sys/fs/pipe-max-size fails with
> EINVAL.  However, the F_SETPIPE_SZ fcntl silently truncates such values
> to 32 bits, rather than failing with EINVAL as expected.  (It *does*
> fail with EINVAL for values above (1 << 31) but <= UINT_MAX.)
>
> Fix this by moving the check against UINT_MAX into round_pipe_size()
> which is called in both cases.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  fs/pipe.c                 | 5 ++++-
>  include/linux/pipe_fs_i.h | 2 +-
>  kernel/sysctl.c           | 3 ---
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 9f20e7128578..f1ee1e599495 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1020,10 +1020,13 @@ const struct file_operations pipefifo_fops = {
>   * Currently we rely on the pipe array holding a power-of-2 number
>   * of pages. Returns 0 on error.
>   */
> -unsigned int round_pipe_size(unsigned int size)
> +unsigned int round_pipe_size(unsigned long size)
>  {
>         unsigned long nr_pages;
>
> +       if (size > UINT_MAX)
> +               return 0;
> +
>         /* Minimum pipe size, as required by POSIX */
>         if (size < PAGE_SIZE)
>                 size = PAGE_SIZE;
> diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
> index 5028bd4b2c96..5a3bb3b7c9ad 100644
> --- a/include/linux/pipe_fs_i.h
> +++ b/include/linux/pipe_fs_i.h
> @@ -190,6 +190,6 @@ long pipe_fcntl(struct file *, unsigned int, unsigned long arg);
>  struct pipe_inode_info *get_pipe_info(struct file *file);
>
>  int create_pipe_files(struct file **, int);
> -unsigned int round_pipe_size(unsigned int size);
> +unsigned int round_pipe_size(unsigned long size);
>
>  #endif
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 33e2f0f02000..31fe10fd745f 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2630,9 +2630,6 @@ static int do_proc_dopipe_max_size_conv(unsigned long *lvalp,
>         if (write) {
>                 unsigned int val;
>
> -               if (*lvalp > UINT_MAX)
> -                       return -EINVAL;
> -
>                 val = round_pipe_size(*lvalp);
>                 if (val == 0)
>                         return -EINVAL;
> --
> 2.15.1
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 6/7] pipe: simplify round_pipe_size()

[Kees Cook]

On Sun, Jan 7, 2018 at 9:35 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> round_pipe_size() calculates the number of pages the requested size
> corresponds to, then rounds the page count up to the next power of 2.
>
> However, it also rounds everything < PAGE_SIZE up to PAGE_SIZE.
> Therefore, there's no need to actually translate the size into a page
> count; we just need to round the size up to the next power of 2.
>
> We do need to verify that bit 31 isn't set, since on 32-bit systems
> roundup_pow_of_two() would be undefined in that case.  But that can just
> be combined with the UINT_MAX check which we need anyway now.
>
> Finally, also remove the check for '!nr_pages' in pipe_set_size(), since
> round_pipe_size() always returns either 0 or a multiple of PAGE_SIZE.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/pipe.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index f1ee1e599495..774cafd947dc 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1022,20 +1022,14 @@ const struct file_operations pipefifo_fops = {
>   */
>  unsigned int round_pipe_size(unsigned long size)
>  {
> -       unsigned long nr_pages;
> -
> -       if (size > UINT_MAX)
> +       if (size > (1U << 31))
>                 return 0;
>
>         /* Minimum pipe size, as required by POSIX */
>         if (size < PAGE_SIZE)
> -               size = PAGE_SIZE;
> -
> -       nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> -       if (nr_pages == 0)
> -               return 0;
> +               return PAGE_SIZE;
>
> -       return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
> +       return roundup_pow_of_two(size);
>  }
>
>  /*

Above looks good.

> @@ -1054,9 +1048,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
>                 return -EINVAL;
>         nr_pages = size >> PAGE_SHIFT;
>
> -       if (!nr_pages)
> -               return -EINVAL;
> -

I would just leave this hunk anyway: it's defensive for any future
changes. Maybe add a comment describing why it's currently redundant?

-Kees

>         /*
>          * If trying to increase the pipe capacity, check that an
>          * unprivileged user is not trying to exceed various limits
> --
> 2.15.1
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 7/7] pipe: read buffer limits atomically

[Kees Cook]

On Sun, Jan 7, 2018 at 9:35 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> The pipe buffer limits are accessed without any locking, and may be
> changed at any time by the sysctl handlers.  In theory this could cause
> problems for expressions like the following:
>
>     pipe_user_pages_hard && user_bufs > pipe_user_pages_hard
>
> ... since the assembly code might reference the 'pipe_user_pages_hard'
> memory location multiple times, and if the admin removes the limit by
> setting it to 0, there is a very brief window where processes could
> incorrectly observe the limit to be exceeded.
>
> Fix this by loading the limits with READ_ONCE() prior to use.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  fs/pipe.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 774cafd947dc..2e2349602815 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -605,12 +605,16 @@ static unsigned long account_pipe_buffers(struct user_struct *user,
>
>  static bool too_many_pipe_buffers_soft(unsigned long user_bufs)
>  {
> -       return pipe_user_pages_soft && user_bufs > pipe_user_pages_soft;
> +       unsigned long soft_limit = READ_ONCE(pipe_user_pages_soft);
> +
> +       return soft_limit && user_bufs > soft_limit;
>  }
>
>  static bool too_many_pipe_buffers_hard(unsigned long user_bufs)
>  {
> -       return pipe_user_pages_hard && user_bufs > pipe_user_pages_hard;
> +       unsigned long hard_limit = READ_ONCE(pipe_user_pages_hard);
> +
> +       return hard_limit && user_bufs > hard_limit;
>  }
>
>  static bool is_unprivileged_user(void)
> @@ -624,13 +628,14 @@ struct pipe_inode_info *alloc_pipe_info(void)
>         unsigned long pipe_bufs = PIPE_DEF_BUFFERS;
>         struct user_struct *user = get_current_user();
>         unsigned long user_bufs;
> +       unsigned int max_size = READ_ONCE(pipe_max_size);
>
>         pipe = kzalloc(sizeof(struct pipe_inode_info), GFP_KERNEL_ACCOUNT);
>         if (pipe == NULL)
>                 goto out_free_uid;
>
> -       if (pipe_bufs * PAGE_SIZE > pipe_max_size && !capable(CAP_SYS_RESOURCE))
> -               pipe_bufs = pipe_max_size >> PAGE_SHIFT;
> +       if (pipe_bufs * PAGE_SIZE > max_size && !capable(CAP_SYS_RESOURCE))
> +               pipe_bufs = max_size >> PAGE_SHIFT;
>
>         user_bufs = account_pipe_buffers(user, 0, pipe_bufs);
>
> --
> 2.15.1
>



-- 
Kees Cook
Pixel Security

Re: [PATCH 1/7] pipe, sysctl: drop 'min' parameter from pipe-max-size converter

[Eric Biggers]

On Tue, Jan 09, 2018 at 02:20:45PM -0800, Kees Cook wrote:
> On Sun, Jan 7, 2018 at 9:35 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > Before validating the given value against pipe_min_size,
> > do_proc_dopipe_max_size_conv() calls round_pipe_size(), which rounds the
> > value up to pipe_min_size.  Therefore, the second check against
> > pipe_min_size is redundant.  Remove it.
> 
> Well, it's not redundant: it provides a hint to anyone trying to tweak
> the sysctl about the minimum value. I think this should stay, but that
> pipe_min_size should be made const.
> 
> -Kees
> 

It *is* redundant, because it doesn't do anything.  round_pipe_size() already
rounds the value up to the minimum.

Eric

Re: [PATCH 3/7] pipe: actually allow root to exceed the pipe buffer limits

[Eric Biggers]

On Tue, Jan 09, 2018 at 02:23:32PM -0800, Kees Cook wrote:
> On Sun, Jan 7, 2018 at 9:35 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > pipe-user-pages-hard and pipe-user-pages-soft are only supposed to apply
> > to unprivileged users, as documented in both Documentation/sysctl/fs.txt
> > and the pipe(7) man page.
> >
> > However, the capabilities are actually only checked when increasing a
> > pipe's size using F_SETPIPE_SZ, not when creating a new pipe.
> > Therefore, if pipe-user-pages-hard has been set, the root user can run
> > into it and be unable to create pipes.  Similarly, if
> > pipe-user-pages-soft has been set, the root user can run into it and
> > have their pipes limited to 1 page each.
> >
> > Fix this by allowing the privileged override in both cases.
> 
> Should this be controlled per-namespace instead of via init-ns caps?
> 

I don't think so.  Users shouldn't be able to bypass the limits by creating a
user namespace.

Eric

Re: [PATCH 6/7] pipe: simplify round_pipe_size()

[Eric Biggers]

On Tue, Jan 09, 2018 at 02:27:10PM -0800, Kees Cook wrote:
> 
> > @@ -1054,9 +1048,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
> >                 return -EINVAL;
> >         nr_pages = size >> PAGE_SHIFT;
> >
> > -       if (!nr_pages)
> > -               return -EINVAL;
> > -
> 
> I would just leave this hunk anyway: it's defensive for any future
> changes. Maybe add a comment describing why it's currently redundant?
> 

I don't know; I find it really confusing to have two slightly different checks
for the same thing, as it implies that they actually need to be there for a
reason.  How about just checking nr_pages?

        size = round_pipe_size(arg);
        nr_pages = size >> PAGE_SHIFT;

        if (nr_pages == 0)
                return -EINVAL;

Eric

Re: [PATCH 6/7] pipe: simplify round_pipe_size()

[Kees Cook]

On Tue, Jan 9, 2018 at 6:52 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> On Tue, Jan 09, 2018 at 02:27:10PM -0800, Kees Cook wrote:
>>
>> > @@ -1054,9 +1048,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
>> >                 return -EINVAL;
>> >         nr_pages = size >> PAGE_SHIFT;
>> >
>> > -       if (!nr_pages)
>> > -               return -EINVAL;
>> > -
>>
>> I would just leave this hunk anyway: it's defensive for any future
>> changes. Maybe add a comment describing why it's currently redundant?
>>
>
> I don't know; I find it really confusing to have two slightly different checks
> for the same thing, as it implies that they actually need to be there for a
> reason.  How about just checking nr_pages?
>
>         size = round_pipe_size(arg);
>         nr_pages = size >> PAGE_SHIFT;
>
>         if (nr_pages == 0)
>                 return -EINVAL;

Oh yeah! I like that.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 1/7] pipe, sysctl: drop 'min' parameter from pipe-max-size converter

[Kees Cook]

On Tue, Jan 9, 2018 at 6:29 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> On Tue, Jan 09, 2018 at 02:20:45PM -0800, Kees Cook wrote:
>> On Sun, Jan 7, 2018 at 9:35 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
>> > From: Eric Biggers <ebiggers@google.com>
>> >
>> > Before validating the given value against pipe_min_size,
>> > do_proc_dopipe_max_size_conv() calls round_pipe_size(), which rounds the
>> > value up to pipe_min_size.  Therefore, the second check against
>> > pipe_min_size is redundant.  Remove it.
>>
>> Well, it's not redundant: it provides a hint to anyone trying to tweak
>> the sysctl about the minimum value. I think this should stay, but that
>> pipe_min_size should be made const.
>>
>> -Kees
>>
>
> It *is* redundant, because it doesn't do anything.  round_pipe_size() already
> rounds the value up to the minimum.

Ah, yes, I see it now. Wow are the sysctl functions convoluted here!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Pixel Security