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

[Stefan Roese]

Added Tom to Cc...

On 23.05.22 11:25, Pali Rohár wrote:
> On Thursday 19 May 2022 10:04:31 Pali Rohár wrote:
>> On Thursday 19 May 2022 07:01:19 Stefan Roese wrote:
>>> 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>
> 
> Anyway, who is maintainer of fs / ubifs code and can take this bugfix patch?

Tom, could you please pick this patch up? I've seen that you've already
assigned it to yourself in patchwork:

http://patchwork.ozlabs.org/project/uboot/patch/20220517204528.7277-1-pali@kernel.org/

Thanks,
Stefan

>>>
>>>> ---
>>>>
>>>> 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.
>>
>> It is used on Turris 1.x (powerpc).
>>
>>> 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

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