linux-kernel - Re: [PATCH V7] squashfs: Add symlink size check in squash_read

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240805014037.GF5334@ZenIV>
Date: Mon, 5 Aug 2024 02:40:37 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Lizhi Xu <lizhi.xu@...driver.com>
Cc: brauner@...nel.org, jack@...e.cz, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, phillip@...ashfs.org.uk,
	squashfs-devel@...ts.sourceforge.net,
	syzbot+24ac24ff58dc5b0d26b9@...kaller.appspotmail.com,
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH V7] squashfs: Add symlink size check in squash_read_inode

On Mon, Aug 05, 2024 at 09:02:31AM +0800, Lizhi Xu wrote:
> On Sun, 4 Aug 2024 22:20:34 +0100, Al Viro wrote:
> > Alternatively, just check ->i_size after assignment.  loff_t is
> > always a 64bit signed; le32_to_cpu() returns 32bit unsigned.
> > Conversion from u32 to s64 is always going to yield a non-negative
> > result; comparison with PAGE_SIZE is all you need there.

> It is int overflow, not others. 

Excuse me, what?

> Please see my V7 patch,
> Link: https://lore.kernel.org/all/20240803074349.3599957-1-lizhi.xu@windriver.com/

I have seen your patch.  Integer overflow has nothing to do with
the problem here.

Please, show me an unsigned int value N such that

_Bool mismatch(unsigned int N)
{
	u32 v32 = N;
	loff_t v64 = N;

	return (v32 > PAGE_SIZE) != (v64 > PAGE_SIZE);
}

would yield true if passed that value as an argument.

Note that assignment of le32_to_cpu() result to inode->i_size
does conversion from unsigned 32bit integer type to a signed 64bit
integer type.  Since the range of the former fits into the range of the
latter, conversion preserves value.  In other words, possible values
of inode->i_size after such assignment are from 0 to (loff_t)0xfffffff
and (inode->i_size > PAGE_SIZE) is true in exactly the same cases when
(symlink_size > PAGE_SIZE) is true with your patch.

Again, on all architectures inode->i_size is capable of representing
all values in range 0..4G-1 (for rather obvious reasons - we want the
kernel to be able to work with files larger than 4Gb).  There is
no wraparound of any kind on that assignment.

See https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf,
in particular sections 6.5.16.1[2] and 6.3.1.3[1]