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 <p...@kernel.org>
> >
> > Reviewed-by: Stefan Roese <s...@denx.de>
Anyway, who is maintainer of fs / ubifs code and can take this bugfix patch?
> >
> > > ---
> > >
> > > 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: s...@denx.de
