[REPOST PATCH v3 0/2] vfs: fix a mount table handling problem

[REPOST PATCH v3 0/2] vfs: fix a mount table handling problem

[Ian Kent]

Whenever a mount has an empty "source" (aka mnt_fsname), the glibc
function getmntent incorrectly parses its input, resulting in reporting
incorrect data to the caller.

The problem is that the get_mnt_entry() function in glibc's
misc/mntent_r.c assumes that leading whitespace on a line can always
be discarded because it will always be followed by a # for the case
of a comment or a non-whitespace character that's part of the value
of the first field. However, this assumption is violated when the
value of the first field is an empty string.

This is fixed in the mount API code by simply checking for a pointer
that contains a NULL and treating it as a NULL pointer.

Changes:

v3: added patch to fix zero length string access violation caused after
    fs parser patch is applied.

v2: fix possible oops if conversion functions such as fs_param_is_u32()
    are called.

Signed-off-by: Ian Kent <raven@themaw.net>
---

Ian Kent (2):
      ext4: fix possible null pointer dereference
      vfs: parse: deal with zero length string value


 fs/ext4/super.c            |  4 ++--
 fs/fs_context.c            | 17 ++++++++++++-----
 fs/fs_parser.c             | 16 ++++++++++++++++
 include/linux/fs_context.h |  3 ++-
 4 files changed, 32 insertions(+), 8 deletions(-)

--
Ian

[REPOST PATCH v3 1/2] ext4: fix possible null pointer dereference

[Ian Kent]

It could be the case that the file system parameter ->string value is
NULL rather than a zero length string.

Guard against this possibility in ext4_parse_param().

