* [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list instead of free items
@ 2021-10-16 2:19 Jincheng Wang
2021-10-26 19:25 ` Tom Rini
0 siblings, 1 reply; 6+ messages in thread
From: Jincheng Wang @ 2021-10-16 2:19 UTC (permalink / raw)
To: Tom Rini; +Cc: u-boot
We can delete two lines of code to avoid double free bug, but still a wild
pointers bug.
A test for wild pointers:
sqfsls host 0 1//2/3//4/5
Fill the tokens list can solve it well.
Signed-off-by: Jincheng Wang <jc.w4ng@gmail.com>
---
fs/squashfs/sqfs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
index e2d91c654c..50d3f8b71e 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -303,8 +303,9 @@ static int sqfs_tokenize(char **tokens, int count,
const char *str)
aux = strtok(!j ? strc : NULL, "/");
tokens[j] = strdup(aux);
if (!tokens[j]) {
- for (i = 0; i < j; i++)
- free(tokens[i]);
+ /* fill tokens list to avoid wild pointers being freed*/
+ for (i = j + 1; i < count; i++)
+ tokens[i] = 0;
ret = -ENOMEM;
goto free_strc;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list instead of free items
2021-10-16 2:19 [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list instead of free items Jincheng Wang
@ 2021-10-26 19:25 ` Tom Rini
2021-10-27 8:17 ` Miquel Raynal
0 siblings, 1 reply; 6+ messages in thread
From: Tom Rini @ 2021-10-26 19:25 UTC (permalink / raw)
To: Jincheng Wang, Joao Marcos Costa, Thomas Petazzoni, Miquel Raynal; +Cc: u-boot
[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]
On Sat, Oct 16, 2021 at 10:19:48AM +0800, Jincheng Wang wrote:
> We can delete two lines of code to avoid double free bug, but still a wild
> pointers bug.
>
> A test for wild pointers:
> sqfsls host 0 1//2/3//4/5
>
> Fill the tokens list can solve it well.
>
>
> Signed-off-by: Jincheng Wang <jc.w4ng@gmail.com>
> ---
> fs/squashfs/sqfs.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> }
>
> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> index e2d91c654c..50d3f8b71e 100644
> --- a/fs/squashfs/sqfs.c
> +++ b/fs/squashfs/sqfs.c
> @@ -303,8 +303,9 @@ static int sqfs_tokenize(char **tokens, int count,
> const char *str)
> aux = strtok(!j ? strc : NULL, "/");
> tokens[j] = strdup(aux);
> if (!tokens[j]) {
> - for (i = 0; i < j; i++)
> - free(tokens[i]);
> + /* fill tokens list to avoid wild pointers being freed*/
> + for (i = j + 1; i < count; i++)
> + tokens[i] = 0;
> ret = -ENOMEM;
> goto free_strc;
Aside from the whitespace having been destroyed, any comments from the
maintainers / reviewers? Thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list instead of free items
2021-10-26 19:25 ` Tom Rini
@ 2021-10-27 8:17 ` Miquel Raynal
[not found] ` <CALO=DHHq4hSsrAUorgxeTGOFh_HjTr00vFaGVEVQytvNiLiEJA@mail.gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Miquel Raynal @ 2021-10-27 8:17 UTC (permalink / raw)
To: Tom Rini; +Cc: Jincheng Wang, Joao Marcos Costa, Thomas Petazzoni, u-boot
Hello,
trini@konsulko.com wrote on Tue, 26 Oct 2021 15:25:56 -0400:
> On Sat, Oct 16, 2021 at 10:19:48AM +0800, Jincheng Wang wrote:
>
> > We can delete two lines of code to avoid double free bug, but still a wild
> > pointers bug.
> >
> > A test for wild pointers:
> > sqfsls host 0 1//2/3//4/5
> >
> > Fill the tokens list can solve it well.
> >
> >
> > Signed-off-by: Jincheng Wang <jc.w4ng@gmail.com>
> > ---
> > fs/squashfs/sqfs.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > }
> >
> > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> > index e2d91c654c..50d3f8b71e 100644
> > --- a/fs/squashfs/sqfs.c
> > +++ b/fs/squashfs/sqfs.c
> > @@ -303,8 +303,9 @@ static int sqfs_tokenize(char **tokens, int count,
> > const char *str)
> > aux = strtok(!j ? strc : NULL, "/");
> > tokens[j] = strdup(aux);
> > if (!tokens[j]) {
> > - for (i = 0; i < j; i++)
> > - free(tokens[i]);
Where is the double free here? Why do you stop freeing the tokens list?
> > + /* fill tokens list to avoid wild pointers being freed*/
> > + for (i = j + 1; i < count; i++)
> > + tokens[i] = 0;
This does not feel right. Perhaps it's just me not getting through the
lack of spacing successfully but this deserves more explanations.
> > ret = -ENOMEM;
> > goto free_strc;
>
> Aside from the whitespace having been destroyed, any comments from the
> maintainers / reviewers? Thanks!
>
Thanks,
Miquèl
^ permalink raw reply [flat|nested] 6+ messages in thread
* Fwd: [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list instead of free items
[not found] ` <CALO=DHHq4hSsrAUorgxeTGOFh_HjTr00vFaGVEVQytvNiLiEJA@mail.gmail.com>
@ 2021-10-31 10:24 ` Jincheng Wang
2021-11-02 7:11 ` Miquel Raynal
0 siblings, 1 reply; 6+ messages in thread
From: Jincheng Wang @ 2021-10-31 10:24 UTC (permalink / raw)
To: Miquel Raynal; +Cc: Tom Rini, u-boot
---------- Forwarded message ---------
发件人： Jincheng Wang <jc.w4ng@gmail.com>
Date: 2021年10月31日周日 下午6:23
Subject: Re: [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list
instead of free items
To: Miquel Raynal <miquel.raynal@bootlin.com>
Hello,
Apologize for a late reply ,and I made a mistake in wirting mail.
Here is the email before I commit the patch.
Double free vulnerability in sqfs commands ("sqfsls" and "sqfsload")
On Tue, Oct 12, 2021 at 04:07:43PM +0800, Jincheng Wang wrote:
> Hello U-Boot lists!
> I had been doing a fuzz testing in U-Boot .
> There is a double free bug in U-Boot, in /fs/squashfs/sqfs.c in the
> function sqfs_tokenize( ).
> On the line 307, tokens[i] is being freed and the ret is being set
-ENOMEM,
> it will go to the out: label and free tokens[i] again (just like
> CVE-2020-8432, double free in do_rename_gpt_parts() ).
> Here is a sample command cause to crash in sandbox environment:
> host bind 0 test.sqsh
> sqfsls host 0 1//3
------------------------------
The vulnerability can be fixed with a new return value and a new label. But
I didn't want to creat these, I chose another way.
Regards,
Jincheng
Miquel Raynal <miquel.raynal@bootlin.com> 于2021年10月27日周三 下午4:17写道：
> Hello,
>
> trini@konsulko.com wrote on Tue, 26 Oct 2021 15:25:56 -0400:
>
> > On Sat, Oct 16, 2021 at 10:19:48AM +0800, Jincheng Wang wrote:
> >
> > > We can delete two lines of code to avoid double free bug, but still a
> wild
> > > pointers bug.
> > >
> > > A test for wild pointers:
> > > sqfsls host 0 1//2/3//4/5
> > >
> > > Fill the tokens list can solve it well.
> > >
> > >
> > > Signed-off-by: Jincheng Wang <jc.w4ng@gmail.com>
> > > ---
> > > fs/squashfs/sqfs.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > }
> > >
> > > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> > > index e2d91c654c..50d3f8b71e 100644
> > > --- a/fs/squashfs/sqfs.c
> > > +++ b/fs/squashfs/sqfs.c
> > > @@ -303,8 +303,9 @@ static int sqfs_tokenize(char **tokens, int count,
> > > const char *str)
> > > aux = strtok(!j ? strc : NULL, "/");
> > > tokens[j] = strdup(aux);
> > > if (!tokens[j]) {
> > > - for (i = 0; i < j; i++)
> > > - free(tokens[i]);
>
> Where is the double free here? Why do you stop freeing the tokens list?
>
> > > + /* fill tokens list to avoid wild pointers being freed*/
> > > + for (i = j + 1; i < count; i++)
> > > + tokens[i] = 0;
>
> This does not feel right. Perhaps it's just me not getting through the
> lack of spacing successfully but this deserves more explanations.
>
> > > ret = -ENOMEM;
> > > goto free_strc;
> >
> > Aside from the whitespace having been destroyed, any comments from the
> > maintainers / reviewers? Thanks!
> >
>
> Thanks,
> Miquèl
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list instead of free items
2021-10-31 10:24 ` Fwd: " Jincheng Wang
@ 2021-11-02 7:11 ` Miquel Raynal
0 siblings, 0 replies; 6+ messages in thread
From: Miquel Raynal @ 2021-11-02 7:11 UTC (permalink / raw)
To: Jincheng Wang; +Cc: Tom Rini, u-boot
Hi Jincheng,
jc.w4ng@gmail.com wrote on Sun, 31 Oct 2021 18:24:47 +0800:
> ---------- Forwarded message ---------
> 发件人： Jincheng Wang <jc.w4ng@gmail.com>
> Date: 2021年10月31日周日 下午6:23
> Subject: Re: [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list
> instead of free items
> To: Miquel Raynal <miquel.raynal@bootlin.com>
>
>
> Hello,
>
> Apologize for a late reply ,and I made a mistake in wirting mail.
>
> Here is the email before I commit the patch.
>
> Double free vulnerability in sqfs commands ("sqfsls" and "sqfsload")
>
> On Tue, Oct 12, 2021 at 04:07:43PM +0800, Jincheng Wang wrote:
>
> > Hello U-Boot lists!
> > I had been doing a fuzz testing in U-Boot .
> > There is a double free bug in U-Boot, in /fs/squashfs/sqfs.c in the
> > function sqfs_tokenize( ).
> > On the line 307, tokens[i] is being freed and the ret is being set
> -ENOMEM,
> > it will go to the out: label and free tokens[i] again (just like
> > CVE-2020-8432, double free in do_rename_gpt_parts() ).
> > Here is a sample command cause to crash in sandbox environment:
> > host bind 0 test.sqsh
> > sqfsls host 0 1//3
>
> ------------------------------
>
> The vulnerability can be fixed with a new return value and a new label. But
> I didn't want to creat these, I chose another way.
You need to answer my questions below if you don't want to propose
another solution. But creating a new goto label is perhaps the good way
to solve this, provided that there is actually a leak (I am waiting for
the explanation of how it happens).
Cheers,
Miquèl
>
> Regards,
> Jincheng
>
>
> Miquel Raynal <miquel.raynal@bootlin.com> 于2021年10月27日周三 下午4:17写道：
>
> > Hello,
> >
> > trini@konsulko.com wrote on Tue, 26 Oct 2021 15:25:56 -0400:
> >
> > > On Sat, Oct 16, 2021 at 10:19:48AM +0800, Jincheng Wang wrote:
> > >
> > > > We can delete two lines of code to avoid double free bug, but still a
> > wild
> > > > pointers bug.
> > > >
> > > > A test for wild pointers:
> > > > sqfsls host 0 1//2/3//4/5
> > > >
> > > > Fill the tokens list can solve it well.
> > > >
> > > >
> > > > Signed-off-by: Jincheng Wang <jc.w4ng@gmail.com>
> > > > ---
> > > > fs/squashfs/sqfs.c | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > }
> > > >
> > > > diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> > > > index e2d91c654c..50d3f8b71e 100644
> > > > --- a/fs/squashfs/sqfs.c
> > > > +++ b/fs/squashfs/sqfs.c
> > > > @@ -303,8 +303,9 @@ static int sqfs_tokenize(char **tokens, int count,
> > > > const char *str)
> > > > aux = strtok(!j ? strc : NULL, "/");
> > > > tokens[j] = strdup(aux);
> > > > if (!tokens[j]) {
> > > > - for (i = 0; i < j; i++)
> > > > - free(tokens[i]);
> >
> > Where is the double free here? Why do you stop freeing the tokens list?
> >
> > > > + /* fill tokens list to avoid wild pointers being freed*/
> > > > + for (i = j + 1; i < count; i++)
> > > > + tokens[i] = 0;
> >
> > This does not feel right. Perhaps it's just me not getting through the
> > lack of spacing successfully but this deserves more explanations.
> >
> > > > ret = -ENOMEM;
> > > > goto free_strc;
> > >
> > > Aside from the whitespace having been destroyed, any comments from the
> > > maintainers / reviewers? Thanks!
> > >
> >
> > Thanks,
> > Miquèl
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list instead of free items
@ 2021-11-02 9:13 jc.w4ng
0 siblings, 0 replies; 6+ messages in thread
From: jc.w4ng @ 2021-11-02 9:13 UTC (permalink / raw)
To: Miquel Raynal; +Cc: u-boot@lists.denx.de, Tom Rini
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-02 9:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-16 2:19 [PATCH1/1]sqfs: sqfs_tokenize() should fill the tokens list instead of free items Jincheng Wang
2021-10-26 19:25 ` Tom Rini
2021-10-27 8:17 ` Miquel Raynal
[not found] ` <CALO=DHHq4hSsrAUorgxeTGOFh_HjTr00vFaGVEVQytvNiLiEJA@mail.gmail.com>
2021-10-31 10:24 ` Fwd: " Jincheng Wang
2021-11-02 7:11 ` Miquel Raynal
-- strict thread matches above, loose matches on Subject: below --
2021-11-02 9:13 jc.w4ng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox