[v2,v2] fs/ovelayfs: Fix crash on overlayfs mount

Submitted by Andrey Ryabinin on Jan. 18, 2021, 12:47 p.m.

Details

Message ID 20210118124726.8458-1-aryabinin@virtuozzo.com
State New
Series "fs/ovelayfs: Fix crash on overlayfs mount"
Headers show

Commit Message

Andrey Ryabinin Jan. 18, 2021, 12:47 p.m.
Kdump kernel fails to load because of crash on mount of overlayfs:

 BUG: unable to handle kernel NULL pointer dereference at 0000000000000060
....
 Call Trace:
  seq_path+0x64/0xb0
  print_paths_option+0x79/0xa0
  ovl_show_options+0x3a/0x320
  show_mountinfo+0x1ee/0x290
  seq_read+0x2f8/0x400
  vfs_read+0x9d/0x150
  ksys_read+0x4f/0xb0
  do_syscall_64+0x5b/0x1a0

This is cause by OOB access of ofs->lowerpaths.
We transfer to print_paths_option() ofs->numlayer as size of ->lowerpaths
array, but it's not.

The correct number of lowerpaths elements is ->numlower in 'struct ovl_entry'.
So move lowerpaths there and use oe->numlower as array size.

Fixes: 17fc61697f73 ("overlayfs: add dynamic path resolving in mount options")
Fixes: 2191d729083d ("overlayfs: add mnt_id paths options")

https://jira.sw.ru/browse/PSBM-123508
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 fs/overlayfs/ovl_entry.h |  2 +-
 fs/overlayfs/super.c     | 37 ++++++++++++++++++++-----------------
 2 files changed, 21 insertions(+), 18 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index ea1906448ec5..2315089a0211 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -54,7 +54,6 @@  struct ovl_fs {
 	unsigned int numlayer;
 	/* Number of unique fs among layers including upper fs */
 	unsigned int numfs;
-	struct path *lowerpaths;
 	const struct ovl_layer *layers;
 	struct ovl_sb *fs;
 	/* workbasepath is the path at workdir= mount option */
@@ -98,6 +97,7 @@  struct ovl_entry {
 		struct rcu_head rcu;
 	};
 	unsigned numlower;
+	struct path *lowerpaths;
 	struct ovl_path lowerstack[];
 };
 
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 3755f280036f..fb419617564c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -70,8 +70,12 @@  static void ovl_entry_stack_free(struct ovl_entry *oe)
 {
 	unsigned int i;
 
-	for (i = 0; i < oe->numlower; i++)
+	for (i = 0; i < oe->numlower; i++) {
 		dput(oe->lowerstack[i].dentry);
+		if (oe->lowerpaths)
+			path_put(&oe->lowerpaths[i]);
+	}
+	kfree(oe->lowerpaths);
 }
 
 static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
@@ -241,11 +245,6 @@  static void ovl_free_fs(struct ovl_fs *ofs)
 		ovl_inuse_unlock(ofs->upper_mnt->mnt_root);
 	mntput(ofs->upper_mnt);
 	path_put(&ofs->upperpath);
