2018-01-11 05:30:59

by Eric Biggers

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

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


2018-01-11 05:31:11

by Eric Biggers

[permalink] [raw]
Subject: [PATCH v2 6/7] pipe: simplify round_pipe_size()

From: Eric Biggers <[email protected]>

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 <[email protected]>
---
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

2018-01-11 05:31:09

by Eric Biggers

[permalink] [raw]
Subject: [PATCH v2 7/7] pipe: read buffer limits atomically

From: Eric Biggers <[email protected]>

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 <[email protected]>
Signed-off-by: Eric Biggers <[email protected]>
---
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

2018-01-11 05:31:07

by Eric Biggers

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

From: Eric Biggers <[email protected]>

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: [email protected]
Signed-off-by: Eric Biggers <[email protected]>
---
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

2018-01-11 05:31:03

by Eric Biggers

[permalink] [raw]
Subject: [PATCH v2 2/7] pipe, sysctl: remove pipe_proc_fn()

From: Eric Biggers <[email protected]>

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 <[email protected]>
---
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

2018-01-11 05:32:09

by Eric Biggers

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

From: Eric Biggers <[email protected]>

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: [email protected]
Acked-by: Willy Tarreau <[email protected]>
Signed-off-by: Eric Biggers <[email protected]>
---
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

2018-01-11 05:32:08

by Eric Biggers

[permalink] [raw]
Subject: [PATCH v2 5/7] pipe: reject F_SETPIPE_SZ with size over UINT_MAX

From: Eric Biggers <[email protected]>

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 <[email protected]>
Signed-off-by: Eric Biggers <[email protected]>
---
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

2018-01-11 05:32:56

by Eric Biggers

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

From: Eric Biggers <[email protected]>

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 <[email protected]>
Signed-off-by: Eric Biggers <[email protected]>
---
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

2018-01-11 20:36:21

by Kees Cook

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

On Wed, Jan 10, 2018 at 9:28 PM, Eric Biggers <[email protected]> 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 <[email protected]>

-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

2018-01-11 21:41:20

by Joe Lawrence

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

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 <[email protected]>

-- Joe