pipe: buffer limits fixes and cleanups
Subsystems (3)
v2 cover 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(-)
Signature · 1 line
2.15.1
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.
--- fs/pipe.c | 10 +++------- include/linux/pipe_fs_i.h | 2 +- kernel/sysctl.c | 15 +-------------- 3 files changed, 5 insertions(+), 22 deletions(-)
fs/pipe.c +3 -7
@@ -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 )
include/linux/pipe_fs_i.h +1 -1
@@ -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 *);
kernel/sysctl.c +1 -14
@@ -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, ¶m); } -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, ¶m); + do_proc_dopipe_max_size_conv,NULL ); }static void validate_coredump_safety (void )
Signature · 1 line
2.15.1
PATCH v2 2/7 pipe, sysctl: remove pipe_proc_fn()
In reply to Eric BiggersFrom: 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.)
--- 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(-)
fs/pipe.c +0 -10
@@ -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
include/linux/pipe_fs_i.h +0 -1
@@ -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);
include/linux/sysctl.h +0 -3
@@ -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,
kernel/sysctl.c +5 -10
@@ -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);
Signature · 1 line
2.15.1
PATCH v2 3/7 pipe: actually allow root to exceed the pipe buffer limits
In reply to Eric BiggersFrom: 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.
--- fs/pipe.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
fs/pipe.c +8 -3
@@ -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; }
Signature · 1 line
2.15.1
PATCH v2 4/7 pipe: fix off-by-one error when checking buffer limits
In reply to Eric BiggersFrom: 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.
--- fs/pipe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
fs/pipe.c +2 -2
@@ -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 )
Signature · 1 line
2.15.1
PATCH v2 5/7 pipe: reject F_SETPIPE_SZ with size over UINT_MAX
In reply to Eric BiggersFrom: 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.
--- fs/pipe.c | 5 ++++- include/linux/pipe_fs_i.h | 2 +- kernel/sysctl.c | 3 --- 3 files changed, 5 insertions(+), 5 deletions(-)
fs/pipe.c +4 -1
@@ -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 ;
include/linux/pipe_fs_i.h +1 -1
@@ -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
kernel/sysctl.c +0 -3
@@ -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 ;
Signature · 1 line
2.15.1
PATCH v2 6/7 pipe: simplify round_pipe_size()
In reply to Eric BiggersFrom: 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.
--- fs/pipe.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
fs/pipe.c +3 -11
@@ -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)
Signature · 1 line
2.15.1
PATCH v2 7/7 pipe: read buffer limits atomically
In reply to Eric BiggersFrom: 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.
--- fs/pipe.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
fs/pipe.c +9 -4
@@ -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);
Signature · 1 line
2.15.1
Quoted 11 lines · 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_SZaccepted values overUINT_MAX.
Quoted 7 lines
- 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"
- Inpipe_set_size(), checked 'nr_pages' rather than 'size'
- Fixed commit message for "pipe: simplifyround_pipe_size()"
Thanks for the fixes! This looks good to me. Please consider the whole series:
-Kees
Quoted 19 lines
Eric Biggers (7):
pipe, sysctl: drop 'min' parameter from pipe-max-size converter pipe, sysctl: removepipe_proc_fn()
pipe: actually allow root to exceed the pipe buffer limits
pipe: fix off-by-one error when checking buffer limits
pipe: rejectF_SETPIPE_SZwith size overUINT_MAX
pipe: simplifyround_pipe_size()
pipe: read buffer limits atomicallyfs/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
Signature · 2 lines
Kees Cook Pixel Security
Quoted 28 lines · 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_SZaccepted values overUINT_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"
- Inpipe_set_size(), checked 'nr_pages' rather than 'size'
- Fixed commit message for "pipe: simplifyround_pipe_size()"Eric Biggers (7):
pipe, sysctl: drop 'min' parameter from pipe-max-size converter pipe, sysctl: removepipe_proc_fn()
pipe: actually allow root to exceed the pipe buffer limits
pipe: fix off-by-one error when checking buffer limits
pipe: rejectF_SETPIPE_SZwith size overUINT_MAX
pipe: simplifyround_pipe_size()
pipe: read buffer limits atomically
Quoted 6 lines
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:
-- Joe