linux-ext4 - 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: <20251031182448-c6e06b81d18b41af5704f2ba-pchelkin@ispras>
Date: Fri, 31 Oct 2025 18:44:40 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Jan Kara <jack@...e.cz>
Cc: Theodore Ts'o <tytso@....edu>, 
	Andreas Dilger <adilger.kernel@...ger.ca>, "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 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@...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

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