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;
Date:   Sun, 23 Oct 2022 11:50:09 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
cc:     syzbot <syzbot+db1d2ea936378be0e4ea@syzkaller.appspotmail.com>,
        syzkaller-bugs@googlegroups.com, Ian Kent <raven@themaw.net>,
        Andrew Morton <akpm@linux-foundation.org>, hughd@google.com,
        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>
Subject: Re: [syzbot] general protection fault in
 _parse_integer_fixup_radix
In-Reply-To: <ce198a72-92c9-e09a-ca92-2860326c2938@I-love.SAKURA.ne.jp>
Message-ID: <17a1fdc-14a0-cf3c-784f-baa939895aef@google.com>
References: <0000000000002feb6605eb71458e@google.com> <ce198a72-92c9-e09a-ca92-2860326c2938@I-love.SAKURA.ne.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.

Hugh