| Message ID | 20210517212038.117284-1-jmcosta944@gmail.com |
|---|---|
| State | Accepted |
| Commit | 0008d8086649d3bb3afd0c4697f5b73ccf6f293d |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fs/squashfs: fix reading of fragmented files | expand |
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: >
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
By the way, I apologize for the top-posting on this previous email I sent.
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!
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:
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(-)