Re: [PATCH] bfs: add sanity check at bfs_fill_super().

From: Dmitry Vyukov <dvyukov@google.com>
To: Tigran Aivazian <aivazian.tigran@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	syzbot <syzbot+71c6b5d68e91149fc8a4@syzkaller.appspotmail.com>,
	syzkaller-bugs <syzkaller-bugs@googlegroups.com>
Subject: Re: [PATCH] bfs: add sanity check at bfs_fill_super().
Date: Wed, 13 Jun 2018 18:00:26 +0200	[thread overview]
Message-ID: <CACT4Y+aFF4qCDT70Um-CR17bZc3UzL=hJYJsR0BmSvdPw7w_KQ@mail.gmail.com> (raw)
In-Reply-To: <CAK+_RLko_OCepN4FCmsaQPAKkt9JNGe8pNRK7SO-onhw5zCneA@mail.gmail.com>

On Wed, Jun 13, 2018 at 3:49 PM, Tigran Aivazian
<aivazian.tigran@gmail.com> wrote:
> Having read the discussion carefully, I personally prefer to ignore
> the fix as invalid, because mounting a filesystem image is a
> privileged operation and if attempting to mount a corrupted image
> causes a panic, that is no big deal, imho.

Even if not a big deal, but still a bug?

Also note that this is kind a chicken and egg problem. The only reason
why these operations are privileged is that we have bugs and we are
not fixing them.

Also keep this in mind whenever you insert anything into usb and
automount if turned on ;) You are basically giving permission to that
thing to do everything with the machine. Or when you mount any image
not created by you with trusted tools that you build yourself from
trusted sources with a trusted compiler.

Also keep in mind Android and other systems where root is not a trusted entity.

> On 13 June 2018 at 14:33, Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>> On 2018/05/10 8:53, Andrew Morton wrote:
>>> On Thu, 10 May 2018 08:46:18 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>>
>>>>> page-allocation-fauilure warning and a nice backtrace, etc.  Why
>>>>> suppress all of that and add our custom warning instead?
>>>>
>>>> the intent of this patch is to avoid panic() by panic_on_warn == 1
>>>> due to hitting
>>>>
>>>> struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
>>>> {
>>>>         unsigned int index;
>>>>
>>>>         if (unlikely(size > KMALLOC_MAX_SIZE)) {
>>>>                 WARN_ON_ONCE(!(flags & __GFP_NOWARN)); /* <= this line */
>>>>                 return NULL;
>>>>         }
>>>>
>>>> when size to allocate is controlled by the filesystem image.
>>>
>>> Well, the same could happen with many many memory-allocation sites.
>>> What's special about BFS?  If someone sets panic_on_warn=1 then
>>> presumably this panic is the behaviour they wanted in this case.
>>>
>>
>> Tigran, this patch is stalling. Do we want to apply this? Or, ignore as invalid?
>>
>> errors=panic mount option for ext4 case was ignored as invalid.
>> http://lkml.kernel.org/r/CACT4Y+Z+2YW_VALJzzQr6hLsviA=dXk3iFqwVf+P5zqojeC9Zg@mail.gmail.com
>>
>> But I prefer avoiding crashes if we can fix it.
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/CAK%2B_RLko_OCepN4FCmsaQPAKkt9JNGe8pNRK7SO-onhw5zCneA%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.