From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754559AbdKIWJR (ORCPT ); Thu, 9 Nov 2017 17:09:17 -0500 Received: from merlin.infradead.org ([205.233.59.134]:42676 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751174AbdKIWJP (ORCPT ); Thu, 9 Nov 2017 17:09:15 -0500 Subject: Re: [RFC PATCH] fs/pipe.c: implement minimum pipe size for arg==0 To: Josh Poimboeuf Cc: LKML , Linux FS Devel , Al Viro , Andrew Morton , Shankara Pailoor , Michael Kerrisk References: <20171109214507.p6q5xpdmmiqd47uo@treble> From: Randy Dunlap Message-ID: <6f9f4e86-9102-a8a1-7c6a-8586f47be044@infradead.org> Date: Thu, 9 Nov 2017 14:09:03 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171109214507.p6q5xpdmmiqd47uo@treble> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> >> 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 >> 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 >> Signed-off-by: Randy Dunlap >> --- >> 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