-	if (ofs->lowerpaths) {
-		for (i = 0; i < ofs->numlayer; i++)
-			path_put(&ofs->lowerpaths[i]);
-		kfree(ofs->lowerpaths);
-	}
 	for (i = 1; i < ofs->numlayer; i++) {
 		iput(ofs->layers[i].trap);
 		mntput(ofs->layers[i].mnt);
@@ -359,9 +358,10 @@  static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 {
 	struct super_block *sb = dentry->d_sb;
 	struct ovl_fs *ofs = sb->s_fs_info;
+	struct ovl_entry *oe = OVL_E(dentry);
 
 	if (ovl_dyn_path_opts) {
-		print_paths_option(m, "lowerdir", ofs->lowerpaths, ofs->numlayer);
+		print_paths_option(m, "lowerdir", oe->lowerpaths, oe->numlower);
 		if (ofs->config.upperdir) {
 			print_path_option(m, "upperdir", &ofs->upperpath);
 			print_path_option(m, "workdir", &ofs->workbasepath);
@@ -375,7 +375,7 @@  static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	}
 
 	if (ovl_mnt_id_path_opts) {
-		print_mnt_ids_option(m, "lowerdir_mnt_id", ofs->lowerpaths, ofs->numlayer);
+		print_mnt_ids_option(m, "lowerdir_mnt_id", oe->lowerpaths, oe->numlower);
 		/*
 		 * We don't need to show mnt_id for workdir because it
 		 * on the same mount as upperdir.
@@ -1626,6 +1626,7 @@  static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	int err;
 	char *lowertmp, *lower;
 	unsigned int stacklen, numlower = 0, i;
+	struct path *stack = NULL;
 	struct ovl_entry *oe;
 
 	err = -ENOMEM;
@@ -1649,14 +1650,14 @@  static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	}
 
 	err = -ENOMEM;
-	ofs->lowerpaths = kcalloc(stacklen, sizeof(struct path), GFP_KERNEL);
-	if (!ofs->lowerpaths)
+	stack = kcalloc(stacklen, sizeof(struct path), GFP_KERNEL);
+	if (!stack)
 		goto out_err;
 
 	err = -EINVAL;
 	lower = lowertmp;
 	for (numlower = 0; numlower < stacklen; numlower++) {
-		err = ovl_lower_dir(lower, &ofs->lowerpaths[numlower], ofs,
+		err = ovl_lower_dir(lower, &stack[numlower], ofs,
 				    &sb->s_stack_depth);
 		if (err)
 			goto out_err;
@@ -1671,7 +1672,7 @@  static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 		goto out_err;
 	}
 
-	err = ovl_get_layers(sb, ofs, ofs->lowerpaths, numlower);
+	err = ovl_get_layers(sb, ofs, stack, numlower);
 	if (err)
 		goto out_err;
 
@@ -1681,9 +1682,10 @@  static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 		goto out_err;
 
 	for (i = 0; i < numlower; i++) {
-		oe->lowerstack[i].dentry = dget(ofs->lowerpaths[i].dentry);
+		oe->lowerstack[i].dentry = dget(stack[i].dentry);
 		oe->lowerstack[i].layer = &ofs->layers[i+1];
 	}
+	oe->lowerpaths = stack;
 
 out:
 	kfree(lowertmp);
@@ -1691,10 +1693,11 @@  static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	return oe;
 
 out_err:
-	for (i = 0; i < numlower; i++)
-		path_put_init(&ofs->lowerpaths[i]);
-	kfree(ofs->lowerpaths);
-	ofs->lowerpaths = NULL;
+	if (stack) {
+		for (i = 0; i < numlower; i++)
+			path_put(&stack[i]);
+		kfree(stack);
+	}
 	oe = ERR_PTR(err);
 	goto out;
 }

Comments

Alexander Mikhalitsyn Jan. 22, 2021, 10:17 a.m.
On Mon, 18 Jan 2021 15:47:26 +0300
Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:

> Kdump kernel fails to load because of crash on mount of overlayfs:
> 
>  BUG: unable to handle kernel NULL pointer dereference at 0000000000000060
> ....
>  Call Trace:
>   seq_path+0x64/0xb0
>   print_paths_option+0x79/0xa0
>   ovl_show_options+0x3a/0x320
>   show_mountinfo+0x1ee/0x290
>   seq_read+0x2f8/0x400
>   vfs_read+0x9d/0x150
>   ksys_read+0x4f/0xb0
>   do_syscall_64+0x5b/0x1a0
> 
> This is cause by OOB access of ofs->lowerpaths.
> We transfer to print_paths_option() ofs->numlayer as size of ->lowerpaths
> array, but it's not.
> 
> The correct number of lowerpaths elements is ->numlower in 'struct ovl_entry'.
> So move lowerpaths there and use oe->numlower as array size.
> 
> Fixes: 17fc61697f73 ("overlayfs: add dynamic path resolving in mount options")
> Fixes: 2191d729083d ("overlayfs: add mnt_id paths options")
> 
> https://jira.sw.ru/browse/PSBM-123508
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>

Reviewed-by: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>

> ---
>  fs/overlayfs/ovl_entry.h |  2 +-
>  fs/overlayfs/super.c     | 37 ++++++++++++++++++++-----------------
>  2 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index ea1906448ec5..2315089a0211 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -54,7 +54,6 @@ struct ovl_fs {
>  	unsigned int numlayer;
>  	/* Number of unique fs among layers including upper fs */
>  	unsigned int numfs;
> -	struct path *lowerpaths;
>  	const struct ovl_layer *layers;
>  	struct ovl_sb *fs;
>  	/* workbasepath is the path at workdir= mount option */
> @@ -98,6 +97,7 @@ struct ovl_entry {
>  		struct rcu_head rcu;
>  	};
>  	unsigned numlower;
> +	struct path *lowerpaths;
>  	struct ovl_path lowerstack[];
>  };
>  
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 3755f280036f..fb419617564c 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -70,8 +70,12 @@ static void ovl_entry_stack_free(struct ovl_entry *oe)
>  {
>  	unsigned int i;
>  
> -	for (i = 0; i < oe->numlower; i++)
> +	for (i = 0; i < oe->numlower; i++) {
>  		dput(oe->lowerstack[i].dentry);
> +		if (oe->lowerpaths)
> +			path_put(&oe->lowerpaths[i]);
> +	}
> +	kfree(oe->lowerpaths);
>  }
>  
>  static bool ovl_metacopy_def = IS_ENABLED(CONFIG_OVERLAY_FS_METACOPY);
> @@ -241,11 +245,6 @@ static void ovl_free_fs(struct ovl_fs *ofs)
>  		ovl_inuse_unlock(ofs->upper_mnt->mnt_root);
>  	mntput(ofs->upper_mnt);
>  	path_put(&ofs->upperpath);
> -	if (ofs->lowerpaths) {
> -		for (i = 0; i < ofs->numlayer; i++)
> -			path_put(&ofs->lowerpaths[i]);
> -		kfree(ofs->lowerpaths);
> -	}
>  	for (i = 1; i < ofs->numlayer; i++) {
>  		iput(ofs->layers[i].trap);
>  		mntput(ofs->layers[i].mnt);
> @@ -359,9 +358,10 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>  {
>  	struct super_block *sb = dentry->d_sb;
>  	struct ovl_fs *ofs = sb->s_fs_info;
> +	struct ovl_entry *oe = OVL_E(dentry);
>  
>  	if (ovl_dyn_path_opts) {
> -		print_paths_option(m, "lowerdir", ofs->lowerpaths, ofs->numlayer);
> +		print_paths_option(m, "lowerdir", oe->lowerpaths, oe->numlower);
>  		if (ofs->config.upperdir) {
>  			print_path_option(m, "upperdir", &ofs->upperpath);
>  			print_path_option(m, "workdir", &ofs->workbasepath);
> @@ -375,7 +375,7 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>  	}
>  
>  	if (ovl_mnt_id_path_opts) {
> -		print_mnt_ids_option(m, "lowerdir_mnt_id", ofs->lowerpaths, ofs->numlayer);
> +		print_mnt_ids_option(m, "lowerdir_mnt_id", oe->lowerpaths, oe->numlower);
>  		/*
>  		 * We don't need to show mnt_id for workdir because it
>  		 * on the same mount as upperdir.
> @@ -1626,6 +1626,7 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>  	int err;
>  	char *lowertmp, *lower;
>  	unsigned int stacklen, numlower = 0, i;
> +	struct path *stack = NULL;
>  	struct ovl_entry *oe;
>  
>  	err = -ENOMEM;
> @@ -1649,14 +1650,14 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>  	}
>  
>  	err = -ENOMEM;
> -	ofs->lowerpaths = kcalloc(stacklen, sizeof(struct path), GFP_KERNEL);
> -	if (!ofs->lowerpaths)
> +	stack = kcalloc(stacklen, sizeof(struct path), GFP_KERNEL);
> +	if (!stack)
>  		goto out_err;
>  
>  	err = -EINVAL;
>  	lower = lowertmp;
>  	for (numlower = 0; numlower < stacklen; numlower++) {
> -		err = ovl_lower_dir(lower, &ofs->lowerpaths[numlower], ofs,
> +		err = ovl_lower_dir(lower, &stack[numlower], ofs,
>  				    &sb->s_stack_depth);
>  		if (err)
>  			goto out_err;
> @@ -1671,7 +1672,7 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>  		goto out_err;
>  	}
>  
> -	err = ovl_get_layers(sb, ofs, ofs->lowerpaths, numlower);
> +	err = ovl_get_layers(sb, ofs, stack, numlower);
>  	if (err)
>  		goto out_err;
>  
> @@ -1681,9 +1682,10 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>  		goto out_err;
>  
>  	for (i = 0; i < numlower; i++) {
> -		oe->lowerstack[i].dentry = dget(ofs->lowerpaths[i].dentry);
> +		oe->lowerstack[i].dentry = dget(stack[i].dentry);
>  		oe->lowerstack[i].layer = &ofs->layers[i+1];
>  	}
> +	oe->lowerpaths = stack;
>  
>  out:
>  	kfree(lowertmp);
> @@ -1691,10 +1693,11 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>  	return oe;
>  
>  out_err:
> -	for (i = 0; i < numlower; i++)
> -		path_put_init(&ofs->lowerpaths[i]);
> -	kfree(ofs->lowerpaths);
> -	ofs->lowerpaths = NULL;
> +	if (stack) {
> +		for (i = 0; i < numlower; i++)
> +			path_put(&stack[i]);
> +		kfree(stack);
> +	}
>  	oe = ERR_PTR(err);
>  	goto out;
>  }
> -- 
> 2.26.2
> 
> _______________________________________________
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel