linux-kernel - Re: [syzbot] general protection fault in _parse_integer_fixup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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