* [PATCH v1 0/2] fs/squashfs: fix symlink load failure on large images [not found] <"CACgNL-F2=KJtZ+gThpx_BuWsn6puqFxK0uLOmnABSS9=rRQmeQ@mail.gmail.com"> @ 2026-05-14 18:18 ` Allan ELKAIM 2026-05-14 18:18 ` [PATCH v1 1/2] fs/squashfs: fix heap exhaustion during symlink resolution Allan ELKAIM ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Allan ELKAIM @ 2026-05-14 18:18 UTC (permalink / raw) To: u-boot Cc: Miquel Raynal, Joao Marcos Costa, Thomas Petazzoni, Tom Rini, Allan ELKAIM sqfsload fails to load a file through a symlink when the squashfs image contains a large number of inodes (e.g. a rootfs that includes the tzdata timezone database). Root cause: sqfs_read_nest() resolves the symlink by calling itself recursively without first freeing the parent directory's inode and directory table buffers. This causes a temporary double allocation that can exhaust the U-Boot heap. When malloc() subsequently fails inside sqfs_read_directory_table(), the error goes undetected and sqfs_search_dir() is called with a NULL pos_list pointer, leading to: Error: invalid inode reference to directory table. Failed to load '/boot/Image' Patch 1 fixes the structural problem (temporary double allocation) and plugs the silent NULL pointer path in sqfs_read_directory_table(). Patch 2 adds the missing return-value checks on sqfs_dir_offset() that turn any residual lookup failure into a clean error propagation. Both patches are independent and can be reviewed separately. The bug was first observed on U-Boot v2024.01 and is still present on v2026.04. The patches have been tested on a Raspberry Pi CM4 running U-Boot v2026.04 (Yocto Scarthgap 5.0.17) with a 325 MB squashfs rootfs containing 22 517 inodes. The symlink /boot/Image -> Image-6.6.63-v8 now resolves successfully. This series addresses the bug reported at: https://lists.denx.de/pipermail/u-boot/2026-May/618533.html Allan ELKAIM (2): fs/squashfs: fix heap exhaustion during symlink resolution fs/squashfs: add sqfs_dir_offset() error checks fs/squashfs/sqfs.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) -- 2.53.0 base-commit: 88dc2788777babfd6322fa655df549a019aa1e69 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 1/2] fs/squashfs: fix heap exhaustion during symlink resolution 2026-05-14 18:18 ` [PATCH v1 0/2] fs/squashfs: fix symlink load failure on large images Allan ELKAIM @ 2026-05-14 18:18 ` Allan ELKAIM 2026-05-22 13:28 ` Richard GENOUD 2026-05-14 18:18 ` [PATCH v1 2/2] fs/squashfs: add sqfs_dir_offset() error checks Allan ELKAIM 2026-05-22 9:43 ` [PATCH v1 0/2] fs/squashfs: fix symlink load failure on large images Miquel Raynal 2 siblings, 1 reply; 9+ messages in thread From: Allan ELKAIM @ 2026-05-14 18:18 UTC (permalink / raw) To: u-boot Cc: Miquel Raynal, Joao Marcos Costa, Thomas Petazzoni, Tom Rini, Allan ELKAIM 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 Signed-off-by: Allan ELKAIM <allan.elkaim@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; -- 2.53.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] fs/squashfs: fix heap exhaustion during symlink resolution 2026-05-14 18:18 ` [PATCH v1 1/2] fs/squashfs: fix heap exhaustion during symlink resolution Allan ELKAIM @ 2026-05-22 13:28 ` Richard GENOUD 0 siblings, 0 replies; 9+ messages in thread From: Richard GENOUD @ 2026-05-22 13:28 UTC (permalink / raw) To: Allan ELKAIM, u-boot Cc: Miquel Raynal, Joao Marcos Costa, Thomas Petazzoni, Tom Rini 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@bootlin.com> > Signed-off-by: Allan ELKAIM <allan.elkaim@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! ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 2/2] fs/squashfs: add sqfs_dir_offset() error checks 2026-05-14 18:18 ` [PATCH v1 0/2] fs/squashfs: fix symlink load failure on large images Allan ELKAIM 2026-05-14 18:18 ` [PATCH v1 1/2] fs/squashfs: fix heap exhaustion during symlink resolution Allan ELKAIM @ 2026-05-14 18:18 ` Allan ELKAIM 2026-05-22 13:29 ` Richard GENOUD 2026-05-22 9:43 ` [PATCH v1 0/2] fs/squashfs: fix symlink load failure on large images Miquel Raynal 2 siblings, 1 reply; 9+ messages in thread From: Allan ELKAIM @ 2026-05-14 18:18 UTC (permalink / raw) To: u-boot Cc: Miquel Raynal, Joao Marcos Costa, Thomas Petazzoni, Tom Rini, Allan ELKAIM sqfs_dir_offset() returns a negative errno on failure, but three call sites in sqfs_search_dir() use the return value as an array index without checking for errors first. If the lookup fails, dirs->table is set to an invalid address, leading to undefined behavior. Add negative-value guards after each sqfs_dir_offset() call so that any lookup failure propagates cleanly as an error rather than producing incorrect results. Note: the corresponding sqfs_find_inode() NULL checks and the heap exhaustion fix during symlink resolution are applied in separate patches. Signed-off-by: Allan ELKAIM <allan.elkaim@gmail.com> --- fs/squashfs/sqfs.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c index 07e2bd82..430e9bac 100644 --- a/fs/squashfs/sqfs.c +++ b/fs/squashfs/sqfs.c @@ -496,6 +496,8 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, /* 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 */ @@ -627,6 +629,10 @@ 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) { + ret = offset; + goto out; + } dirs->table = &dirs->dir_table[offset]; /* Copy directory header */ @@ -651,6 +657,10 @@ 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) { + ret = offset; + goto out; + } dirs->table = &dirs->dir_table[offset]; if (get_unaligned_le16(&dir->inode_type) == SQFS_DIR_TYPE) -- 2.53.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] fs/squashfs: add sqfs_dir_offset() error checks 2026-05-14 18:18 ` [PATCH v1 2/2] fs/squashfs: add sqfs_dir_offset() error checks Allan ELKAIM @ 2026-05-22 13:29 ` Richard GENOUD 2026-05-23 14:35 ` Allan Elkaim 2026-05-23 14:48 ` [PATCH v2] fs/squashfs: fix missing error checks causing Synchronous Abort Allan ELKAIM 0 siblings, 2 replies; 9+ messages in thread From: Richard GENOUD @ 2026-05-22 13:29 UTC (permalink / raw) To: Allan ELKAIM, u-boot Cc: Miquel Raynal, Joao Marcos Costa, Thomas Petazzoni, Tom Rini Hi Allan, Le 14/05/2026 à 20:18, Allan ELKAIM a écrit : > sqfs_dir_offset() returns a negative errno on failure, but three > call sites in sqfs_search_dir() use the return value as an array > index without checking for errors first. If the lookup fails, > dirs->table is set to an invalid address, leading to undefined > behavior. > > Add negative-value guards after each sqfs_dir_offset() call so > that any lookup failure propagates cleanly as an error rather > than producing incorrect results. > > Note: the corresponding sqfs_find_inode() NULL checks and the > heap exhaustion fix during symlink resolution are applied in > separate patches. > > Signed-off-by: Allan ELKAIM <allan.elkaim@gmail.com> > --- > > fs/squashfs/sqfs.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c > index 07e2bd82..430e9bac 100644 > --- a/fs/squashfs/sqfs.c > +++ b/fs/squashfs/sqfs.c > @@ -496,6 +496,8 @@ static int sqfs_search_dir(struct squashfs_dir_stream *dirs, char **token_list, > > /* 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 */ > @@ -627,6 +629,10 @@ 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) { > + ret = offset; > + goto out; > + } Don't we need to: free(dirs->entry); dirs->entry = NULL; here? > dirs->table = &dirs->dir_table[offset]; > > /* Copy directory header */ > @@ -651,6 +657,10 @@ 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) { > + ret = offset; > + goto out; > + } same here? > dirs->table = &dirs->dir_table[offset]; > > if (get_unaligned_le16(&dir->inode_type) == SQFS_DIR_TYPE) Thanks, Regards, Richard ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] fs/squashfs: add sqfs_dir_offset() error checks 2026-05-22 13:29 ` Richard GENOUD @ 2026-05-23 14:35 ` Allan Elkaim 2026-05-23 14:48 ` [PATCH v2] fs/squashfs: fix missing error checks causing Synchronous Abort Allan ELKAIM 1 sibling, 0 replies; 9+ messages in thread From: Allan Elkaim @ 2026-05-23 14:35 UTC (permalink / raw) To: Richard GENOUD Cc: u-boot, Miquel Raynal, Joao Marcos Costa, Thomas Petazzoni, Tom Rini Hi Richard, Yes you are right, thank you for catching that ! I'll publish a v2. By the way, I hope I'm doing this the right way, it's the first time I contribute to a project via email. I'm more used to contribute through github/gitlab's PR/MR, if I am not doing it properly feel free to let me know ! Regards, Allan Le ven. 22 mai 2026, 15:29, Richard GENOUD <richard.genoud@bootlin.com> a écrit : > Hi Allan, > Le 14/05/2026 à 20:18, Allan ELKAIM a écrit : > > sqfs_dir_offset() returns a negative errno on failure, but three > > call sites in sqfs_search_dir() use the return value as an array > > index without checking for errors first. If the lookup fails, > > dirs->table is set to an invalid address, leading to undefined > > behavior. > > > > Add negative-value guards after each sqfs_dir_offset() call so > > that any lookup failure propagates cleanly as an error rather > > than producing incorrect results. > > > > Note: the corresponding sqfs_find_inode() NULL checks and the > > heap exhaustion fix during symlink resolution are applied in > > separate patches. > > > > Signed-off-by: Allan ELKAIM <allan.elkaim@gmail.com> > > --- > > > > fs/squashfs/sqfs.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c > > index 07e2bd82..430e9bac 100644 > > --- a/fs/squashfs/sqfs.c > > +++ b/fs/squashfs/sqfs.c > > @@ -496,6 +496,8 @@ static int sqfs_search_dir(struct > squashfs_dir_stream *dirs, char **token_list, > > > > /* 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 */ > > @@ -627,6 +629,10 @@ 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) { > > + ret = offset; > > + goto out; > > + } > Don't we need to: > free(dirs->entry); > dirs->entry = NULL; > here? > > > dirs->table = &dirs->dir_table[offset]; > > > > /* Copy directory header */ > > @@ -651,6 +657,10 @@ 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) { > > + ret = offset; > > + goto out; > > + } > same here? > > > dirs->table = &dirs->dir_table[offset]; > > > > if (get_unaligned_le16(&dir->inode_type) == SQFS_DIR_TYPE) > > Thanks, > Regards, > Richard > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] fs/squashfs: fix missing error checks causing Synchronous Abort 2026-05-22 13:29 ` Richard GENOUD 2026-05-23 14:35 ` Allan Elkaim @ 2026-05-23 14:48 ` Allan ELKAIM 2026-05-26 7:35 ` Richard GENOUD 1 sibling, 1 reply; 9+ messages in thread From: Allan ELKAIM @ 2026-05-23 14:48 UTC (permalink / raw) To: u-boot Cc: richard.genoud, jmcosta944, miquel.raynal, thomas.petazzoni, trini, Allan ELKAIM 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@gmail.com> --- 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; 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)) { -- 2.53.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fs/squashfs: fix missing error checks causing Synchronous Abort 2026-05-23 14:48 ` [PATCH v2] fs/squashfs: fix missing error checks causing Synchronous Abort Allan ELKAIM @ 2026-05-26 7:35 ` Richard GENOUD 0 siblings, 0 replies; 9+ messages in thread From: Richard GENOUD @ 2026-05-26 7:35 UTC (permalink / raw) To: Allan ELKAIM, u-boot; +Cc: jmcosta944, miquel.raynal, thomas.petazzoni, trini 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@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 0/2] fs/squashfs: fix symlink load failure on large images 2026-05-14 18:18 ` [PATCH v1 0/2] fs/squashfs: fix symlink load failure on large images Allan ELKAIM 2026-05-14 18:18 ` [PATCH v1 1/2] fs/squashfs: fix heap exhaustion during symlink resolution Allan ELKAIM 2026-05-14 18:18 ` [PATCH v1 2/2] fs/squashfs: add sqfs_dir_offset() error checks Allan ELKAIM @ 2026-05-22 9:43 ` Miquel Raynal 2 siblings, 0 replies; 9+ messages in thread From: Miquel Raynal @ 2026-05-22 9:43 UTC (permalink / raw) To: Allan ELKAIM Cc: u-boot, Joao Marcos Costa, Thomas Petazzoni, Tom Rini, Richard Genoud Hello Allan, On 14/05/2026 at 20:18:50 +02, Allan ELKAIM <allan.elkaim@gmail.com> wrote: > sqfsload fails to load a file through a symlink when the squashfs > image contains a large number of inodes (e.g. a rootfs that includes > the tzdata timezone database). > > Root cause: sqfs_read_nest() resolves the symlink by calling itself > recursively without first freeing the parent directory's inode and > directory table buffers. This causes a temporary double allocation > that can exhaust the U-Boot heap. When malloc() subsequently fails > inside sqfs_read_directory_table(), the error goes undetected and > sqfs_search_dir() is called with a NULL pos_list pointer, leading to: > > Error: invalid inode reference to directory table. > Failed to load '/boot/Image' > > Patch 1 fixes the structural problem (temporary double allocation) > and plugs the silent NULL pointer path in sqfs_read_directory_table(). > Patch 2 adds the missing return-value checks on sqfs_dir_offset() that > turn any residual lookup failure into a clean error propagation. > > Both patches are independent and can be reviewed separately. > > The bug was first observed on U-Boot v2024.01 and is still present > on v2026.04. The patches have been tested on a Raspberry Pi CM4 > running U-Boot v2026.04 (Yocto Scarthgap 5.0.17) with a 325 MB > squashfs rootfs containing 22 517 inodes. The symlink > /boot/Image -> Image-6.6.63-v8 now resolves successfully. > > This series addresses the bug reported at: > https://lists.denx.de/pipermail/u-boot/2026-May/618533.html I haven't looked very deeply but changes look good. Acked-by: Miquel Raynal <miquel.raynal@bootlin.com> I am adding Richard in case he wants to have a look. Thanks, Miquèl ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-05-26 7:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <"CACgNL-F2=KJtZ+gThpx_BuWsn6puqFxK0uLOmnABSS9=rRQmeQ@mail.gmail.com">
2026-05-14 18:18 ` [PATCH v1 0/2] fs/squashfs: fix symlink load failure on large images Allan ELKAIM
2026-05-14 18:18 ` [PATCH v1 1/2] fs/squashfs: fix heap exhaustion during symlink resolution Allan ELKAIM
2026-05-22 13:28 ` Richard GENOUD
2026-05-14 18:18 ` [PATCH v1 2/2] fs/squashfs: add sqfs_dir_offset() error checks Allan ELKAIM
2026-05-22 13:29 ` Richard GENOUD
2026-05-23 14:35 ` Allan Elkaim
2026-05-23 14:48 ` [PATCH v2] fs/squashfs: fix missing error checks causing Synchronous Abort Allan ELKAIM
2026-05-26 7:35 ` Richard GENOUD
2026-05-22 9:43 ` [PATCH v1 0/2] fs/squashfs: fix symlink load failure on large images Miquel Raynal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.