lkml.org 
[lkml]   [2016]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ
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

\
 
 \ /
  Last update: 2016-09-17 09:56    [W:0.040 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site