Re: [Devel] [PATCH v2 v2] fs/ovelayfs: Fix crash on overlayfs mount
On Mon, 18 Jan 2021 15:47:26 +0300 Andrey Ryabinin wrote: > Kdump kernel fails to load because of crash on mount of overlayfs: > > BUG: unable to handle kernel NULL pointer dereference at 0060 > > 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 Reviewed-by: Alexander Mikhalitsyn > --- > 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(>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(>upperpath); > - if (ofs->lowerpaths) { > - for (i = 0; i < ofs->numlayer; i++) > - path_put(>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", >upperpath); > print_path_option(m, "workdir", >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, >lowerpaths[numlower], ofs, > + err = ovl_lower_dir(lower, [numlower], ofs, >
[Devel] [PATCH v2 v2] fs/ovelayfs: Fix crash on overlayfs mount
Kdump kernel fails to load because of crash on mount of overlayfs: BUG: unable to handle kernel NULL pointer dereference at 0060 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 --- 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(>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(>upperpath); - if (ofs->lowerpaths) { - for (i = 0; i < ofs->numlayer; i++) - path_put(>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", >upperpath); print_path_option(m, "workdir", >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, >lowerpaths[numlower], ofs, + err = ovl_lower_dir(lower, [numlower], ofs, >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;