[PATCH] fs/squashfs: fix reading of fragmented files

[PATCH] fs/squashfs: fix reading of fragmented files

[Joao Marcos Costa]

The fragmented files were not correctly read because of two issues:

- The squashfs_file_info struct has a field named 'comp', which tells if
the file's fragment is compressed or not. This field was always set to
'true' in sqfs_get_regfile_info and sqfs_get_lregfile_info. It should
actually take sqfs_frag_lookup's return value. This patch addresses
these two assignments.

- In sqfs_read, the fragments (compressed or not) were copied to the
output buffer through a for loop which was reading data at the wrong
offset. Replace these loops by equivalent calls to memcpy, with the
right parameters.

I tested this patch by comparing the MD5 checksum of a few fragmented
files with the respective md5sum output in sandbox, considering both
compressed and uncompressed fragments.

Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
---
 fs/squashfs/sqfs.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
index 29805c3c6f..22ef4f2691 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -1253,7 +1253,7 @@ static int sqfs_get_regfile_info(struct squashfs_reg_inode *reg,
 				       fentry);
 		if (ret < 0)
 			return -EINVAL;
-		finfo->comp = true;
+		finfo->comp = ret;
 		if (fentry->size < 1 || fentry->start == 0x7FFFFFFF)
 			return -EINVAL;
 	} else {
@@ -1291,7 +1291,7 @@ static int sqfs_get_lregfile_info(struct squashfs_lreg_inode *lreg,
 				       fentry);
 		if (ret < 0)
 			return -EINVAL;
-		finfo->comp = true;
+		finfo->comp = ret;
 		if (fentry->size < 1 || fentry->start == 0x7FFFFFFF)
 			return -EINVAL;
 	} else {
@@ -1547,20 +1547,16 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len,
 			goto out;
 		}
 
-		for (j = *actread; j < finfo.size; j++) {
-			memcpy(buf + j, &fragment_block[finfo.offset + j], 1);
-			(*actread)++;
-		}
+		memcpy(buf + *actread, &fragment_block[finfo.offset], finfo.size - *actread);
+		*actread = finfo.size;
 
 		free(fragment_block);
 
 	} else if (finfo.frag && !finfo.comp) {
 		fragment_block = (void *)fragment + table_offset;
 
-		for (j = *actread; j < finfo.size; j++) {
-			memcpy(buf + j, &fragment_block[finfo.offset + j], 1);
-			(*actread)++;
-		}
+		memcpy(buf + *actread, &fragment_block[finfo.offset], finfo.size - *actread);
+		*actread = finfo.size;
 	}
 
 out:
-- 
2.25.1

[PATCH] fs/squashfs: fix reading of fragmented files

[Richard Genoud]

Le 17/05/2021 ? 23:20, Joao Marcos Costa a ?crit?:
> The fragmented files were not correctly read because of two issues:
> 
> - The squashfs_file_info struct has a field named 'comp', which tells if
> the file's fragment is compressed or not. This field was always set to
> 'true' in sqfs_get_regfile_info and sqfs_get_lregfile_info. It should
> actually take sqfs_frag_lookup's return value. This patch addresses
> these two assignments.
> 
> - In sqfs_read, the fragments (compressed or not) were copied to the
> output buffer through a for loop which was reading data at the wrong
> offset. Replace these loops by equivalent calls to memcpy, with the
> right parameters.
> 
> I tested this patch by comparing the MD5 checksum of a few fragmented
> files with the respective md5sum output in sandbox, considering both
> compressed and uncompressed fragments.
> 
> Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
Tested-by: Richard Genoud <richard.genoud@posteo.net>

Tested it on real hardware, with mksquashfs -always-use-fragments, loading a kernel (fit) from squashfs.
It wasn't working before the patch, and works after.