Reported-by: kernel test robot <oliver.sang@intel.com>
Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/ext4/super.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9a66abcca1a8..9f0ecb25c9a6 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2110,12 +2110,12 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	switch (token) {
 #ifdef CONFIG_QUOTA
 	case Opt_usrjquota:
-		if (!*param->string)
+		if (!param->string || !*param->string)
 			return unnote_qf_name(fc, USRQUOTA);
 		else
 			return note_qf_name(fc, USRQUOTA, param);
 	case Opt_grpjquota:
-		if (!*param->string)
+		if (!param->string || !*param->string)
 			return unnote_qf_name(fc, GRPQUOTA);
 		else
 			return note_qf_name(fc, GRPQUOTA, param);

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

[Ian Kent]

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.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/fs_context.c            |   17 ++++++++++++-----
 fs/fs_parser.c             |   16 ++++++++++++++++
 include/linux/fs_context.h |    3 ++-
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index 24ce12f0db32..df04e5fc6d66 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -96,7 +96,9 @@ int vfs_parse_fs_param_source(struct fs_context *fc, struct fs_parameter *param)
 	if (strcmp(param->key, "source") != 0)
 		return -ENOPARAM;
 
-	if (param->type != fs_value_is_string)
+	/* source value may be NULL */
+	if (param->type != fs_value_is_string &&
+	    param->type != fs_value_is_empty)
 		return invalf(fc, "Non-string source");
 
 	if (fc->source)
@@ -175,10 +177,15 @@ int vfs_parse_fs_string(struct fs_context *fc, const char *key,
 	};
 
 	if (value) {
-		param.string = kmemdup_nul(value, v_size, GFP_KERNEL);
-		if (!param.string)
-			return -ENOMEM;
-		param.type = fs_value_is_string;
+		if (!v_size) {
+			param.string = NULL;
+			param.type = fs_value_is_empty;
+		} else {
+			param.string = kmemdup_nul(value, v_size, GFP_KERNEL);
+			if (!param.string)
+				return -ENOMEM;
+			param.type = fs_value_is_string;
+		}
 	}
 
 	ret = vfs_parse_fs_param(fc, &param);
diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index ed40ce5742fd..2046f41ab00b 100644
--- 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))
@@ -226,6 +230,8 @@ EXPORT_SYMBOL(fs_param_is_u32);
 int fs_param_is_s32(struct p_log *log, const struct fs_parameter_spec *p,
 		    struct fs_parameter *param, struct fs_parse_result *result)
 {
+	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))
@@ -239,6 +245,8 @@ EXPORT_SYMBOL(fs_param_is_s32);
 int fs_param_is_u64(struct p_log *log, const struct fs_parameter_spec *p,
 		    struct fs_parameter *param, struct fs_parse_result *result)
 {
+	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))
@@ -253,6 +261,8 @@ int fs_param_is_enum(struct p_log *log, const struct fs_parameter_spec *p,
 		     struct fs_parameter *param, struct fs_parse_result *result)
 {
 	const struct constant_table *c;
+	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))
@@ -268,6 +278,8 @@ EXPORT_SYMBOL(fs_param_is_enum);
 int fs_param_is_string(struct p_log *log, const struct fs_parameter_spec *p,
 		       struct fs_parameter *param, struct fs_parse_result *result)
 {
+	if (param->type == fs_value_is_empty)
+		return 0;
 	if (param->type != fs_value_is_string ||
 	    (!*param->string && !(p->flags & fs_param_can_be_empty)))
 		return fs_param_bad_value(log, param);
@@ -278,6 +290,8 @@ EXPORT_SYMBOL(fs_param_is_string);
 int fs_param_is_blob(struct p_log *log, const struct fs_parameter_spec *p,
 		     struct fs_parameter *param, struct fs_parse_result *result)
 {
+	if (param->type == fs_value_is_empty)
+		return 0;
 	if (param->type != fs_value_is_blob)
 		return fs_param_bad_value(log, param);
 	return 0;
@@ -287,6 +301,8 @@ EXPORT_SYMBOL(fs_param_is_blob);
 int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
 		  struct fs_parameter *param, struct fs_parse_result *result)
 {
+	if (param->type == fs_value_is_empty)
+		return 0;
 	switch (param->type) {
 	case fs_value_is_string:
 		if ((!*param->string && !(p->flags & fs_param_can_be_empty)) ||
diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
index 13fa6f3df8e4..ff1375a16c8c 100644
--- a/include/linux/fs_context.h
+++ b/include/linux/fs_context.h
@@ -50,7 +50,8 @@ enum fs_context_phase {
  */
 enum fs_value_type {
 	fs_value_is_undefined,
-	fs_value_is_flag,		/* Value not given a value */
+	fs_value_is_flag,		/* Does not take a value */
+	fs_value_is_empty,		/* Value is not given */
 	fs_value_is_string,		/* Value is a string */
 	fs_value_is_blob,		/* Value is a binary blob */
 	fs_value_is_filename,		/* Value is a filename* + dirfd */

Re: [REPOST PATCH v3 0/2] vfs: fix a mount table handling problem

[Theodore Ts'o]

On Tue, Sep 20, 2022 at 03:26:17PM +0800, Ian Kent wrote:
> Whenever a mount has an empty "source" (aka mnt_fsname), the glibc
> function getmntent incorrectly parses its input, resulting in reporting
> incorrect data to the caller.
> 
> The problem is that the get_mnt_entry() function in glibc's
> misc/mntent_r.c assumes that leading whitespace on a line can always
> be discarded because it will always be followed by a # for the case
> of a comment or a non-whitespace character that's part of the value
> of the first field. However, this assumption is violated when the
> value of the first field is an empty string.
> 
> This is fixed in the mount API code by simply checking for a pointer
> that contains a NULL and treating it as a NULL pointer.

Why not simply have the mount API code disallow a zero-length "source"
/ mnt_fsname?

					- Ted

Re: [REPOST PATCH v3 0/2] vfs: fix a mount table handling problem

[Ian Kent]

On 21/9/22 09:20, Theodore Ts'o wrote:
> On Tue, Sep 20, 2022 at 03:26:17PM +0800, Ian Kent wrote:
>> Whenever a mount has an empty "source" (aka mnt_fsname), the glibc
>> function getmntent incorrectly parses its input, resulting in reporting
>> incorrect data to the caller.
>>
>> The problem is that the get_mnt_entry() function in glibc's
>> misc/mntent_r.c assumes that leading whitespace on a line can always
>> be discarded because it will always be followed by a # for the case
>> of a comment or a non-whitespace character that's part of the value
>> of the first field. However, this assumption is violated when the
>> value of the first field is an empty string.
>>
>> This is fixed in the mount API code by simply checking for a pointer
>> that contains a NULL and treating it as a NULL pointer.
> Why not simply have the mount API code disallow a zero-length "source"
> / mnt_fsname?

Hi Ted,


I suppose but it seems to me that, for certain file systems, mostly

pseudo file systems, the source isn't needed and is left out ... so

disallowing a zero length source could lead to quite a bit of breakage.


Ian

Re: [REPOST PATCH v3 0/2] vfs: fix a mount table handling problem

[Ian Kent]

On 21/9/22 12:38, Ian Kent wrote:
>
> On 21/9/22 09:20, Theodore Ts'o wrote:
>> On Tue, Sep 20, 2022 at 03:26:17PM +0800, Ian Kent wrote:
>>> Whenever a mount has an empty "source" (aka mnt_fsname), the glibc
>>> function getmntent incorrectly parses its input, resulting in reporting
>>> incorrect data to the caller.
>>>
>>> The problem is that the get_mnt_entry() function in glibc's
>>> misc/mntent_r.c assumes that leading whitespace on a line can always
>>> be discarded because it will always be followed by a # for the case
>>> of a comment or a non-whitespace character that's part of the value
>>> of the first field. However, this assumption is violated when the
>>> value of the first field is an empty string.
>>>
>>> This is fixed in the mount API code by simply checking for a pointer
>>> that contains a NULL and treating it as a NULL pointer.
>> Why not simply have the mount API code disallow a zero-length "source"
>> / mnt_fsname?
>
> Hi Ted,
>
>
> I suppose but it seems to me that, for certain file systems, mostly
>
> pseudo file systems, the source isn't needed and is left out ... so
>
> disallowing a zero length source could lead to quite a bit of breakage.

There's handling consistency too.


Ideally any empty string parameter will print "(none)" when listing

the proc mount tables. Mostly that happens in the proc code because

the field is NULL so if an empty string is specified due to having

to provide a positional parameter or for some other reason then

handling it by setting the zero length string to NULL in the mount

API is conveniently central. We could fix it in the proc code too

but then we might see cases get missed over time and we sacrifice

an opportunity to improve consistency.


To my mind continuing to allow it and dealing with what needs to be

done to make that work consistently seemed like the better long

term approach.


So based on that logic, sticky speaking, the ext patch shouldn't

retain the zero length string check but for now ...


>
>
> Ian
>

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

[Andrew Morton]

On Tue, 20 Sep 2022 15:26:29 +0800 Ian Kent <raven@themaw.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?

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

[Ian Kent]

On 18/10/22 09:55, Andrew Morton wrote:
> On Tue, 20 Sep 2022 15:26:29 +0800 Ian Kent <raven@themaw.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