* [PATCH] jffs2: Fix integer underflow in jffs2_rtime_compress
@ 2018-12-15 16:23 Richard Weinberger
2018-12-20 10:43 ` Hou Tao
0 siblings, 1 reply; 5+ messages in thread
From: Richard Weinberger @ 2018-12-15 16:23 UTC (permalink / raw)
To: linux-mtd; +Cc: linux-kernel, dwmw2, Richard Weinberger, stable
The rtime compressor assumes that at least two bytes are
compressed.
If we try to compress just one byte, the loop condition will
wrap around and an out-of-bounds write happens.
Cc: <stable@vger.kernel.org>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
fs/jffs2/compr_rtime.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/jffs2/compr_rtime.c b/fs/jffs2/compr_rtime.c
index 406d9cc84ba8..cbf700001fc9 100644
--- a/fs/jffs2/compr_rtime.c
+++ b/fs/jffs2/compr_rtime.c
@@ -39,6 +39,9 @@ static int jffs2_rtime_compress(unsigned char *data_in,
memset(positions,0,sizeof(positions));
+ if (*dstlen < 2)
+ return -1;
+
while (pos < (*sourcelen) && outpos <= (*dstlen)-2) {
int backpos, runlen=0;
unsigned char value;
--
2.20.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] jffs2: Fix integer underflow in jffs2_rtime_compress
2018-12-15 16:23 [PATCH] jffs2: Fix integer underflow in jffs2_rtime_compress Richard Weinberger
@ 2018-12-20 10:43 ` Hou Tao
2018-12-20 10:45 ` Richard Weinberger
0 siblings, 1 reply; 5+ messages in thread
From: Hou Tao @ 2018-12-20 10:43 UTC (permalink / raw)
To: Richard Weinberger, linux-mtd; +Cc: dwmw2, linux-kernel, stable
On 2018/12/16 0:23, Richard Weinberger wrote:
> The rtime compressor assumes that at least two bytes are
> compressed.
> If we try to compress just one byte, the loop condition will
> wrap around and an out-of-bounds write happens.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> fs/jffs2/compr_rtime.c | 3 +++
> 1 file changed, 3 insertions(+)
> It seems that it doesn't incur any harm because the minimal allocated
size will be 8-bytes and jffs2_rtime_compress() will write 2-bytes into
the allocated buffer.
> diff --git a/fs/jffs2/compr_rtime.c b/fs/jffs2/compr_rtime.c
> index 406d9cc84ba8..cbf700001fc9 100644
> --- a/fs/jffs2/compr_rtime.c
> +++ b/fs/jffs2/compr_rtime.c
> @@ -39,6 +39,9 @@ static int jffs2_rtime_compress(unsigned char *data_in,
>
> memset(positions,0,sizeof(positions));
>
> + if (*dstlen < 2)
> + return -1;
> +
> while (pos < (*sourcelen) && outpos <= (*dstlen)-2) {
> int backpos, runlen=0;
> unsigned char value;
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] jffs2: Fix integer underflow in jffs2_rtime_compress
2018-12-20 10:43 ` Hou Tao
@ 2018-12-20 10:45 ` Richard Weinberger
2020-01-23 2:24 ` Hou Tao
0 siblings, 1 reply; 5+ messages in thread
From: Richard Weinberger @ 2018-12-20 10:45 UTC (permalink / raw)
To: Hou Tao; +Cc: linux-mtd, dwmw2, linux-kernel, stable
Am Donnerstag, 20. Dezember 2018, 11:43:08 CET schrieb Hou Tao:
>
> On 2018/12/16 0:23, Richard Weinberger wrote:
> > The rtime compressor assumes that at least two bytes are
> > compressed.
> > If we try to compress just one byte, the loop condition will
> > wrap around and an out-of-bounds write happens.
> >
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Richard Weinberger <richard@nod.at>
> > ---
> > fs/jffs2/compr_rtime.c | 3 +++
> > 1 file changed, 3 insertions(+)
> > It seems that it doesn't incur any harm because the minimal allocated
> size will be 8-bytes and jffs2_rtime_compress() will write 2-bytes into
> the allocated buffer.
Are you sure about that? I saw odd kernel behavior and KASAN complained too.
Thanks,
//richard
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] jffs2: Fix integer underflow in jffs2_rtime_compress
2018-12-20 10:45 ` Richard Weinberger
@ 2020-01-23 2:24 ` Hou Tao
0 siblings, 0 replies; 5+ messages in thread
From: Hou Tao @ 2020-01-23 2:24 UTC (permalink / raw)
To: Richard Weinberger; +Cc: dwmw2, linux-mtd, linux-kernel, stable
Hi Richard,
On 2018/12/20 18:45, Richard Weinberger wrote:
> Am Donnerstag, 20. Dezember 2018, 11:43:08 CET schrieb Hou Tao:
>>
>> On 2018/12/16 0:23, Richard Weinberger wrote:
>>> The rtime compressor assumes that at least two bytes are
>>> compressed.
>>> If we try to compress just one byte, the loop condition will
>>> wrap around and an out-of-bounds write happens.
>>>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>> ---
>>> fs/jffs2/compr_rtime.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> It seems that it doesn't incur any harm because the minimal allocated
>> size will be 8-bytes and jffs2_rtime_compress() will write 2-bytes into
>> the allocated buffer.
>
> Are you sure about that? I saw odd kernel behavior and KASAN complained too.
>
Sorry for the later reply.
Yes. KASAN complains but it doesn't incur any harm because the minimal allocated
size returned by kmalloc() will be 8-bytes.
But we better fix it, because it's bad to depend on the implementation of kmalloc().
It seems that mtd-utils has already fixed it years ago. Maybe we can use it directly ?
And your fix also looks good to me, so
Reviewed-by: Hou Tao <houtao1@huawei.com>
commit e8457f16306ad6e2c8708275bf42b5dfff40fffd
Author: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Date: Thu Jun 24 15:02:40 2010 +0200
mkfs.jffs2: fix integer underflow in jffs2_rtime_compress()
When '*dstlen' is 0 or 1, comparison will return wrong result. Reported
by valgrind as
==5919== Invalid write of size 1
==5919== at 0x40564E: jffs2_rtime_compress (compr_rtime.c:51)
==5919== by 0x40676B: jffs2_compress (compr.c:246)
==5919== by 0x403EE4: recursive_populate_directory (mkfs.jffs2.c:884)
==5919== Address 0x4e1bdb1 is 0 bytes after a block of size 1 alloc'd
==5919== at 0x4A0515D: malloc (vg_replace_malloc.c:195)
==5919== by 0x40671C: jffs2_compress (compr.c:229)
==5919== by 0x403EE4: recursive_populate_directory (mkfs.jffs2.c:884)
Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
diff --git a/compr_rtime.c b/compr_rtime.c
index 131536c..5613963 100644
--- a/compr_rtime.c
+++ b/compr_rtime.c
@@ -32,7 +32,7 @@ static int jffs2_rtime_compress(unsigned char *data_in, unsigned char *cpage_out
memset(positions,0,sizeof(positions));
- while (pos < (*sourcelen) && outpos <= (*dstlen)-2) {
+ while (pos < (*sourcelen) && outpos+2 <= (*dstlen)) {
int backpos, runlen=0;
unsigned char value;
> Thanks,
> //richard
>
>
>
> .
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] jffs2: Fix integer underflow in jffs2_rtime_compress
@ 2020-01-23 2:24 ` Hou Tao
0 siblings, 0 replies; 5+ messages in thread
From: Hou Tao @ 2020-01-23 2:24 UTC (permalink / raw)
To: Richard Weinberger; +Cc: linux-mtd, dwmw2, linux-kernel, stable
Hi Richard,
On 2018/12/20 18:45, Richard Weinberger wrote:
> Am Donnerstag, 20. Dezember 2018, 11:43:08 CET schrieb Hou Tao:
>>
>> On 2018/12/16 0:23, Richard Weinberger wrote:
>>> The rtime compressor assumes that at least two bytes are
>>> compressed.
>>> If we try to compress just one byte, the loop condition will
>>> wrap around and an out-of-bounds write happens.
>>>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>> ---
>>> fs/jffs2/compr_rtime.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> It seems that it doesn't incur any harm because the minimal allocated
>> size will be 8-bytes and jffs2_rtime_compress() will write 2-bytes into
>> the allocated buffer.
>
> Are you sure about that? I saw odd kernel behavior and KASAN complained too.
>
Sorry for the later reply.
Yes. KASAN complains but it doesn't incur any harm because the minimal allocated
size returned by kmalloc() will be 8-bytes.
But we better fix it, because it's bad to depend on the implementation of kmalloc().
It seems that mtd-utils has already fixed it years ago. Maybe we can use it directly ?
And your fix also looks good to me, so
Reviewed-by: Hou Tao <houtao1@huawei.com>
commit e8457f16306ad6e2c8708275bf42b5dfff40fffd
Author: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Date: Thu Jun 24 15:02:40 2010 +0200
mkfs.jffs2: fix integer underflow in jffs2_rtime_compress()
When '*dstlen' is 0 or 1, comparison will return wrong result. Reported
by valgrind as
==5919== Invalid write of size 1
==5919== at 0x40564E: jffs2_rtime_compress (compr_rtime.c:51)
==5919== by 0x40676B: jffs2_compress (compr.c:246)
==5919== by 0x403EE4: recursive_populate_directory (mkfs.jffs2.c:884)
==5919== Address 0x4e1bdb1 is 0 bytes after a block of size 1 alloc'd
==5919== at 0x4A0515D: malloc (vg_replace_malloc.c:195)
==5919== by 0x40671C: jffs2_compress (compr.c:229)
==5919== by 0x403EE4: recursive_populate_directory (mkfs.jffs2.c:884)
Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
diff --git a/compr_rtime.c b/compr_rtime.c
index 131536c..5613963 100644
--- a/compr_rtime.c
+++ b/compr_rtime.c
@@ -32,7 +32,7 @@ static int jffs2_rtime_compress(unsigned char *data_in, unsigned char *cpage_out
memset(positions,0,sizeof(positions));
- while (pos < (*sourcelen) && outpos <= (*dstlen)-2) {
+ while (pos < (*sourcelen) && outpos+2 <= (*dstlen)) {
int backpos, runlen=0;
unsigned char value;
> Thanks,
> //richard
>
>
>
> .
>
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-23 2:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-15 16:23 [PATCH] jffs2: Fix integer underflow in jffs2_rtime_compress Richard Weinberger
2018-12-20 10:43 ` Hou Tao
2018-12-20 10:45 ` Richard Weinberger
2020-01-23 2:24 ` Hou Tao
2020-01-23 2:24 ` Hou Tao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.