Thanks !
> ---
>   fs/squashfs/sqfs.c | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> index 29805c3c6f..22ef4f2691 100644
> --- a/fs/squashfs/sqfs.c
> +++ b/fs/squashfs/sqfs.c
> @@ -1253,7 +1253,7 @@ static int sqfs_get_regfile_info(struct squashfs_reg_inode *reg,
>   				       fentry);
>   		if (ret < 0)
>   			return -EINVAL;
> -		finfo->comp = true;
> +		finfo->comp = ret;
>   		if (fentry->size < 1 || fentry->start == 0x7FFFFFFF)
>   			return -EINVAL;
>   	} else {
> @@ -1291,7 +1291,7 @@ static int sqfs_get_lregfile_info(struct squashfs_lreg_inode *lreg,
>   				       fentry);
>   		if (ret < 0)
>   			return -EINVAL;
> -		finfo->comp = true;
> +		finfo->comp = ret;
>   		if (fentry->size < 1 || fentry->start == 0x7FFFFFFF)
>   			return -EINVAL;
>   	} else {
> @@ -1547,20 +1547,16 @@ int sqfs_read(const char *filename, void *buf, loff_t offset, loff_t len,
>   			goto out;
>   		}
>   
> -		for (j = *actread; j < finfo.size; j++) {
> -			memcpy(buf + j, &fragment_block[finfo.offset + j], 1);
> -			(*actread)++;
> -		}
> +		memcpy(buf + *actread, &fragment_block[finfo.offset], finfo.size - *actread);
> +		*actread = finfo.size;
>   
>   		free(fragment_block);
>   
>   	} else if (finfo.frag && !finfo.comp) {
>   		fragment_block = (void *)fragment + table_offset;
>   
> -		for (j = *actread; j < finfo.size; j++) {
> -			memcpy(buf + j, &fragment_block[finfo.offset + j], 1);
> -			(*actread)++;
> -		}
> +		memcpy(buf + *actread, &fragment_block[finfo.offset], finfo.size - *actread);
> +		*actread = finfo.size;
>   	}
>   
>   out:
> 

[PATCH] fs/squashfs: fix reading of fragmented files

[João Marcos Costa]

Hello,

Em qui., 20 de mai. de 2021 ?s 06:54, Richard Genoud <
richard.genoud@posteo.net> escreveu:

> Le 17/05/2021 ? 23:20, Joao Marcos Costa a ?crit :
> > The fragmented files were not correctly read because of two issues:
> >
> > - The squashfs_file_info struct has a field named 'comp', which tells if
> > the file's fragment is compressed or not. This field was always set to
> > 'true' in sqfs_get_regfile_info and sqfs_get_lregfile_info. It should
> > actually take sqfs_frag_lookup's return value. This patch addresses
> > these two assignments.
> >
> > - In sqfs_read, the fragments (compressed or not) were copied to the
> > output buffer through a for loop which was reading data at the wrong
> > offset. Replace these loops by equivalent calls to memcpy, with the
> > right parameters.
> >
> > I tested this patch by comparing the MD5 checksum of a few fragmented
> > files with the respective md5sum output in sandbox, considering both
> > compressed and uncompressed fragments.
> >
> > Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
> Tested-by: Richard Genoud <richard.genoud@posteo.net>
>
> Tested it on real hardware, with mksquashfs -always-use-fragments, loading
> a kernel (fit) from squashfs.
> It wasn't working before the patch, and works after.
>
> Thanks !
>

Great! Thanks for testing!

-- 
Atenciosamente,
Jo?o Marcos Costa

www.linkedin.com/in/jmarcoscosta/
https://github.com/jmarcoscosta

Re: [PATCH] fs/squashfs: fix reading of fragmented files

[Miquel Raynal]

Hi Joao,

Joao Marcos Costa <jmcosta944@gmail.com> wrote on Mon, 17 May 2021
18:20:38 -0300:

> The fragmented files were not correctly read because of two issues:
> 
> - The squashfs_file_info struct has a field named 'comp', which tells if
> the file's fragment is compressed or not. This field was always set to
> 'true' in sqfs_get_regfile_info and sqfs_get_lregfile_info. It should
> actually take sqfs_frag_lookup's return value. This patch addresses
> these two assignments.
> 
> - In sqfs_read, the fragments (compressed or not) were copied to the
> output buffer through a for loop which was reading data at the wrong
> offset. Replace these loops by equivalent calls to memcpy, with the
> right parameters.

Good idea to get rid of these memcpy of 1 byte :)

> I tested this patch by comparing the MD5 checksum of a few fragmented
> files with the respective md5sum output in sandbox, considering both
> compressed and uncompressed fragments.
> 
> Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

But next time, when you fix two issues (even if they fix the same
feature) please provide two patches ;)

Thanks,
Miquèl

Re: [PATCH] fs/squashfs: fix reading of fragmented files

[João Marcos Costa]

Hello, Miquèl

Em qua., 26 de mai. de 2021 às 04:52, Miquel Raynal <
miquel.raynal@bootlin.com> escreveu:

> Hi Joao,
>
> Joao Marcos Costa <jmcosta944@gmail.com> wrote on Mon, 17 May 2021
> 18:20:38 -0300:
>
> > The fragmented files were not correctly read because of two issues:
> >
> > - The squashfs_file_info struct has a field named 'comp', which tells if
> > the file's fragment is compressed or not. This field was always set to
> > 'true' in sqfs_get_regfile_info and sqfs_get_lregfile_info. It should
> > actually take sqfs_frag_lookup's return value. This patch addresses
> > these two assignments.
> >
> > - In sqfs_read, the fragments (compressed or not) were copied to the
> > output buffer through a for loop which was reading data at the wrong
> > offset. Replace these loops by equivalent calls to memcpy, with the
> > right parameters.
>
> Good idea to get rid of these memcpy of 1 byte :)
>
> > I tested this patch by comparing the MD5 checksum of a few fragmented
> > files with the respective md5sum output in sandbox, considering both
> > compressed and uncompressed fragments.
> >
> > Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> But next time, when you fix two issues (even if they fix the same
> feature) please provide two patches ;)
>
> Thanks,
> Miquèl
>

