| Message ID | 20181215162350.12489-1-richard@nod.at |
|---|---|
| State | New, archived |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | jffs2: Fix integer underflow in jffs2_rtime_compress | expand |
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; >
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
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 > > > > . >
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;
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(+)