[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <17a1fdc-14a0-cf3c-784f-baa939895aef@google.com>
Date: Sun, 23 Oct 2022 11:50:09 -0700 (PDT)
From: Hugh Dickins <hughd@...gle.com>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
cc: syzbot <syzbot+db1d2ea936378be0e4ea@...kaller.appspotmail.com>,
syzkaller-bugs@...glegroups.com, Ian Kent <raven@...maw.net>,
Andrew Morton <akpm@...ux-foundation.org>, hughd@...gle.com,
linux-kernel@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
Carlos Maiolino <cmaiolino@...hat.com>,
David Howells <dhowells@...hat.com>,
kernel test robot <oliver.sang@...el.com>,
Miklos Szeredi <miklos@...redi.hu>,
Siddhesh Poyarekar <siddhesh@...plt.org>,
Theodore Ts'o <tytso@....edu>,
Hawkins Jiawei <yin31149@...il.com>
Subject: Re: [syzbot] general protection fault in
_parse_integer_fixup_radix
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
Powered by blists - more mailing lists