Re: [PATCH] squashfs: bound the fragment offset stored in an inode

[Phillip Lougher]

On 03/08/2026 04:27, Yuejie Shi wrote:
> Both the REG and LREG inode readers take the fragment offset verbatim
> off disk:
>
> 	frag = le32_to_cpu(sqsh_ino->fragment);
> 	if (frag != SQUASHFS_INVALID_FRAG) {
> 		/*
> 		 * the file cannot have a fragment (tailend) and have a
> 		 * file size a multiple of the block size
> 		 */
> 		if ((inode->i_size & (msblk->block_size - 1)) == 0) { ... }
> 		frag_offset = le32_to_cpu(sqsh_ino->offset);
> 		...
> 	}
> 	...
> 	squashfs_i(inode)->fragment_offset = frag_offset;
>
> The check added by commit 9ee94bfbe930 ("Squashfs: add additional inode
> sanity checking") is about i_size, not about offset, and nothing else
> looks at offset at all.  It reaches squashfs_copy_data() unchanged, for
> instance from squashfs_readahead_fragment():
>
> 	bytes = squashfs_copy_data(addr, buffer, offset +
> 			squashfs_i(inode)->fragment_offset, avail);
>
> where the parameter is a signed int:
>
> 	while (offset < entry->length) {
> 		void *buff = entry->data[offset / PAGE_SIZE]
> 				+ (offset % PAGE_SIZE);

NACK.

You have found a bug, but your fix is completely wrong.

Also curiously this got raised as an issue with Squashfs-tools recently
https://github.com/plougher/squashfs-tools/issues/398

and I fixed it in a commit last week
https://github.com/plougher/squashfs-tools/commit/88c47b58df408e8106814079e6979a454269018c

Whether AI is involved or not, this is part of the growing trend to
have multiple reports of the same or related issues by different people,
within days or weeks of each other.  Which is making it more and more
difficult and time consuming to deal with them fairly and quickly.

Comments below

> ---
>   fs/squashfs/inode.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c
> index 82b687414e65..4872b28f1b77 100644
> --- a/fs/squashfs/inode.c
> +++ b/fs/squashfs/inode.c
> @@ -156,6 +156,16 @@ int squashfs_read_inode(struct inode *inode, long long
>   				goto failed_read;
>   			}
>   			frag_offset = le32_to_cpu(sqsh_ino->offset);
> +			/*
> +			 * the tailend has to lie within the uncompressed
> +			 * fragment block, which is at most block_size bytes
> +			 */
> +			if (frag_offset +
> +			    (inode->i_size & (msblk->block_size - 1)) >
> +					msblk->block_size) {
> +				err = -EINVAL;
> +				goto failed_read;
> +			}

This is remarkably like the fix I put into Squashfs-tools last week.  Except I
put it in the file write path, when the fragment is about to be written out to
disk.

Why? Because the fragment is only accessed when you read data, it isn't necessary
to create the inode.  Putting the check in the inode instantiation means you
won't be able to use Unsquashfs to do a file listing (unsquashfs -l example.sqsh).

Exactly the same here.  You are prematurely rejecting something before you get to
where the problem is.  Doing it here means you can't stat the file, and if the
tailend is say the last 100 bytes of a 3 Megabyte file, you now can't read any of
it, rather than just the last 100 bytes.

It is also using a hammer to crush a nut, complete over-kill. Cases where
frag_offset (or the combination of frag_offset and the tailend size) go beyond
the end of the buffer are rejected when the tailend is copied.  The only thing which
isn't at that point is a negative offset, and that can be checked then.

Plus this "fix" doesn't even do what it pretends to do either, which is to
prevent accesses beyond the end of the fragment block.  A fragment block may
be a maximum of a block size, but, it is almost always smaller, and can be anything
from 1 byte in size to the block size.

So if the fragment block decompresses to 5 bytes, the fact the frag_offset has
been bounded  to block size is completely useless.  It is the underlying code
which prevents access beyond the actual size of the fragment block when the
tailend is copied.

So NACK.

Phillip
  >   			frag_size = squashfs_frag_lookup(sb, frag, &frag_blk);
>   			if (frag_size < 0) {
>   				err = frag_size;
> @@ -212,6 +222,16 @@ int squashfs_read_inode(struct inode *inode, long long
>   				goto failed_read;
>   			}
>   			frag_offset = le32_to_cpu(sqsh_ino->offset);
> +			/*
> +			 * the tailend has to lie within the uncompressed
> +			 * fragment block, which is at most block_size bytes
> +			 */
> +			if (frag_offset +
> +			    (inode->i_size & (msblk->block_size - 1)) >
> +					msblk->block_size) {
> +				err = -EINVAL;
> +				goto failed_read;
> +			}
>   			frag_size = squashfs_frag_lookup(sb, frag, &frag_blk);
>   			if (frag_size < 0) {
>   				err = frag_size;
> --
> 2.51.0