linux-kernel - Re: [REPOST PATCH v3 2/2] vfs: parse: deal with zero length string value

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] [day] [month] [year] [list]

Message-ID: <cf9a666c-e663-fb9e-ba3f-4052edf17536@themaw.net>
Date:   Tue, 18 Oct 2022 10:07:13 +0800
From:   Ian Kent <raven@...maw.net>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Al Viro <viro@...IV.linux.org.uk>,
        Siddhesh Poyarekar <siddhesh@...plt.org>,
        David Howells <dhowells@...hat.com>,
        Miklos Szeredi <miklos@...redi.hu>,
        Carlos Maiolino <cmaiolino@...hat.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [REPOST PATCH v3 2/2] vfs: parse: deal with zero length string
 value


On 18/10/22 09:55, Andrew Morton wrote:
> On Tue, 20 Sep 2022 15:26:29 +0800 Ian Kent <raven@...maw.net> wrote:
>
>> Parsing an fs string that has zero length should result in the parameter
>> being set to NULL so that downstream processing handles it correctly.
>> 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.
>>
>> ...
>>
>> --- a/fs/fs_parser.c
>> +++ b/fs/fs_parser.c
>> @@ -197,6 +197,8 @@ int fs_param_is_bool(struct p_log *log, const struct fs_parameter_spec *p,
>>   		     struct fs_parameter *param, struct fs_parse_result *result)
>>   {
>>   	int b;
>> +	if (param->type == fs_value_is_empty)
>> +		return 0;
>>   	if (param->type != fs_value_is_string)
>>   		return fs_param_bad_value(log, param);
>>   	if (!*param->string && (p->flags & fs_param_can_be_empty))
>> @@ -213,6 +215,8 @@ int fs_param_is_u32(struct p_log *log, const struct fs_parameter_spec *p,
>>   		    struct fs_parameter *param, struct fs_parse_result *result)
>>   {
>>   	int base = (unsigned long)p->data;
>> +	if (param->type == fs_value_is_empty)
>> +		return 0;
>>   	if (param->type != fs_value_is_string)
>>   		return fs_param_bad_value(log, param);
>>   	if (!*param->string && (p->flags & fs_param_can_be_empty))
>>
>> [etcetera]
> This feels wrong.  Having to check for fs_value_is_empty in so many
> places makes me think "we just shouldn't have got this far".  Am I
> right for once?


Maybe, did you have a different approach in mind ... ?


My thought was that these helper functions need to protect themselves

against things that could creep in and we don't know what they may be.


I'm not sure the best strategy is to not ever do this type of call in 
the first

place ...


Ian