Will do! Thanks for the review!


Best regards,
Joao

-- 
Atenciosamente,
João Marcos Costa

www.linkedin.com/in/jmarcoscosta/
https://github.com/jmarcoscosta

Re: [PATCH] fs/squashfs: fix reading of fragmented files

[João Marcos Costa]

Hello, everyone

Em qua., 26 de mai. de 2021 às 09:35, João Marcos Costa <
jmcosta944@gmail.com> escreveu:

> Hello, Miquèl
>
> Em qua., 26 de mai. de 2021 às 04:52, Miquel Raynal <
> miquel.raynal@bootlin.com> escreveu:
>
>> Hi Joao,
>>
>> Joao Marcos Costa <jmcosta944@gmail.com> wrote on Mon, 17 May 2021
>> 18:20:38 -0300:
>>
>> > The fragmented files were not correctly read because of two issues:
>> >
>> > - The squashfs_file_info struct has a field named 'comp', which tells if
>> > the file's fragment is compressed or not. This field was always set to
>> > 'true' in sqfs_get_regfile_info and sqfs_get_lregfile_info. It should
>> > actually take sqfs_frag_lookup's return value. This patch addresses
>> > these two assignments.
>> >
>> > - In sqfs_read, the fragments (compressed or not) were copied to the
>> > output buffer through a for loop which was reading data at the wrong
>> > offset. Replace these loops by equivalent calls to memcpy, with the
>> > right parameters.
>>
>> Good idea to get rid of these memcpy of 1 byte :)
>>
>> > I tested this patch by comparing the MD5 checksum of a few fragmented
>> > files with the respective md5sum output in sandbox, considering both
>> > compressed and uncompressed fragments.
>> >
>> > Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
>>
>> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>
>> But next time, when you fix two issues (even if they fix the same
>> feature) please provide two patches ;)
>>
>> Thanks,
>> Miquèl
>>
>
>
Any updates on this patch review?

Thanks!


-- 
Atenciosamente,
João Marcos Costa

www.linkedin.com/in/jmarcoscosta/
https://github.com/jmarcoscosta

Re: [PATCH] fs/squashfs: fix reading of fragmented files

[Tom Rini]

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

On Wed, Jun 09, 2021 at 10:16:53AM -0300, João Marcos Costa wrote:
> Hello, everyone
> 
> Em qua., 26 de mai. de 2021 às 09:35, João Marcos Costa <
> jmcosta944@gmail.com> escreveu:
> 
> > Hello, Miquèl
> >
> > Em qua., 26 de mai. de 2021 às 04:52, Miquel Raynal <
> > miquel.raynal@bootlin.com> escreveu:
> >
> >> Hi Joao,
> >>
> >> Joao Marcos Costa <jmcosta944@gmail.com> wrote on Mon, 17 May 2021
> >> 18:20:38 -0300:
> >>
> >> > The fragmented files were not correctly read because of two issues:
> >> >
> >> > - The squashfs_file_info struct has a field named 'comp', which tells if
> >> > the file's fragment is compressed or not. This field was always set to
> >> > 'true' in sqfs_get_regfile_info and sqfs_get_lregfile_info. It should
> >> > actually take sqfs_frag_lookup's return value. This patch addresses
> >> > these two assignments.
> >> >
> >> > - In sqfs_read, the fragments (compressed or not) were copied to the
> >> > output buffer through a for loop which was reading data at the wrong
> >> > offset. Replace these loops by equivalent calls to memcpy, with the
> >> > right parameters.
> >>
> >> Good idea to get rid of these memcpy of 1 byte :)
> >>
> >> > I tested this patch by comparing the MD5 checksum of a few fragmented
> >> > files with the respective md5sum output in sandbox, considering both
> >> > compressed and uncompressed fragments.
> >> >
> >> > Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
> >>
> >> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>
> >> But next time, when you fix two issues (even if they fix the same
> >> feature) please provide two patches ;)
> >>
> >> Thanks,
> >> Miquèl
> >>
> >
> >
> Any updates on this patch review?

Seems fine, but I'm also leaning on grabbing all of the squashfs patches
for -next at this point, unless people have strong feelings about it
being safe at this point for master, thanks.

-- 
Tom

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

Re: [PATCH] fs/squashfs: fix reading of fragmented files

