Re: [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ

[Vegard Nossum <vegard.nossum@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

On 08/15/2016 10:06 AM, Willy Tarreau wrote:
> On Fri, Aug 12, 2016 at 02:35:40PM +0200, Vegard Nossum wrote:
>> The problem is that if the argument (an unsigned long) passed to
>> F_SETPIPE_SZ is either 0 or greater than UINT_MAX, then
>> roundup_pow_of_two() will hit undefined behavior because the shift
>> width will be 64.
>>
>> Even if we limited the argument to UINT_MAX, we would still need to
>> keep the !nr_pages check, as passing anything greater than INT_MAX
>> will give a nr_pages inside round_pipe_size() of (1 << 20) which
>> then gets truncated to 0 when we convert it to an unsigned int
>> (because (1 << 20) << PAGE_SHIFT == 1 << 32).
>
> Why wouldn't we limit it to LONG_MAX and change round_pipe_size() to
> take an unsigned long in argument instead ? On 64-bit it would allow
> more than 2GB (even if I really doubt anybody will ever need this).
>
> Also, strictly speaking in your case it's not INT_MAX which is the
> absolute limit but UINT_MAX - PAGE_SIZE since it's a round up issue
> before being a shift issue. But that's mostly a detail I guess.
>
> Overall I think your change is right.

Hi,

Thanks for having a look.

In both cases I found it better to be more conservative in what we
accept, i.e. I haven't checked whether the rest of the code would
support pipe buffers > INT_MAX on 64-bit and I think it's a slightly
bigger job to check that (not just for the person making the change, but
for everybody else looking at/reviewing it) -- it's already tricky
enough to verify that this change by itself is safe and correct IMHO.

But I completely agree that being consistent in our int vs. long usage
would make the code easier to follow in terms of when something is
truncated or overflowing, so we should do that if we can.

Maybe we can relax the restrictions in a follow-up patch?

I was also playing around with consistently counting buffer sizes in
pages rather than bytes to avoid some of the shifts by PAGE_SHIFT,
although it doesn't win us very much. What do you think of the attached
patch?


Vegard

[-- Attachment #2: 0001-fs-pipe-simplify-pipe-size-handling-a-bit.patch --]
[-- Type: text/x-patch, Size: 3014 bytes --]

>From 466fcd213793238c0283f71252dd1652dcd21e10 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@oracle.com>
Date: Fri, 12 Aug 2016 13:08:55 +0200
Subject: [PATCH] fs/pipe: simplify pipe size handling a bit

Pipe sizes are converted back and forth between pages and bytes. To make
things a bit simpler we can do most of it in pages only. The exception
is /proc/sys/fs/pipe-max-size which must be in bytes for backwards
compatibility.

There are two related races in the original code when a value is written
to /proc/sys/fs/pipe-max-size:

 1) reading /proc/sys/fs/pipe-max-size can yield the non-adjusted value

 2) F_SETPIPE_SZ compares the argument to the non-adjusted value

These races are harmless, however, because although somebody can see a
lower value than they should, the value is never lower than what was
written to /proc/sys/fs/pipe-max-size in the first place.

Nevertheless, the patch should fix race #2.

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 fs/pipe.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 42ea89f..841f5bd 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -32,7 +32,9 @@
  * The max size that a non-root user is allowed to grow the pipe. Can
  * be set by root in /proc/sys/fs/pipe-max-size
  */
-unsigned int pipe_max_size = 1048576;
+#define PIPE_MAX_PAGES 256
+unsigned int pipe_max_pages = PIPE_MAX_PAGES;
+unsigned int pipe_max_size = PIPE_MAX_PAGES << PAGE_SHIFT;
 
 /*
  * Minimum pipe size, as required by POSIX
@@ -1065,12 +1067,12 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long nr_pages)
  * Currently we rely on the pipe array holding a power-of-2 number
  * of pages.
  */
-static inline unsigned int round_pipe_size(unsigned int size)
+static inline unsigned int round_pipe_pages(unsigned int size)
 {
 	unsigned long nr_pages;
 
 	nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
+	return roundup_pow_of_two(nr_pages);
 }
 
 /*
@@ -1086,7 +1088,9 @@ int pipe_proc_fn(struct ctl_table *table, int write, void __user *buf,
 	if (ret < 0 || !write)
 		return ret;
 
-	pipe_max_size = round_pipe_size(pipe_max_size);
+	/* Slightly racy; a reader may see the non-sounded pipe_max_size */
+	pipe_max_pages = round_pipe_pages(pipe_max_size);
+	pipe_max_size = pipe_max_pages << PAGE_SHIFT;
 	return ret;
 }
 
@@ -1113,16 +1117,14 @@ long pipe_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
 
 	switch (cmd) {
 	case F_SETPIPE_SZ: {
-		unsigned int size, nr_pages;
+		unsigned int nr_pages;
 
 		ret = -EINVAL;
 		if (!arg || arg > INT_MAX)
 			goto out;
 
-		size = round_pipe_size(arg);
-		nr_pages = size >> PAGE_SHIFT;
-
-		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
+		nr_pages = round_pipe_pages(arg);
+		if (!capable(CAP_SYS_RESOURCE) && nr_pages > pipe_max_pages) {
 			ret = -EPERM;
 			goto out;
 		} else if ((too_many_pipe_buffers_hard(pipe->user) ||
-- 
1.9.1