Re: [v2] ubifs: Fix read out-of-bounds in ubifs_jnl_write_inode()
ping >> Reproducer: >> 0. config KASAN && apply print.patch >> 1. mount ubifs on /root/temp >> 2. run test.sh > > What does test.sh do? Go to Link: https://bugzilla.kernel.org/show_bug.cgi?id=210865. test.sh creates a very long path file test_file, and then create a symbol link link_file for test_file, so ubifs inode for link_file will be assigned a big value for ui->data_len. When we change atime for link_file, ubifs_jnl_write_inode will be executed by wb_writeback. By this way, write_len could be not aligned with 8 bytes. > >> 3. cd /root/temp && ls // change atime for link_file >> 4. wait 1~2 minutes >> >> In order to solve the read oob problem in ubifs_wbuf_write_nolock, just align >> the write_len to >> 8 bytes when alloc the memory. So that this patch will not affect the use of >> write_len in other >> functions, such as ubifs_jnl_write_inode->make_reservation and >> ubifs_jnl_write_inode->ubifs_node_calc_hash. > > I gave this a second thought and I'm not so sure anymore what exactly is > going on. > The problem is real, I fully agree with you but I need to dig deeper into > the journal and wbuf code to double check that we really fix the right thing > and not just paper other something. > > Thanks, > //richard > . >
[PATCH v2] ubifs: Fix read out-of-bounds in ubifs_jnl_write_inode()
/0x910
[ 336.493890] ubifs_jnl_write_inode.cold+0x6f/0x878 [ubifs]
[ 336.494744] ubifs_write_inode+0x1c3/0x290 [ubifs]
[ 336.495500] __writeback_single_inode+0x6cc/0x880
[ 336.496179] writeback_sb_inodes+0x3a9/0x9a0
[ 336.496791] __writeback_inodes_wb+0xc8/0x170
[ 336.497417] wb_writeback+0x637/0x700
[ 336.497949] wb_workfn+0x8af/0xb80
[ 336.498440] process_one_work+0x467/0x9f0
[ 336.499023] worker_thread+0x34d/0x8e0
[ 336.499567] kthread+0x204/0x280
[ 336.500050] ret_from_fork+0x1f/0x30
[ 336.500570]
[ 336.500793] The buggy address belongs to the object at 888019612000
[ 336.500793] which belongs to the cache kmalloc-4k of size 4096
[ 336.502550] The buggy address is located 4088 bytes inside of
[ 336.502550] 4096-byte region [888019612000, 888019613000)
[ 336.504231] The buggy address belongs to the page:
[ 336.504917] page:3204ded8 refcount:1 mapcount:0
mapping: index:0x0 pfn:0x19610
[ 336.506234] head:3204ded8 order:3 compound_mapcount:0
compound_pincount:0
[ 336.507293] flags: 0x1f80010200(slab|head)
[ 336.507934] raw: 001f80010200 ea667000 00020002
888010842140
[ 336.509038] raw: 80040004 0001
88801956e3c1
[ 336.510132] page dumped because: kasan: bad access detected
[ 336.510923] pages's memcg:88801956e3c1
[ 336.511509]
[ 336.511730] Memory state around the buggy address:
[ 336.512421] 888019612e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00
[ 336.513446] 888019612f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00
[ 336.514468] >888019612f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
01
[ 336.515494] ^
[ 336.516506] 888019613000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
fc
[ 336.517535] 888019613080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
fc
[ 336.518560]
==
The memory area allocated in ubifs_jnl_write_inode() is not aligned with 8
bytes:
ino_start = ino = kmalloc(write_len, GFP_NOFS);
When ino_start passed into write_head -> ubifs_wbuf_write_nolock:
n = aligned_len >> c->max_write_shift;
if (n) {
n <<= c->max_write_shift;
err = ubifs_leb_write(c, wbuf->lnum, buf + written, wbuf->offs, n);
// Read oob occurs here, read n bytes from buf, and buf is passed from
@ino_start which is
// not 8 bytes aligned(write_len < n). Program read (n - write_len) more
bytes.
}
Reproducer:
0. config KASAN && apply print.patch
1. mount ubifs on /root/temp
2. run test.sh
3. cd /root/temp && ls // change atime for link_file
4. wait 1~2 minutes
In order to solve the read oob problem in ubifs_wbuf_write_nolock, just align
the write_len to
8 bytes when alloc the memory. So that this patch will not affect the use of
write_len in other
functions, such as ubifs_jnl_write_inode->make_reservation and
ubifs_jnl_write_inode->ubifs_node_calc_hash.
Cc:
Fixes: 1e51764a3c2ac0 ("UBIFS: add new flash file system")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=210865
Signed-off-by: Chengsong Ke
---
fs/ubifs/journal.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 03410ae0813a..fc918a66d208 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -577,7 +577,7 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct
inode *dir,
else
len += host_ui->data_len;
- dent = kzalloc(len, GFP_NOFS);
+ dent = kzalloc(ALIGN(len, 8), GFP_NOFS);
if (!dent)
return -ENOMEM;
@@ -866,7 +866,7 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const
struct inode *inode)
else
write_len += ilen;
- ino_start = ino = kmalloc(write_len, GFP_NOFS);
+ ino_start = ino = kzalloc(ALIGN(write_len, 8), GFP_NOFS);
if (!ino)
return -ENOMEM;
--
2.12.3
[PATCH] ubifs: Fix read out-of-bounds in ubifs_jnl_write_inode()
/0x910
[ 336.493890] ubifs_jnl_write_inode.cold+0x6f/0x878 [ubifs]
[ 336.494744] ubifs_write_inode+0x1c3/0x290 [ubifs]
[ 336.495500] __writeback_single_inode+0x6cc/0x880
[ 336.496179] writeback_sb_inodes+0x3a9/0x9a0
[ 336.496791] __writeback_inodes_wb+0xc8/0x170
[ 336.497417] wb_writeback+0x637/0x700
[ 336.497949] wb_workfn+0x8af/0xb80
[ 336.498440] process_one_work+0x467/0x9f0
[ 336.499023] worker_thread+0x34d/0x8e0
[ 336.499567] kthread+0x204/0x280
[ 336.500050] ret_from_fork+0x1f/0x30
[ 336.500570]
[ 336.500793] The buggy address belongs to the object at 888019612000
[ 336.500793] which belongs to the cache kmalloc-4k of size 4096
[ 336.502550] The buggy address is located 4088 bytes inside of
[ 336.502550] 4096-byte region [888019612000, 888019613000)
[ 336.504231] The buggy address belongs to the page:
[ 336.504917] page:3204ded8 refcount:1 mapcount:0
mapping: index:0x0 pfn:0x19610
[ 336.506234] head:3204ded8 order:3 compound_mapcount:0
compound_pincount:0
[ 336.507293] flags: 0x1f80010200(slab|head)
[ 336.507934] raw: 001f80010200 ea667000 00020002
888010842140
[ 336.509038] raw: 80040004 0001
88801956e3c1
[ 336.510132] page dumped because: kasan: bad access detected
[ 336.510923] pages's memcg:88801956e3c1
[ 336.511509]
[ 336.511730] Memory state around the buggy address:
[ 336.512421] 888019612e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00
[ 336.513446] 888019612f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00
[ 336.514468] >888019612f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
01
[ 336.515494] ^
[ 336.516506] 888019613000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
fc
[ 336.517535] 888019613080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
fc
[ 336.518560]
==
The memory area allocated in ubifs_jnl_write_inode() is not aligned with 8
bytes:
ino_start = ino = kmalloc(write_len, GFP_NOFS);
When ino_start passed into write_head -> ubifs_wbuf_write_nolock:
n = aligned_len >> c->max_write_shift;
if (n) {
n <<= c->max_write_shift;
err = ubifs_leb_write(c, wbuf->lnum, buf + written, wbuf->offs, n);
// Read oob occurs here, read n bytes from buf, and buf is passed from
@ino_start which is
// not 8 bytes aligned(write_len < n). Program read (n - write_len) more
bytes.
}
Reproducer:
0. config KASAN && apply print.patch
1. mount ubifs on /root/temp
2. run test.sh
3. cd /root/temp && ls // change atime for link_file
4. wait 1~2 minutes
Cc:
Fixes: 1e51764a3c2ac0 ("UBIFS: add new flash file system")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=210865
Signed-off-by: Chengsong Ke
---
fs/ubifs/io.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
index 00b61dba62b7..6b9d550341e5 100644
--- a/fs/ubifs/io.c
+++ b/fs/ubifs/io.c
@@ -836,13 +836,20 @@ int ubifs_wbuf_write_nolock(struct ubifs_wbuf *wbuf, void
*buf, int len)
n <<= c->max_write_shift;
dbg_io("write %d bytes to LEB %d:%d", n, wbuf->lnum,
wbuf->offs);
- err = ubifs_leb_write(c, wbuf->lnum, buf + written,
+
+ memcpy(wbuf->buf, buf + written, min(len, n));
+ if (n > len) {
+ ubifs_assert(c, n - len < 8);
+ ubifs_pad(c, wbuf->buf + len, n - len);
+ }
+
+ err = ubifs_leb_write(c, wbuf->lnum, wbuf->buf,
wbuf->offs, n);
if (err)
goto out;
wbuf->offs += n;
aligned_len -= n;
- len -= n;
+ len -= min(len, n);
written += n;
}
--
2.12.3
ubifs: Remove the redundant return in dbg_check_nondata_nodes_order
ping //Chengsong Ke
Re: [PATCH v2] mtd:ubi: Remove useless code in bytes_str_to_int
On Tue, Nov 3, 2020 at 1:00 PM Chengsong Ke wrote: > > From: Chengsong Ke > > As a local variable, "endp" is neither refered nor returned after this > line "endp += 2", it looks like a useless code, suggest to remove it. > > Signed-off-by: Chengsong Ke > --- > drivers/mtd/ubi/build.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index > e85b04e9716b..46a6dd75e533 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -1351,8 +1351,6 @@ static int bytes_str_to_int(const char *str) > fallthrough; > case 'K': > result *= 1024; > - if (endp[1] == 'i' && endp[2] == 'B') > - endp += 2; > Makes sense. But why did you send a v2? > -- > Thanks, > //richard > I just send the v1 with the wrong module name 'ubifs'. > [PATCH] ubifs: Remove useless code in bytes_str_to_int > :-) > Thanks, > //Chengsong Ke ping
[PATCH v2] mtd:ubi: Remove useless code in bytes_str_to_int
From: k00524021 As a local variable, "endp" is neither refered nor returned after this line "endp += 2", it looks like a useless code, suggest to remove it. Signed-off-by: Chengsong Ke --- drivers/mtd/ubi/build.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index e85b04e9716b..46a6dd75e533 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -1351,8 +1351,6 @@ static int bytes_str_to_int(const char *str) fallthrough; case 'K': result *= 1024; - if (endp[1] == 'i' && endp[2] == 'B') - endp += 2; case '\0': break; default: -- 2.12.3
[PATCH] ubifs: Remove useless code in bytes_str_to_int
From: k00524021 As a local variable, "endp" is neither refered nor returned after this line "endp += 2", it looks like a useless code, suggest to remove it. Signed-off-by: Chengsong Ke --- drivers/mtd/ubi/build.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index e85b04e9716b..46a6dd75e533 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -1351,8 +1351,6 @@ static int bytes_str_to_int(const char *str) fallthrough; case 'K': result *= 1024; - if (endp[1] == 'i' && endp[2] == 'B') - endp += 2; case '\0': break; default: -- 2.12.3
Re: [PATCH v2] ubifs: Fix the printing type of c->big_lpt
On Sat, Oct 31, 2020 at 9:56 AM Chengsong Ke wrote: > > Ubifs uses %d to print c->big_lpt, but c->big_lpt is a variable of > type unsigned int and should be printed with %u. > > Well, it is: > unsigned int big_lpt:1; > So, either 0 or 1. > > Does changing it to %u silence some static checker or is there some > other problem I don't see right now? :-) > > Thanks, > //Richard This is just a coding style issue, I found in the ubifs code. :-) Thanks, //Chengsong Ke
[PATCH] ubifs: Remove the redundant return in dbg_check_nondata_nodes_order
There is a redundant return in dbg_check_nondata_nodes_order, which will be never reached. In addition, error code should be returned instead of zero in this branch. Signed-off-by: Chengsong Ke --- fs/ubifs/debug.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c index ebff43f8009c..b2db518056cb 100644 --- a/fs/ubifs/debug.c +++ b/fs/ubifs/debug.c @@ -2442,7 +2442,6 @@ int dbg_check_nondata_nodes_order(struct ubifs_info *c, struct list_head *head) ubifs_msg(c, "dumping second node"); ubifs_dump_node(c, sb->node); return -EINVAL; - return 0; } static inline int chance(unsigned int n, unsigned int out_of) -- 2.12.3
[PATCH v2] ubifs: Fix the printing type of c->big_lpt
Ubifs uses %d to print c->big_lpt, but c->big_lpt is a variable
of type unsigned int and should be printed with %u.
Signed-off-by: Chengsong Ke
---
fs/ubifs/debug.c | 2 +-
fs/ubifs/lpt.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index ebff43f8009c..ef1a02ee076f 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -764,7 +764,7 @@ void ubifs_dump_lpt_info(struct ubifs_info *c)
pr_err("\tnnode_sz: %d\n", c->nnode_sz);
pr_err("\tltab_sz: %d\n", c->ltab_sz);
pr_err("\tlsave_sz: %d\n", c->lsave_sz);
- pr_err("\tbig_lpt: %d\n", c->big_lpt);
+ pr_err("\tbig_lpt: %u\n", c->big_lpt);
pr_err("\tlpt_hght: %d\n", c->lpt_hght);
pr_err("\tpnode_cnt: %d\n", c->pnode_cnt);
pr_err("\tnnode_cnt: %d\n", c->nnode_cnt);
diff --git a/fs/ubifs/lpt.c b/fs/ubifs/lpt.c
index 6e0a153b7194..778a22bf9a92 100644
--- a/fs/ubifs/lpt.c
+++ b/fs/ubifs/lpt.c
@@ -851,7 +851,7 @@ int ubifs_create_dflt_lpt(struct ubifs_info *c, int
*main_lebs, int lpt_first,
dbg_lp("lsave_sz %d", c->lsave_sz);
dbg_lp("lsave_cnt %d", c->lsave_cnt);
dbg_lp("lpt_hght %d", c->lpt_hght);
- dbg_lp("big_lpt %d", c->big_lpt);
+ dbg_lp("big_lpt %u", c->big_lpt);
dbg_lp("LPT root is at %d:%d", c->lpt_lnum, c->lpt_offs);
dbg_lp("LPT head is at %d:%d", c->nhead_lnum, c->nhead_offs);
dbg_lp("LPT ltab is at %d:%d", c->ltab_lnum, c->ltab_offs);
@@ -1824,7 +1824,7 @@ static int lpt_init_rd(struct ubifs_info *c)
dbg_lp("lsave_sz %d", c->lsave_sz);
dbg_lp("lsave_cnt %d", c->lsave_cnt);
dbg_lp("lpt_hght %d", c->lpt_hght);
- dbg_lp("big_lpt %d", c->big_lpt);
+ dbg_lp("big_lpt %u", c->big_lpt);
dbg_lp("LPT root is at %d:%d", c->lpt_lnum, c->lpt_offs);
dbg_lp("LPT head is at %d:%d", c->nhead_lnum, c->nhead_offs);
dbg_lp("LPT ltab is at %d:%d", c->ltab_lnum, c->ltab_offs);
--
2.12.3
[PATCH] ubifs: Fix the printing type of c->big_lpt
Ubifs uses %d to print c->big_lpt, but c->big_lpt is a variable
of type unsigned int and should be printed with %u.
Reviewed-by: Fangpeng Wang
Signed-off-by: Chengsong Ke
---
fs/ubifs/debug.c | 2 +-
fs/ubifs/lpt.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index ebff43f8009c..ef1a02ee076f 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -764,7 +764,7 @@ void ubifs_dump_lpt_info(struct ubifs_info *c)
pr_err("\tnnode_sz: %d\n", c->nnode_sz);
pr_err("\tltab_sz: %d\n", c->ltab_sz);
pr_err("\tlsave_sz: %d\n", c->lsave_sz);
- pr_err("\tbig_lpt: %d\n", c->big_lpt);
+ pr_err("\tbig_lpt: %u\n", c->big_lpt);
pr_err("\tlpt_hght: %d\n", c->lpt_hght);
pr_err("\tpnode_cnt: %d\n", c->pnode_cnt);
pr_err("\tnnode_cnt: %d\n", c->nnode_cnt);
diff --git a/fs/ubifs/lpt.c b/fs/ubifs/lpt.c
index 6e0a153b7194..778a22bf9a92 100644
--- a/fs/ubifs/lpt.c
+++ b/fs/ubifs/lpt.c
@@ -851,7 +851,7 @@ int ubifs_create_dflt_lpt(struct ubifs_info *c, int
*main_lebs, int lpt_first,
dbg_lp("lsave_sz %d", c->lsave_sz);
dbg_lp("lsave_cnt %d", c->lsave_cnt);
dbg_lp("lpt_hght %d", c->lpt_hght);
- dbg_lp("big_lpt %d", c->big_lpt);
+ dbg_lp("big_lpt %u", c->big_lpt);
dbg_lp("LPT root is at %d:%d", c->lpt_lnum, c->lpt_offs);
dbg_lp("LPT head is at %d:%d", c->nhead_lnum, c->nhead_offs);
dbg_lp("LPT ltab is at %d:%d", c->ltab_lnum, c->ltab_offs);
@@ -1824,7 +1824,7 @@ static int lpt_init_rd(struct ubifs_info *c)
dbg_lp("lsave_sz %d", c->lsave_sz);
dbg_lp("lsave_cnt %d", c->lsave_cnt);
dbg_lp("lpt_hght %d", c->lpt_hght);
- dbg_lp("big_lpt %d", c->big_lpt);
+ dbg_lp("big_lpt %u", c->big_lpt);
dbg_lp("LPT root is at %d:%d", c->lpt_lnum, c->lpt_offs);
dbg_lp("LPT head is at %d:%d", c->nhead_lnum, c->nhead_offs);
dbg_lp("LPT ltab is at %d:%d", c->ltab_lnum, c->ltab_offs);
--
2.12.3
