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: <625dfe41-c665-fc9c-00a3-e8e2e13c07bc@themaw.net>
Date:   Mon, 24 Oct 2022 07:47:23 +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: [syzbot] general protection fault in _parse_integer_fixup_radix
To:     Hugh Dickins <hughd@google.com>,
        Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc:     syzbot <syzbot+db1d2ea936378be0e4ea@syzkaller.appspotmail.com>,
        syzkaller-bugs@googlegroups.com,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>,
        Carlos Maiolino <cmaiolino@redhat.com>,
        David Howells <dhowells@redhat.com>,
        kernel test robot <oliver.sang@intel.com>,
        Miklos Szeredi <miklos@szeredi.hu>,
        Siddhesh Poyarekar <siddhesh@gotplt.org>,
        Theodore Ts'o <tytso@mit.edu>,
        Hawkins Jiawei <yin31149@gmail.com>
References: <0000000000002feb6605eb71458e@google.com>
 <ce198a72-92c9-e09a-ca92-2860326c2938@I-love.SAKURA.ne.jp>
 <17a1fdc-14a0-cf3c-784f-baa939895aef@google.com>
Content-Language: en-US
From:   Ian Kent <raven@themaw.net>
In-Reply-To: <17a1fdc-14a0-cf3c-784f-baa939895aef@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 24/10/22 02:50, Hugh Dickins wrote:
> On Sun, 23 Oct 2022, Tetsuo Handa wrote:
>
>> syzbot is reporting that "vfs: parse: deal with zero length string value"
>> in linux-next.git broke tmpfs's mount option parsing, for tmpfs is expecting that
>> vfs_parse_fs_string() returning 0 implies that param.string != NULL.
>>
>> The "nr_inodes" parameter for tmpfs is interpreted as "nr_inodes=$integer", but
>> the addition of
>>
>> 	if (!v_size) {
>> 		param.string = NULL;
>> 		param.type = fs_value_is_empty;
>> 	} else {
>>
>> to vfs_parse_fs_string() and
>>
>> 	if (param->type == fs_value_is_empty)
>> 		return 0;
>>
>> to fs_param_is_string() broke expectation by tmpfs.
>>
>>    Parsing an fs string that has zero length should result in the parameter
>>    being set to NULL so that downstream processing handles it correctly.
>>
>> is wrong and
>>
>>    Parsing an fs string that has zero length should result in invalid argument
>>    error so that downstream processing does not dereference NULL param.string
>>    field.
>>
>> is correct for the "nr_inodes" parameter.
>>
>>
>>
>> How do we want to fix?
>> Should we add param.string != NULL checks into the downstream callers (like
>> Hawkins Jiawei did for https://syzkaller.appspot.com/bug?extid=a3e6acd85ded5c16a709 ) ?
>> Or should we add
>>
>> 	if (!*param.string)
>> 		param.string = NULL;
>>
>> rewriting into downstream callers which expect
>>
>>    For example, the proc mount table processing should print "(none)" in this
>>    case to preserve mount record field count, but if the value points to the
>>    NULL string this doesn't happen.
>>
>> behavior?
> I've given it no thought at all: I was hoping, as Al suggests in
> https://lore.kernel.org/lkml/Y1VwdUYGvDE4yUoI@ZenIV/
> that the breaking commit would soon be reverted, and Ian think again.

Except that I didn't see the message so I haven't given it extra thought

myself either, oops!


akpm and Theodore also had concerns about the series.


The other way to fix this is to modify the proc processing to check

for zero length strings and check for any other places that need

fixing. But that means handling it downstream for individual allocated

empty string instances rather than at the source which is what I was

hoping to avoid.


But clearly there are hard to find assumptions in code that I've missed

and this instance isn't the first case of it so may be we have to drop

the series.


I can't think of any other way to do this without requiring NULL be

handled, does anyone have any thoughts to offer?


Ian