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

[Willy Tarreau <w@1wt.eu>]

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