[PATCH v2] fs/squashfs: fix missing error checks causing Synchronous Abort

[Richard GENOUD]

Hi Allan,

Le 23/05/2026 à 16:48, Allan ELKAIM a écrit :
> sqfs_find_inode() returns NULL on failure, and sqfs_dir_offset()
> returns -EINVAL on failure. At 8 call sites in sqfs.c neither return
> value was validated before the result was cast to a struct pointer or
> used as an array index.
> 
> On squashfs images with large inode/directory tables (e.g. containing
> the tzdata timezone database), sqfs_dir_offset() fails to locate a
> start_block in the metadata-block position list and returns -EINVAL.
> The caller then computes &dir_table[-22], producing a faulting address
> of 0xffffffffffffffea and a Synchronous Abort on ARM64:
> 
>    "Synchronous Abort" handler, esr 0x96000004, far 0xffffffffffffffea
> 
> The "Error: invalid inode reference to directory table." message
> printed just before the abort confirms the sqfs_dir_offset() path.
> Additionally, sqfs_find_inode() can return NULL when the inode table
> scan encounters an unknown or malformed inode type, after which any
> dereference of the cast pointer crashes.
> 
> Add NULL guards after every sqfs_find_inode() call and negative-value
> guards after every sqfs_dir_offset() call so that any lookup failure
> propagates as -EINVAL rather than crashing.
> 
> Signed-off-by: Allan Elkaim <allan.elkaim at gmail.com>

Actually, when sending a v2, you should re-send the whole series (patches
0/2, 1/2, 2/2), as a new thread (not replying to v1).
Then, in cover letter (v0), you should describe the changes from v1 to v2
ex:
https://lore.kernel.org/u-boot/20260523163239.8EEA768BEB@verein.lst.de/T/
(example taken randomly)
(here, it would be
The subject should remain the same so that we don't get lost.
Also, it's nice to use --base argument on git format-patch so that we know
which commit this series is based on.

On a v2, you are also expected to gather the Reviewed-by/Acked-by (here,
it would be my reviewed-by on patch 1/2, and Miquel's Acked-by on both patches)

Some documentation about that:
https://elixir.bootlin.com/u-boot/v2026.07-rc2/source/doc/develop/sending_patches.rst

> ---
>   fs/squashfs/sqfs.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> index 1430e671..da8fbb5a 100644
> --- a/fs/squashfs/sqfs.c
> +++ b/fs/squashfs/sqfs.c
> @@ -472,12 +472,16 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list,
>   	/* Start by root inode */
>   	table = sqfs_find_inode(dirs->inode_table, le32_to_cpu(sblk->inodes),
>   				sblk->inodes, sblk->block_size);
> +	if (!table)
> +		return -EINVAL;
This is weird since this check is already in uboot tree, so this patch
doesn't apply.
(it was introduced by: 3fb1df1e57ee ("squashfs: Check sqfs_find_inode() return value"))

Please resent a v3 (whole series), with changes described and the commit
it's based on. (v2026.07-rc3 for example)

Thanks!

Regards,
Richard
>   
>   	dir = (struct squashfs_dir_inode *)table;
>   	ldir = (struct squashfs_ldir_inode *)table;
>   
>   	/* get directory offset in directory table */
>   	offset = sqfs_dir_offset(table, m_list, m_count);
> +	if (offset < 0)
> +		return offset;
>   	dirs->table = &dirs->dir_table[offset];
>   
>   	/* Setup directory header */
> @@ -527,6 +531,12 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list,
>   		/* Get reference to inode in the inode table */
>   		table = sqfs_find_inode(dirs->inode_table, new_inode_number,
>   					sblk->inodes, sblk->block_size);
> +		if (!table) {
> +			free(dirs->entry);
> +			dirs->entry = NULL;
> +			ret = -EINVAL;
> +			goto out;
> +		}
>   		dir = (struct squashfs_dir_inode *)table;
>   
>   		/* Check for symbolic link and inode type sanity */
> @@ -599,6 +609,12 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list,
>   
>   		/* Get dir. offset into the directory table */
>   		offset = sqfs_dir_offset(table, m_list, m_count);
> +		if (offset < 0) {
> +			free(dirs->entry);
> +			dirs->entry = NULL;
> +			ret = offset;
> +			goto out;
> +		}
>   		dirs->table = &dirs->dir_table[offset];
>   
>   		/* Copy directory header */
> @@ -623,6 +639,12 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list,
>   	}
>   
>   	offset = sqfs_dir_offset(table, m_list, m_count);
> +	if (offset < 0) {
> +		free(dirs->entry);
> +		dirs->entry = NULL;
> +		ret = offset;
> +		goto out;
> +	}
>   	dirs->table = &dirs->dir_table[offset];
>   
>   	if (get_unaligned_le16(&dir->inode_type) == SQFS_DIR_TYPE)
> @@ -1023,6 +1045,8 @@ int sqfs_readdir(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp)
>   	i_number = dirs->dir_header->inode_number + dirs->entry->inode_offset;
>   	ipos = sqfs_find_inode(dirs->inode_table, i_number, sblk->inodes,
>   			       sblk->block_size);
> +	if (!ipos)
> +		return -SQFS_STOP_READDIR;
>   
>   	base = (struct squashfs_base_inode *)ipos;
>   
> @@ -1379,6 +1403,10 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len,
>   	i_number = dirs->dir_header->inode_number + dirs->entry->inode_offset;
>   	ipos = sqfs_find_inode(dirs->inode_table, i_number, sblk->inodes,
>   			       sblk->block_size);
> +	if (!ipos) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>   
>   	base = (struct squashfs_base_inode *)ipos;
>   	switch (get_unaligned_le16(&base->inode_type)) {
> @@ -1629,6 +1657,11 @@ int sqfs_size(const char *filename, loff_t *size)
>   			       sblk->block_size);
>   	free(dirs->entry);
>   	dirs->entry = NULL;
> +	if (!ipos) {
> +		*size = 0;
> +		ret = -EINVAL;
> +		goto free_strings;
> +	}
>   
>   	base = (struct squashfs_base_inode *)ipos;
>   	switch (get_unaligned_le16(&base->inode_type)) {


-- 
Richard Genoud, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com