1. Patchew
  2. linux
  3. View series

[PATCH] btrfs: harden parsing of compress mount option

Daniel Vacek posted 1 patch 8 months ago
There is a newer version of this series
fs/btrfs/super.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
[PATCH] btrfs: harden parsing of compress mount option
Posted by Daniel Vacek 8 months ago 
Btrfs happily but incorrectly accepts the `-o compress=zlib+foo` and similar
options with any random suffix. Let's handle that correctly.

Signed-off-by: Daniel Vacek <neelx@suse.com>
---
 fs/btrfs/super.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 40709e2a44fce..f7e064b8c6d88 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -354,7 +354,10 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 			btrfs_set_opt(ctx->mount_opt, COMPRESS);
 			btrfs_clear_opt(ctx->mount_opt, NODATACOW);
 			btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-		} else if (strncmp(param->string, "zlib", 4) == 0) {
+		} else if (strncmp(param->string, "zlib", 4) == 0 &&
+				(param->string[4] == ':' ||
+				 param->string[4] == ',' ||
+				 param->string[4] == '\0')) {
 			ctx->compress_type = BTRFS_COMPRESS_ZLIB;
 			ctx->compress_level =
 				btrfs_compress_str2level(BTRFS_COMPRESS_ZLIB,
@@ -362,13 +365,18 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 			btrfs_set_opt(ctx->mount_opt, COMPRESS);
 			btrfs_clear_opt(ctx->mount_opt, NODATACOW);
 			btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-		} else if (strncmp(param->string, "lzo", 3) == 0) {
+		} else if (strncmp(param->string, "lzo", 3) == 0 &&
+				(param->string[3] == ',' ||
+				 param->string[3] == '\0')) {
 			ctx->compress_type = BTRFS_COMPRESS_LZO;
 			ctx->compress_level = 0;
 			btrfs_set_opt(ctx->mount_opt, COMPRESS);
 			btrfs_clear_opt(ctx->mount_opt, NODATACOW);
 			btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-		} else if (strncmp(param->string, "zstd", 4) == 0) {
+		} else if (strncmp(param->string, "zstd", 4) == 0 &&
+				(param->string[4] == ':' ||
+				 param->string[4] == ',' ||
+				 param->string[4] == '\0')) {
 			ctx->compress_type = BTRFS_COMPRESS_ZSTD;
 			ctx->compress_level =
 				btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD,
@@ -376,7 +384,12 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 			btrfs_set_opt(ctx->mount_opt, COMPRESS);
 			btrfs_clear_opt(ctx->mount_opt, NODATACOW);
 			btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-		} else if (strncmp(param->string, "no", 2) == 0) {
+		} else if ((strncmp(param->string, "no", 2) == 0 &&
+				(param->string[2] == ',' ||
+				 param->string[2] == '\0')) ||
+			   (strncmp(param->string, "none", 4) == 0 &&
+				(param->string[4] == ',' ||
+				 param->string[4] == '\0'))) {
 			ctx->compress_level = 0;
 			ctx->compress_type = 0;
 			btrfs_clear_opt(ctx->mount_opt, COMPRESS);
-- 
2.47.2
Re: [PATCH] btrfs: harden parsing of compress mount option
Posted by David Sterba 8 months ago 
On Wed, Apr 23, 2025 at 09:33:28AM +0200, Daniel Vacek wrote:
> Btrfs happily but incorrectly accepts the `-o compress=zlib+foo` and similar
> options with any random suffix. Let's handle that correctly.
> 
> Signed-off-by: Daniel Vacek <neelx@suse.com>
> ---
>  fs/btrfs/super.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 40709e2a44fce..f7e064b8c6d88 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -354,7 +354,10 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  			btrfs_set_opt(ctx->mount_opt, COMPRESS);
>  			btrfs_clear_opt(ctx->mount_opt, NODATACOW);
>  			btrfs_clear_opt(ctx->mount_opt, NODATASUM);
> -		} else if (strncmp(param->string, "zlib", 4) == 0) {
> +		} else if (strncmp(param->string, "zlib", 4) == 0 &&
> +				(param->string[4] == ':' ||
> +				 param->string[4] == ',' ||
> +				 param->string[4] == '\0')) {
>  			ctx->compress_type = BTRFS_COMPRESS_ZLIB;
>  			ctx->compress_level =
>  				btrfs_compress_str2level(BTRFS_COMPRESS_ZLIB,

With more checks the block under the Opt_compress* has grown beyond what
I'd consider reasonable. It would be good to factor it out to a helper,
pass the param->string and context.

Extending with more checks will be easier, we can also validate the
negative levels or other garbage in the string.
Re: [PATCH] btrfs: harden parsing of compress mount option
Posted by David Sterba 8 months ago 
On Wed, Apr 23, 2025 at 09:33:28AM +0200, Daniel Vacek wrote:
> Btrfs happily but incorrectly accepts the `-o compress=zlib+foo` and similar
> options with any random suffix. Let's handle that correctly.
> 
> Signed-off-by: Daniel Vacek <neelx@suse.com>
> ---
>  fs/btrfs/super.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 40709e2a44fce..f7e064b8c6d88 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -354,7 +354,10 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  			btrfs_set_opt(ctx->mount_opt, COMPRESS);
>  			btrfs_clear_opt(ctx->mount_opt, NODATACOW);
>  			btrfs_clear_opt(ctx->mount_opt, NODATASUM);
> -		} else if (strncmp(param->string, "zlib", 4) == 0) {
> +		} else if (strncmp(param->string, "zlib", 4) == 0 &&
> +				(param->string[4] == ':' ||
> +				 param->string[4] == ',' ||

Can there be ',' in the string? The option parser splits the options at
exactly that so it won't be part of the string, so just ':' or 0.

> +				 param->string[4] == '\0')) {
>  			ctx->compress_type = BTRFS_COMPRESS_ZLIB;
>  			ctx->compress_level =
>  				btrfs_compress_str2level(BTRFS_COMPRESS_ZLIB,
[PATCH v2] btrfs: harden parsing of compress mount option
Posted by Daniel Vacek 8 months ago 
Btrfs happily but incorrectly accepts the `-o compress=zlib+foo` and similar
options with any random suffix. Let's handle that correctly.

Signed-off-by: Daniel Vacek <neelx@suse.com>
---
v2: Drop useless check for comma and split compress options
    into a separate helper function

 fs/btrfs/super.c | 108 +++++++++++++++++++++++++++--------------------
 1 file changed, 62 insertions(+), 46 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 40709e2a44fce..422fb82279877 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -270,6 +270,67 @@ static inline blk_mode_t btrfs_open_mode(struct fs_context *fc)
 	return sb_open_mode(fc->sb_flags) & ~BLK_OPEN_RESTRICT_WRITES;
 }
 
+static int btrfs_parse_compress(struct btrfs_fs_context *ctx,
+				struct fs_parameter *param, int opt)
+{
+	/*
+	 * Provide the same semantics as older kernels that don't use fs
+	 * context, specifying the "compress" option clears
+	 * "force-compress" without the need to pass
+	 * "compress-force=[no|none]" before specifying "compress".
+	 */
+	if (opt != Opt_compress_force && opt != Opt_compress_force_type)
+		btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
+
+	if (opt == Opt_compress || opt == Opt_compress_force) {
+		ctx->compress_type = BTRFS_COMPRESS_ZLIB;
+		ctx->compress_level = BTRFS_ZLIB_DEFAULT_LEVEL;
+		btrfs_set_opt(ctx->mount_opt, COMPRESS);
+		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
+		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
+	} else if (strncmp(param->string, "zlib", 4) == 0 &&
+			(param->string[4] == ':' ||
+			 param->string[4] == '\0')) {
+		ctx->compress_type = BTRFS_COMPRESS_ZLIB;
+		ctx->compress_level =
+			btrfs_compress_str2level(BTRFS_COMPRESS_ZLIB,
+						 param->string + 4);
+		btrfs_set_opt(ctx->mount_opt, COMPRESS);
+		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
+		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
+	} else if (strncmp(param->string, "lzo", 3) == 0 &&
+			param->string[3] == '\0') {
+		ctx->compress_type = BTRFS_COMPRESS_LZO;
+		ctx->compress_level = 0;
+		btrfs_set_opt(ctx->mount_opt, COMPRESS);
+		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
+		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
+	} else if (strncmp(param->string, "zstd", 4) == 0 &&
+			(param->string[4] == ':' ||
+			 param->string[4] == '\0')) {
+		ctx->compress_type = BTRFS_COMPRESS_ZSTD;
+		ctx->compress_level =
+			btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD,
+						 param->string + 4);
+		btrfs_set_opt(ctx->mount_opt, COMPRESS);
+		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
+		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
+	} else if ((strncmp(param->string, "no", 2) == 0 &&
+			param->string[2] == '\0') ||
+		   (strncmp(param->string, "none", 4) == 0 &&
+			param->string[4] == '\0')) {
+		ctx->compress_level = 0;
+		ctx->compress_type = 0;
+		btrfs_clear_opt(ctx->mount_opt, COMPRESS);
+		btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
+	} else {
+		btrfs_err(NULL, "unrecognized compression value %s",
+			  param->string);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 {
 	struct btrfs_fs_context *ctx = fc->fs_private;
@@ -339,53 +400,8 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		fallthrough;
 	case Opt_compress:
 	case Opt_compress_type:
-		/*
-		 * Provide the same semantics as older kernels that don't use fs
-		 * context, specifying the "compress" option clears
-		 * "force-compress" without the need to pass
-		 * "compress-force=[no|none]" before specifying "compress".
-		 */
-		if (opt != Opt_compress_force && opt != Opt_compress_force_type)
-			btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
-
-		if (opt == Opt_compress || opt == Opt_compress_force) {
-			ctx->compress_type = BTRFS_COMPRESS_ZLIB;
-			ctx->compress_level = BTRFS_ZLIB_DEFAULT_LEVEL;
-			btrfs_set_opt(ctx->mount_opt, COMPRESS);
-			btrfs_clear_opt(ctx->mount_opt, NODATACOW);
-			btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-		} else if (strncmp(param->string, "zlib", 4) == 0) {
-			ctx->compress_type = BTRFS_COMPRESS_ZLIB;
-			ctx->compress_level =
-				btrfs_compress_str2level(BTRFS_COMPRESS_ZLIB,
-							 param->string + 4);
-			btrfs_set_opt(ctx->mount_opt, COMPRESS);
-			btrfs_clear_opt(ctx->mount_opt, NODATACOW);
-			btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-		} else if (strncmp(param->string, "lzo", 3) == 0) {
-			ctx->compress_type = BTRFS_COMPRESS_LZO;
-			ctx->compress_level = 0;
-			btrfs_set_opt(ctx->mount_opt, COMPRESS);
-			btrfs_clear_opt(ctx->mount_opt, NODATACOW);
-			btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-		} else if (strncmp(param->string, "zstd", 4) == 0) {
-			ctx->compress_type = BTRFS_COMPRESS_ZSTD;
-			ctx->compress_level =
-				btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD,
-							 param->string + 4);
-			btrfs_set_opt(ctx->mount_opt, COMPRESS);
-			btrfs_clear_opt(ctx->mount_opt, NODATACOW);
-			btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-		} else if (strncmp(param->string, "no", 2) == 0) {
-			ctx->compress_level = 0;
-			ctx->compress_type = 0;
-			btrfs_clear_opt(ctx->mount_opt, COMPRESS);
-			btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
-		} else {
-			btrfs_err(NULL, "unrecognized compression value %s",
-				  param->string);
+		if (btrfs_parse_compress(ctx, param, opt))
 			return -EINVAL;
-		}
 		break;
 	case Opt_ssd:
 		if (result.negated) {
-- 
2.47.2
Re: [PATCH v2] btrfs: harden parsing of compress mount option
Posted by David Sterba 7 months, 4 weeks ago 
On Wed, Apr 23, 2025 at 03:22:19PM +0200, Daniel Vacek wrote:
> Btrfs happily but incorrectly accepts the `-o compress=zlib+foo` and similar
> options with any random suffix. Let's handle that correctly.

Please split the patch. Moving code and adding a fix obscures the fix.
As we'll want to backport more than just the validation of ':' it
makes more sense to do the full move first and then add the individual
fixes on top of that. Thanks.

> Signed-off-by: Daniel Vacek <neelx@suse.com>
> ---
> v2: Drop useless check for comma and split compress options
>     into a separate helper function
> 
>  fs/btrfs/super.c | 108 +++++++++++++++++++++++++++--------------------
>  1 file changed, 62 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 40709e2a44fce..422fb82279877 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -270,6 +270,67 @@ static inline blk_mode_t btrfs_open_mode(struct fs_context *fc)
>  	return sb_open_mode(fc->sb_flags) & ~BLK_OPEN_RESTRICT_WRITES;
>  }
>  
> +static int btrfs_parse_compress(struct btrfs_fs_context *ctx,
> +				struct fs_parameter *param, int opt)
> +{
> +	/*
> +	 * Provide the same semantics as older kernels that don't use fs
> +	 * context, specifying the "compress" option clears
> +	 * "force-compress" without the need to pass
> +	 * "compress-force=[no|none]" before specifying "compress".
> +	 */
> +	if (opt != Opt_compress_force && opt != Opt_compress_force_type)
> +		btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
> +
> +	if (opt == Opt_compress || opt == Opt_compress_force) {
> +		ctx->compress_type = BTRFS_COMPRESS_ZLIB;
> +		ctx->compress_level = BTRFS_ZLIB_DEFAULT_LEVEL;
> +		btrfs_set_opt(ctx->mount_opt, COMPRESS);
> +		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
> +		btrfs_clear_opt(ctx->mount_opt, NODATASUM);

Additional cleanups can reorganize the checks so the option clearing
is done once (and not repeated for each compression algorithm).

> +	} else if (strncmp(param->string, "zlib", 4) == 0 &&
> +			(param->string[4] == ':' ||
> +			 param->string[4] == '\0')) {

Matching the name also looks like it can be done by a helper like

	match_compresssion(param, "zlib")

and implemented like

	int len = strlen(compression);

	if (strncmp(param->string, compression, len) == 0 &&
		(param->string[len] ... etc

> +		ctx->compress_type = BTRFS_COMPRESS_ZLIB;
> +		ctx->compress_level =
> +			btrfs_compress_str2level(BTRFS_COMPRESS_ZLIB,
> +						 param->string + 4);
> +		btrfs_set_opt(ctx->mount_opt, COMPRESS);
> +		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
> +		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
> +	} else if (strncmp(param->string, "lzo", 3) == 0 &&
> +			param->string[3] == '\0') {
> +		ctx->compress_type = BTRFS_COMPRESS_LZO;
> +		ctx->compress_level = 0;
> +		btrfs_set_opt(ctx->mount_opt, COMPRESS);
> +		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
> +		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
> +	} else if (strncmp(param->string, "zstd", 4) == 0 &&
> +			(param->string[4] == ':' ||
> +			 param->string[4] == '\0')) {
> +		ctx->compress_type = BTRFS_COMPRESS_ZSTD;
> +		ctx->compress_level =
> +			btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD,
> +						 param->string + 4);
> +		btrfs_set_opt(ctx->mount_opt, COMPRESS);
> +		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
> +		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
> +	} else if ((strncmp(param->string, "no", 2) == 0 &&
> +			param->string[2] == '\0') ||
> +		   (strncmp(param->string, "none", 4) == 0 &&
> +			param->string[4] == '\0')) {
> +		ctx->compress_level = 0;
> +		ctx->compress_type = 0;
> +		btrfs_clear_opt(ctx->mount_opt, COMPRESS);
> +		btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
> +	} else {
> +		btrfs_err(NULL, "unrecognized compression value %s",
> +			  param->string);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
Re: [PATCH v2] btrfs: harden parsing of compress mount option
Posted by David Sterba 6 months, 3 weeks ago 
On Thu, Apr 24, 2025 at 09:29:56PM +0200, David Sterba wrote:
> On Wed, Apr 23, 2025 at 03:22:19PM +0200, Daniel Vacek wrote:
> > Btrfs happily but incorrectly accepts the `-o compress=zlib+foo` and similar
> > options with any random suffix. Let's handle that correctly.
> 
> Please split the patch. Moving code and adding a fix obscures the fix.
> As we'll want to backport more than just the validation of ':' it
> makes more sense to do the full move first and then add the individual
> fixes on top of that. Thanks.

As we've discussed it, both ways how to split it are ok, so please first
factor out the code to a helper and the add the fix or any other
validation that would make sense. Thanks.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.