[João Marcos Costa]

Hello,

In fact, I really think this patch should be applied to master as soon
as possible, since the actual unsafety comes from the current code,
which may read past the fragment_block buffer size.
Besides, the patch series I sent to rewrite the test suite needs this
fix, and the current test suite is error-prone, as it was already
reported.

Best regards,


Em qua., 9 de jun. de 2021 às 14:40, Tom Rini <trini@konsulko.com> escreveu:
>
> On Wed, Jun 09, 2021 at 10:16:53AM -0300, João Marcos Costa wrote:
> > Hello, everyone
> >
> > Em qua., 26 de mai. de 2021 às 09:35, João Marcos Costa <
> > jmcosta944@gmail.com> escreveu:
> >
> > > Hello, Miquèl
> > >
> > > Em qua., 26 de mai. de 2021 às 04:52, Miquel Raynal <
> > > miquel.raynal@bootlin.com> escreveu:
> > >
> > >> Hi Joao,
> > >>
> > >> Joao Marcos Costa <jmcosta944@gmail.com> wrote on Mon, 17 May 2021
> > >> 18:20:38 -0300:
> > >>
> > >> > The fragmented files were not correctly read because of two issues:
> > >> >
> > >> > - The squashfs_file_info struct has a field named 'comp', which tells if
> > >> > the file's fragment is compressed or not. This field was always set to
> > >> > 'true' in sqfs_get_regfile_info and sqfs_get_lregfile_info. It should
> > >> > actually take sqfs_frag_lookup's return value. This patch addresses
> > >> > these two assignments.
> > >> >
> > >> > - In sqfs_read, the fragments (compressed or not) were copied to the
> > >> > output buffer through a for loop which was reading data at the wrong
> > >> > offset. Replace these loops by equivalent calls to memcpy, with the
> > >> > right parameters.
> > >>
> > >> Good idea to get rid of these memcpy of 1 byte :)
> > >>
> > >> > I tested this patch by comparing the MD5 checksum of a few fragmented
> > >> > files with the respective md5sum output in sandbox, considering both
> > >> > compressed and uncompressed fragments.
> > >> >
> > >> > Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
> > >>
> > >> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > >>
> > >> But next time, when you fix two issues (even if they fix the same
> > >> feature) please provide two patches ;)
> > >>
> > >> Thanks,
> > >> Miquèl
> > >>
> > >
> > >
> > Any updates on this patch review?
>
> Seems fine, but I'm also leaning on grabbing all of the squashfs patches
> for -next at this point, unless people have strong feelings about it
> being safe at this point for master, thanks.
>
> --
> Tom



-- 
Atenciosamente,
João Marcos Costa

www.linkedin.com/in/jmarcoscosta/
https://github.com/jmarcoscosta

Re: [PATCH] fs/squashfs: fix reading of fragmented files

[João Marcos Costa]

By the way, I apologize for the top-posting on this previous email I sent.

Re: [PATCH] fs/squashfs: fix reading of fragmented files

[Tom Rini]

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

On Wed, Jun 09, 2021 at 03:21:54PM -0300, João Marcos Costa wrote:

> Hello,
> 
> In fact, I really think this patch should be applied to master as soon
> as possible, since the actual unsafety comes from the current code,
> which may read past the fragment_block buffer size.
> Besides, the patch series I sent to rewrite the test suite needs this
> fix, and the current test suite is error-prone, as it was already
> reported.

OK.  So, some local testing shows that we'll need to move the CI images
up to a newer base in order to have mksquashfs new enough to support
zstd.  Today I guess the failure is just ignored (not great!).  I'll
take the bugfix now and we'll do the tests soon.

-- 
Tom

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

Re: [PATCH] fs/squashfs: fix reading of fragmented files

[Tom Rini]

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

On Mon, May 17, 2021 at 06:20:38PM -0300, Joao Marcos Costa wrote:

> The fragmented files were not correctly read because of two issues:
> 
> - The squashfs_file_info struct has a field named 'comp', which tells if
> the file's fragment is compressed or not. This field was always set to
> 'true' in sqfs_get_regfile_info and sqfs_get_lregfile_info. It should
> actually take sqfs_frag_lookup's return value. This patch addresses
> these two assignments.
> 
> - In sqfs_read, the fragments (compressed or not) were copied to the
> output buffer through a for loop which was reading data at the wrong
> offset. Replace these loops by equivalent calls to memcpy, with the
> right parameters.
> 
> I tested this patch by comparing the MD5 checksum of a few fragmented
> files with the respective md5sum output in sandbox, considering both
> compressed and uncompressed fragments.
> 
> Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
> Tested-by: Richard Genoud <richard.genoud@posteo.net>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to u-boot/master, thanks!

-- 
Tom

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