[PATCH v2 0/4] enhance the path resolution capability in fs_parser

[PATCH v2 0/4] enhance the path resolution capability in fs_parser

[Hongbo Li]

Mount options with path should be parsed into block device or inode. As
the new mount API provides a serial of parsers, path should also be 
looked up into block device within these parsers, not in each specific
filesystem.

The following is a brief overview of the patches, see the patches for
more details.

Patch 1-2: Enhance the path resolution capability in fs_parser.
Patch 3: Fix the `journal_path` options error in ext4. 
Patch 4: Remove the `fs_lookup_param` and its description.

Comments and questions are, as always, welcome.

Thanks,
Hongbo

Changes since v1:
	* Fix test robot warning and rebase on latest code.

Hongbo Li (4):
  fs: add blockdev parser for filesystem mount option.
  fs: add path parser for filesystem mount option.
  fs: ext4: support relative path for `journal_path` in mount option.
  fs: remove fs_lookup_param and its description.

 Documentation/filesystems/mount_api.rst |  17 +---
 fs/ext4/super.c                         |  26 +----
 fs/fs_parser.c                          | 125 +++++++++++++-----------
 include/linux/fs_parser.h               |   7 +-
 4 files changed, 71 insertions(+), 104 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] fs: add blockdev parser for filesystem mount option.

[Hongbo Li]

`fsparam_bdev` uses `fs_param_is_blockdev` to parse the option, but it
is currently empty. Filesystems like ext4 use the `fsparam_bdev` to
parse `journal_path` into the block device. This general logic should
be moved to the vfs layer, not the specific filesystem. Therefore, we
implement block device parser in `fs_param_is_blockdev`. And the logic
is similar with `fs_lookup_param`.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 fs/fs_parser.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index a4d6ca0b8971..2aa208cf2027 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -311,7 +311,56 @@ EXPORT_SYMBOL(fs_param_is_fd);
 int fs_param_is_blockdev(struct p_log *log, const struct fs_parameter_spec *p,
 		  struct fs_parameter *param, struct fs_parse_result *result)
 {
-	return 0;
+	int ret;
+	int dfd;
+	struct filename *f;
+	struct inode *dev_inode;
+	struct path path;
+	bool put_f;
+
+	switch (param->type) {
+	case fs_value_is_string:
+		if (!*param->string) {
+			if (p->flags & fs_param_can_be_empty)
+				return 0;
+			return fs_param_bad_value(log, param);
+		}
+		f = getname_kernel(param->string);
+		if (IS_ERR(f))
+			return fs_param_bad_value(log, param);
+		dfd = AT_FDCWD;
+		put_f = true;
+		break;
+	case fs_value_is_filename:
+		f = param->name;
+		dfd = param->dirfd;
+		put_f = false;
+		break;
+	default:
+		return fs_param_bad_value(log, param);
+	}
+
+	ret = filename_lookup(dfd, f, LOOKUP_FOLLOW, &path, NULL);
+	if (ret < 0) {
+		error_plog(log, "%s: Lookup failure for '%s'", param->key, f->name);
+		goto out_putname;
+	}
+
+	dev_inode = d_backing_inode(path.dentry);
+	if (!S_ISBLK(dev_inode->i_mode)) {
+		error_plog(log, "%s: Non-blockdev passed as '%s'", param->key, f->name);
+		ret = -1;
+		goto out_putpath;
+	}
+	result->uint_32 = new_encode_dev(dev_inode->i_rdev);
+
+out_putpath:
+	path_put(&path);
+out_putname:
+	if (put_f)
+		putname(f);
+
+	return (ret < 0) ? fs_param_bad_value(log, param) : 0;
 }
 EXPORT_SYMBOL(fs_param_is_blockdev);
 
-- 
2.34.1

[PATCH v2 2/4] fs: add path parser for filesystem mount option.

[Hongbo Li]

