| Message ID | 20230129115021.25778-1-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] fsconfig: New case cover CVE-2022-0185 | expand |
Hi Wei, ... > +++ b/include/lapi/fsmount.h > @@ -11,12 +11,15 @@ > #include "config.h" > #include <sys/syscall.h> > #include <sys/types.h> > -#include <sys/mount.h> > #ifndef HAVE_FSOPEN > # ifdef HAVE_LINUX_MOUNT_H > # include <linux/mount.h> > +# else > +# include <sys/mount.h> > # endif > +#else > +# include <sys/mount.h> > #endif Does <linux/mount.h> conflicts with <sys/mount.h>? Or why is this needed? > #include "lapi/fcntl.h" > diff --git a/runtest/syscalls b/runtest/syscalls > index ae37a1192..b4cde8071 100644 > --- a/runtest/syscalls > +++ b/runtest/syscalls > @@ -383,6 +383,7 @@ fremovexattr02 fremovexattr02 > fsconfig01 fsconfig01 > fsconfig02 fsconfig02 > +fsconfig03 fsconfig03 NOTE: you also need to add a new record in testcases/kernel/syscalls/fsconfig/.gitignore. > diff --git a/testcases/kernel/syscalls/fsconfig/fsconfig03.c b/testcases/kernel/syscalls/fsconfig/fsconfig03.c > new file mode 100644 > index 000000000..e076c2f09 > --- /dev/null > +++ b/testcases/kernel/syscalls/fsconfig/fsconfig03.c > @@ -0,0 +1,58 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2023 Wei Gao <wegao@suse.com> > + */ > + > +/*\ NOTE, there should be docparse label: * [Description] > + * Test add some coverage to CVE-2022-0185. > + * Try to trigger a crash. > + * References links: > + * https://www.openwall.com/lists/oss-security/2022/01/25/14 > + * https://github.com/Crusaders-of-Rust/CVE-2022-0185 > + */ > + > +#include "tst_test.h" > +#include "lapi/fsmount.h" > + > +#define MNTPOINT "mntpoint" > + > +static int fd = -1; > + > +static void setup(void) > +{ > + fsopen_supported_by_kernel(); > + > + TEST(fd = fsopen(tst_device->fs_type, 0)); > + if (fd == -1) > + tst_brk(TBROK | TTERRNO, "fsopen() failed"); Sooner or later we should add SAFE_FSOPEN(), but that can wait. > + > +} > + > +static void cleanup(void) > +{ > + if (fd != -1) > + SAFE_CLOSE(fd); > +} > + > +static void run(void) > +{ > + char *val = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; > + > + for (unsigned int i = 0; i < 2; i++) { > + TEST(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); > + if (TST_RET == -1) > + tst_brk(TFAIL | TTERRNO, "fsconfig(FSCONFIG_SET_STRING) failed"); TST_EXP_PASS() or other could here be used (it should be changes also in fsconfig01.c). Hm, there is a kernel fix from 5.17 [1]. But test fails when I run it on 6.2.0-rc5: tst_supported_fs_types.c:165: TINFO: Skipping FUSE based ntfs as requested by the test tst_supported_fs_types.c:157: TINFO: Skipping tmpfs as requested by the test tst_test.c:1634: TINFO: === Testing on ext3 === tst_test.c:1093: TINFO: Formatting /dev/loop0 with ext3 opts='' extra opts='' mke2fs 1.46.5 (30-Dec-2021) fsconfig03.c:44: TFAIL: fsconfig(FSCONFIG_SET_STRING) failed: EINVAL (22) Isn't it the opposite: we expect to fail, thus TST_EXP_FAIL() should here be used? > + } > + tst_res(TPASS, "Try fsconfig overflow on %s done!", tst_device->fs_type); > +} > + > +static struct tst_test test = { > + .test_all = run, > + .setup = setup, > + .cleanup = cleanup, > + .needs_root = 1, > + .format_device = 1, > + .mntpoint = MNTPOINT, > + .all_filesystems = 1, > + .skip_filesystems = (const char *const []){"fuse", "ext2", "xfs", "tmpfs", NULL}, I wonder why this is should not be run on XFS and ext2. Also, while we have CVE and kernel fix [1], it should be marked in struct tst_test: .tags = (const struct tst_tag[]) { {"linux-git", "722d94847de2"}, {"CVE", "2020-29373"}, {"CVE", "2022-0185"}, {} } Kind regards, Petr [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=722d94847de2 > +};
On Wed, Feb 01, 2023 at 01:49:43PM +0100, Petr Vorel wrote: > Hi Wei, > > ... > > +++ b/include/lapi/fsmount.h > > @@ -11,12 +11,15 @@ > > #include "config.h" > > #include <sys/syscall.h> > > #include <sys/types.h> > > -#include <sys/mount.h> > > > #ifndef HAVE_FSOPEN > > # ifdef HAVE_LINUX_MOUNT_H > > # include <linux/mount.h> > > +# else > > +# include <sys/mount.h> > > # endif > > +#else > > +# include <sys/mount.h> > > #endif > Does <linux/mount.h> conflicts with <sys/mount.h>? Or why is this needed? > > > #include "lapi/fcntl.h" > > diff --git a/runtest/syscalls b/runtest/syscalls > > index ae37a1192..b4cde8071 100644 > > --- a/runtest/syscalls > > +++ b/runtest/syscalls > > @@ -383,6 +383,7 @@ fremovexattr02 fremovexattr02 > > > fsconfig01 fsconfig01 > > fsconfig02 fsconfig02 > > +fsconfig03 fsconfig03 > > NOTE: you also need to add a new record in testcases/kernel/syscalls/fsconfig/.gitignore. > > > diff --git a/testcases/kernel/syscalls/fsconfig/fsconfig03.c b/testcases/kernel/syscalls/fsconfig/fsconfig03.c > > new file mode 100644 > > index 000000000..e076c2f09 > > --- /dev/null > > +++ b/testcases/kernel/syscalls/fsconfig/fsconfig03.c > > @@ -0,0 +1,58 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (c) 2023 Wei Gao <wegao@suse.com> > > + */ > > + > > +/*\ > NOTE, there should be docparse label: > * [Description] > > + * Test add some coverage to CVE-2022-0185. > > + * Try to trigger a crash. > > + * References links: > > + * https://www.openwall.com/lists/oss-security/2022/01/25/14 > > + * https://github.com/Crusaders-of-Rust/CVE-2022-0185 > > + */ > > + > > +#include "tst_test.h" > > +#include "lapi/fsmount.h" > > + > > +#define MNTPOINT "mntpoint" > > + > > +static int fd = -1; > > + > > +static void setup(void) > > +{ > > + fsopen_supported_by_kernel(); > > + > > + TEST(fd = fsopen(tst_device->fs_type, 0)); > > + if (fd == -1) > > + tst_brk(TBROK | TTERRNO, "fsopen() failed"); > Sooner or later we should add SAFE_FSOPEN(), but that can wait. > > > + > > +} > > + > > +static void cleanup(void) > > +{ > > + if (fd != -1) > > + SAFE_CLOSE(fd); > > +} > > + > > +static void run(void) > > +{ > > + char *val = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; > > + > > + for (unsigned int i = 0; i < 2; i++) { > > + TEST(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); > > + if (TST_RET == -1) > > + tst_brk(TFAIL | TTERRNO, "fsconfig(FSCONFIG_SET_STRING) failed"); > TST_EXP_PASS() or other could here be used (it should be changes also in fsconfig01.c). > > Hm, there is a kernel fix from 5.17 [1]. But test fails when I run it on 6.2.0-rc5: > > tst_supported_fs_types.c:165: TINFO: Skipping FUSE based ntfs as requested by the test > tst_supported_fs_types.c:157: TINFO: Skipping tmpfs as requested by the test > tst_test.c:1634: TINFO: === Testing on ext3 === > tst_test.c:1093: TINFO: Formatting /dev/loop0 with ext3 opts='' extra opts='' > mke2fs 1.46.5 (30-Dec-2021) > fsconfig03.c:44: TFAIL: fsconfig(FSCONFIG_SET_STRING) failed: EINVAL (22) > > Isn't it the opposite: we expect to fail, thus TST_EXP_FAIL() should here be > used? > I have not test on 6.2.0 kernel, i need reproduce this firstly. > > + } > > + tst_res(TPASS, "Try fsconfig overflow on %s done!", tst_device->fs_type); > > +} > > + > > +static struct tst_test test = { > > + .test_all = run, > > + .setup = setup, > > + .cleanup = cleanup, > > + .needs_root = 1, > > + .format_device = 1, > > + .mntpoint = MNTPOINT, > > + .all_filesystems = 1, > > + .skip_filesystems = (const char *const []){"fuse", "ext2", "xfs", "tmpfs", NULL}, > > I wonder why this is should not be run on XFS and ext2. ext2: this if failed on one of my specific machine, so normally this should run. xfs: this always error happen with error code "TFAIL: fsconfig(FSCONFIG_SET_STRING) failed: EINVAL (22)", i need debug kernel to check what's happen. > > Also, while we have CVE and kernel fix [1], it should be marked in struct tst_test: > > .tags = (const struct tst_tag[]) { > {"linux-git", "722d94847de2"}, > {"CVE", "2020-29373"}, > {"CVE", "2022-0185"}, > {} > } > > Kind regards, > Petr > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=722d94847de2 > > > > +};
Hi Wei, ... > > Hm, there is a kernel fix from 5.17 [1]. But test fails when I run it on 6.2.0-rc5: > > tst_supported_fs_types.c:165: TINFO: Skipping FUSE based ntfs as requested by the test > > tst_supported_fs_types.c:157: TINFO: Skipping tmpfs as requested by the test > > tst_test.c:1634: TINFO: === Testing on ext3 === > > tst_test.c:1093: TINFO: Formatting /dev/loop0 with ext3 opts='' extra opts='' > > mke2fs 1.46.5 (30-Dec-2021) > > fsconfig03.c:44: TFAIL: fsconfig(FSCONFIG_SET_STRING) failed: EINVAL (22) > > Isn't it the opposite: we expect to fail, thus TST_EXP_FAIL() should here be > > used? > I have not test on 6.2.0 kernel, i need reproduce this firstly. FYI 6.0.6 is also broken, works on 5.10.46. Kind regards, Petr
On Mon, Feb 06, 2023 at 05:38:18AM -0500, Wei Gao via ltp wrote: > On Wed, Feb 01, 2023 at 01:49:43PM +0100, Petr Vorel wrote: > > Hi Wei, > > > > ... > > > +++ b/include/lapi/fsmount.h > > > @@ -11,12 +11,15 @@ > > > #include "config.h" > > > #include <sys/syscall.h> > > > #include <sys/types.h> > > > -#include <sys/mount.h> > > > > > #ifndef HAVE_FSOPEN > > > # ifdef HAVE_LINUX_MOUNT_H > > > # include <linux/mount.h> > > > +# else > > > +# include <sys/mount.h> > > > # endif > > > +#else > > > +# include <sys/mount.h> > > > #endif > > Does <linux/mount.h> conflicts with <sys/mount.h>? Or why is this needed? > > > > > #include "lapi/fcntl.h" > > > diff --git a/runtest/syscalls b/runtest/syscalls > > > index ae37a1192..b4cde8071 100644 > > > --- a/runtest/syscalls > > > +++ b/runtest/syscalls > > > @@ -383,6 +383,7 @@ fremovexattr02 fremovexattr02 > > > > > fsconfig01 fsconfig01 > > > fsconfig02 fsconfig02 > > > +fsconfig03 fsconfig03 > > > > NOTE: you also need to add a new record in testcases/kernel/syscalls/fsconfig/.gitignore. > > > > > diff --git a/testcases/kernel/syscalls/fsconfig/fsconfig03.c b/testcases/kernel/syscalls/fsconfig/fsconfig03.c > > > new file mode 100644 > > > index 000000000..e076c2f09 > > > --- /dev/null > > > +++ b/testcases/kernel/syscalls/fsconfig/fsconfig03.c > > > @@ -0,0 +1,58 @@ > > > +// SPDX-License-Identifier: GPL-2.0-or-later > > > +/* > > > + * Copyright (c) 2023 Wei Gao <wegao@suse.com> > > > + */ > > > + > > > +/*\ > > NOTE, there should be docparse label: > > * [Description] > > > + * Test add some coverage to CVE-2022-0185. > > > + * Try to trigger a crash. > > > + * References links: > > > + * https://www.openwall.com/lists/oss-security/2022/01/25/14 > > > + * https://github.com/Crusaders-of-Rust/CVE-2022-0185 > > > + */ > > > + > > > +#include "tst_test.h" > > > +#include "lapi/fsmount.h" > > > + > > > +#define MNTPOINT "mntpoint" > > > + > > > +static int fd = -1; > > > + > > > +static void setup(void) > > > +{ > > > + fsopen_supported_by_kernel(); > > > + > > > + TEST(fd = fsopen(tst_device->fs_type, 0)); > > > + if (fd == -1) > > > + tst_brk(TBROK | TTERRNO, "fsopen() failed"); > > Sooner or later we should add SAFE_FSOPEN(), but that can wait. > > > > > + > > > +} > > > + > > > +static void cleanup(void) > > > +{ > > > + if (fd != -1) > > > + SAFE_CLOSE(fd); > > > +} > > > + > > > +static void run(void) > > > +{ > > > + char *val = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; > > > + > > > + for (unsigned int i = 0; i < 2; i++) { > > > + TEST(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); > > > + if (TST_RET == -1) > > > + tst_brk(TFAIL | TTERRNO, "fsconfig(FSCONFIG_SET_STRING) failed"); > > TST_EXP_PASS() or other could here be used (it should be changes also in fsconfig01.c). > > > > Hm, there is a kernel fix from 5.17 [1]. But test fails when I run it on 6.2.0-rc5: > > > > tst_supported_fs_types.c:165: TINFO: Skipping FUSE based ntfs as requested by the test > > tst_supported_fs_types.c:157: TINFO: Skipping tmpfs as requested by the test > > tst_test.c:1634: TINFO: === Testing on ext3 === > > tst_test.c:1093: TINFO: Formatting /dev/loop0 with ext3 opts='' extra opts='' > > mke2fs 1.46.5 (30-Dec-2021) > > fsconfig03.c:44: TFAIL: fsconfig(FSCONFIG_SET_STRING) failed: EINVAL (22) > > > > Isn't it the opposite: we expect to fail, thus TST_EXP_FAIL() should here be > > used? > > > I have not test on 6.2.0 kernel, i need reproduce this firstly. > > > + } > > > + tst_res(TPASS, "Try fsconfig overflow on %s done!", tst_device->fs_type); > > > +} > > > + > > > +static struct tst_test test = { > > > + .test_all = run, > > > + .setup = setup, > > > + .cleanup = cleanup, > > > + .needs_root = 1, > > > + .format_device = 1, > > > + .mntpoint = MNTPOINT, > > > + .all_filesystems = 1, > > > + .skip_filesystems = (const char *const []){"fuse", "ext2", "xfs", "tmpfs", NULL}, > > > > I wonder why this is should not be run on XFS and ext2. > ext2: this if failed on one of my specific machine, so normally this should run. > xfs: this always error happen with error code "TFAIL: fsconfig(FSCONFIG_SET_STRING) failed: EINVAL (22)", i > need debug kernel to check what's happen. Update xfs investigation progress: After some debug check ths kernel file system code, i found xfs ONLY can accept key which defined in xfs_fs_parameters, so EINVAL returned. ===xfs kernel logic tracking detail start === SYSCALL_DEFINE5(fsconfig fs/fsopen.c vfs_fsconfig_locked vfs_parse_fs_param ret = fc->ops->parse_param(fc, param); // this is xfs_fs_parse_param xfs_fs_parse_param fs_parse __fs_parse 103 int __fs_parse(struct p_log *log, 104 const struct fs_parameter_spec *desc, 105 struct fs_parameter *param, 106 struct fs_parse_result *result) 107 { 108 const struct fs_parameter_spec *p; 109 110 result->uint_64 = 0; 111 (gdb) l 112 p = fs_lookup_key(desc, param, &result->negated); 113 if (!p) 114 return -ENOPARAM; <==== this error static const struct fs_parameter_spec xfs_fs_parameters[] = { fsparam_u32("logbufs", Opt_logbufs), fsparam_string("logbsize", Opt_logbsize), fsparam_string("logdev", Opt_logdev), <=== ONLY can accept this table as KEY!! fsparam_string("rtdev", Opt_rtdev), fsparam_flag("wsync", Opt_wsync), ===xfs kernel logic tracking detail end=== > > > > Also, while we have CVE and kernel fix [1], it should be marked in struct tst_test: > > > > .tags = (const struct tst_tag[]) { > > {"linux-git", "722d94847de2"}, > > {"CVE", "2020-29373"}, > > {"CVE", "2022-0185"}, > > {} > > } > > > > Kind regards, > > Petr > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=722d94847de2 > > > > > > > +}; > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
On Mon, Feb 06, 2023 at 05:19:53PM +0100, Petr Vorel wrote: > Hi Wei, > > ... > > > Hm, there is a kernel fix from 5.17 [1]. But test fails when I run it on 6.2.0-rc5: > > > > tst_supported_fs_types.c:165: TINFO: Skipping FUSE based ntfs as requested by the test > > > tst_supported_fs_types.c:157: TINFO: Skipping tmpfs as requested by the test > > > tst_test.c:1634: TINFO: === Testing on ext3 === > > > tst_test.c:1093: TINFO: Formatting /dev/loop0 with ext3 opts='' extra opts='' > > > mke2fs 1.46.5 (30-Dec-2021) > > > fsconfig03.c:44: TFAIL: fsconfig(FSCONFIG_SET_STRING) failed: EINVAL (22) > > > > Isn't it the opposite: we expect to fail, thus TST_EXP_FAIL() should here be > > > used? > > > I have not test on 6.2.0 kernel, i need reproduce this firstly. > > FYI 6.0.6 is also broken, works on 5.10.46. > After long investigation i finally get what's happen now since i have never touch kernel fs code before :) The root caused is cebe85d570cf8 which make udpate initialize of ext3_fs_type. Each file system will initialize a struct file_system_type and ext3 initialize in fs/ext4/super.c(maybe ext3 much same as ext4 so they put in same file). This patch add new memeber .init_fs_context in ext3 file_system_type struct and this new member will lead pase function which called by fsconfig change from legacy_parse_param to ext4_parse_param(this function will check parameter and not allow 0x00) ===key change part of cebe85d570cf8=== static struct file_system_type ext3_fs_type = { - .owner = THIS_MODULE, - .name = "ext3", - .mount = ext4_mount, - .kill_sb = kill_block_super, - .fs_flags = FS_REQUIRES_DEV, + .owner = THIS_MODULE, + .name = "ext3", + .init_fs_context = ext4_init_fs_context, // in this patch init_fs_context start set ext4_init_fs_context + .parameters = ext4_param_specs, + .kill_sb = kill_block_super, + .fs_flags = FS_REQUIRES_DEV, }; ===key change part of cebe85d570cf8=== Following logic will decide whether use legacy_init_fs_context base on exist of init_fs_context, obviously before patch we have no init_fs_context but after patch we have it ==function alloc_fs_context== ..... init_fs_context = fc->fs_type->init_fs_context; if (!init_fs_context) init_fs_context = legacy_init_fs_context; //before patch cebe85d570cf8, legacy_init_fs_context will be set. ret = init_fs_context(fc); ==function alloc_fs_context== ====code example for set parse function used by fsconfig=== const struct fs_context_operations legacy_fs_context_ops = { .free = legacy_fs_context_free, .dup = legacy_fs_context_dup, .parse_param = legacy_parse_param, .parse_monolithic = legacy_parse_monolithic, .get_tree = legacy_get_tree, .reconfigure = legacy_reconfigure, }; /* * Initialise a legacy context for a filesystem that doesn't support * fs_context. */ static int legacy_init_fs_context(struct fs_context *fc) { fc->fs_private = kzalloc(sizeof(struct legacy_fs_context), GFP_KERNEL_ACCOUNT); if (!fc->fs_private) return -ENOMEM; fc->ops = &legacy_fs_context_ops; return 0; } ====code for set parse function used by fsconfig=== ====final call parse function within fsconfig logic== vfs_parse_fs_param 145 if (fc->ops->parse_param) { 146 ret = fc->ops->parse_param(fc, param); //this will call legacy_parse_param or ext4_parse_param 147 if (ret != -ENOPARAM) 148 return ret; 149 } ====final call parse function within fsconfig logic== Just FYI the fs_type real data show in GDB(init_fs_context= 0 in kernel5.x but in kernel 6.x init_fs_context=ext4_parse_param): (gdb) p *fs_type $4 = {name = 0xffffffff822278e1 "ext3", fs_flags = 1, init_fs_context = 0x0 <fixed_percpu_data>, parameters = 0x0 <fixed_percpu_data>, mount = 0xffffffff812ec510 <ext4_mount>, kill_sb = 0xffffffff811f7220 <kill_block_super>, owner = 0x0 <fixed_percpu_data>, next = 0xffffffff82564600 <ext2_fs_type>, fs_supers = {first = 0x0 <fixed_percpu_data>}, s_lock_key = {<No data fields>}, s_umount_key = {<No data fields>}, s_vfs_rename_key = {<No data fields>}, s_writers_key = 0xffffffff825645e8, i_lock_key = {<No data fields>}, i_mutex_key = {<No data fields>}, invalidate_lock_key = {<No data fields>}, i_mutex_dir_key = {<No data fields>}} > Kind regards, > Petr
> On Mon, Feb 06, 2023 at 05:19:53PM +0100, Petr Vorel wrote: > > Hi Wei, > > ... > > > > Hm, there is a kernel fix from 5.17 [1]. But test fails when I run it on 6.2.0-rc5: > > > > tst_supported_fs_types.c:165: TINFO: Skipping FUSE based ntfs as requested by the test > > > > tst_supported_fs_types.c:157: TINFO: Skipping tmpfs as requested by the test > > > > tst_test.c:1634: TINFO: === Testing on ext3 === > > > > tst_test.c:1093: TINFO: Formatting /dev/loop0 with ext3 opts='' extra opts='' > > > > mke2fs 1.46.5 (30-Dec-2021) > > > > fsconfig03.c:44: TFAIL: fsconfig(FSCONFIG_SET_STRING) failed: EINVAL (22) > > > > Isn't it the opposite: we expect to fail, thus TST_EXP_FAIL() should here be > > > > used? > > > I have not test on 6.2.0 kernel, i need reproduce this firstly. > > FYI 6.0.6 is also broken, works on 5.10.46. > After long investigation i finally get what's happen now since i have > never touch kernel fs code before :) > The root caused is cebe85d570cf8 which make udpate initialize of > ext3_fs_type. What exactly happen in cebe85d570cf ("ext4: switch to the new mount api") from v5.17-rc1? This enables fsconfig, thus test *should* be working on >= v5.17-rc1, right? But it does not. BTW xfs also uses new mount API 73e5fff98b64 ("xfs: switch to use the new mount-api") in v5.5-rc1. Obviously legacy ext2 driver does not use it. I'd expect fsopen_supported_by_kernel() is enough and no filesystem needs to be filtered out (at least not real linux filesystems like ext2 or xfs). > Each file system will initialize a struct file_system_type and ext3 initialize > in fs/ext4/super.c(maybe ext3 much same as ext4 so they put in same file). Yes, ext3 and ext4 share the same code (ext4 driver). This code also servers ext2 filesystem if CONFIG_EXT4_USE_FOR_EXT2=y (mostly the default), otherwise there is ext2 driver: CONFIG_EXT2_FS). Kind regards, Petr > This patch add new memeber .init_fs_context in ext3 file_system_type struct and > this new member will lead pase function which called by fsconfig change > from legacy_parse_param to ext4_parse_param(this function will check > parameter and not allow 0x00) > ===key change part of cebe85d570cf8=== > static struct file_system_type ext3_fs_type = { > - .owner = THIS_MODULE, > - .name = "ext3", > - .mount = ext4_mount, > - .kill_sb = kill_block_super, > - .fs_flags = FS_REQUIRES_DEV, > + .owner = THIS_MODULE, > + .name = "ext3", > + .init_fs_context = ext4_init_fs_context, // in this patch init_fs_context start set ext4_init_fs_context > + .parameters = ext4_param_specs, > + .kill_sb = kill_block_super, > + .fs_flags = FS_REQUIRES_DEV, > }; > ===key change part of cebe85d570cf8=== > Following logic will decide whether use legacy_init_fs_context base on > exist of init_fs_context, obviously before patch we have no > init_fs_context but after patch we have it > ==function alloc_fs_context== > ..... > init_fs_context = fc->fs_type->init_fs_context; > if (!init_fs_context) > init_fs_context = legacy_init_fs_context; //before patch cebe85d570cf8, legacy_init_fs_context will be set. > ret = init_fs_context(fc); > ==function alloc_fs_context== > ====code example for set parse function used by fsconfig=== > const struct fs_context_operations legacy_fs_context_ops = { > .free = legacy_fs_context_free, > .dup = legacy_fs_context_dup, > .parse_param = legacy_parse_param, > .parse_monolithic = legacy_parse_monolithic, > .get_tree = legacy_get_tree, > .reconfigure = legacy_reconfigure, > }; > /* > * Initialise a legacy context for a filesystem that doesn't support > * fs_context. > */ > static int legacy_init_fs_context(struct fs_context *fc) > { > fc->fs_private = kzalloc(sizeof(struct legacy_fs_context), GFP_KERNEL_ACCOUNT); > if (!fc->fs_private) > return -ENOMEM; > fc->ops = &legacy_fs_context_ops; > return 0; > } > ====code for set parse function used by fsconfig=== > ====final call parse function within fsconfig logic== > vfs_parse_fs_param > 145 if (fc->ops->parse_param) { > 146 ret = fc->ops->parse_param(fc, param); //this will call legacy_parse_param or ext4_parse_param > 147 if (ret != -ENOPARAM) > 148 return ret; > 149 } > ====final call parse function within fsconfig logic== > Just FYI the fs_type real data show in GDB(init_fs_context= 0 in kernel5.x but in kernel 6.x init_fs_context=ext4_parse_param): > (gdb) p *fs_type > $4 = {name = 0xffffffff822278e1 "ext3", fs_flags = 1, init_fs_context = 0x0 <fixed_percpu_data>, parameters = 0x0 <fixed_percpu_data>, > mount = 0xffffffff812ec510 <ext4_mount>, kill_sb = 0xffffffff811f7220 <kill_block_super>, owner = 0x0 <fixed_percpu_data>, > next = 0xffffffff82564600 <ext2_fs_type>, fs_supers = {first = 0x0 <fixed_percpu_data>}, s_lock_key = {<No data fields>}, > s_umount_key = {<No data fields>}, s_vfs_rename_key = {<No data fields>}, s_writers_key = 0xffffffff825645e8, i_lock_key = {<No data fields>}, > i_mutex_key = {<No data fields>}, invalidate_lock_key = {<No data fields>}, i_mutex_dir_key = {<No data fields>}} > > Kind regards, > > Petr
On Wed, Feb 08, 2023 at 04:48:23PM +0100, Petr Vorel wrote: > > On Mon, Feb 06, 2023 at 05:19:53PM +0100, Petr Vorel wrote: > > > Hi Wei, > > > > ... > > > > > Hm, there is a kernel fix from 5.17 [1]. But test fails when I run it on 6.2.0-rc5: > > > > > > tst_supported_fs_types.c:165: TINFO: Skipping FUSE based ntfs as requested by the test > > > > > tst_supported_fs_types.c:157: TINFO: Skipping tmpfs as requested by the test > > > > > tst_test.c:1634: TINFO: === Testing on ext3 === > > > > > tst_test.c:1093: TINFO: Formatting /dev/loop0 with ext3 opts='' extra opts='' > > > > > mke2fs 1.46.5 (30-Dec-2021) > > > > > fsconfig03.c:44: TFAIL: fsconfig(FSCONFIG_SET_STRING) failed: EINVAL (22) > > > > > > Isn't it the opposite: we expect to fail, thus TST_EXP_FAIL() should here be > > > > > used? > > > > > I have not test on 6.2.0 kernel, i need reproduce this firstly. > > > > FYI 6.0.6 is also broken, works on 5.10.46. > > > > After long investigation i finally get what's happen now since i have > > never touch kernel fs code before :) > > > The root caused is cebe85d570cf8 which make udpate initialize of > > ext3_fs_type. > What exactly happen in cebe85d570cf ("ext4: switch to the new mount api") > from v5.17-rc1? This enables fsconfig, thus test *should* be working on >= > v5.17-rc1, right? But it does not. > In my test case use "\x00" as fsconfig key parameter, this parameter ONLY can be accept if kernel using legacy_parse_param. But cebe85d570cf8 make a change lead kernel start using ext4_parse_param then error will happen in your test in v5.17, it's normal behaviro. So the correct behaviro are: 1)you will encounter crash if you running my ltp test case before v5.17. 2)if you test from v5.17, the security fix 722d94847de29 together with internal logic change cebe85d570cf8 both happen, and test case will failed since cebe85d570cf8 change the whole logic and workaround the security fix 722d94847de29(More detail please see below summary). Why 1) not happen now in your test env? Since currently my v1 patch has a mistake on loop i number, if you increase loop number i to 5000 then you will encounter crash on kernel before v5.17(ext2/3/4). + for (unsigned int i = 0; i < 2; i++) { <====increase this to i < 5000 + TEST(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); + if (TST_RET == -1) + tst_brk(TFAIL | TTERRNO, "fsconfig(FSCONFIG_SET_STRING) failed"); + } Let me explain more detail for this: CVE-2022-0185 security bug popped up since 5.1-rc1 and fixed by 722d94847de29 in v5.17-rc1~50, so normally we should check build from v5.17. Most important thing is this security issue ONLY happen if fsconfig go through legacy_parse_param function(security issue happen and fixed within this function). But: For xfs filesystem, from v5.5-rc1 already start use xfs_fs_parse_param instead of legacy_parse_param, so make no sense check this secruity issue For ext2&ext3&ext4, after patch cebe85d570cf8 in v5.17-rc1~131^2~36, use ext4_parse_param instead of legacy_parse_param, so also make no sense check In summary, we can reject this test case since from v5.17, ext2/ext4/xfs not go through legacy_parse_param and means we can not verify security fix 722d94847de29(this fix happen in legacy_parse_param.) Just FYI : fix bug patch which within legacy_parse_param 722d94847de29 (Jamie Hill-Daniel 2022-01-18 08:06:04 +0100 551) if (size + len + 2 > PAGE_SIZE) git describe --contains 722d94847de29 v5.17-rc1~50 > BTW xfs also uses new mount API 73e5fff98b64 ("xfs: switch to use the new > mount-api") in v5.5-rc1. Obviously legacy ext2 driver does not use it. > > I'd expect fsopen_supported_by_kernel() is enough and no filesystem needs to be > filtered out (at least not real linux filesystems like ext2 or xfs). > > > Each file system will initialize a struct file_system_type and ext3 initialize > > in fs/ext4/super.c(maybe ext3 much same as ext4 so they put in same file). > Yes, ext3 and ext4 share the same code (ext4 driver). This code also servers > ext2 filesystem if CONFIG_EXT4_USE_FOR_EXT2=y (mostly the default), otherwise > there is ext2 driver: CONFIG_EXT2_FS). > > Kind regards, > Petr > > > This patch add new memeber .init_fs_context in ext3 file_system_type struct and > > this new member will lead pase function which called by fsconfig change > > from legacy_parse_param to ext4_parse_param(this function will check > > parameter and not allow 0x00) > > > ===key change part of cebe85d570cf8=== > > static struct file_system_type ext3_fs_type = { > > - .owner = THIS_MODULE, > > - .name = "ext3", > > - .mount = ext4_mount, > > - .kill_sb = kill_block_super, > > - .fs_flags = FS_REQUIRES_DEV, > > + .owner = THIS_MODULE, > > + .name = "ext3", > > + .init_fs_context = ext4_init_fs_context, // in this patch init_fs_context start set ext4_init_fs_context > > + .parameters = ext4_param_specs, > > + .kill_sb = kill_block_super, > > + .fs_flags = FS_REQUIRES_DEV, > > }; > > ===key change part of cebe85d570cf8=== > > > > Following logic will decide whether use legacy_init_fs_context base on > > exist of init_fs_context, obviously before patch we have no > > init_fs_context but after patch we have it > > > ==function alloc_fs_context== > > ..... > > init_fs_context = fc->fs_type->init_fs_context; > > if (!init_fs_context) > > init_fs_context = legacy_init_fs_context; //before patch cebe85d570cf8, legacy_init_fs_context will be set. > > ret = init_fs_context(fc); > > > ==function alloc_fs_context== > > > > ====code example for set parse function used by fsconfig=== > > const struct fs_context_operations legacy_fs_context_ops = { > > .free = legacy_fs_context_free, > > .dup = legacy_fs_context_dup, > > .parse_param = legacy_parse_param, > > .parse_monolithic = legacy_parse_monolithic, > > .get_tree = legacy_get_tree, > > .reconfigure = legacy_reconfigure, > > }; > > > /* > > * Initialise a legacy context for a filesystem that doesn't support > > * fs_context. > > */ > > static int legacy_init_fs_context(struct fs_context *fc) > > { > > fc->fs_private = kzalloc(sizeof(struct legacy_fs_context), GFP_KERNEL_ACCOUNT); > > if (!fc->fs_private) > > return -ENOMEM; > > fc->ops = &legacy_fs_context_ops; > > return 0; > > } > > ====code for set parse function used by fsconfig=== > > > > ====final call parse function within fsconfig logic== > > vfs_parse_fs_param > > 145 if (fc->ops->parse_param) { > > 146 ret = fc->ops->parse_param(fc, param); //this will call legacy_parse_param or ext4_parse_param > > 147 if (ret != -ENOPARAM) > > 148 return ret; > > 149 } > > ====final call parse function within fsconfig logic== > > > > > Just FYI the fs_type real data show in GDB(init_fs_context= 0 in kernel5.x but in kernel 6.x init_fs_context=ext4_parse_param): > > > (gdb) p *fs_type > > $4 = {name = 0xffffffff822278e1 "ext3", fs_flags = 1, init_fs_context = 0x0 <fixed_percpu_data>, parameters = 0x0 <fixed_percpu_data>, > > mount = 0xffffffff812ec510 <ext4_mount>, kill_sb = 0xffffffff811f7220 <kill_block_super>, owner = 0x0 <fixed_percpu_data>, > > next = 0xffffffff82564600 <ext2_fs_type>, fs_supers = {first = 0x0 <fixed_percpu_data>}, s_lock_key = {<No data fields>}, > > s_umount_key = {<No data fields>}, s_vfs_rename_key = {<No data fields>}, s_writers_key = 0xffffffff825645e8, i_lock_key = {<No data fields>}, > > i_mutex_key = {<No data fields>}, invalidate_lock_key = {<No data fields>}, i_mutex_dir_key = {<No data fields>}} > > > > > Kind regards, > > > Petr
Hi! > Let me explain more detail for this: > > CVE-2022-0185 security bug popped up since 5.1-rc1 and fixed by 722d94847de29 in v5.17-rc1~50, so normally we should check build from v5.17. > Most important thing is this security issue ONLY happen if fsconfig go through legacy_parse_param function(security issue happen and fixed within this function). > > But: > For xfs filesystem, from v5.5-rc1 already start use xfs_fs_parse_param instead of legacy_parse_param, so make no sense check this secruity issue > For ext2&ext3&ext4, after patch cebe85d570cf8 in v5.17-rc1~131^2~36, use ext4_parse_param instead of legacy_parse_param, so also make no sense check > > In summary, we can reject this test case since from v5.17, ext2/ext4/xfs not go through legacy_parse_param and means we can not verify security fix > 722d94847de29(this fix happen in legacy_parse_param.) Quite contrary it make sense to add regression tests for kernel and keep them running on all filesystems and never releases since you never know when similar mistake will make it into the kernel code again. It does not make much sense to invest time into tests only to keep them disabled later on. More generally it makes sense to try to throw all kind of garbage strings into fsconfig() and expect to get EINVAL or other sane behavior, writing such tests is the only way to avoid or at least catch most CVEs before they happen.
On Thu, Feb 09, 2023 at 11:10:46AM +0100, Cyril Hrubis wrote: > Hi! > > Let me explain more detail for this: > > > > CVE-2022-0185 security bug popped up since 5.1-rc1 and fixed by 722d94847de29 in v5.17-rc1~50, so normally we should check build from v5.17. > > Most important thing is this security issue ONLY happen if fsconfig go through legacy_parse_param function(security issue happen and fixed within this function). > > > > But: > > For xfs filesystem, from v5.5-rc1 already start use xfs_fs_parse_param instead of legacy_parse_param, so make no sense check this secruity issue > > For ext2&ext3&ext4, after patch cebe85d570cf8 in v5.17-rc1~131^2~36, use ext4_parse_param instead of legacy_parse_param, so also make no sense check > > > > In summary, we can reject this test case since from v5.17, ext2/ext4/xfs not go through legacy_parse_param and means we can not verify security fix > > 722d94847de29(this fix happen in legacy_parse_param.) > > Quite contrary it make sense to add regression tests for kernel and keep them > running on all filesystems and never releases since you never know when > similar mistake will make it into the kernel code again. It does not > make much sense to invest time into tests only to keep them disabled > later on. > > More generally it makes sense to try to throw all kind of garbage > strings into fsconfig() and expect to get EINVAL or other sane behavior, > writing such tests is the only way to avoid or at least catch most CVEs > before they happen. > Thanks for review this, i will update the case later. > -- > Cyril Hrubis > chrubis@suse.cz
diff --git a/include/lapi/fsmount.h b/include/lapi/fsmount.h index 07eb42ffa..252accb0f 100644 --- a/include/lapi/fsmount.h +++ b/include/lapi/fsmount.h @@ -11,12 +11,15 @@ #include "config.h" #include <sys/syscall.h> #include <sys/types.h> -#include <sys/mount.h> #ifndef HAVE_FSOPEN # ifdef HAVE_LINUX_MOUNT_H # include <linux/mount.h> +# else +# include <sys/mount.h> # endif +#else +# include <sys/mount.h> #endif #include "lapi/fcntl.h" diff --git a/runtest/syscalls b/runtest/syscalls index ae37a1192..b4cde8071 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -383,6 +383,7 @@ fremovexattr02 fremovexattr02 fsconfig01 fsconfig01 fsconfig02 fsconfig02 +fsconfig03 fsconfig03 fsmount01 fsmount01 fsmount02 fsmount02 diff --git a/testcases/kernel/syscalls/fsconfig/fsconfig03.c b/testcases/kernel/syscalls/fsconfig/fsconfig03.c new file mode 100644 index 000000000..e076c2f09 --- /dev/null +++ b/testcases/kernel/syscalls/fsconfig/fsconfig03.c @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2023 Wei Gao <wegao@suse.com> + */ + +/*\ + * Test add some coverage to CVE-2022-0185. + * Try to trigger a crash. + * References links: + * https://www.openwall.com/lists/oss-security/2022/01/25/14 + * https://github.com/Crusaders-of-Rust/CVE-2022-0185 + */ + +#include "tst_test.h" +#include "lapi/fsmount.h" + +#define MNTPOINT "mntpoint" + +static int fd = -1; + +static void setup(void) +{ + fsopen_supported_by_kernel(); + + TEST(fd = fsopen(tst_device->fs_type, 0)); + if (fd == -1) + tst_brk(TBROK | TTERRNO, "fsopen() failed"); + +} + +static void cleanup(void) +{ + if (fd != -1) + SAFE_CLOSE(fd); +} + +static void run(void) +{ + char *val = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + + for (unsigned int i = 0; i < 2; i++) { + TEST(fsconfig(fd, FSCONFIG_SET_STRING, "\x00", val, 0)); + if (TST_RET == -1) + tst_brk(TFAIL | TTERRNO, "fsconfig(FSCONFIG_SET_STRING) failed"); + } + tst_res(TPASS, "Try fsconfig overflow on %s done!", tst_device->fs_type); +} + +static struct tst_test test = { + .test_all = run, + .setup = setup, + .cleanup = cleanup, + .needs_root = 1, + .format_device = 1, + .mntpoint = MNTPOINT, + .all_filesystems = 1, + .skip_filesystems = (const char *const []){"fuse", "ext2", "xfs", "tmpfs", NULL}, +};
There are reproducers available for CVE-2022-0185 https://www.openwall.com/lists/oss-security/2022/01/25/14 has links or even a zip file for an exploit https://github.com/Crusaders-of-Rust/CVE-2022-0185 The exploits are kind of complicated as they try to be complete, but the exploitation vector is the fsconfig() syscall, this case used for add some coverage to that to detect it. Signed-off-by: Wei Gao <wegao@suse.com> --- include/lapi/fsmount.h | 5 +- runtest/syscalls | 1 + .../kernel/syscalls/fsconfig/fsconfig03.c | 58 +++++++++++++++++++ 3 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 testcases/kernel/syscalls/fsconfig/fsconfig03.c