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

[Tigran Aivazian <aivazian.tigran@gmail.com>]

Hello Dmitry,

Historically, Unix mount(2)  (and mount(8)) are normally privileged
unless the filesystem is "trusted" by sysadmin who recorded this fact
in the corresponding entry in /etc/fstab (see fstab(5)). I agree that
the filesystem mounting code should perform enough validation on the
superblock to avoid crashing on an invalid filesystem image. But I
disagree that some specific filesystem (and least of all BFS!) should
modify the panic_on_warn semantics and replace it with its own private
warning message on kernel memory allocation failures.

Kind regards,
Tigran

On 13 June 2018 at 17:00, Dmitry Vyukov <dvyukov@google.com> wrote:
> 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.