[PATCH v1 1/2] fs/squashfs: fix heap exhaustion during symlink resolution

[Richard GENOUD]

Le 14/05/2026 à 20:18, Allan ELKAIM a écrit :
> When sqfs_read_nest() encounters a symlink it resolves it by calling
> itself recursively. In the unfixed code this looks like:
> 
>    // dirsp is open: inode_table + dir_table still on heap
>    resolved = sqfs_resolve_symlink(symlink, filename);
>    ret = sqfs_read_nest(resolved, ...); // recursive: allocates a new
>                                         // inode_table + dir_table pair
>    free(resolved);
>    goto out;
>    // out: sqfs_closedir(dirsp) <- parent tables freed HERE, too late
> 
> There is no permanent leak: the parent's tables are freed at the
> out: label once the recursive call returns. However, for the entire
> duration of the recursive call both the parent's inode_table +
> dir_table and the child's inode_table + dir_table are live on the
> heap simultaneously. On large squashfs images these tables can be
> significant in size, and this temporary double allocation may exhaust
> the heap budget.
> 
> A superficial workaround would be to increase CONFIG_SYS_MALLOC_LEN,
> but that wastes memory on all boards and does not address the
> structural problem. The correct fix is to change the freeing order:
> release the parent directory's resources before recursing. This way
> only one set of inode and directory tables is live at any given time,
> halving the peak heap usage during symlink resolution.
> 
> When heap exhaustion does occur and malloc returns NULL for dir_table
> or pos_list inside sqfs_read_directory_table(), the failure is
> currently silent and cascading:
> 
>    - metablks_count is not reset to -1 before the goto out, so the
>      function returns a positive block count alongside a NULL pointer.
>    - sqfs_opendir_nest() does not detect the failure (it only checks
>      metablks_count < 1) and calls sqfs_search_dir() with m_list=NULL.
>    - sqfs_dir_offset() iterates over m_list[0..n], reading from
>      addresses 0x0, 0x4, 0x8, ... None of those values match the
>      inode's start_block, so the function returns -EINVAL.
>    - The error propagates up as a load failure with no indication
>      that the root cause was heap exhaustion:
> 
>        Error: invalid inode reference to directory table.
>        Failed to load '<symlink path>'
> 
> Two fixes:
> 1. In sqfs_read_directory_table(), set metablks_count = -1 whenever
>     malloc fails after sqfs_count_metablks() returns a positive value,
>     so that the caller's "metablks_count < 1" check correctly detects
>     the failure and avoids calling sqfs_search_dir() with a NULL
>     pos_list.
> 2. In sqfs_read_nest() and sqfs_size_nest(), call sqfs_closedir() on
>     the parent dirsp before the recursive call so that the parent's
>     inode and directory tables are freed before the child allocates
>     its own. Only one set of tables is then live at any given time,
>     halving peak heap usage during symlink resolution.
> 
> Link: https://lists.denx.de/pipermail/u-boot/2026-May/618533.html
> 
This looks great!

Reviewed-by: Richard Genoud <richard.genoud at bootlin.com>
> Signed-off-by: Allan ELKAIM <allan.elkaim at gmail.com>
> ---
> 
>   fs/squashfs/sqfs.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> index 9cb8b4af..07e2bd82 100644
> --- a/fs/squashfs/sqfs.c
> +++ b/fs/squashfs/sqfs.c
> @@ -853,12 +853,16 @@ static int sqfs_read_directory_table(unsigned char **dir_table, u32 **pos_list)
>   		goto out;
>   
>   	*dir_table = malloc(metablks_count * SQFS_METADATA_BLOCK_SIZE);
> -	if (!*dir_table)
> +	if (!*dir_table) {
> +		metablks_count = -1;
>   		goto out;
> +	}
>   
>   	*pos_list = malloc(metablks_count * sizeof(u32));
> -	if (!*pos_list)
> +	if (!*pos_list) {
> +		metablks_count = -1;
>   		goto out;
> +	}
>   
>   	ret = sqfs_get_metablk_pos(*pos_list, dtb, table_offset,
>   				   metablks_count);
> @@ -1473,6 +1477,15 @@ static int sqfs_read_nest(const char *filename, void *buf, loff_t offset,
>   
>   		symlink = (struct squashfs_symlink_inode *)ipos;
>   		resolved = sqfs_resolve_symlink(symlink, filename);
> +		/*
> +		 * Free the parent directory resources before recursing so that
> +		 * the recursive call can allocate its own inode and directory
> +		 * tables without exhausting the heap.
> +		 */
> +		free(dirs->entry);
> +		dirs->entry = NULL;
> +		sqfs_closedir(dirsp);
> +		dirsp = NULL;
>   		ret = sqfs_read_nest(resolved, buf, offset, len, actread);
>   		free(resolved);
>   		goto out;
> @@ -1731,6 +1744,11 @@ static int sqfs_size_nest(const char *filename, loff_t *size)
>   
>   		symlink = (struct squashfs_symlink_inode *)ipos;
>   		resolved = sqfs_resolve_symlink(symlink, filename);
> +		/*
> +		 * Free the parent directory resources before recursing.
> +		 */
> +		sqfs_closedir(dirsp);
> +		dirsp = NULL;
>   		ret = sqfs_size(resolved, size);
>   		free(resolved);
>   		break;

Thanks!