linux-kernel - Re: [PATCH 1/2] ext4: fix up copying of mount

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <yq6rbx54jt4btntsh37urd6u63wwcd3lyhovbrm6w7occaveea@riljfkx5jmhi>
Date: Thu, 30 Oct 2025 12:32:01 +0100
From: Jan Kara <jack@...e.cz>
To: Fedor Pchelkin <pchelkin@...ras.ru>
Cc: Theodore Ts'o <tytso@....edu>, 
	Andreas Dilger <adilger.kernel@...ger.ca>, Jan Kara <jack@...e.cz>, "Darrick J. Wong" <djwong@...nel.org>, 
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org, Kees Cook <kees@...nel.org>, 
	lvc-project@...uxtesting.org
Subject: Re: [PATCH 1/2] ext4: fix up copying of mount_opts in superblock
 tuning ioctls

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@...ras.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@...e.com>
SUSE Labs, CR