[PATCH 0/2] vfs: fix a couple of mount table handling problems

[PATCH 0/2] vfs: fix a couple of mount table handling problems

[Ian Kent]

First, 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.

Second, when a filesystem is mounted with a name that starts with
a # the glibc finction getmentent() will interpret the leading #
as a comment so that the mount line will not appear in the output.

This is fixed by adding a # to the to be translated string in
fs/proc_namespace.c:mangle().

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

Ian Kent (1):
      vfs: parse: deal with zero length string value

Siddhesh Poyarekar (1):
      vfs: escape hash as well


 fs/fs_context.c     | 10 +++++++---
 fs/proc_namespace.c |  2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

--
Ian

[PATCH 1/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 |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index 24ce12f0db32..4c735d0ce3cb 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -175,9 +175,13 @@ 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;
+		if (!v_size)
+			param.string = NULL;
+		else {
+			param.string = kmemdup_nul(value, v_size, GFP_KERNEL);
+			if (!param.string)
+				return -ENOMEM;
+		}
 		param.type = fs_value_is_string;
 	}
 

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

[Al Viro]

On Tue, Jun 28, 2022 at 08:30:52AM +0800, Ian Kent 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.

	Hmmm...  And what happens if you feed that to ->parse_param(), which
calls fs_parse(), which decides that param->key looks like a name of e.g.
u32 option and calls fs_param_is_u32() to see what's what?  OOPS is a form
of rejection, I suppose, but...

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

[Ian Kent]

On 29/6/22 01:55, Al Viro wrote:
> On Tue, Jun 28, 2022 at 08:30:52AM +0800, Ian Kent 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.
> 	Hmmm...  And what happens if you feed that to ->parse_param(), which
> calls fs_parse(), which decides that param->key looks like a name of e.g.
> u32 option and calls fs_param_is_u32() to see what's what?  OOPS is a form
> of rejection, I suppose, but...

Oh ... yes, would you be ok with an update that moves the

"param.type = fs_value_is_string;" inside the above else

clause?


Ian

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

[Ian Kent]

On 29/6/22 09:06, Ian Kent wrote:
>
> On 29/6/22 01:55, Al Viro wrote:
>> On Tue, Jun 28, 2022 at 08:30:52AM +0800, Ian Kent 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.
>>     Hmmm...  And what happens if you feed that to ->parse_param(), which
>> calls fs_parse(), which decides that param->key looks like a name of 
>> e.g.
>> u32 option and calls fs_param_is_u32() to see what's what?  OOPS is a 
>> form
>> of rejection, I suppose, but...
>
> Oh ... yes, would you be ok with an update that moves the
>
> "param.type = fs_value_is_string;" inside the above else
>
> clause?

Looks like I'll need to use a type other than fs_value_is_string

so I can identify the case in those conversion functions when

there's no value for the parameter.


I'm tempted to use fs_value_is_flag since it's already present but

a new type of fs_value_is_empty is probably better.


What do you think about doing it like this and that type naming too?


Ian

[PATCH 2/2] vfs: escape hash as well

[Ian Kent]

From: Siddhesh Poyarekar <siddhesh@gotplt.org>

When a filesystem is mounted with a name that starts with a #:

 # mount '#name' /mnt/bad -t tmpfs

this will cause the entry to look like this (leading space added so
that git does not strip it out):

 #name /mnt/bad tmpfs rw,seclabel,relatime,inode64 0 0

This breaks getmntent and any code that aims to parse fstab as well as
/proc/mounts with the same logic since they need to strip leading spaces
or skip over comment lines, due to which they report incorrect output or
skip over the line respectively.

Solve this by translating the hash character into its octal encoding
equivalent so that applications can decode the name correctly.

Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/proc_namespace.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
index 49650e54d2f8..846f9455ae22 100644
--- a/fs/proc_namespace.c
+++ b/fs/proc_namespace.c
@@ -86,7 +86,7 @@ static void show_mnt_opts(struct seq_file *m, struct vfsmount *mnt)
 
 static inline void mangle(struct seq_file *m, const char *s)
 {
-	seq_escape(m, s, " \t\n\\");
+	seq_escape(m, s, " \t\n\\#");
 }
 
 static void show_type(struct seq_file *m, struct super_block *sb)

Re: [PATCH 2/2] vfs: escape hash as well

[Al Viro]

On Tue, Jun 28, 2022 at 08:30:58AM +0800, Ian Kent wrote:
> From: Siddhesh Poyarekar <siddhesh@gotplt.org>
> 
> When a filesystem is mounted with a name that starts with a #:
> 
>  # mount '#name' /mnt/bad -t tmpfs
> 
> this will cause the entry to look like this (leading space added so
> that git does not strip it out):
> 
>  #name /mnt/bad tmpfs rw,seclabel,relatime,inode64 0 0
> 
> This breaks getmntent and any code that aims to parse fstab as well as
> /proc/mounts with the same logic since they need to strip leading spaces
> or skip over comment lines, due to which they report incorrect output or
> skip over the line respectively.
> 
> Solve this by translating the hash character into its octal encoding
> equivalent so that applications can decode the name correctly.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
> Signed-off-by: Ian Kent <raven@themaw.net>

ACK; I'll grab that one (in #work.misc - I don't believe it's #fixes
fodder).

[PATCH 0/2] vfs: fix a couple of mount table handling problems

[Ian Kent]

First, 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.

Second, when a filesystem is mounted with a name that starts with
a # the glibc finction getmentent() will interpret the leading #
as a comment so that the mount line will not appear in the output.

This is fixed by adding a # to the to be translated string in
fs/proc_namespace.c:mangle().

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

Ian Kent (1):
      vfs: parse: deal with zero length string value

Siddhesh Poyarekar (1):
      vfs: escape hash as well


 fs/fs_context.c     | 10 +++++++---
 fs/proc_namespace.c |  2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

--
Ian Kent

[PATCH 1/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 |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index 24ce12f0db32..4c735d0ce3cb 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -175,9 +175,13 @@ 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;
+		if (!v_size)
+			param.string = NULL;
+		else {
+			param.string = kmemdup_nul(value, v_size, GFP_KERNEL);
+			if (!param.string)
+				return -ENOMEM;
+		}
 		param.type = fs_value_is_string;
 	}
 

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

[Christian Brauner]

On Fri, Jun 17, 2022 at 01:09:03PM +0800, Ian Kent 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.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---

Makes sense. Though I feel this is might be one of those instances where
we detect that some code isn't prepared for param.string to be NULL at
some point...
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

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

[Ian Kent]

From: Ian Kent <ikent@redhat.com>

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 <ikent@redhat.com>
---
 fs/fs_context.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index b7e43a780a62..a949cceccbfd 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -175,9 +175,13 @@ 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;
+		if (!v_size)
+			param.string = NULL;
+		else {
+			param.string = kmemdup_nul(value, v_size, GFP_KERNEL);
+			if (!param.string)
+				return -ENOMEM;
+		}
 		param.type = fs_value_is_string;
 	}