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

[PATCH v2 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.

Changed since v1:

  - Added "Fixes" tag to "pipe: fix off-by-one error when checking buffer limits"
  - In pipe_set_size(), checked 'nr_pages' rather than 'size'
  - Fixed commit message for "pipe: simplify round_pipe_size()"

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                 | 57 ++++++++++++++++++++---------------------------
 include/linux/pipe_fs_i.h |  5 ++---
 include/linux/sysctl.h    |  3 ---
 kernel/sysctl.c           | 33 +++++----------------------
 4 files changed, 32 insertions(+), 66 deletions(-)

-- 
2.15.1

[PATCH v2 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.

Acked-by: Kees Cook <keescook@chromium.org>
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 v2 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 v2 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 v2 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.

Fixes: b0b91d18e2e9 ("pipe: fix limit checking in pipe_set_size()")
Cc: stable@vger.kernel.org
Acked-by: Willy Tarreau <w@1wt.eu>
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 v2 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.

Acked-by: Kees Cook <keescook@chromium.org>
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 v2 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 the size isn't greater than (1 << 31), 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, update pipe_set_size() to not redundantly check the return
value of round_pipe_size() for the "invalid size" case twice.

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

diff --git a/fs/pipe.c b/fs/pipe.c
index f1ee1e599495..458f866caf53 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);
 }
 
 /*
@@ -1050,8 +1044,6 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 	long ret = 0;
 
 	size = round_pipe_size(arg);
-	if (size == 0)
-		return -EINVAL;
 	nr_pages = size >> PAGE_SHIFT;
 
 	if (!nr_pages)
-- 
2.15.1

[PATCH v2 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.

Acked-by: Kees Cook <keescook@chromium.org>
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 458f866caf53..deeb303aa4ce 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 v2 0/7] pipe: buffer limits fixes and cleanups

[Kees Cook]

On Wed, Jan 10, 2018 at 9:28 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> 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.
>
> Changed since v1:
>
>   - Added "Fixes" tag to "pipe: fix off-by-one error when checking buffer limits"
>   - In pipe_set_size(), checked 'nr_pages' rather than 'size'
>   - Fixed commit message for "pipe: simplify round_pipe_size()"

Thanks for the fixes! This looks good to me. Please consider the whole series:

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

-Kees

>
> 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                 | 57 ++++++++++++++++++++---------------------------
>  include/linux/pipe_fs_i.h |  5 ++---
>  include/linux/sysctl.h    |  3 ---
>  kernel/sysctl.c           | 33 +++++----------------------
>  4 files changed, 32 insertions(+), 66 deletions(-)
>
> --
> 2.15.1
>



-- 
Kees Cook
Pixel Security

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

[Joe Lawrence]

On 01/11/2018 12:28 AM, Eric Biggers wrote:
> 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.
> 
> Changed since v1:
> 
>   - Added "Fixes" tag to "pipe: fix off-by-one error when checking buffer limits"
>   - In pipe_set_size(), checked 'nr_pages' rather than 'size'
>   - Fixed commit message for "pipe: simplify round_pipe_size()"
> 
> 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                 | 57 ++++++++++++++++++++---------------------------
>  include/linux/pipe_fs_i.h |  5 ++---
>  include/linux/sysctl.h    |  3 ---
>  kernel/sysctl.c           | 33 +++++----------------------
>  4 files changed, 32 insertions(+), 66 deletions(-)
> 

Bug fixes look good and thanks for continuing the code cleanup!

For the series:
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe