[PATCH] fs/squashfs: fix reading of fragmented files
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:
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;@@ -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;@@ -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:
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!
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
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
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!
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.
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
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.
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!
participants (5)
-
Joao Marcos Costa -
João Marcos Costa
-
Miquel Raynal -
Richard Genoud -
Tom Rini