Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1461144 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ Vegard Nossum <vegard.nossum@oracle.com> - 2016-08-12 14:40 +0200
    Re: [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ Willy Tarreau <w@1wt.eu> - 2016-08-15 10:10 +0200
      Re: [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ Vegard Nossum <vegard.nossum@oracle.com> - 2016-08-15 10:30 +0200
        Re: [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ Willy Tarreau <w@1wt.eu> - 2016-08-15 10:40 +0200
          Re: [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ Vegard Nossum <vegard.nossum@oracle.com> - 2016-08-15 11:00 +0200

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

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

[toc] | [next] | [standalone]

#1462620

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

[toc] | [prev] | [next] | [standalone]

#1462628

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1462630

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

[toc] | [prev] | [next] | [standalone]

#1462641

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web