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

[Dmitry Vyukov <dvyukov@google.com>]

On Thu, Jun 14, 2018 at 2:23 PM, Tigran Aivazian
<aivazian.tigran@gmail.com> wrote:
> 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.

This is not a general memory allocation failure. Nothing is printed on
memory allocation failures.
This is a very specific case of memory allocation failure, namely when
kernel code asks for too large block, such large that it can't be
possible allocated regardless of amount of available memory. For
allocations with user-controllable size kernel code either needs to
use __GFP_NOWARN, or (better) impose own limits such that it can
always be allocated with kmalloc.
Consider, currently we can have a bfs image that works fine on one
kernel, but fails to mount on another just because it happens so that
one could allocate 4MB with kmalloc, but another can't (different
allocator/different settings/different kernel revision). This looks
like pretty unfortunate property for persistent data formats in general.



> 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.