[PATCH v2 3/3] fs/squashfs: fix dirs->entry leak on inode lookup failure

[Richard GENOUD]

Le 11/06/2026 à 11:43, Allan ELKAIM a écrit :
> In sqfs_search_dir(), when sqfs_find_inode() fails to locate the inode
> of a directory entry just returned by sqfs_readdir_nest(), the function
> returns directly while dirs->entry still holds the entry allocation,
> leaking it. The bare return also bypasses the regular error path.
> 
> Free the entry and leave through the out label instead, consistent
> with the other error paths in this function.
> 
> Signed-off-by: Allan ELKAIM <allan.elkaim at gmail.com>
> ---
> 
> Changes in v2:
> - New patch, fixing a pre-existing leak of the same kind as the ones
>    addressed in patch 2 (suggested by Richard Genoud's review)
> 
>   fs/squashfs/sqfs.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> index af32d008..df988774 100644
> --- a/fs/squashfs/sqfs.c
> +++ b/fs/squashfs/sqfs.c
> @@ -547,8 +547,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)
> -			return -EINVAL;
> +		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 */

Having a 2nd look at the whole function, it seems that there a lot of:
ret = -Exxx;
goto out;
without freeing dirs->entry

Like here:
			[...]
			if (++symlinknest == MAX_SYMLINK_NEST) {
				ret = -ELOOP;
				goto out;
			}

			sym = (struct squashfs_symlink_inode *)table;
			/* Get first j + 1 tokens */
			path = sqfs_concat_tokens(token_list, j + 1);
			if (!path) {
				ret = -ENOMEM;
				goto out;
			}
			/* Resolve for these tokens */
			target = sqfs_resolve_symlink(sym, path);
			if (!target) {
				ret = -ENOMEM;
				goto out;
			}
			[...]

It seems to me that we could keep free(dirs->entry):
- in the while (!sqfs_readdir_nest(dirsp, &dent)) loop
- at the end of the for (j = 0; j < token_count; j++) loop
- and before the recursive call to sqfs_search_dir()

And remove all the rest to add something like:
@@ -677,6 +677,10 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list,
                 memcpy(&dirs->i_ldir, ldir, sizeof(*ldir));
  
  out:
+       if (ret < 0) {
+               free(dirs->entry);
+               dirs->entry = NULL;
+       }
         free(res);
         free(rem);
         free(path);

(not tested)

What do you think?


Regards,
Richard