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

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

[Pali Rohár]

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@kernel.org>
---

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.
---
 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;
-- 
2.20.1

Re: [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@kernel.org>

Reviewed-by: Stefan Roese <sr@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@denx.de

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

[Pali Rohár]

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@kernel.org>
> 
> Reviewed-by: Stefan Roese <sr@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.

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@denx.de

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

[Pali Rohár]

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@kernel.org>
> > 
> > Reviewed-by: Stefan Roese <sr@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: sr@denx.de

Re: [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@kernel.org>
>>>
>>> Reviewed-by: Stefan Roese <sr@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@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@denx.de

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

On Mon, May 30, 2022 at 08:11:26AM +0200, Stefan Roese wrote:
> 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@kernel.org>
> > > > 
> > > > Reviewed-by: Stefan Roese <sr@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/

I've put it in my queue, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

On Tue, May 17, 2022 at 10:45:28PM +0200, 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@kernel.org>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]