linux-kernel - Re: [RFC PATCH] fs/pipe.c: implement minimum pipe size for arg==0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 9 Nov 2017 14:09:03 -0800
From:   Randy Dunlap <rdunlap@...radead.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux FS Devel <linux-fsdevel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Shankara Pailoor <sp3485@...umbia.edu>,
        Michael Kerrisk <mtk.manpages@...il.com>
Subject: Re: [RFC PATCH] fs/pipe.c: implement minimum pipe size for arg==0

On 11/09/2017 01:45 PM, Josh Poimboeuf wrote:
> On Mon, Sep 11, 2017 at 04:15:49PM -0700, Randy Dunlap wrote:
>> From: Randy Dunlap <rdunlap@...radead.org>
>>
>> Shankara reports that running Syskaller with UBSAN causes this message:
>>   UBSAN: Undefined behaviour in ./include/linux/log2.h:57:13
>>
>> Syzkaller is trying to set the pipe size to 0UL. The call chain is:
>> 	pipe_set_size(pipe, 0UL)
>> 	...
>> 	size = round_pipe_size(arg); // arg == 0UL
>> which does
>> 	nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT; // = 0UL
>> 	return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
>> which is undefined when the argument is 0... and which calls
>> 	fls_long(-1) // == 64
>> and then returns 1UL << 64. This is where UBSAN kicks in.
>>
>> The fcntl() man page [http://man7.org/linux/man-pages/man2/fcntl.2.html]
>> says that:
>> 	Attempts to set the pipe capacity below the page size are
>> 	silently rounded up to the page size.
>>
>> We could try to fix the basic low-level functions to handle 0 (where
>> <linux/log2.h> says the result is undefined when n == 0), but the safest
>> path for now is probably just to patch fs/pipe.c to make the documented
>> default happen when arg is 0.
>>
>> Reported-by: Shankara Pailoor <sp3485@...umbia.edu>
>> Signed-off-by: Randy Dunlap <rdunlap@...radead.org>
>> ---
>>  fs/pipe.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> We could just return -EINVAL when arg == 0, but we don't know how that might
>> adversely affect some programs.
>>
>>
>> --- lnx-413.orig/fs/pipe.c
>> +++ lnx-413/fs/pipe.c
>> @@ -1038,6 +1038,8 @@ static long pipe_set_size(struct pipe_in
>>  	unsigned long user_bufs;
>>  	long ret = 0;
>>  
>> +	if (!arg)
>> +		arg = PAGE_SIZE;
>>  	size = round_pipe_size(arg);
>>  	nr_pages = size >> PAGE_SHIFT;
> 
> Searching through lkml, it looks like there have been four attempts to
> fix this issue in the last year or so, but none of them have been
> merged.  It would be nice to finally get a fix in.
> 
> I agree with the patch, though I would argue that the fix belongs in
> round_pipe_size() so that other callers don't get the same bug.

Hi Josh,

Yes, I debated with myself on where to put the patch. I made it more local
instead of global just to have less accidental impact.


Andrew has these 4 patches from Joe Lawrence in mmotm:

pipe-add-proc_dopipe_max_size-to-safely-assign-pipe_max_size.patch
pipe-avoid-round_pipe_size-nr_pages-overflow-on-32-bit.patch
pipe-match-pipe_max_size-data-type-with-procfs.patch
sysctl-check-for-uint_max-before-unsigned-int-min-max.patch


Hopefully they can be merged soon.
I blame people who travel a lot.  ;)

-- 
~Randy