Message ID | CALO=DHH-Jn17DQ=UqXiKmpW+2svKVeadYUP6haATcNvP3Y19xg@mail.gmail.com |
---|---|
State | Deferred |
Delegated to: | Tom Rini |
Headers | show |
Series | [PATCH1/1] sqfs: sqfs_tokenize() should fill the tokens list instead of free items | expand |
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!
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
---------- 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 >
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 > >
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;
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(-) }