[PATCH 1/2] ext4: fix up copying of mount_opts in superblock tuning ioctls

[PATCH 1/2] ext4: fix up copying of mount_opts in superblock tuning ioctls

[Fedor Pchelkin]

Judging by commit 8ecb790ea8c3 ("ext4: avoid potential buffer over-read in
parse_apply_sb_mount_options()"), the contents of s_mount_opts should be
treated as __nonstring, i.e. there might be no NUL-terminator in the
provided buffer.

Then the same holds for the corresponding mount_opts field of the struct
ext4_tune_sb_params exchanged with userspace via a recently implemented
superblock tuning ioctl.

The problem is that strscpy_pad() can't work properly with non-NUL-term
strings.  String fortifying infrastructure would complain if that happens.
Commit 0efc5990bca5 ("string.h: Introduce memtostr() and memtostr_pad()")
gives additional information in that regard.

Both buffers are just raw arrays of the similar fixed size, essentially
they should represent the same contents.  As they don't necessarily have
NUL-terminators, in both directions use plain memcpy() to copy their
contents.

Found by Linux Verification Center (linuxtesting.org).

Fixes: 04a91570ac67 ("ext4: implemet new ioctls to set and get superblock parameters")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---

The 1/2 patch of the current series fixes an issue existing only in 6.18-rc
while 2/2 fixes the commit which was in turn backported to stable kernels.
That's the reasoning for separation.

 fs/ext4/ioctl.c           | 4 ++--
 include/uapi/linux/ext4.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index a93a7baae990..c39b87d52cb0 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1292,7 +1292,7 @@ static int ext4_ioctl_get_tune_sb(struct ext4_sb_info *sbi,
 	ret.raid_stripe_width = le32_to_cpu(es->s_raid_stripe_width);
 	ret.encoding = le16_to_cpu(es->s_encoding);
 	ret.encoding_flags = le16_to_cpu(es->s_encoding_flags);
-	strscpy_pad(ret.mount_opts, es->s_mount_opts);
+	memcpy(ret.mount_opts, es->s_mount_opts, sizeof(ret.mount_opts));
 	ret.feature_compat = le32_to_cpu(es->s_feature_compat);
 	ret.feature_incompat = le32_to_cpu(es->s_feature_incompat);
 	ret.feature_ro_compat = le32_to_cpu(es->s_feature_ro_compat);
@@ -1353,7 +1353,7 @@ static void ext4_sb_setparams(struct ext4_sb_info *sbi,
 		es->s_encoding = cpu_to_le16(params->encoding);
 	if (params->set_flags & EXT4_TUNE_FL_ENCODING_FLAGS)
 		es->s_encoding_flags = cpu_to_le16(params->encoding_flags);
-	strscpy_pad(es->s_mount_opts, params->mount_opts);
+	memcpy(es->s_mount_opts, params->mount_opts, sizeof(es->s_mount_opts));
 	if (params->set_flags & EXT4_TUNE_FL_EDIT_FEATURES) {
 		es->s_feature_compat |=
 			cpu_to_le32(params->set_feature_compat_mask);
diff --git a/include/uapi/linux/ext4.h b/include/uapi/linux/ext4.h
index 411dcc1e4a35..8ed9acbd0e03 100644
--- a/include/uapi/linux/ext4.h
+++ b/include/uapi/linux/ext4.h
@@ -138,7 +138,7 @@ struct ext4_tune_sb_params {
 	__u32 clear_feature_compat_mask;
 	__u32 clear_feature_incompat_mask;
 	__u32 clear_feature_ro_compat_mask;
-	__u8  mount_opts[64];
+	__u8  mount_opts[64] __nonstring;
 	__u8  pad[64];
 };
 
-- 
2.51.0

[PATCH 2/2] ext4: fix string copying in parse_apply_sb_mount_options()

[Fedor Pchelkin]

strscpy_pad() can't be used to copy a possibly non-NUL-term string into a
NUL-term string.  Commit 0efc5990bca5 ("string.h: Introduce memtostr() and
memtostr_pad()") provides additional information in that regard.  So if
this happens, the following warning is observed:

strnlen: detected buffer overflow: 65 byte read of buffer size 64
WARNING: CPU: 0 PID: 28655 at lib/string_helpers.c:1032 __fortify_report+0x96/0xc0 lib/string_helpers.c:1032
Modules linked in:
CPU: 0 UID: 0 PID: 28655 Comm: syz-executor.3 Not tainted 6.12.54-syzkaller-00144-g5f0270f1ba00 #0
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
RIP: 0010:__fortify_report+0x96/0xc0 lib/string_helpers.c:1032
Call Trace:
 <TASK>
 __fortify_panic+0x1f/0x30 lib/string_helpers.c:1039
 strnlen include/linux/fortify-string.h:235 [inline]
 sized_strscpy include/linux/fortify-string.h:309 [inline]
 parse_apply_sb_mount_options fs/ext4/super.c:2504 [inline]
 __ext4_fill_super fs/ext4/super.c:5261 [inline]
 ext4_fill_super+0x3c35/0xad00 fs/ext4/super.c:5706
 get_tree_bdev_flags+0x387/0x620 fs/super.c:1636
 vfs_get_tree+0x93/0x380 fs/super.c:1814
 do_new_mount fs/namespace.c:3553 [inline]
 path_mount+0x6ae/0x1f70 fs/namespace.c:3880
 do_mount fs/namespace.c:3893 [inline]
 __do_sys_mount fs/namespace.c:4103 [inline]
 __se_sys_mount fs/namespace.c:4080 [inline]
 __x64_sys_mount+0x280/0x300 fs/namespace.c:4080
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0x64/0x140 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x76/0x7e

Since s_es->s_mount_opts might be non-NUL-term, annotate it with
__nonstring and use the proper memtostr_pad() routine to get its NULL-term
copy.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fixes: 8ecb790ea8c3 ("ext4: avoid potential buffer over-read in parse_apply_sb_mount_options()")
Cc: stable@vger.kernel.org
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 fs/ext4/ext4.h  | 2 +-
 fs/ext4/super.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 57087da6c7be..4c8698316457 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1429,7 +1429,7 @@ struct ext4_super_block {
 	__le64	s_last_error_block;	/* block involved of last error */
 	__u8	s_last_error_func[32] __nonstring;	/* function where the error happened */
 #define EXT4_S_ERR_END offsetof(struct ext4_super_block, s_mount_opts)
-	__u8	s_mount_opts[64];
+	__u8	s_mount_opts[64] __nonstring;
 	__le32	s_usr_quota_inum;	/* inode for tracking user quota */
 	__le32	s_grp_quota_inum;	/* inode for tracking group quota */
 	__le32	s_overhead_clusters;	/* overhead blocks/clusters in fs */
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 33e7c08c9529..57df129873e3 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2483,7 +2483,7 @@ static int parse_apply_sb_mount_options(struct super_block *sb,
 	if (!sbi->s_es->s_mount_opts[0])
 		return 0;
 
-	strscpy_pad(s_mount_opts, sbi->s_es->s_mount_opts);
+	memtostr_pad(s_mount_opts, sbi->s_es->s_mount_opts);
 
 	fc = kzalloc(sizeof(struct fs_context), GFP_KERNEL);
 	if (!fc)
-- 
2.51.0

Re: [PATCH 2/2] ext4: fix string copying in parse_apply_sb_mount_options()

[Fedor Pchelkin]

On Tue, 28. Oct 16:09, Fedor Pchelkin wrote:
> strscpy_pad() can't be used to copy a possibly non-NUL-term string into a
> NUL-term string.  Commit 0efc5990bca5 ("string.h: Introduce memtostr() and
> memtostr_pad()") provides additional information in that regard.  So if
> this happens, the following warning is observed:
> 
> strnlen: detected buffer overflow: 65 byte read of buffer size 64
> WARNING: CPU: 0 PID: 28655 at lib/string_helpers.c:1032 __fortify_report+0x96/0xc0 lib/string_helpers.c:1032
> Modules linked in:
> CPU: 0 UID: 0 PID: 28655 Comm: syz-executor.3 Not tainted 6.12.54-syzkaller-00144-g5f0270f1ba00 #0
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> RIP: 0010:__fortify_report+0x96/0xc0 lib/string_helpers.c:1032
> Call Trace:
>  <TASK>
>  __fortify_panic+0x1f/0x30 lib/string_helpers.c:1039
>  strnlen include/linux/fortify-string.h:235 [inline]
>  sized_strscpy include/linux/fortify-string.h:309 [inline]
>  parse_apply_sb_mount_options fs/ext4/super.c:2504 [inline]
>  __ext4_fill_super fs/ext4/super.c:5261 [inline]
>  ext4_fill_super+0x3c35/0xad00 fs/ext4/super.c:5706
>  get_tree_bdev_flags+0x387/0x620 fs/super.c:1636
>  vfs_get_tree+0x93/0x380 fs/super.c:1814
>  do_new_mount fs/namespace.c:3553 [inline]
>  path_mount+0x6ae/0x1f70 fs/namespace.c:3880
>  do_mount fs/namespace.c:3893 [inline]
>  __do_sys_mount fs/namespace.c:4103 [inline]
>  __se_sys_mount fs/namespace.c:4080 [inline]
>  __x64_sys_mount+0x280/0x300 fs/namespace.c:4080
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0x64/0x140 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> Since s_es->s_mount_opts might be non-NUL-term, annotate it with
> __nonstring and use the proper memtostr_pad() routine to get its NULL-term
> copy.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: 8ecb790ea8c3 ("ext4: avoid potential buffer over-read in parse_apply_sb_mount_options()")


Though giving it a second glance I wonder what overflow problem does the
commit in Fixes address?

Behavior actually stays the same: before the blamed patch in case of a
non-NUL-term string kstrndup() just allocates a buffer of `size + 1`
(which equals to 65 here) and no overflow is possible, as far as I can
see.

If it's actually true, maybe we'd better just revert 8ecb790ea8c3 ("ext4:
avoid potential buffer over-read in parse_apply_sb_mount_options()") ?


> Cc: stable@vger.kernel.org
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
>  fs/ext4/ext4.h  | 2 +-
>  fs/ext4/super.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 57087da6c7be..4c8698316457 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1429,7 +1429,7 @@ struct ext4_super_block {
>  	__le64	s_last_error_block;	/* block involved of last error */
>  	__u8	s_last_error_func[32] __nonstring;	/* function where the error happened */
>  #define EXT4_S_ERR_END offsetof(struct ext4_super_block, s_mount_opts)
> -	__u8	s_mount_opts[64];
> +	__u8	s_mount_opts[64] __nonstring;
>  	__le32	s_usr_quota_inum;	/* inode for tracking user quota */
>  	__le32	s_grp_quota_inum;	/* inode for tracking group quota */
>  	__le32	s_overhead_clusters;	/* overhead blocks/clusters in fs */
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 33e7c08c9529..57df129873e3 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2483,7 +2483,7 @@ static int parse_apply_sb_mount_options(struct super_block *sb,
>  	if (!sbi->s_es->s_mount_opts[0])
>  		return 0;
>  
> -	strscpy_pad(s_mount_opts, sbi->s_es->s_mount_opts);
> +	memtostr_pad(s_mount_opts, sbi->s_es->s_mount_opts);
>  
>  	fc = kzalloc(sizeof(struct fs_context), GFP_KERNEL);
>  	if (!fc)
> -- 
> 2.51.0
> 

Re: [PATCH 1/2] ext4: fix up copying of mount_opts in superblock tuning ioctls

[kernel test robot]

Hi Fedor,

kernel test robot noticed the following build errors:

[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on linus/master v6.18-rc3 next-20251029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Fedor-Pchelkin/ext4-fix-string-copying-in-parse_apply_sb_mount_options/20251028-211235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link:    https://lore.kernel.org/r/20251028130949.599847-1-pchelkin%40ispras.ru
patch subject: [PATCH 1/2] ext4: fix up copying of mount_opts in superblock tuning ioctls
config: x86_64-rhel-9.4 (https://download.01.org/0day-ci/archive/20251029/202510291703.2nhl90qz-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251029/202510291703.2nhl90qz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510291703.2nhl90qz-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from <command-line>:
>> ./usr/include/linux/ext4.h:141:30: error: expected ':', ',', ';', '}' or '__attribute__' before '__nonstring'
     141 |         __u8  mount_opts[64] __nonstring;
         |                              ^~~~~~~~~~~

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] ext4: fix up copying of mount_opts in superblock tuning ioctls

[Fedor Pchelkin]

On Wed, 29. Oct 17:22, kernel test robot wrote:
> All errors (new ones prefixed by >>):
> 
>    In file included from <command-line>:
> >> ./usr/include/linux/ext4.h:141:30: error: expected ':', ',', ';', '}' or '__attribute__' before '__nonstring'
>      141 |         __u8  mount_opts[64] __nonstring;
>          |                              ^~~~~~~~~~~

That's my fault, apologies.  HDRTEST was not in my build testing for
these patches due to mistake.

There should be '__kernel_nonstring', not '__nonstring'.  The whole
annotation part is questionable though, maybe you think it's not needed at
all here?

Before sending out v2, I'll wait for the feedback on patch 2/2 - whether
the commit 8ecb790ea8c3 ("ext4: avoid potential buffer over-read in
parse_apply_sb_mount_options()") should be reverted or fixed with extra
memtostr() stuff like that patch proposes..?

Thanks!

Re: [PATCH 1/2] ext4: fix up copying of mount_opts in superblock tuning ioctls

[kernel test robot]

Hi Fedor,

kernel test robot noticed the following build errors:

[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on linus/master v6.18-rc3 next-20251029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Fedor-Pchelkin/ext4-fix-string-copying-in-parse_apply_sb_mount_options/20251028-211235
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link:    https://lore.kernel.org/r/20251028130949.599847-1-pchelkin%40ispras.ru
patch subject: [PATCH 1/2] ext4: fix up copying of mount_opts in superblock tuning ioctls
config: x86_64-buildonly-randconfig-002-20251029 (https://download.01.org/0day-ci/archive/20251029/202510291846.pFj7PlQQ-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251029/202510291846.pFj7PlQQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510291846.pFj7PlQQ-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from <built-in>:1:
>> ./usr/include/linux/ext4.h:141:22: error: expected ';' at end of declaration list
     141 |         __u8  mount_opts[64] __nonstring;
         |                             ^
         |                             ;
   1 error generated.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] ext4: fix up copying of mount_opts in superblock tuning ioctls

[Jan Kara]

On Tue 28-10-25 16:09:47, Fedor Pchelkin wrote:
> Judging by commit 8ecb790ea8c3 ("ext4: avoid potential buffer over-read in
> parse_apply_sb_mount_options()"), the contents of s_mount_opts should be
> treated as __nonstring, i.e. there might be no NUL-terminator in the
> provided buffer.
> 
> Then the same holds for the corresponding mount_opts field of the struct
> ext4_tune_sb_params exchanged with userspace via a recently implemented
> superblock tuning ioctl.
> 
> The problem is that strscpy_pad() can't work properly with non-NUL-term
> strings.  String fortifying infrastructure would complain if that happens.
> Commit 0efc5990bca5 ("string.h: Introduce memtostr() and memtostr_pad()")
> gives additional information in that regard.
> 
> Both buffers are just raw arrays of the similar fixed size, essentially
> they should represent the same contents.  As they don't necessarily have
> NUL-terminators, in both directions use plain memcpy() to copy their
> contents.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Fixes: 04a91570ac67 ("ext4: implemet new ioctls to set and get superblock parameters")
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

I agree there are some holes in the logic of 8ecb790ea8c3 ("ext4: avoid
potential buffer over-read in parse_apply_sb_mount_options()") and
consequently 04a91570ac67 may need fixing up as well. But I think the fixes
should look differently. The clear intended use of s_mount_opts field is
that it is at most 63 characters long with the last byte guaranteed to be
0. This is how userspace utilities use it and they complain if you try
setting more than 63 characters long string. So I think strscpy_pad() use
in ext4_ioctl_get_tune_sb() is actually fine (sizes of both buffers match).
In ext4_sb_setparams() we should actually make sure userspace buffer is
properly Nul-terminated and return error otherwise. And the buffer in
parse_apply_sb_mount_options() should actually be only 64 bytes long to
match the size of the source buffer at which point using strscpy_pad()
becomes correct. How does that sound?

								Honza

> ---
> 
> The 1/2 patch of the current series fixes an issue existing only in 6.18-rc
> while 2/2 fixes the commit which was in turn backported to stable kernels.
> That's the reasoning for separation.
> 
>  fs/ext4/ioctl.c           | 4 ++--
>  include/uapi/linux/ext4.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index a93a7baae990..c39b87d52cb0 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1292,7 +1292,7 @@ static int ext4_ioctl_get_tune_sb(struct ext4_sb_info *sbi,
>  	ret.raid_stripe_width = le32_to_cpu(es->s_raid_stripe_width);
>  	ret.encoding = le16_to_cpu(es->s_encoding);
>  	ret.encoding_flags = le16_to_cpu(es->s_encoding_flags);
> -	strscpy_pad(ret.mount_opts, es->s_mount_opts);
> +	memcpy(ret.mount_opts, es->s_mount_opts, sizeof(ret.mount_opts));
>  	ret.feature_compat = le32_to_cpu(es->s_feature_compat);
>  	ret.feature_incompat = le32_to_cpu(es->s_feature_incompat);
>  	ret.feature_ro_compat = le32_to_cpu(es->s_feature_ro_compat);
> @@ -1353,7 +1353,7 @@ static void ext4_sb_setparams(struct ext4_sb_info *sbi,
>  		es->s_encoding = cpu_to_le16(params->encoding);
>  	if (params->set_flags & EXT4_TUNE_FL_ENCODING_FLAGS)
>  		es->s_encoding_flags = cpu_to_le16(params->encoding_flags);
> -	strscpy_pad(es->s_mount_opts, params->mount_opts);
> +	memcpy(es->s_mount_opts, params->mount_opts, sizeof(es->s_mount_opts));
>  	if (params->set_flags & EXT4_TUNE_FL_EDIT_FEATURES) {
>  		es->s_feature_compat |=
>  			cpu_to_le32(params->set_feature_compat_mask);
> diff --git a/include/uapi/linux/ext4.h b/include/uapi/linux/ext4.h
> index 411dcc1e4a35..8ed9acbd0e03 100644
> --- a/include/uapi/linux/ext4.h
> +++ b/include/uapi/linux/ext4.h
> @@ -138,7 +138,7 @@ struct ext4_tune_sb_params {
>  	__u32 clear_feature_compat_mask;
>  	__u32 clear_feature_incompat_mask;
>  	__u32 clear_feature_ro_compat_mask;
> -	__u8  mount_opts[64];
> +	__u8  mount_opts[64] __nonstring;
>  	__u8  pad[64];
>  };
>  
> -- 
> 2.51.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] ext4: fix up copying of mount_opts in superblock tuning ioctls

[Fedor Pchelkin]

On Thu, 30. Oct 12:32, Jan Kara wrote:
> On Tue 28-10-25 16:09:47, Fedor Pchelkin wrote:
> > Judging by commit 8ecb790ea8c3 ("ext4: avoid potential buffer over-read in
> > parse_apply_sb_mount_options()"), the contents of s_mount_opts should be
> > treated as __nonstring, i.e. there might be no NUL-terminator in the
> > provided buffer.
> > 
> > Then the same holds for the corresponding mount_opts field of the struct
> > ext4_tune_sb_params exchanged with userspace via a recently implemented
> > superblock tuning ioctl.
> > 
> > The problem is that strscpy_pad() can't work properly with non-NUL-term
> > strings.  String fortifying infrastructure would complain if that happens.
> > Commit 0efc5990bca5 ("string.h: Introduce memtostr() and memtostr_pad()")
> > gives additional information in that regard.
> > 
> > Both buffers are just raw arrays of the similar fixed size, essentially
> > they should represent the same contents.  As they don't necessarily have
> > NUL-terminators, in both directions use plain memcpy() to copy their
> > contents.
> > 
> > Found by Linux Verification Center (linuxtesting.org).
> > 
> > Fixes: 04a91570ac67 ("ext4: implemet new ioctls to set and get superblock parameters")
> > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> 
> I agree there are some holes in the logic of 8ecb790ea8c3 ("ext4: avoid
> potential buffer over-read in parse_apply_sb_mount_options()") and
> consequently 04a91570ac67 may need fixing up as well. But I think the fixes
> should look differently. The clear intended use of s_mount_opts field is
> that it is at most 63 characters long with the last byte guaranteed to be
> 0. This is how userspace utilities use it and they complain if you try
> setting more than 63 characters long string. So I think strscpy_pad() use
> in ext4_ioctl_get_tune_sb() is actually fine (sizes of both buffers match).
> In ext4_sb_setparams() we should actually make sure userspace buffer is
> properly Nul-terminated and return error otherwise. And the buffer in
> parse_apply_sb_mount_options() should actually be only 64 bytes long to
> match the size of the source buffer at which point using strscpy_pad()
> becomes correct. How does that sound?
> 
> 								Honza

Thanks, Jan!  Sounds reasonable.  I was a bit confused by the __nonstring
declaration of s_mount_opts at e2fsprogs [1] but rechecked - tune2fs side
has always validated it to have at most 63 characters with the last one
being NUL in the corner case, just as you say.

ext4 kernel docs also specify it to be ASCIIZ string [2].

I'll rework the series.

[1]: https://github.com/tytso/e2fsprogs/blob/13dfdf2410648c361dfd49b28d7dbeac8a580532/lib/ext2fs/ext2_fs.h#L756
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/ext4/super.rst?id=58fdd8484c05a19942690008304228ad784771e9#n407