Re: [PATCH v2] tmpfs: fix unicode_map leaks in casefold option handling

[Gabriel Krisman Bertazi <gabriel@krisman.be>]

Kazuki Hanai <hnkz.64@gmail.com> writes:

> shmem_parse_opt_casefold() stores the unicode_map returned by
> utf8_load() in ctx->encoding. The casefold parameter can be supplied
> more than once for the same filesystem context, but replacing the
> stored map does not release the previous reference.
>
> The final reference is also leaked when an unmounted filesystem
> context is freed.
>
> Release the previous map before replacing it, clear ctx->encoding
> after transferring ownership to the superblock, and release any
> remaining reference from shmem_free_fc().
>
> An unprivileged user can repeatedly set the casefold parameter on a
> tmpfs filesystem context from a user namespace. This causes
> unbounded kernel memory consumption and can result in a local denial
> of service.
>
> Fixes: 58e55efd6c72 ("tmpfs: Add casefold lookup support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kazuki Hanai <hnkz.64@gmail.com>
> ---
> Changes in v2:
> - Keep Fixes, Cc, and Signed-off-by in a single trailer block.
>
>  mm/shmem.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 89a1495e55f7..62440caf2df5 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -4508,6 +4508,7 @@ static int shmem_parse_opt_casefold(struct fs_context *fc, struct fs_parameter *
>  	pr_info("tmpfs: Using encoding : utf8-%u.%u.%u\n",
>  		unicode_major(version), unicode_minor(version), unicode_rev(version));
>  
> +	utf8_unload(ctx->encoding);
>  	ctx->encoding = encoding;

This patch fixes two bugs at once, as shown by the other patchset that
does it separately. One is the leak during 'mount -o remount', fixed by
the second hunk, which is fine.  The other is when multiple casefold=
parameters are passed at once.  I think that fix is wrong.

I can't think of a real case where it makes sense to pass multiple
casefold parameters, besides user error.  even if you are trying to
outsmart the kernel and handle cases where a new encoding might not be
available, giving it a fallback version, a failure in the first
utf8_load will back off the mount.  IMO, we should just reject the mount
right away instead of silently swallowing the first table here.

This hunk should have been something like this instead:

diff --git a/mm/shmem.c b/mm/shmem.c
index e46bd4fc7e41..06503858dae4 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -4506,6 +4506,9 @@ static int shmem_parse_opt_casefold(struct fs_context *fc, struct fs_parameter *
 	struct unicode_map *encoding;
 	char *version_str = param->string + 5;
 
+	if (ctx->encoding)
+		return invalfc(fc, "casefold parameter cannot be specified twice\n");
+
 	if (!latest_version) {
 		if (strncmp(param->string, "utf8-", 5))
 			return invalfc(fc, "Only UTF-8 encodings are supported "

> @@ -4976,6 +4977,7 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
>  
>  	if (ctx->encoding) {
>  		sb->s_encoding = ctx->encoding;
> +		ctx->encoding = NULL;
>  		set_default_d_op(sb, &shmem_ci_dentry_ops);
>  		if (ctx->strict_encoding)
>  			sb->s_encoding_flags = SB_ENC_STRICT_MODE_FL;
> @@ -5073,6 +5075,9 @@ static void shmem_free_fc(struct fs_context *fc)
>  	struct shmem_options *ctx = fc->fs_private;
>  
>  	if (ctx) {
> +#if IS_ENABLED(CONFIG_UNICODE)
> +		utf8_unload(ctx->encoding);
> +#endif
>  		mpol_put(ctx->mpol);
>  		kfree(ctx);
>  	}
> -- 
> 2.53.0

This part looks fine.


-- 
Gabriel Krisman Bertazi