Re: [syzbot] general protection fault in _parse_integer_fixup_radix

[Hugh Dickins]

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