`fsparam_path` uses `fs_param_is_path` to parse the option, but it
is currently empty. The new mount api has considered this option in
`fsconfig`(that is FSCONFIG_SET_PATH). Here we add general path parser
in filesystem layer. Currently, no filesystem uses this function to
parse parameters, we add `void *ptr` in `fs_parse_result` to point to
the target structure(such as `struct inode *`).

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 fs/fs_parser.c            | 18 ++++++++++++++++++
 include/linux/fs_parser.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index 2aa208cf2027..5d0adcc514d8 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -367,6 +367,24 @@ EXPORT_SYMBOL(fs_param_is_blockdev);
 int fs_param_is_path(struct p_log *log, const struct fs_parameter_spec *p,
 		     struct fs_parameter *param, struct fs_parse_result *result)
 {
+	int ret;
+	struct filename *f;
+	struct path path;
+
+	if (param->type != fs_value_is_filename)
+		return fs_param_bad_value(log, param);
+	if (!*param->string && (p->flags & fs_param_can_be_empty))
+		return 0;
+
+	f = param->name;
+	ret = filename_lookup(param->dirfd, f, LOOKUP_FOLLOW, &path, NULL);
+	if (ret < 0) {
+		error_plog(log, "%s: Lookup failure for '%s'", param->key, f->name);
+		return fs_param_bad_value(log, param);
+	}
+	result->ptr = d_backing_inode(path.dentry);
+	path_put(&path);
+
 	return 0;
 }
 EXPORT_SYMBOL(fs_param_is_path);
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index d3350979115f..489c71d06a5f 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -57,6 +57,7 @@ struct fs_parse_result {
 		int		int_32;		/* For spec_s32/spec_enum */
 		unsigned int	uint_32;	/* For spec_u32{,_octal,_hex}/spec_enum */
 		u64		uint_64;	/* For spec_u64 */
+		const void	*ptr;		/* For spec_ptr */
 	};
 };
 
-- 
2.34.1

Re: [PATCH v2 2/4] fs: add path parser for filesystem mount option.

[Christian Brauner]

