[PATCH] ubifs: Fix lockup/crash when reading files

[Stefan Roese]

On 17.05.22 22:45, Pali Rohár wrote:
> Commit b1a14f8a1c2e ("UBIFS: Change ubifsload to not read beyond the
> requested size") added optimization to do not read more bytes than it is
> really needed. But this commit introduced incorrect handling of the hole at
> the end of file. This logic cause U-Boot to crash or lockup when trying to
> read from the ubifs filesystem.
> 
> When read_block() call returns -ENOENT error (not an error, but the hole)
> then dn-> structure is not filled and contain garbage. So using of dn->size
> for memcpy() argument cause that U-Boot tries to copy unspecified amount of
> bytes from possible unmapped memory. Which randomly cause lockup of P2020
> CPU.
> 
> Fix this issue by copying UBIFS_BLOCK_SIZE bytes from read buffer when
> dn->size is not available. UBIFS_BLOCK_SIZE is the size of the buffer
> itself and read_block() fills buffer by zeros when it returns -ENOENT.
> 
> This patch fixes ubifsload on P2020.
> 
> Fixes: b1a14f8a1c2e ("UBIFS: Change ubifsload to not read beyond the requested size")
> Signed-off-by: Pali Rohár <pali at kernel.org>

Reviewed-by: Stefan Roese <sr at denx.de>

> ---
> 
> Stefan, could you please look at this patch? Mentioned commit was
> introduced by you more than 11 years ago. I'm surprised that nobody hit
> this issue yet, but it looks like older U-Boot versions somehow filled
> small garbage number into dn->size and did not cause U-Boot
> lockup/crash.

So long ago. I don't really remember the details. IIRC, I introduced the
UBIFS support for some MIPS based board at that time. My best assumption
is, that UBIFS is rarely used in U-Boot in general.

Thanks,
Stefan

> ---
>   fs/ubifs/ubifs.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> index d6be5c947d7e..d3026e310168 100644
> --- a/fs/ubifs/ubifs.c
> +++ b/fs/ubifs/ubifs.c
> @@ -771,40 +771,42 @@ static int do_readpage(struct ubifs_info *c, struct inode *inode,
>   				buff = malloc_cache_aligned(UBIFS_BLOCK_SIZE);
>   				if (!buff) {
>   					printf("%s: Error, malloc fails!\n",
>   					       __func__);
>   					err = -ENOMEM;
>   					break;
>   				}
>   
>   				/* Read block-size into temp buffer */
>   				ret = read_block(inode, buff, block, dn);
>   				if (ret) {
>   					err = ret;
>   					if (err != -ENOENT) {
>   						free(buff);
>   						break;
>   					}
>   				}
>   
>   				if (last_block_size)
>   					dlen = last_block_size;
> +				else if (ret)
> +					dlen = UBIFS_BLOCK_SIZE;
>   				else
>   					dlen = le32_to_cpu(dn->size);
>   
>   				/* Now copy required size back to dest */
>   				memcpy(addr, buff, dlen);
>   
>   				free(buff);
>   			} else {
>   				ret = read_block(inode, addr, block, dn);
>   				if (ret) {
>   					err = ret;
>   					if (err != -ENOENT)
>   						break;
>   				}
>   			}
>   		}
>   		if (++i >= UBIFS_BLOCKS_PER_PAGE)
>   			break;
>   		block += 1;
>   		addr += UBIFS_BLOCK_SIZE;

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de