diff mbox series

[REPOST,v2] vfs: parse: deal with zero length string value

Message ID 165724435867.30814.6980005089665688371.stgit@donald.themaw.net (mailing list archive)
State New
Headers show
Series [REPOST,v2] vfs: parse: deal with zero length string value | expand

Commit Message

Ian Kent July 8, 2022, 1:39 a.m. UTC
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(-)

Comments

kernel test robot July 18, 2022, 2:35 p.m. UTC | #1
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.
Ian Kent July 19, 2022, 5:30 a.m. UTC | #2
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 mbox series

Patch

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, &param);
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 */