On Mon, May 27, 2024 at 03:58:52PM +0800, Hongbo Li wrote:
> `fsparam_path` uses `fs_param_is_path` to parse the option, but it
> is currently empty. The new mount api has considered this option in
> `fsconfig`(that is FSCONFIG_SET_PATH). Here we add general path parser
> in filesystem layer. Currently, no filesystem uses this function to
> parse parameters, we add `void *ptr` in `fs_parse_result` to point to
> the target structure(such as `struct inode *`).
> 
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
>  fs/fs_parser.c            | 18 ++++++++++++++++++
>  include/linux/fs_parser.h |  1 +
>  2 files changed, 19 insertions(+)
> 
> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> index 2aa208cf2027..5d0adcc514d8 100644
> --- a/fs/fs_parser.c
> +++ b/fs/fs_parser.c
> @@ -367,6 +367,24 @@ EXPORT_SYMBOL(fs_param_is_blockdev);
>  int fs_param_is_path(struct p_log *log, const struct fs_parameter_spec *p,
>  		     struct fs_parameter *param, struct fs_parse_result *result)
>  {
> +	int ret;
> +	struct filename *f;
> +	struct path path;
> +
> +	if (param->type != fs_value_is_filename)
> +		return fs_param_bad_value(log, param);
> +	if (!*param->string && (p->flags & fs_param_can_be_empty))
> +		return 0;
> +
> +	f = param->name;
> +	ret = filename_lookup(param->dirfd, f, LOOKUP_FOLLOW, &path, NULL);
> +	if (ret < 0) {
> +		error_plog(log, "%s: Lookup failure for '%s'", param->key, f->name);
> +		return fs_param_bad_value(log, param);
> +	}
> +	result->ptr = d_backing_inode(path.dentry);
> +	path_put(&path);

That smells like a UAF:

dfd = open("/bla");
fsconfig(FSCONFIG_SET_PATH, dfd, "blub", 0);
close(dfd);
umount("/bla");

and that result->ptr now has a dangling pointer which will be triggered by:

fsconfig(FSCONFIG_CMD_CREATE);

Re: [PATCH v2 2/4] fs: add path parser for filesystem mount option.

[Christoph Hellwig]

On Mon, May 27, 2024 at 04:32:00PM +0200, Christian Brauner wrote:
> That smells like a UAF:
> 
> dfd = open("/bla");
> fsconfig(FSCONFIG_SET_PATH, dfd, "blub", 0);
> close(dfd);
> umount("/bla");
> 
> and that result->ptr now has a dangling pointer which will be triggered by:
> 
> fsconfig(FSCONFIG_CMD_CREATE);

Yeah.  Also the whole path thing is entirely unused.  The best thing
to do about it is to remove it, we can always resurrect if if/when we
actually need it.

[PATCH v2 3/4] fs: ext4: support relative path for `journal_path` in mount option.

[Hongbo Li]

After `fs_param_is_blockdev` is implemented(in fs: add blockdev parser
for filesystem mount option.), `journal_devnum` can be obtained from
`result.uint_32` directly.

Additionally, the `fs_lookup_param` did not consider the relative path
for block device. When we mount ext4 with `journal_path` option using
relative path, `param->dirfd` was not set which will cause mounting
error.

This can be reproduced easily like this:

mke2fs -F -O journal_dev $JOURNAL_DEV -b 4096 100M
mkfs.ext4 -F -J device=$JOURNAL_DEV -b 4096 $FS_DEV
cd /dev; mount -t ext4 -o journal_path=`basename $JOURNAL_DEV` $FS_DEV $MNT

Fixes: 461c3af045d3 ("ext4: Change handle_mount_opt() to use fs_parameter")
Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 fs/ext4/super.c | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c682fb927b64..94b39bcae99d 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2290,39 +2290,15 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		ctx->spec |= EXT4_SPEC_s_resgid;
 		return 0;
 	case Opt_journal_dev:
-		if (is_remount) {
-			ext4_msg(NULL, KERN_ERR,
-				 "Cannot specify journal on remount");
-			return -EINVAL;
-		}
-		ctx->journal_devnum = result.uint_32;
-		ctx->spec |= EXT4_SPEC_JOURNAL_DEV;
-		return 0;
 	case Opt_journal_path:
-	{
-		struct inode *journal_inode;
-		struct path path;
-		int error;
-
 		if (is_remount) {
 			ext4_msg(NULL, KERN_ERR,
 				 "Cannot specify journal on remount");
 			return -EINVAL;
 		}
-
-		error = fs_lookup_param(fc, param, 1, LOOKUP_FOLLOW, &path);
-		if (error) {
-			ext4_msg(NULL, KERN_ERR, "error: could not find "
-				 "journal device path");
-			return -EINVAL;
-		}
-
-		journal_inode = d_inode(path.dentry);
-		ctx->journal_devnum = new_encode_dev(journal_inode->i_rdev);
+		ctx->journal_devnum = result.uint_32;
 		ctx->spec |= EXT4_SPEC_JOURNAL_DEV;
-		path_put(&path);
 		return 0;
-	}
 	case Opt_journal_ioprio:
 		if (result.uint_32 > 7) {
 			ext4_msg(NULL, KERN_ERR, "Invalid journal IO priority"
-- 
2.34.1

Re: [PATCH v2 3/4] fs: ext4: support relative path for `journal_path` in mount option.

[Christian Brauner]

On Mon, May 27, 2024 at 03:58:53PM +0800, Hongbo Li wrote:
> After `fs_param_is_blockdev` is implemented(in fs: add blockdev parser
> for filesystem mount option.), `journal_devnum` can be obtained from
> `result.uint_32` directly.
> 
> Additionally, the `fs_lookup_param` did not consider the relative path
> for block device. When we mount ext4 with `journal_path` option using
> relative path, `param->dirfd` was not set which will cause mounting
> error.

That's a failure in userspace though. If a relative pathname is provided
then AT_FDCWD or a valid file descriptor must be provided:

fsconfig(fd_fs, FSCONFIG_SET_PATH, "journal_path", "relative/path", AT_FDCWD);

But if you're seeing this from mount(8) then util-linux very likely
does (cf. [1]):

fsconfig(fd_fs, FSCONFIG_SET_STRING, "journal_path", "relative/path", 0);

So mount(8) is passing that as a string as it has no way to figure out
that this should be passed as FSCONFIG_SET_PATH. So to allow relative
paths in string options we'd need something (untested) like:

diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index a4d6ca0b8971..18ca40408e91 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc,
                f = getname_kernel(param->string);
                if (IS_ERR(f))
                        return PTR_ERR(f);
+               /* relative path */
+               if (*f->name[0] != '/')
+                       param->dirfd = AT_FDCWD;
                put_f = true;
                break;
        case fs_value_is_filename:

https://github.com/util-linux/util-linux/blob/55ca447a6a95226fd031a126fb48b01b3efd6284/libmount/src/hook_mount.c#L178

> 
> This can be reproduced easily like this:
> 
> mke2fs -F -O journal_dev $JOURNAL_DEV -b 4096 100M
> mkfs.ext4 -F -J device=$JOURNAL_DEV -b 4096 $FS_DEV
> cd /dev; mount -t ext4 -o journal_path=`basename $JOURNAL_DEV` $FS_DEV $MNT
> 
> Fixes: 461c3af045d3 ("ext4: Change handle_mount_opt() to use fs_parameter")
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
>  fs/ext4/super.c | 26 +-------------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c682fb927b64..94b39bcae99d 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2290,39 +2290,15 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  		ctx->spec |= EXT4_SPEC_s_resgid;
>  		return 0;
>  	case Opt_journal_dev:
> -		if (is_remount) {
> -			ext4_msg(NULL, KERN_ERR,
> -				 "Cannot specify journal on remount");
> -			return -EINVAL;
> -		}
> -		ctx->journal_devnum = result.uint_32;
> -		ctx->spec |= EXT4_SPEC_JOURNAL_DEV;
> -		return 0;
>  	case Opt_journal_path:
> -	{
> -		struct inode *journal_inode;
> -		struct path path;
> -		int error;
> -
>  		if (is_remount) {
>  			ext4_msg(NULL, KERN_ERR,
>  				 "Cannot specify journal on remount");
>  			return -EINVAL;
>  		}

This would cause a path lookup in the VFS layer only to immediately
reject it on remount.

[PATCH v2 4/4] fs: remove fs_lookup_param and its description.

[Hongbo Li]

After `fs_param_is_blockdev` and `fs_param_is_path` are implemented,
there are no need to lookup the path with `fs_lookup_param`, and
`fs_parse` is sufficient.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 Documentation/filesystems/mount_api.rst | 17 +-------
 fs/fs_parser.c                          | 56 -------------------------
 include/linux/fs_parser.h               |  6 ---
 3 files changed, 1 insertion(+), 78 deletions(-)

diff --git a/Documentation/filesystems/mount_api.rst b/Documentation/filesystems/mount_api.rst
index 9aaf6ef75eb5..f92d96758e57 100644
--- a/Documentation/filesystems/mount_api.rst
+++ b/Documentation/filesystems/mount_api.rst
@@ -253,7 +253,7 @@ manage the filesystem context.  They are as follows:
      will have been weeded out and fc->sb_flags updated in the context.
      Security options will also have been weeded out and fc->security updated.
 
-     The parameter can be parsed with fs_parse() and fs_lookup_param().  Note
+     The parameter can be parsed with fs_parse().  Note
      that the source(s) are presented as parameters named "source".
 
      If successful, 0 should be returned or a negative error code otherwise.
@@ -796,18 +796,3 @@ process the parameters it is given.
      If the parameter isn't matched, -ENOPARAM will be returned; if the
      parameter is matched, but the value is erroneous, -EINVAL will be
      returned; otherwise the parameter's option number will be returned.
-
-   * ::
-
-       int fs_lookup_param(struct fs_context *fc,
-			   struct fs_parameter *value,
-			   bool want_bdev,
-			   unsigned int flags,
-			   struct path *_path);
-
-     This takes a parameter that carries a string or filename type and attempts
-     to do a path lookup on it.  If the parameter expects a blockdev, a check
-     is made that the inode actually represents one.
-
-     Returns 0 if successful and ``*_path`` will be set; returns a negative
-     error code if not.
diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index 5d0adcc514d8..962adb1fec80 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -133,62 +133,6 @@ int __fs_parse(struct p_log *log,
 }
 EXPORT_SYMBOL(__fs_parse);
 
-/**
- * fs_lookup_param - Look up a path referred to by a parameter
- * @fc: The filesystem context to log errors through.
- * @param: The parameter.
- * @want_bdev: T if want a blockdev
- * @flags: Pathwalk flags passed to filename_lookup()
- * @_path: The result of the lookup
- */
-int fs_lookup_param(struct fs_context *fc,
-		    struct fs_parameter *param,
-		    bool want_bdev,
-		    unsigned int flags,
-		    struct path *_path)
-{
-	struct filename *f;
-	bool put_f;
-	int ret;
-
-	switch (param->type) {
-	case fs_value_is_string:
-		f = getname_kernel(param->string);
-		if (IS_ERR(f))
-			return PTR_ERR(f);
-		put_f = true;
-		break;
-	case fs_value_is_filename:
-		f = param->name;
-		put_f = false;
-		break;
-	default:
-		return invalf(fc, "%s: not usable as path", param->key);
-	}
-
-	ret = filename_lookup(param->dirfd, f, flags, _path, NULL);
-	if (ret < 0) {
-		errorf(fc, "%s: Lookup failure for '%s'", param->key, f->name);
-		goto out;
-	}
-
-	if (want_bdev &&
-	    !S_ISBLK(d_backing_inode(_path->dentry)->i_mode)) {
-		path_put(_path);
-		_path->dentry = NULL;
-		_path->mnt = NULL;
-		errorf(fc, "%s: Non-blockdev passed as '%s'",
-		       param->key, f->name);
-		ret = -ENOTBLK;
-	}
-
-out:
-	if (put_f)
-		putname(f);
-	return ret;
-}
-EXPORT_SYMBOL(fs_lookup_param);
-
 static int fs_param_bad_value(struct p_log *log, struct fs_parameter *param)
 {
 	return inval_plog(log, "Bad value for '%s'", param->key);
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index 489c71d06a5f..fa2745254818 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -74,12 +74,6 @@ static inline int fs_parse(struct fs_context *fc,
 	return __fs_parse(&fc->log, desc, param, result);
 }
 
-extern int fs_lookup_param(struct fs_context *fc,
-			   struct fs_parameter *param,
-			   bool want_bdev,
-			   unsigned int flags,
-			   struct path *_path);
-
 extern int lookup_constant(const struct constant_table tbl[], const char *name, int not_found);
 
 #ifdef CONFIG_VALIDATE_FS_PARSER
-- 
2.34.1

Re: [PATCH v2 0/4] enhance the path resolution capability in fs_parser

[Christian Brauner]

On Mon, May 27, 2024 at 03:58:50PM +0800, Hongbo Li wrote:
> Mount options with path should be parsed into block device or inode. As
> the new mount API provides a serial of parsers, path should also be 
> looked up into block device within these parsers, not in each specific
> filesystem.

The problem is that by moving those options into the VFS layer we're
dictating when and with what permissions paths are resolved.

Parsing of parameters via fsconfig(FSCONFIG_SET_*) and superblock
creation via fsconfig(FSCONFIG_CMD_CREAT) can happen in different
contexts.

It's entirely possible that the parameter is set by an unprivileged task
that cannot open the provided path while the task that actually creates
the superblock does have the permission to resolve the path.

It's also legitimate that a filesystem may want to explicitly delay all
path resolution until the superblock is created and not resolve the path
right when it's passed.

That wouldn't be possible anymore after your changes. So this is a more
subtle change than it seems and I'm not sure it's worth it.