| Message ID | 165724435867.30814.6980005089665688371.stgit@donald.themaw.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [REPOST,v2] vfs: parse: deal with zero length string value | expand |
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: f756fe900f17af85c3f4bafc9b9e996bcc0fbeb1 ("[REPOST PATCH v2] vfs: parse: deal with zero length string value")
url: https://github.com/intel-lab-lkp/linux/commits/Ian-Kent/vfs-parse-deal-with-zero-length-string-value/20220708-094030
base: https://git.kernel.org/cgit/linux/kernel/git/viro/vfs.git for-next
patch link: https://lore.kernel.org/linux-fsdevel/165724435867.30814.6980005089665688371.stgit@donald.themaw.net
in testcase: xfstests
version: xfstests-x86_64-c1144bf-1_20220711
with following parameters:
disk: 4HDD
fs: ext2
test: ext4-group-02
ucode: 0xec
test-description: xfstests is a regression test suite for xfs and other files ystems.
test-url: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
on test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz with 32G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <oliver.sang@intel.com>
[ 380.748272][ T5965] EXT4-fs (sda4): mounting ext3 file system using the ext4 subsystem
[ 380.856453][ T5993] EXT4-fs: journaled quota format not specified
[ 380.879248][ T5997] EXT4-fs (sda4): mounting ext3 file system using the ext4 subsystem
[ 380.911204][ T6003] EXT4-fs: journaled quota format not specified
[ 380.924796][ T6007] EXT4-fs: journaled quota format not specified
[ 380.964372][ T6012] general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] SMP KASAN PTI
[ 380.975568][ T6012] KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
[ 380.983810][ T6012] CPU: 1 PID: 6012 Comm: mount Tainted: G S I 5.19.0-rc2-00001-gf756fe900f17 #1
[ 380.993786][ T6012] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
[ 381.001854][ T6012] RIP: 0010:ext4_parse_param (kbuild/src/consumer/fs/ext4/super.c:2109)
[ 381.007414][ T6012] Code: 49 8d 7f 10 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 0b 1b 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 6f 10 4c 89 ea 48 c1 ea 03 <0f> b6 04 02 4c 89 ea 83 e2 07 38 d0 7f 08 84 c0 0f 85 ff 1e 00 00
All code
========
0: 49 8d 7f 10 lea 0x10(%r15),%rdi
4: 48 89 fa mov %rdi,%rdx
7: 48 c1 ea 03 shr $0x3,%rdx
b: 80 3c 02 00 cmpb $0x0,(%rdx,%rax,1)
f: 0f 85 0b 1b 00 00 jne 0x1b20
15: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
1c: fc ff df
1f: 4d 8b 6f 10 mov 0x10(%r15),%r13
23: 4c 89 ea mov %r13,%rdx
26: 48 c1 ea 03 shr $0x3,%rdx
2a:* 0f b6 04 02 movzbl (%rdx,%rax,1),%eax <-- trapping instruction
2e: 4c 89 ea mov %r13,%rdx
31: 83 e2 07 and $0x7,%edx
34: 38 d0 cmp %dl,%al
36: 7f 08 jg 0x40
38: 84 c0 test %al,%al
3a: 0f 85 ff 1e 00 00 jne 0x1f3f
Code starting with the faulting instruction
===========================================
0: 0f b6 04 02 movzbl (%rdx,%rax,1),%eax
4: 4c 89 ea mov %r13,%rdx
7: 83 e2 07 and $0x7,%edx
a: 38 d0 cmp %dl,%al
c: 7f 08 jg 0x16
e: 84 c0 test %al,%al
10: 0f 85 ff 1e 00 00 jne 0x1f15
[ 381.026823][ T6012] RSP: 0018:ffffc900036dfac0 EFLAGS: 00010246
[ 381.032731][ T6012] RAX: dffffc0000000000 RBX: ffffffff83ba35c0 RCX: 0000000000000000
[ 381.040539][ T6012] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffc900036dfc00
[ 381.048348][ T6012] RBP: 1ffff920006dbf5c R08: 0000000000000001 R09: ffffffff83ba35d4
[ 381.056158][ T6012] R10: ffffed110d8af749 R11: 0000000000000001 R12: ffff8881acdfbb00
[ 381.063968][ T6012] R13: 0000000000000000 R14: ffff888863e19e00 R15: ffffc900036dfbf0
[ 381.071791][ T6012] FS: 00007fb9236a2840(0000) GS:ffff88871fa80000(0000) knlGS:0000000000000000
[ 381.080553][ T6012] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 381.086977][ T6012] CR2: 00007fb9239e1830 CR3: 00000002734e8003 CR4: 00000000003706e0
[ 381.094787][ T6012] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 381.102595][ T6012] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 381.110403][ T6012] Call Trace:
[ 381.113536][ T6012] <TASK>
[ 381.116324][ T6012] ? note_qf_name+0x300/0x300
[ 381.121452][ T6012] vfs_parse_fs_param (kbuild/src/consumer/fs/fs_context.c:149 kbuild/src/consumer/fs/fs_context.c:129)
[ 381.126319][ T6012] vfs_parse_fs_string (kbuild/src/consumer/fs/fs_context.c:192)
[ 381.131188][ T6012] ? vfs_parse_fs_param (kbuild/src/consumer/fs/fs_context.c:170)
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
On 18/7/22 22:35, kernel test robot wrote: > > Greeting, > > FYI, we noticed the following commit (built with gcc-11): > > commit: f756fe900f17af85c3f4bafc9b9e996bcc0fbeb1 ("[REPOST PATCH v2] vfs: parse: deal with zero length string value") > url: https://github.com/intel-lab-lkp/linux/commits/Ian-Kent/vfs-parse-deal-with-zero-length-string-value/20220708-094030 > base: https://git.kernel.org/cgit/linux/kernel/git/viro/vfs.git for-next > patch link: https://lore.kernel.org/linux-fsdevel/165724435867.30814.6980005089665688371.stgit@donald.themaw.net > > in testcase: xfstests > version: xfstests-x86_64-c1144bf-1_20220711 > with following parameters: > > disk: 4HDD > fs: ext2 > test: ext4-group-02 > ucode: 0xec > > test-description: xfstests is a regression test suite for xfs and other files ystems. > test-url: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git > > > on test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz with 32G memory > > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace): > > > > If you fix the issue, kindly add following tag > Reported-by: kernel test robot <oliver.sang@intel.com> > > > [ 380.748272][ T5965] EXT4-fs (sda4): mounting ext3 file system using the ext4 subsystem > [ 380.856453][ T5993] EXT4-fs: journaled quota format not specified > [ 380.879248][ T5997] EXT4-fs (sda4): mounting ext3 file system using the ext4 subsystem > [ 380.911204][ T6003] EXT4-fs: journaled quota format not specified > [ 380.924796][ T6007] EXT4-fs: journaled quota format not specified > [ 380.964372][ T6012] general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] SMP KASAN PTI > [ 380.975568][ T6012] KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] > [ 380.983810][ T6012] CPU: 1 PID: 6012 Comm: mount Tainted: G S I 5.19.0-rc2-00001-gf756fe900f17 #1 > [ 380.993786][ T6012] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015 > [ 381.001854][ T6012] RIP: 0010:ext4_parse_param (kbuild/src/consumer/fs/ext4/super.c:2109) It has to be this: @@ -2110,12 +2110,12 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param) switch (token) { #ifdef CONFIG_QUOTA case Opt_usrjquota: - if (!*param->string) + if (!param->string || !*param->string) return unnote_qf_name(fc, USRQUOTA); else return note_qf_name(fc, USRQUOTA, param); case Opt_grpjquota: - if (!*param->string) + if (!param->string || !*param->string) return unnote_qf_name(fc, GRPQUOTA); else return note_qf_name(fc, GRPQUOTA, param); IMHO it's fragile without the additional check since the file system has no control over how parameters come to it both in the old and new systems. Ian
diff --git a/fs/fs_context.c b/fs/fs_context.c index 24ce12f0db32..df04e5fc6d66 100644 --- a/fs/fs_context.c +++ b/fs/fs_context.c @@ -96,7 +96,9 @@ int vfs_parse_fs_param_source(struct fs_context *fc, struct fs_parameter *param) if (strcmp(param->key, "source") != 0) return -ENOPARAM; - if (param->type != fs_value_is_string) + /* source value may be NULL */ + if (param->type != fs_value_is_string && + param->type != fs_value_is_empty) return invalf(fc, "Non-string source"); if (fc->source) @@ -175,10 +177,15 @@ int vfs_parse_fs_string(struct fs_context *fc, const char *key, }; if (value) { - param.string = kmemdup_nul(value, v_size, GFP_KERNEL); - if (!param.string) - return -ENOMEM; - param.type = fs_value_is_string; + if (!v_size) { + param.string = NULL; + param.type = fs_value_is_empty; + } else { + param.string = kmemdup_nul(value, v_size, GFP_KERNEL); + if (!param.string) + return -ENOMEM; + param.type = fs_value_is_string; + } } ret = vfs_parse_fs_param(fc, ¶m); diff --git a/fs/fs_parser.c b/fs/fs_parser.c index ed40ce5742fd..2046f41ab00b 100644 --- a/fs/fs_parser.c +++ b/fs/fs_parser.c @@ -197,6 +197,8 @@ int fs_param_is_bool(struct p_log *log, const struct fs_parameter_spec *p, struct fs_parameter *param, struct fs_parse_result *result) { int b; + if (param->type == fs_value_is_empty) + return 0; if (param->type != fs_value_is_string) return fs_param_bad_value(log, param); if (!*param->string && (p->flags & fs_param_can_be_empty)) @@ -213,6 +215,8 @@ int fs_param_is_u32(struct p_log *log, const struct fs_parameter_spec *p, struct fs_parameter *param, struct fs_parse_result *result) { int base = (unsigned long)p->data; + if (param->type == fs_value_is_empty) + return 0; if (param->type != fs_value_is_string) return fs_param_bad_value(log, param); if (!*param->string && (p->flags & fs_param_can_be_empty)) @@ -226,6 +230,8 @@ EXPORT_SYMBOL(fs_param_is_u32); int fs_param_is_s32(struct p_log *log, const struct fs_parameter_spec *p, struct fs_parameter *param, struct fs_parse_result *result) { + if (param->type == fs_value_is_empty) + return 0; if (param->type != fs_value_is_string) return fs_param_bad_value(log, param); if (!*param->string && (p->flags & fs_param_can_be_empty)) @@ -239,6 +245,8 @@ EXPORT_SYMBOL(fs_param_is_s32); int fs_param_is_u64(struct p_log *log, const struct fs_parameter_spec *p, struct fs_parameter *param, struct fs_parse_result *result) { + if (param->type == fs_value_is_empty) + return 0; if (param->type != fs_value_is_string) return fs_param_bad_value(log, param); if (!*param->string && (p->flags & fs_param_can_be_empty)) @@ -253,6 +261,8 @@ int fs_param_is_enum(struct p_log *log, const struct fs_parameter_spec *p, struct fs_parameter *param, struct fs_parse_result *result) { const struct constant_table *c; + if (param->type == fs_value_is_empty) + return 0; if (param->type != fs_value_is_string) return fs_param_bad_value(log, param); if (!*param->string && (p->flags & fs_param_can_be_empty)) @@ -268,6 +278,8 @@ EXPORT_SYMBOL(fs_param_is_enum); int fs_param_is_string(struct p_log *log, const struct fs_parameter_spec *p, struct fs_parameter *param, struct fs_parse_result *result) { + if (param->type == fs_value_is_empty) + return 0; if (param->type != fs_value_is_string || (!*param->string && !(p->flags & fs_param_can_be_empty))) return fs_param_bad_value(log, param); @@ -278,6 +290,8 @@ EXPORT_SYMBOL(fs_param_is_string); int fs_param_is_blob(struct p_log *log, const struct fs_parameter_spec *p, struct fs_parameter *param, struct fs_parse_result *result) { + if (param->type == fs_value_is_empty) + return 0; if (param->type != fs_value_is_blob) return fs_param_bad_value(log, param); return 0; @@ -287,6 +301,8 @@ EXPORT_SYMBOL(fs_param_is_blob); int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p, struct fs_parameter *param, struct fs_parse_result *result) { + if (param->type == fs_value_is_empty) + return 0; switch (param->type) { case fs_value_is_string: if ((!*param->string && !(p->flags & fs_param_can_be_empty)) || diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h index 13fa6f3df8e4..ff1375a16c8c 100644 --- a/include/linux/fs_context.h +++ b/include/linux/fs_context.h @@ -50,7 +50,8 @@ enum fs_context_phase { */ enum fs_value_type { fs_value_is_undefined, - fs_value_is_flag, /* Value not given a value */ + fs_value_is_flag, /* Does not take a value */ + fs_value_is_empty, /* Value is not given */ fs_value_is_string, /* Value is a string */ fs_value_is_blob, /* Value is a binary blob */ fs_value_is_filename, /* Value is a filename* + dirfd */
Parsing an fs string that has zero length should result in the parameter being set to NULL so that downstream processing handles it correctly. For example, the proc mount table processing should print "(none)" in this case to preserve mount record field count, but if the value points to the NULL string this doesn't happen. Changes: v2: fix possible oops if conversion functions such as fs_param_is_u32() are called. Signed-off-by: Ian Kent <raven@themaw.net> --- fs/fs_context.c | 17 ++++++++++++----- fs/fs_parser.c | 16 ++++++++++++++++ include/linux/fs_context.h | 3 ++- 3 files changed, 30 insertions(+), 6 deletions(-)