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

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

[Vegard Nossum]

I got this:

    ================================================================================
    UBSAN: Undefined behaviour in ./include/linux/log2.h:63:13
    shift exponent 64 is too large for 64-bit type 'long unsigned int'
    CPU: 0 PID: 5351 Comm: trinity-c0 Not tainted 4.8.0-rc1+ #84
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2014
     0000000000000000 ffff880115c67c08 ffffffff82344f40 0000000041b58ab3
     ffffffff84f98000 ffffffff82344e94 ffff880115c67c30 ffff880115c67be0
     0000000000000001 ffff880115c679e8 dffffc0000000000 ffffffff85bf0820
    Call Trace:
     [<ffffffff82344f40>] dump_stack+0xac/0xfc
     [<ffffffff8242f5a8>] ubsan_epilogue+0xd/0x8a
     [<ffffffff82430c31>] __ubsan_handle_shift_out_of_bounds+0x255/0x29a
     [<ffffffff818229ab>] pipe_fcntl+0x59b/0x800
     [<ffffffff8184504a>] SyS_fcntl+0x69a/0xe50
     [<ffffffff81007bd3>] do_syscall_64+0x1b3/0x4b0
     [<ffffffff845f946a>] entry_SYSCALL64_slow_path+0x25/0x25
    ================================================================================

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).

If we limit it to INT_MAX, then we know nr_pages will never be 0.
Rudimentary boundary analysis (both 32- and 64-bit):

  arg == 0: gets rejected with -EINVAL by our check
  arg == 1: round_pipe_size() rounds up to PAGE_SIZE and returns PAGE_SIZE
  arg == INT_MAX - 1: round_pipe_size() returns 0x80000000
  arg == INT_MAX: round_pipe_size() returns 0x80000000
  arg > INT_MAX: gets rejected with -EINVAL by our check

In practice the undefined behaviour causes my gcc at least to return
0 for the large shift (i.e. 1ULL << 64 == 0), so nothing bad happens
because this is caught by the if (!nr_pages) check. But I don't think
we can bank on this always being the case. This patch avoids the
undefined behaviour completely. (Stable not on Cc since it violates
the “no "This could be a problem"” rule.)

Tested on 32- and 64-bit x86/UML.

Cc: Willy Tarreau <w@1wt.eu>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 fs/pipe.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 4ebe6b2..42ea89f 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1115,13 +1115,13 @@ long pipe_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
 	case F_SETPIPE_SZ: {
 		unsigned int size, nr_pages;
 
-		size = round_pipe_size(arg);
-		nr_pages = size >> PAGE_SHIFT;
-
 		ret = -EINVAL;
-		if (!nr_pages)
+		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) {
 			ret = -EPERM;
 			goto out;
-- 
1.9.1

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

[Willy Tarreau]

Hi,

On Fri, Aug 12, 2016 at 02:35:40PM +0200, Vegard Nossum wrote:
> I got this:
> 
>     ================================================================================
>     UBSAN: Undefined behaviour in ./include/linux/log2.h:63:13
>     shift exponent 64 is too large for 64-bit type 'long unsigned int'
>     CPU: 0 PID: 5351 Comm: trinity-c0 Not tainted 4.8.0-rc1+ #84
>     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2014
>      0000000000000000 ffff880115c67c08 ffffffff82344f40 0000000041b58ab3
>      ffffffff84f98000 ffffffff82344e94 ffff880115c67c30 ffff880115c67be0
>      0000000000000001 ffff880115c679e8 dffffc0000000000 ffffffff85bf0820
>     Call Trace:
>      [<ffffffff82344f40>] dump_stack+0xac/0xfc
>      [<ffffffff8242f5a8>] ubsan_epilogue+0xd/0x8a
>      [<ffffffff82430c31>] __ubsan_handle_shift_out_of_bounds+0x255/0x29a
>      [<ffffffff818229ab>] pipe_fcntl+0x59b/0x800
>      [<ffffffff8184504a>] SyS_fcntl+0x69a/0xe50
>      [<ffffffff81007bd3>] do_syscall_64+0x1b3/0x4b0
>      [<ffffffff845f946a>] entry_SYSCALL64_slow_path+0x25/0x25
>     ================================================================================
> 
> 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.

Regards,
Willy

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

[Vegard Nossum]

[-- 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

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

[Willy Tarreau]

On Mon, Aug 15, 2016 at 10:22:38AM +0200, Vegard Nossum wrote:
> 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.

Well in fact in my opinion it's the opposite, because if we ensure the
function works well over all its argument type's range, the caller has
less trouble figuring what sub-part of the range is OK. This is exactly
the current issue where you have to ensure that :

     unsigned int arg <= INT_MAX

> 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?

Yes possibly.

> 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?

I'm not sure it improves readability. And in general I don't like having
two variables to set a same limit, because some code parts tend to rely
on one of them and other code parts use the other one, so it's harder to
keep consistency all over the code. If you manage to totally get rid of
pipe_max_size and make pipe_proc_fn() completely emulate it using
pipe_max_pages, then it may be clearer and will also get rid of some of
the comments that are here to explain the inconsistencies.

Willy

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

[Vegard Nossum]

On 08/15/2016 10:34 AM, Willy Tarreau wrote:
> On Mon, Aug 15, 2016 at 10:22:38AM +0200, Vegard Nossum wrote:
>> 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.
>
> Well in fact in my opinion it's the opposite, because if we ensure the
> function works well over all its argument type's range, the caller has
> less trouble figuring what sub-part of the range is OK. This is exactly
> the current issue where you have to ensure that :
>
>       unsigned int arg <= INT_MAX

It's not just about this one function, but all the other code in pipe.c
now has to cope with pipe buffers > INT_MAX as well.

For example all the fields in struct pipe_inode_info referring to
buffers are unsigned int (nrbufs, curbuf, buffers). Unless we also
change those to unsigned long, the code will definitely not support
buffer sizes up to LONG_MAX on 64-bit.

That's why I think it's a much, much bigger task to review (and make)
such a change.


Vegard