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

From: Randy Dunlap <rdunlap@infradead.org>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Linux FS Devel <linux-fsdevel@vger.kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	Shankara Pailoor <sp3485@columbia.edu>,
	Michael Kerrisk <mtk.manpages@gmail.com>
Subject: Re: [RFC PATCH] fs/pipe.c: implement minimum pipe size for arg==0
Date: Thu, 9 Nov 2017 14:09:03 -0800	[thread overview]
Message-ID: <6f9f4e86-9102-a8a1-7c6a-8586f47be044@infradead.org> (raw)
In-Reply-To: <20171109214507.p6q5xpdmmiqd47uo@treble>

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@infradead.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@columbia.edu>
>> Signed-off-by: Randy Dunlap <rdunlap@infradead.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