Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i31e841b0:Fastmail
Message-ID: <f81f0ddc-cff3-b327-f4b2-57102bdf0ecf@themaw.net>
Date:   Tue, 1 Nov 2022 08:32:26 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [PATCH -next 0/5] fs: fix possible null-ptr-deref when parsing
 param
Content-Language: en-US
To:     Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
        Hawkins Jiawei <yin31149@gmail.com>, viro@zeniv.linux.org.uk
Cc:     18801353760@163.com, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, akpm@linux-foundation.org,
        cmaiolino@redhat.com, dhowells@redhat.com, hughd@google.com,
        miklos@szeredi.hu, oliver.sang@intel.com, siddhesh@gotplt.org,
        syzbot+db1d2ea936378be0e4ea@syzkaller.appspotmail.com,
        syzkaller-bugs@googlegroups.com, tytso@mit.edu, smfrench@gmail.com,
        pc@cjr.nz, lsahlber@redhat.com, sprasad@microsoft.com,
        tom@talpey.com
References: <Y1VwdUYGvDE4yUoI@ZenIV>
 <20221024004257.18689-1-yin31149@gmail.com>
 <7ba9257e-0285-117c-eada-04716230d5af@themaw.net>
 <af24da42-02cb-e60e-d1df-365801aa686b@I-love.SAKURA.ne.jp>
From:   Ian Kent <raven@themaw.net>
In-Reply-To: <af24da42-02cb-e60e-d1df-365801aa686b@I-love.SAKURA.ne.jp>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 31/10/22 19:28, Tetsuo Handa wrote:
> On 2022/10/24 12:34, Ian Kent wrote:
>> On 24/10/22 08:42, Hawkins Jiawei wrote:
>>> On Mon, 24 Oct 2022 at 00:48, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>>> On Mon, Oct 24, 2022 at 12:39:41AM +0800, Hawkins Jiawei wrote:
>>>>> According to commit "vfs: parse: deal with zero length string value",
>>>>> kernel will set the param->string to null pointer in vfs_parse_fs_string()
>>>>> if fs string has zero length.
>>>>>
>>>>> Yet the problem is that, when fs parses its mount parameters, it will
>>>>> dereferences the param->string, without checking whether it is a
>>>>> null pointer, which may trigger a null-ptr-deref bug.
>>>>>
>>>>> So this patchset reviews all functions for fs to parse parameters,
>>>>> by using `git grep -n "\.parse_param" fs/*`, and adds sanity check
>>>>> on param->string if its function will dereference param->string
>>>>> without check.
>>>> How about reverting the commit in question instead?  Or dropping it
>>>> from patch series, depending upon the way akpm handles the pile
>>>> these days...
>>> I think both are OK.
>>>
>>> On one hand, commit "vfs: parse: deal with zero length string value"
>>> seems just want to make output more informattive, which probably is not
>>> the one which must be applied immediately to fix the
>>> panic.
>>>
>>> On the other hand, commit "vfs: parse: deal with zero length string value"
>>> affects so many file systems, so there are probably some deeper
>>> null-ptr-deref bugs I ignore, which may take time to review.
>> Yeah, it would be good to make the file system handling consistent
>> but I think there's been a bit too much breakage and it appears not
>> everyone thinks the approach is the right way to do it.
>>
>> I'm thinking of abandoning this and restricting it to the "source"
>> parameter only to solve the user space mount table parser problem but
>> still doing it in the mount context code to keep it general (at least
>> for this case).
> No progress on this problem, and syzbot is reporting one after the other...
>
> I think that reverting is the better choice.

Yes, I agree/


Ian