linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node
@ 2020-06-16  7:11 Zhihao Cheng
  2020-06-16  7:11 ` [PATCH RFC 1/5] ubifs: Limit dumping length by size of memory which is allocated for the node Zhihao Cheng
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Zhihao Cheng @ 2020-06-16  7:11 UTC (permalink / raw)
  To: linux-mtd, linux-kernel; +Cc: richard, yi.zhang, liu.song11

We use function ubifs_dump_node() to dump bad node caused by some
reasons (Such as bit flipping caused by hardware error, writing bypass
ubifs or unknown bugs in ubifs). The node content can not be trusted
anymore, so we should prevent memory out-of-bounds accessing while
dumping node in following situations:

1. bad node_len: Dumping data according to 'ch->len' which may exceed
   the size of memory allocated for node.
2. bad node content: Some kinds of node can record additional data, eg.
   index node and orphan node, make sure the size of additional data
   not beyond the node length.
3. node_type changes: Read data according to type A, but expected type
   B, before that, node is allocated according to type B's size. Length
   of type A node is greater than type B node.

Commit acc5af3efa303d5f3 ("ubifs: Fix out-of-bounds memory access caused
by abnormal value of node_len") handles situation 1 for data node only,
it would be better if we can solve problems in above situations for all
kinds of nodes.

Patch 1 adds a new parameter 'node_len'(size of memory which is allocated
for the node) in function ubifs_dump_node(), safe dumping length of the
node should be: minimum(ch->len, c->ranges[node_type].max_len, node_len).
Besides, c->ranges[node_type].min_len can not greater than safe dumping
length, which may caused by node_type changes(situation 3).

Patch 2 reverts commit acc5af3efa303d5f ("ubifs: Fix out-of-bounds memory
access caused by abnormal value of node_len") to prepare for patch 3.

Patch 3 replaces modified function ubifs_dump_node() in all node dumping
places except for ubifs_dump_sleb().

Patch 4 removes unused function ubifs_dump_sleb(),

Patch 5 allows ubifs_dump_node() to dump all branches of the index node.

Some tests after patchset applied:
https://bugzilla.kernel.org/show_bug.cgi?id=208203

Zhihao Cheng (5):
  ubifs: Limit dumping length by size of memory which is allocated for
    the node
  Revert "ubifs: Fix out-of-bounds memory access caused by abnormal
    value of node_len"
  ubifs: Pass node length in all node dumping callers
  ubifs: ubifs_dump_sleb: Remove unused function
  ubifs: ubifs_dump_node: Dump all branches of the index node

 fs/ubifs/commit.c   |   4 +-
 fs/ubifs/debug.c    | 111 ++++++++++++++++++++++++++------------------
 fs/ubifs/debug.h    |   5 +-
 fs/ubifs/file.c     |   2 +-
 fs/ubifs/io.c       |  37 +++++----------
 fs/ubifs/journal.c  |   3 +-
 fs/ubifs/master.c   |   4 +-
 fs/ubifs/orphan.c   |   6 ++-
 fs/ubifs/recovery.c |   6 +--
 fs/ubifs/replay.c   |   4 +-
 fs/ubifs/sb.c       |   2 +-
 fs/ubifs/scan.c     |   4 +-
 fs/ubifs/super.c    |   2 +-
 fs/ubifs/tnc.c      |   8 ++--
 fs/ubifs/tnc_misc.c |   4 +-
 fs/ubifs/ubifs.h    |   4 +-
 16 files changed, 108 insertions(+), 98 deletions(-)

-- 
2.25.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH RFC 1/5] ubifs: Limit dumping length by size of memory which is allocated for the node
  2020-06-16  7:11 [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
@ 2020-06-16  7:11 ` Zhihao Cheng
  2020-06-16  7:11 ` [PATCH RFC 2/5] Revert "ubifs: Fix out-of-bounds memory access caused by abnormal value of node_len" Zhihao Cheng
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Zhihao Cheng @ 2020-06-16  7:11 UTC (permalink / raw)
  To: linux-mtd, linux-kernel; +Cc: richard, yi.zhang, liu.song11

To prevent memory out-of-bounds accessing in ubifs_dump_node(), actual
dumping length should be restricted by another condition(size of memory
which is allocated for the node).

This patch handles following situations (These situations may be caused
by bit flipping due to hardware error, writing bypass ubifs, unknown
bugs in ubifs, etc.):
1. bad node_len: Dumping data according to 'ch->len' which may exceed
   the size of memory allocated for node.
2. bad node content: Some kinds of node can record additional data, eg.
   index node and orphan node, make sure the size of additional data
   not beyond the node length.
3. node_type changes: Read data according to type A, but expected type
   B, before that, node is allocated according to type B's size. Length
   of type A node is greater than type B node.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/ubifs/debug.c | 63 ++++++++++++++++++++++++++++++++++++++----------
 fs/ubifs/debug.h |  3 ++-
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index 31288d8fa2ce..e995e553541d 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -291,9 +291,9 @@ void ubifs_dump_inode(struct ubifs_info *c, const struct inode *inode)
 	kfree(pdent);
 }
 
-void ubifs_dump_node(const struct ubifs_info *c, const void *node)
+void ubifs_dump_node(const struct ubifs_info *c, const void *node, int node_len)
 {
-	int i, n;
+	int i, n, type, safe_len, max_node_len, min_node_len;
 	union ubifs_key key;
 	const struct ubifs_ch *ch = node;
 	char key_buf[DBG_KEY_BUF_LEN];
@@ -306,10 +306,40 @@ void ubifs_dump_node(const struct ubifs_info *c, const void *node)
 		return;
 	}
 
+	/* Skip dumping unknown type node */
+	type = ch->node_type;
+	if (type < 0 || type >= UBIFS_NODE_TYPES_CNT) {
+		pr_err("node type %d was not recognized\n", type);
+		return;
+	}
+
 	spin_lock(&dbg_lock);
 	dump_ch(node);
 
-	switch (ch->node_type) {
+	if (c->ranges[type].max_len == 0) {
+		max_node_len = min_node_len = c->ranges[type].len;
+	} else {
+		max_node_len = c->ranges[type].max_len;
+		min_node_len = c->ranges[type].min_len;
+	}
+	safe_len = le32_to_cpu(ch->len);
+	safe_len = safe_len > 0 ? safe_len : 0;
+	safe_len = min3(safe_len, max_node_len, node_len);
+	if (safe_len < min_node_len) {
+		pr_err("node len(%d) is too short for %s, left %d bytes:\n",
+		       safe_len, dbg_ntype(type),
+		       safe_len > UBIFS_CH_SZ ?
+		       safe_len - (int)UBIFS_CH_SZ : 0);
+		if (safe_len > UBIFS_CH_SZ)
+			print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 32, 1,
+				       (void *)node + UBIFS_CH_SZ,
+				       safe_len - UBIFS_CH_SZ, 0);
+		goto out_unlock;
+	}
+	if (safe_len != le32_to_cpu(ch->len))
+		pr_err("\ttruncated node length      %d\n", safe_len);
+
+	switch (type) {
 	case UBIFS_PAD_NODE:
 	{
 		const struct ubifs_pad_node *pad = node;
@@ -453,7 +483,8 @@ void ubifs_dump_node(const struct ubifs_info *c, const void *node)
 		pr_err("\tnlen           %d\n", nlen);
 		pr_err("\tname           ");
 
-		if (nlen > UBIFS_MAX_NLEN)
+		if (nlen > UBIFS_MAX_NLEN ||
+		    nlen > safe_len - UBIFS_DENT_NODE_SZ)
 			pr_err("(bad name length, not printing, bad or corrupted node)");
 		else {
 			for (i = 0; i < nlen && dent->name[i]; i++)
@@ -467,7 +498,6 @@ void ubifs_dump_node(const struct ubifs_info *c, const void *node)
 	case UBIFS_DATA_NODE:
 	{
 		const struct ubifs_data_node *dn = node;
-		int dlen = le32_to_cpu(ch->len) - UBIFS_DATA_NODE_SZ;
 
 		key_read(c, &dn->key, &key);
 		pr_err("\tkey            %s\n",
@@ -475,10 +505,13 @@ void ubifs_dump_node(const struct ubifs_info *c, const void *node)
 		pr_err("\tsize           %u\n", le32_to_cpu(dn->size));
 		pr_err("\tcompr_typ      %d\n",
 		       (int)le16_to_cpu(dn->compr_type));
-		pr_err("\tdata size      %d\n", dlen);
-		pr_err("\tdata:\n");
+		pr_err("\tdata size      %u\n",
+		       le32_to_cpu(ch->len) - (unsigned int)UBIFS_DATA_NODE_SZ);
+		pr_err("\tdata (length = %d):\n",
+		       safe_len - (int)UBIFS_DATA_NODE_SZ);
 		print_hex_dump(KERN_ERR, "\t", DUMP_PREFIX_OFFSET, 32, 1,
-			       (void *)&dn->data, dlen, 0);
+			       (void *)&dn->data,
+			       safe_len - (int)UBIFS_DATA_NODE_SZ, 0);
 		break;
 	}
 	case UBIFS_TRUN_NODE:
@@ -495,9 +528,12 @@ void ubifs_dump_node(const struct ubifs_info *c, const void *node)
 	case UBIFS_IDX_NODE:
 	{
 		const struct ubifs_idx_node *idx = node;
+		int max_child_cnt = (safe_len - UBIFS_IDX_NODE_SZ) /
+				    (ubifs_idx_node_sz(c, 1) -
+				    UBIFS_IDX_NODE_SZ);
 
-		n = le16_to_cpu(idx->child_cnt);
-		pr_err("\tchild_cnt      %d\n", n);
+		n = min_t(int, le16_to_cpu(idx->child_cnt), max_child_cnt);
+		pr_err("\tchild_cnt      %d\n", (int)le16_to_cpu(idx->child_cnt));
 		pr_err("\tlevel          %d\n", (int)le16_to_cpu(idx->level));
 		pr_err("\tBranches:\n");
 
@@ -525,7 +561,7 @@ void ubifs_dump_node(const struct ubifs_info *c, const void *node)
 				le64_to_cpu(orph->cmt_no) & LLONG_MAX);
 		pr_err("\tlast node flag %llu\n",
 		       (unsigned long long)(le64_to_cpu(orph->cmt_no)) >> 63);
-		n = (le32_to_cpu(ch->len) - UBIFS_ORPH_NODE_SZ) >> 3;
+		n = (safe_len - UBIFS_ORPH_NODE_SZ) >> 3;
 		pr_err("\t%d orphan inode numbers:\n", n);
 		for (i = 0; i < n; i++)
 			pr_err("\t  ino %llu\n",
@@ -537,9 +573,10 @@ void ubifs_dump_node(const struct ubifs_info *c, const void *node)
 		break;
 	}
 	default:
-		pr_err("node type %d was not recognized\n",
-		       (int)ch->node_type);
+		pr_err("node type %d was not recognized\n", type);
 	}
+
+out_unlock:
 	spin_unlock(&dbg_lock);
 }
 
diff --git a/fs/ubifs/debug.h b/fs/ubifs/debug.h
index 7763639a426b..42610fa5f3a7 100644
--- a/fs/ubifs/debug.h
+++ b/fs/ubifs/debug.h
@@ -242,7 +242,8 @@ const char *dbg_get_key_dump(const struct ubifs_info *c,
 const char *dbg_snprintf_key(const struct ubifs_info *c,
 			     const union ubifs_key *key, char *buffer, int len);
 void ubifs_dump_inode(struct ubifs_info *c, const struct inode *inode);
-void ubifs_dump_node(const struct ubifs_info *c, const void *node);
+void ubifs_dump_node(const struct ubifs_info *c, const void *node,
+		     int node_len);
 void ubifs_dump_budget_req(const struct ubifs_budget_req *req);
 void ubifs_dump_lstats(const struct ubifs_lp_stats *lst);
 void ubifs_dump_budg(struct ubifs_info *c, const struct ubifs_budg_info *bi);
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH RFC 2/5] Revert "ubifs: Fix out-of-bounds memory access caused by abnormal value of node_len"
  2020-06-16  7:11 [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
  2020-06-16  7:11 ` [PATCH RFC 1/5] ubifs: Limit dumping length by size of memory which is allocated for the node Zhihao Cheng
@ 2020-06-16  7:11 ` Zhihao Cheng
  2020-06-16  7:11 ` [PATCH RFC 3/5] ubifs: Pass node length in all node dumping callers Zhihao Cheng
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Zhihao Cheng @ 2020-06-16  7:11 UTC (permalink / raw)
  To: linux-mtd, linux-kernel; +Cc: richard, yi.zhang, liu.song11

This reverts commit acc5af3efa303d5f36cc8c0f61716161f6ca1384.

No need to avoid memory oob in dumping for data node alone. Later, node
length will be passed into function 'ubifs_dump_node()' which replaces
all node dumping places.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/ubifs/io.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
index 7e4bfaf2871f..8ceb51478800 100644
--- a/fs/ubifs/io.c
+++ b/fs/ubifs/io.c
@@ -225,7 +225,7 @@ int ubifs_is_mapped(const struct ubifs_info *c, int lnum)
 int ubifs_check_node(const struct ubifs_info *c, const void *buf, int lnum,
 		     int offs, int quiet, int must_chk_crc)
 {
-	int err = -EINVAL, type, node_len, dump_node = 1;
+	int err = -EINVAL, type, node_len;
 	uint32_t crc, node_crc, magic;
 	const struct ubifs_ch *ch = buf;
 
@@ -278,22 +278,10 @@ int ubifs_check_node(const struct ubifs_info *c, const void *buf, int lnum,
 out_len:
 	if (!quiet)
 		ubifs_err(c, "bad node length %d", node_len);
-	if (type == UBIFS_DATA_NODE && node_len > UBIFS_DATA_NODE_SZ)
-		dump_node = 0;
 out:
 	if (!quiet) {
 		ubifs_err(c, "bad node at LEB %d:%d", lnum, offs);
-		if (dump_node) {
-			ubifs_dump_node(c, buf);
-		} else {
-			int safe_len = min3(node_len, c->leb_size - offs,
-				(int)UBIFS_MAX_DATA_NODE_SZ);
-			pr_err("\tprevent out-of-bounds memory access\n");
-			pr_err("\ttruncated data node length      %d\n", safe_len);
-			pr_err("\tcorrupted data node:\n");
-			print_hex_dump(KERN_ERR, "\t", DUMP_PREFIX_OFFSET, 32, 1,
-					buf, safe_len, 0);
-		}
+		ubifs_dump_node(c, buf);
 		dump_stack();
 	}
 	return err;
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH RFC 3/5] ubifs: Pass node length in all node dumping callers
  2020-06-16  7:11 [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
  2020-06-16  7:11 ` [PATCH RFC 1/5] ubifs: Limit dumping length by size of memory which is allocated for the node Zhihao Cheng
  2020-06-16  7:11 ` [PATCH RFC 2/5] Revert "ubifs: Fix out-of-bounds memory access caused by abnormal value of node_len" Zhihao Cheng
@ 2020-06-16  7:11 ` Zhihao Cheng
  2020-06-16  7:11 ` [PATCH RFC 4/5] ubifs: ubifs_dump_sleb: Remove unused function Zhihao Cheng
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Zhihao Cheng @ 2020-06-16  7:11 UTC (permalink / raw)
  To: linux-mtd, linux-kernel; +Cc: richard, yi.zhang, liu.song11

Function ubifs_dump_node() has been modified to avoid memory oob
accessing while dumping node, node length (corresponding to the
size of allocated memory for node) should be passed into all node
dumping callers.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/ubifs/commit.c   |  4 ++--
 fs/ubifs/debug.c    | 30 +++++++++++++++---------------
 fs/ubifs/file.c     |  2 +-
 fs/ubifs/io.c       | 23 +++++++++++------------
 fs/ubifs/journal.c  |  3 ++-
 fs/ubifs/master.c   |  4 ++--
 fs/ubifs/orphan.c   |  6 ++++--
 fs/ubifs/recovery.c |  6 +++---
 fs/ubifs/replay.c   |  4 ++--
 fs/ubifs/sb.c       |  2 +-
 fs/ubifs/scan.c     |  4 ++--
 fs/ubifs/super.c    |  2 +-
 fs/ubifs/tnc.c      |  8 ++++----
 fs/ubifs/tnc_misc.c |  4 ++--
 fs/ubifs/ubifs.h    |  4 ++--
 15 files changed, 54 insertions(+), 52 deletions(-)

diff --git a/fs/ubifs/commit.c b/fs/ubifs/commit.c
index ad292c5a43a9..2a3963e3b569 100644
--- a/fs/ubifs/commit.c
+++ b/fs/ubifs/commit.c
@@ -701,13 +701,13 @@ int dbg_check_old_index(struct ubifs_info *c, struct ubifs_zbranch *zroot)
 
 out_dump:
 	ubifs_err(c, "dumping index node (iip=%d)", i->iip);
-	ubifs_dump_node(c, idx);
+	ubifs_dump_node(c, idx, ubifs_idx_node_sz(c, c->fanout));
 	list_del(&i->list);
 	kfree(i);
 	if (!list_empty(&list)) {
 		i = list_entry(list.prev, struct idx_node, list);
 		ubifs_err(c, "dumping parent index node");
-		ubifs_dump_node(c, &i->idx);
+		ubifs_dump_node(c, &i->idx, ubifs_idx_node_sz(c, c->fanout));
 	}
 out_free:
 	while (!list_empty(&list)) {
diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index e995e553541d..24a6e6fb5e9a 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -871,7 +871,7 @@ void ubifs_dump_leb(const struct ubifs_info *c, int lnum)
 		cond_resched();
 		pr_err("Dumping node at LEB %d:%d len %d\n", lnum,
 		       snod->offs, snod->len);
-		ubifs_dump_node(c, snod->node);
+		ubifs_dump_node(c, snod->node, c->leb_size - snod->offs);
 	}
 
 	pr_err("(pid %d) finish dumping LEB %d\n", current->pid, lnum);
@@ -1248,7 +1248,7 @@ static int dbg_check_key_order(struct ubifs_info *c, struct ubifs_zbranch *zbr1,
 		ubifs_err(c, "but it should have key %s according to tnc",
 			  dbg_snprintf_key(c, &zbr1->key, key_buf,
 					   DBG_KEY_BUF_LEN));
-		ubifs_dump_node(c, dent1);
+		ubifs_dump_node(c, dent1, UBIFS_MAX_DENT_NODE_SZ);
 		goto out_free;
 	}
 
@@ -1260,7 +1260,7 @@ static int dbg_check_key_order(struct ubifs_info *c, struct ubifs_zbranch *zbr1,
 		ubifs_err(c, "but it should have key %s according to tnc",
 			  dbg_snprintf_key(c, &zbr2->key, key_buf,
 					   DBG_KEY_BUF_LEN));
-		ubifs_dump_node(c, dent2);
+		ubifs_dump_node(c, dent2, UBIFS_MAX_DENT_NODE_SZ);
 		goto out_free;
 	}
 
@@ -1279,9 +1279,9 @@ static int dbg_check_key_order(struct ubifs_info *c, struct ubifs_zbranch *zbr1,
 			  dbg_snprintf_key(c, &key, key_buf, DBG_KEY_BUF_LEN));
 
 	ubifs_msg(c, "first node at %d:%d\n", zbr1->lnum, zbr1->offs);
-	ubifs_dump_node(c, dent1);
+	ubifs_dump_node(c, dent1, UBIFS_MAX_DENT_NODE_SZ);
 	ubifs_msg(c, "second node at %d:%d\n", zbr2->lnum, zbr2->offs);
-	ubifs_dump_node(c, dent2);
+	ubifs_dump_node(c, dent2, UBIFS_MAX_DENT_NODE_SZ);
 
 out_free:
 	kfree(dent2);
@@ -2146,7 +2146,7 @@ static int check_leaf(struct ubifs_info *c, struct ubifs_zbranch *zbr,
 
 out_dump:
 	ubifs_msg(c, "dump of node at LEB %d:%d", zbr->lnum, zbr->offs);
-	ubifs_dump_node(c, node);
+	ubifs_dump_node(c, node, zbr->len);
 out_free:
 	kfree(node);
 	return err;
@@ -2279,7 +2279,7 @@ static int check_inodes(struct ubifs_info *c, struct fsck_data *fsckd)
 
 	ubifs_msg(c, "dump of the inode %lu sitting in LEB %d:%d",
 		  (unsigned long)fscki->inum, zbr->lnum, zbr->offs);
-	ubifs_dump_node(c, ino);
+	ubifs_dump_node(c, ino, zbr->len);
 	kfree(ino);
 	return -EINVAL;
 }
@@ -2350,12 +2350,12 @@ int dbg_check_data_nodes_order(struct ubifs_info *c, struct list_head *head)
 
 		if (sa->type != UBIFS_DATA_NODE) {
 			ubifs_err(c, "bad node type %d", sa->type);
-			ubifs_dump_node(c, sa->node);
+			ubifs_dump_node(c, sa->node, c->leb_size - sa->offs);
 			return -EINVAL;
 		}
 		if (sb->type != UBIFS_DATA_NODE) {
 			ubifs_err(c, "bad node type %d", sb->type);
-			ubifs_dump_node(c, sb->node);
+			ubifs_dump_node(c, sb->node, c->leb_size - sb->offs);
 			return -EINVAL;
 		}
 
@@ -2386,8 +2386,8 @@ int dbg_check_data_nodes_order(struct ubifs_info *c, struct list_head *head)
 	return 0;
 
 error_dump:
-	ubifs_dump_node(c, sa->node);
-	ubifs_dump_node(c, sb->node);
+	ubifs_dump_node(c, sa->node, c->leb_size - sa->offs);
+	ubifs_dump_node(c, sb->node, c->leb_size - sb->offs);
 	return -EINVAL;
 }
 
@@ -2418,13 +2418,13 @@ int dbg_check_nondata_nodes_order(struct ubifs_info *c, struct list_head *head)
 		if (sa->type != UBIFS_INO_NODE && sa->type != UBIFS_DENT_NODE &&
 		    sa->type != UBIFS_XENT_NODE) {
 			ubifs_err(c, "bad node type %d", sa->type);
-			ubifs_dump_node(c, sa->node);
+			ubifs_dump_node(c, sa->node, c->leb_size - sa->offs);
 			return -EINVAL;
 		}
 		if (sb->type != UBIFS_INO_NODE && sb->type != UBIFS_DENT_NODE &&
 		    sb->type != UBIFS_XENT_NODE) {
 			ubifs_err(c, "bad node type %d", sb->type);
-			ubifs_dump_node(c, sb->node);
+			ubifs_dump_node(c, sb->node, c->leb_size - sb->offs);
 			return -EINVAL;
 		}
 
@@ -2474,9 +2474,9 @@ int dbg_check_nondata_nodes_order(struct ubifs_info *c, struct list_head *head)
 
 error_dump:
 	ubifs_msg(c, "dumping first node");
-	ubifs_dump_node(c, sa->node);
+	ubifs_dump_node(c, sa->node, c->leb_size - sa->offs);
 	ubifs_msg(c, "dumping second node");
-	ubifs_dump_node(c, sb->node);
+	ubifs_dump_node(c, sb->node, c->leb_size - sb->offs);
 	return -EINVAL;
 	return 0;
 }
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 49fe062ce45e..c5b09183a8b1 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -92,7 +92,7 @@ static int read_block(struct inode *inode, void *addr, unsigned int block,
 dump:
 	ubifs_err(c, "bad data node (block %u, inode %lu)",
 		  block, inode->i_ino);
-	ubifs_dump_node(c, dn);
+	ubifs_dump_node(c, dn, UBIFS_MAX_DATA_NODE_SZ);
 	return -EINVAL;
 }
 
diff --git a/fs/ubifs/io.c b/fs/ubifs/io.c
index 8ceb51478800..4ef7d0830020 100644
--- a/fs/ubifs/io.c
+++ b/fs/ubifs/io.c
@@ -198,6 +198,7 @@ int ubifs_is_mapped(const struct ubifs_info *c, int lnum)
  * ubifs_check_node - check node.
  * @c: UBIFS file-system description object
  * @buf: node to check
+ * @len: node length
  * @lnum: logical eraseblock number
  * @offs: offset within the logical eraseblock
  * @quiet: print no messages
@@ -222,8 +223,8 @@ int ubifs_is_mapped(const struct ubifs_info *c, int lnum)
  * This function returns zero in case of success and %-EUCLEAN in case of bad
  * CRC or magic.
  */
-int ubifs_check_node(const struct ubifs_info *c, const void *buf, int lnum,
-		     int offs, int quiet, int must_chk_crc)
+int ubifs_check_node(const struct ubifs_info *c, const void *buf, int len,
+		     int lnum, int offs, int quiet, int must_chk_crc)
 {
 	int err = -EINVAL, type, node_len;
 	uint32_t crc, node_crc, magic;
@@ -281,7 +282,7 @@ int ubifs_check_node(const struct ubifs_info *c, const void *buf, int lnum,
 out:
 	if (!quiet) {
 		ubifs_err(c, "bad node at LEB %d:%d", lnum, offs);
-		ubifs_dump_node(c, buf);
+		ubifs_dump_node(c, buf, len);
 		dump_stack();
 	}
 	return err;
@@ -718,7 +719,7 @@ int ubifs_bg_wbufs_sync(struct ubifs_info *c)
 int ubifs_wbuf_write_nolock(struct ubifs_wbuf *wbuf, void *buf, int len)
 {
 	struct ubifs_info *c = wbuf->c;
-	int err, written, n, aligned_len = ALIGN(len, 8);
+	int err, n, written = 0, aligned_len = ALIGN(len, 8);
 
 	dbg_io("%d bytes (%s) to jhead %s wbuf at LEB %d:%d", len,
 	       dbg_ntype(((struct ubifs_ch *)buf)->node_type),
@@ -781,8 +782,6 @@ int ubifs_wbuf_write_nolock(struct ubifs_wbuf *wbuf, void *buf, int len)
 		goto exit;
 	}
 
-	written = 0;
-
 	if (wbuf->used) {
 		/*
 		 * The node is large enough and does not fit entirely within
@@ -878,7 +877,7 @@ int ubifs_wbuf_write_nolock(struct ubifs_wbuf *wbuf, void *buf, int len)
 out:
 	ubifs_err(c, "cannot write %d bytes to LEB %d:%d, error %d",
 		  len, wbuf->lnum, wbuf->offs, err);
-	ubifs_dump_node(c, buf);
+	ubifs_dump_node(c, buf, written + len);
 	dump_stack();
 	ubifs_dump_leb(c, wbuf->lnum);
 	return err;
@@ -921,7 +920,7 @@ int ubifs_write_node_hmac(struct ubifs_info *c, void *buf, int len, int lnum,
 
 	err = ubifs_leb_write(c, lnum, buf, offs, buf_len);
 	if (err)
-		ubifs_dump_node(c, buf);
+		ubifs_dump_node(c, buf, len);
 
 	return err;
 }
@@ -1004,7 +1003,7 @@ int ubifs_read_node_wbuf(struct ubifs_wbuf *wbuf, void *buf, int type, int len,
 		goto out;
 	}
 
-	err = ubifs_check_node(c, buf, lnum, offs, 0, 0);
+	err = ubifs_check_node(c, buf, len, lnum, offs, 0, 0);
 	if (err) {
 		ubifs_err(c, "expected node type %d", type);
 		return err;
@@ -1020,7 +1019,7 @@ int ubifs_read_node_wbuf(struct ubifs_wbuf *wbuf, void *buf, int type, int len,
 
 out:
 	ubifs_err(c, "bad node at LEB %d:%d", lnum, offs);
-	ubifs_dump_node(c, buf);
+	ubifs_dump_node(c, buf, len);
 	dump_stack();
 	return -EINVAL;
 }
@@ -1060,7 +1059,7 @@ int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
 		goto out;
 	}
 
-	err = ubifs_check_node(c, buf, lnum, offs, 0, 0);
+	err = ubifs_check_node(c, buf, len, lnum, offs, 0, 0);
 	if (err) {
 		ubifs_errc(c, "expected node type %d", type);
 		return err;
@@ -1078,7 +1077,7 @@ int ubifs_read_node(const struct ubifs_info *c, void *buf, int type, int len,
 	ubifs_errc(c, "bad node at LEB %d:%d, LEB mapping status %d", lnum,
 		   offs, ubi_is_mapped(c->ubi, lnum));
 	if (!c->probing) {
-		ubifs_dump_node(c, buf);
+		ubifs_dump_node(c, buf, len);
 		dump_stack();
 	}
 	return -EINVAL;
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index e5ec1afe1c66..a62fa19f7bcb 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -1555,7 +1555,8 @@ int ubifs_jnl_truncate(struct ubifs_info *c, const struct inode *inode,
 			if (dn_len <= 0 || dn_len > UBIFS_BLOCK_SIZE) {
 				ubifs_err(c, "bad data node (block %u, inode %lu)",
 					  blk, inode->i_ino);
-				ubifs_dump_node(c, dn);
+				ubifs_dump_node(c, dn, sz - UBIFS_INO_NODE_SZ -
+						UBIFS_TRUN_NODE_SZ);
 				goto out_free;
 			}
 
diff --git a/fs/ubifs/master.c b/fs/ubifs/master.c
index 911d0555b9f2..0df9a3dd0aaa 100644
--- a/fs/ubifs/master.c
+++ b/fs/ubifs/master.c
@@ -314,7 +314,7 @@ static int validate_master(const struct ubifs_info *c)
 
 out:
 	ubifs_err(c, "bad master node at offset %d error %d", c->mst_offs, err);
-	ubifs_dump_node(c, c->mst_node);
+	ubifs_dump_node(c, c->mst_node, c->mst_node_alsz);
 	return -EINVAL;
 }
 
@@ -392,7 +392,7 @@ int ubifs_read_master(struct ubifs_info *c)
 		if (c->leb_cnt < old_leb_cnt ||
 		    c->leb_cnt < UBIFS_MIN_LEB_CNT) {
 			ubifs_err(c, "bad leb_cnt on master node");
-			ubifs_dump_node(c, c->mst_node);
+			ubifs_dump_node(c, c->mst_node, c->mst_node_alsz);
 			return -EINVAL;
 		}
 
diff --git a/fs/ubifs/orphan.c b/fs/ubifs/orphan.c
index 2c294085ffed..30c8f3de91da 100644
--- a/fs/ubifs/orphan.c
+++ b/fs/ubifs/orphan.c
@@ -644,7 +644,8 @@ static int do_kill_orphans(struct ubifs_info *c, struct ubifs_scan_leb *sleb,
 		if (snod->type != UBIFS_ORPH_NODE) {
 			ubifs_err(c, "invalid node type %d in orphan area at %d:%d",
 				  snod->type, sleb->lnum, snod->offs);
-			ubifs_dump_node(c, snod->node);
+			ubifs_dump_node(c, snod->node,
+					c->leb_size - snod->offs);
 			err = -EINVAL;
 			goto out_free;
 		}
@@ -672,7 +673,8 @@ static int do_kill_orphans(struct ubifs_info *c, struct ubifs_scan_leb *sleb,
 			if (!first) {
 				ubifs_err(c, "out of order commit number %llu in orphan node at %d:%d",
 					  cmt_no, sleb->lnum, snod->offs);
-				ubifs_dump_node(c, snod->node);
+				ubifs_dump_node(c, snod->node,
+						c->leb_size - snod->offs);
 				err = -EINVAL;
 				goto out_free;
 			}
diff --git a/fs/ubifs/recovery.c b/fs/ubifs/recovery.c
index f116f7b3f9e5..f0d51dd21c9e 100644
--- a/fs/ubifs/recovery.c
+++ b/fs/ubifs/recovery.c
@@ -352,11 +352,11 @@ int ubifs_recover_master_node(struct ubifs_info *c)
 	ubifs_err(c, "failed to recover master node");
 	if (mst1) {
 		ubifs_err(c, "dumping first master node");
-		ubifs_dump_node(c, mst1);
+		ubifs_dump_node(c, mst1, c->leb_size - ((void *)mst1 - buf1));
 	}
 	if (mst2) {
 		ubifs_err(c, "dumping second master node");
-		ubifs_dump_node(c, mst2);
+		ubifs_dump_node(c, mst2, c->leb_size - ((void *)mst2 - buf2));
 	}
 	vfree(buf2);
 	vfree(buf1);
@@ -469,7 +469,7 @@ static int no_more_nodes(const struct ubifs_info *c, void *buf, int len,
 	 * The area after the common header size is not empty, so the common
 	 * header must be intact. Check it.
 	 */
-	if (ubifs_check_node(c, buf, lnum, offs, 1, 0) != -EUCLEAN) {
+	if (ubifs_check_node(c, buf, len, lnum, offs, 1, 0) != -EUCLEAN) {
 		dbg_rcvry("unexpected bad common header at %d:%d", lnum, offs);
 		return 0;
 	}
diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c
index b69ffac7e415..6966003501bb 100644
--- a/fs/ubifs/replay.c
+++ b/fs/ubifs/replay.c
@@ -827,7 +827,7 @@ static int replay_bud(struct ubifs_info *c, struct bud_entry *b)
 
 out_dump:
 	ubifs_err(c, "bad node is at LEB %d:%d", lnum, snod->offs);
-	ubifs_dump_node(c, snod->node);
+	ubifs_dump_node(c, snod->node, c->leb_size - snod->offs);
 	ubifs_scan_destroy(sleb);
 	return -EINVAL;
 }
@@ -1125,7 +1125,7 @@ static int replay_log_leb(struct ubifs_info *c, int lnum, int offs, void *sbuf)
 out_dump:
 	ubifs_err(c, "log error detected while replaying the log at LEB %d:%d",
 		  lnum, offs + snod->offs);
-	ubifs_dump_node(c, snod->node);
+	ubifs_dump_node(c, snod->node, c->leb_size - snod->offs);
 	ubifs_scan_destroy(sleb);
 	return -EINVAL;
 }
diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 4b4b65b48c57..0e1988d426a0 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -502,7 +502,7 @@ static int validate_sb(struct ubifs_info *c, struct ubifs_sb_node *sup)
 
 failed:
 	ubifs_err(c, "bad superblock, error %d", err);
-	ubifs_dump_node(c, sup);
+	ubifs_dump_node(c, sup, ALIGN(UBIFS_SB_NODE_SZ, c->min_io_size));
 	return -EINVAL;
 }
 
diff --git a/fs/ubifs/scan.c b/fs/ubifs/scan.c
index c69cdb5e65bc..84a9157dcc32 100644
--- a/fs/ubifs/scan.c
+++ b/fs/ubifs/scan.c
@@ -76,7 +76,7 @@ int ubifs_scan_a_node(const struct ubifs_info *c, void *buf, int len, int lnum,
 	dbg_scan("scanning %s at LEB %d:%d",
 		 dbg_ntype(ch->node_type), lnum, offs);
 
-	if (ubifs_check_node(c, buf, lnum, offs, quiet, 1))
+	if (ubifs_check_node(c, buf, len, lnum, offs, quiet, 1))
 		return SCANNED_A_CORRUPT_NODE;
 
 	if (ch->node_type == UBIFS_PAD_NODE) {
@@ -90,7 +90,7 @@ int ubifs_scan_a_node(const struct ubifs_info *c, void *buf, int len, int lnum,
 			if (!quiet) {
 				ubifs_err(c, "bad pad node at LEB %d:%d",
 					  lnum, offs);
-				ubifs_dump_node(c, pad);
+				ubifs_dump_node(c, pad, len);
 			}
 			return SCANNED_A_BAD_PAD_NODE;
 		}
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 7fc2f3f07c16..18f289f2ffb6 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -235,7 +235,7 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum)
 
 out_invalid:
 	ubifs_err(c, "inode %lu validation failed, error %d", inode->i_ino, err);
-	ubifs_dump_node(c, ino);
+	ubifs_dump_node(c, ino, UBIFS_MAX_INO_NODE_SZ);
 	ubifs_dump_inode(c, inode);
 	err = -EINVAL;
 out_ino:
diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c
index e8e7b0e9532e..b2f9e927acc3 100644
--- a/fs/ubifs/tnc.c
+++ b/fs/ubifs/tnc.c
@@ -316,7 +316,7 @@ static int lnc_add(struct ubifs_info *c, struct ubifs_zbranch *zbr,
 	err = ubifs_validate_entry(c, dent);
 	if (err) {
 		dump_stack();
-		ubifs_dump_node(c, dent);
+		ubifs_dump_node(c, dent, zbr->len);
 		return err;
 	}
 
@@ -349,7 +349,7 @@ static int lnc_add_directly(struct ubifs_info *c, struct ubifs_zbranch *zbr,
 	err = ubifs_validate_entry(c, node);
 	if (err) {
 		dump_stack();
-		ubifs_dump_node(c, node);
+		ubifs_dump_node(c, node, zbr->len);
 		return err;
 	}
 
@@ -1700,7 +1700,7 @@ static int validate_data_node(struct ubifs_info *c, void *buf,
 		goto out_err;
 	}
 
-	err = ubifs_check_node(c, buf, zbr->lnum, zbr->offs, 0, 0);
+	err = ubifs_check_node(c, buf, zbr->len, zbr->lnum, zbr->offs, 0, 0);
 	if (err) {
 		ubifs_err(c, "expected node type %d", UBIFS_DATA_NODE);
 		goto out;
@@ -1734,7 +1734,7 @@ static int validate_data_node(struct ubifs_info *c, void *buf,
 	err = -EINVAL;
 out:
 	ubifs_err(c, "bad node at LEB %d:%d", zbr->lnum, zbr->offs);
-	ubifs_dump_node(c, buf);
+	ubifs_dump_node(c, buf, zbr->len);
 	dump_stack();
 	return err;
 }
diff --git a/fs/ubifs/tnc_misc.c b/fs/ubifs/tnc_misc.c
index 49cb34c3f324..302fe1d8719d 100644
--- a/fs/ubifs/tnc_misc.c
+++ b/fs/ubifs/tnc_misc.c
@@ -390,7 +390,7 @@ static int read_znode(struct ubifs_info *c, struct ubifs_zbranch *zzbr,
 
 out_dump:
 	ubifs_err(c, "bad indexing node at LEB %d:%d, error %d", lnum, offs, err);
-	ubifs_dump_node(c, idx);
+	ubifs_dump_node(c, idx, c->max_idx_node_sz);
 	kfree(idx);
 	return -EINVAL;
 }
@@ -489,7 +489,7 @@ int ubifs_tnc_read_node(struct ubifs_info *c, struct ubifs_zbranch *zbr,
 			  zbr->lnum, zbr->offs);
 		dbg_tnck(key, "looked for key ");
 		dbg_tnck(&key1, "but found node's key ");
-		ubifs_dump_node(c, node);
+		ubifs_dump_node(c, node, zbr->len);
 		return -EINVAL;
 	}
 
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index bff682309fbe..2393715a9251 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1718,8 +1718,8 @@ int ubifs_write_node(struct ubifs_info *c, void *node, int len, int lnum,
 		     int offs);
 int ubifs_write_node_hmac(struct ubifs_info *c, void *buf, int len, int lnum,
 			  int offs, int hmac_offs);
-int ubifs_check_node(const struct ubifs_info *c, const void *buf, int lnum,
-		     int offs, int quiet, int must_chk_crc);
+int ubifs_check_node(const struct ubifs_info *c, const void *buf, int len,
+		     int lnum, int offs, int quiet, int must_chk_crc);
 void ubifs_init_node(struct ubifs_info *c, void *buf, int len, int pad);
 void ubifs_crc_node(struct ubifs_info *c, void *buf, int len);
 void ubifs_prepare_node(struct ubifs_info *c, void *buf, int len, int pad);
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH RFC 4/5] ubifs: ubifs_dump_sleb: Remove unused function
  2020-06-16  7:11 [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
                   ` (2 preceding siblings ...)
  2020-06-16  7:11 ` [PATCH RFC 3/5] ubifs: Pass node length in all node dumping callers Zhihao Cheng
@ 2020-06-16  7:11 ` Zhihao Cheng
  2020-06-16  7:11 ` [PATCH RFC 5/5] ubifs: ubifs_dump_node: Dump all branches of the index node Zhihao Cheng
  2020-10-19  3:05 ` [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
  5 siblings, 0 replies; 8+ messages in thread
From: Zhihao Cheng @ 2020-06-16  7:11 UTC (permalink / raw)
  To: linux-mtd, linux-kernel; +Cc: richard, yi.zhang, liu.song11

Function ubifs_dump_sleb() is defined but unused, it can be removed.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/ubifs/debug.c | 16 ----------------
 fs/ubifs/debug.h |  2 --
 2 files changed, 18 deletions(-)

diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index 24a6e6fb5e9a..2d07615369f9 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -828,22 +828,6 @@ void ubifs_dump_lpt_info(struct ubifs_info *c)
 	spin_unlock(&dbg_lock);
 }
 
-void ubifs_dump_sleb(const struct ubifs_info *c,
-		     const struct ubifs_scan_leb *sleb, int offs)
-{
-	struct ubifs_scan_node *snod;
-
-	pr_err("(pid %d) start dumping scanned data from LEB %d:%d\n",
-	       current->pid, sleb->lnum, offs);
-
-	list_for_each_entry(snod, &sleb->nodes, list) {
-		cond_resched();
-		pr_err("Dumping node at LEB %d:%d len %d\n",
-		       sleb->lnum, snod->offs, snod->len);
-		ubifs_dump_node(c, snod->node);
-	}
-}
-
 void ubifs_dump_leb(const struct ubifs_info *c, int lnum)
 {
 	struct ubifs_scan_leb *sleb;
diff --git a/fs/ubifs/debug.h b/fs/ubifs/debug.h
index 42610fa5f3a7..ed966108da80 100644
--- a/fs/ubifs/debug.h
+++ b/fs/ubifs/debug.h
@@ -252,8 +252,6 @@ void ubifs_dump_lprop(const struct ubifs_info *c,
 void ubifs_dump_lprops(struct ubifs_info *c);
 void ubifs_dump_lpt_info(struct ubifs_info *c);
 void ubifs_dump_leb(const struct ubifs_info *c, int lnum);
-void ubifs_dump_sleb(const struct ubifs_info *c,
-		     const struct ubifs_scan_leb *sleb, int offs);
 void ubifs_dump_znode(const struct ubifs_info *c,
 		      const struct ubifs_znode *znode);
 void ubifs_dump_heap(struct ubifs_info *c, struct ubifs_lpt_heap *heap,
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH RFC 5/5] ubifs: ubifs_dump_node: Dump all branches of the index node
  2020-06-16  7:11 [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
                   ` (3 preceding siblings ...)
  2020-06-16  7:11 ` [PATCH RFC 4/5] ubifs: ubifs_dump_sleb: Remove unused function Zhihao Cheng
@ 2020-06-16  7:11 ` Zhihao Cheng
  2020-10-19  3:05 ` [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
  5 siblings, 0 replies; 8+ messages in thread
From: Zhihao Cheng @ 2020-06-16  7:11 UTC (permalink / raw)
  To: linux-mtd, linux-kernel; +Cc: richard, yi.zhang, liu.song11

An index node can have up to c->fanout branches, all branches should be
displayed while dumping index node.

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/ubifs/debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index 2d07615369f9..a65f73e91b4f 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -537,7 +537,7 @@ void ubifs_dump_node(const struct ubifs_info *c, const void *node, int node_len)
 		pr_err("\tlevel          %d\n", (int)le16_to_cpu(idx->level));
 		pr_err("\tBranches:\n");
 
-		for (i = 0; i < n && i < c->fanout - 1; i++) {
+		for (i = 0; i < n && i < c->fanout; i++) {
 			const struct ubifs_branch *br;
 
 			br = ubifs_idx_branch(c, idx, i);
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node
  2020-06-16  7:11 [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
                   ` (4 preceding siblings ...)
  2020-06-16  7:11 ` [PATCH RFC 5/5] ubifs: ubifs_dump_node: Dump all branches of the index node Zhihao Cheng
@ 2020-10-19  3:05 ` Zhihao Cheng
  2020-10-31 21:13   ` Richard Weinberger
  5 siblings, 1 reply; 8+ messages in thread
From: Zhihao Cheng @ 2020-10-19  3:05 UTC (permalink / raw)
  To: linux-mtd, linux-kernel; +Cc: richard, liu.song11, yi.zhang

在 2020/6/16 15:11, Zhihao Cheng 写道:
> We use function ubifs_dump_node() to dump bad node caused by some
> reasons (Such as bit flipping caused by hardware error, writing bypass
> ubifs or unknown bugs in ubifs). The node content can not be trusted
> anymore, so we should prevent memory out-of-bounds accessing while
> dumping node in following situations:
> 
> 1. bad node_len: Dumping data according to 'ch->len' which may exceed
>     the size of memory allocated for node.
> 2. bad node content: Some kinds of node can record additional data, eg.
>     index node and orphan node, make sure the size of additional data
>     not beyond the node length.
> 3. node_type changes: Read data according to type A, but expected type
>     B, before that, node is allocated according to type B's size. Length
>     of type A node is greater than type B node.
> 
> Commit acc5af3efa303d5f3 ("ubifs: Fix out-of-bounds memory access caused
> by abnormal value of node_len") handles situation 1 for data node only,
> it would be better if we can solve problems in above situations for all
> kinds of nodes.
> 
> Patch 1 adds a new parameter 'node_len'(size of memory which is allocated
> for the node) in function ubifs_dump_node(), safe dumping length of the
> node should be: minimum(ch->len, c->ranges[node_type].max_len, node_len).
> Besides, c->ranges[node_type].min_len can not greater than safe dumping
> length, which may caused by node_type changes(situation 3).
> 
> Patch 2 reverts commit acc5af3efa303d5f ("ubifs: Fix out-of-bounds memory
> access caused by abnormal value of node_len") to prepare for patch 3.
> 
> Patch 3 replaces modified function ubifs_dump_node() in all node dumping
> places except for ubifs_dump_sleb().
> 
> Patch 4 removes unused function ubifs_dump_sleb(),
> 
> Patch 5 allows ubifs_dump_node() to dump all branches of the index node.
> 
> Some tests after patchset applied:
> https://bugzilla.kernel.org/show_bug.cgi?id=208203
> 
> Zhihao Cheng (5):
>    ubifs: Limit dumping length by size of memory which is allocated for
>      the node
>    Revert "ubifs: Fix out-of-bounds memory access caused by abnormal
>      value of node_len"
>    ubifs: Pass node length in all node dumping callers
>    ubifs: ubifs_dump_sleb: Remove unused function
>    ubifs: ubifs_dump_node: Dump all branches of the index node
> 
>   fs/ubifs/commit.c   |   4 +-
>   fs/ubifs/debug.c    | 111 ++++++++++++++++++++++++++------------------
>   fs/ubifs/debug.h    |   5 +-
>   fs/ubifs/file.c     |   2 +-
>   fs/ubifs/io.c       |  37 +++++----------
>   fs/ubifs/journal.c  |   3 +-
>   fs/ubifs/master.c   |   4 +-
>   fs/ubifs/orphan.c   |   6 ++-
>   fs/ubifs/recovery.c |   6 +--
>   fs/ubifs/replay.c   |   4 +-
>   fs/ubifs/sb.c       |   2 +-
>   fs/ubifs/scan.c     |   4 +-
>   fs/ubifs/super.c    |   2 +-
>   fs/ubifs/tnc.c      |   8 ++--
>   fs/ubifs/tnc_misc.c |   4 +-
>   fs/ubifs/ubifs.h    |   4 +-
>   16 files changed, 108 insertions(+), 98 deletions(-)
> 
ping, although it is not a serious problem for ubifs, but dumping extra 
memory by formating specified ubifs img may cause security problem.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node
  2020-10-19  3:05 ` [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
@ 2020-10-31 21:13   ` Richard Weinberger
  0 siblings, 0 replies; 8+ messages in thread
From: Richard Weinberger @ 2020-10-31 21:13 UTC (permalink / raw)
  To: Zhihao Cheng; +Cc: linux-mtd, LKML, Richard Weinberger, liu.song11, zhangyi (F)

On Mon, Oct 19, 2020 at 5:13 AM Zhihao Cheng <chengzhihao1@huawei.com> wrote:
>
> 在 2020/6/16 15:11, Zhihao Cheng 写道:
> > We use function ubifs_dump_node() to dump bad node caused by some
> > reasons (Such as bit flipping caused by hardware error, writing bypass
> > ubifs or unknown bugs in ubifs). The node content can not be trusted
> > anymore, so we should prevent memory out-of-bounds accessing while
> > dumping node in following situations:
> >
> > 1. bad node_len: Dumping data according to 'ch->len' which may exceed
> >     the size of memory allocated for node.
> > 2. bad node content: Some kinds of node can record additional data, eg.
> >     index node and orphan node, make sure the size of additional data
> >     not beyond the node length.
> > 3. node_type changes: Read data according to type A, but expected type
> >     B, before that, node is allocated according to type B's size. Length
> >     of type A node is greater than type B node.
> >
> > Commit acc5af3efa303d5f3 ("ubifs: Fix out-of-bounds memory access caused
> > by abnormal value of node_len") handles situation 1 for data node only,
> > it would be better if we can solve problems in above situations for all
> > kinds of nodes.
> >
> > Patch 1 adds a new parameter 'node_len'(size of memory which is allocated
> > for the node) in function ubifs_dump_node(), safe dumping length of the
> > node should be: minimum(ch->len, c->ranges[node_type].max_len, node_len).
> > Besides, c->ranges[node_type].min_len can not greater than safe dumping
> > length, which may caused by node_type changes(situation 3).
> >
> > Patch 2 reverts commit acc5af3efa303d5f ("ubifs: Fix out-of-bounds memory
> > access caused by abnormal value of node_len") to prepare for patch 3.
> >
> > Patch 3 replaces modified function ubifs_dump_node() in all node dumping
> > places except for ubifs_dump_sleb().
> >
> > Patch 4 removes unused function ubifs_dump_sleb(),
> >
> > Patch 5 allows ubifs_dump_node() to dump all branches of the index node.
> >
> > Some tests after patchset applied:
> > https://bugzilla.kernel.org/show_bug.cgi?id=208203
> >
> > Zhihao Cheng (5):
> >    ubifs: Limit dumping length by size of memory which is allocated for
> >      the node
> >    Revert "ubifs: Fix out-of-bounds memory access caused by abnormal
> >      value of node_len"
> >    ubifs: Pass node length in all node dumping callers
> >    ubifs: ubifs_dump_sleb: Remove unused function
> >    ubifs: ubifs_dump_node: Dump all branches of the index node
> >
> >   fs/ubifs/commit.c   |   4 +-
> >   fs/ubifs/debug.c    | 111 ++++++++++++++++++++++++++------------------
> >   fs/ubifs/debug.h    |   5 +-
> >   fs/ubifs/file.c     |   2 +-
> >   fs/ubifs/io.c       |  37 +++++----------
> >   fs/ubifs/journal.c  |   3 +-
> >   fs/ubifs/master.c   |   4 +-
> >   fs/ubifs/orphan.c   |   6 ++-
> >   fs/ubifs/recovery.c |   6 +--
> >   fs/ubifs/replay.c   |   4 +-
> >   fs/ubifs/sb.c       |   2 +-
> >   fs/ubifs/scan.c     |   4 +-
> >   fs/ubifs/super.c    |   2 +-
> >   fs/ubifs/tnc.c      |   8 ++--
> >   fs/ubifs/tnc_misc.c |   4 +-
> >   fs/ubifs/ubifs.h    |   4 +-
> >   16 files changed, 108 insertions(+), 98 deletions(-)
> >
> ping, although it is not a serious problem for ubifs, but dumping extra
> memory by formating specified ubifs img may cause security problem.

Thanks for reminding me, yes this needs fixing.
I'll give it a try and then apply it for next.

-- 
Thanks,
//richard

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-10-31 21:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-16  7:11 [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
2020-06-16  7:11 ` [PATCH RFC 1/5] ubifs: Limit dumping length by size of memory which is allocated for the node Zhihao Cheng
2020-06-16  7:11 ` [PATCH RFC 2/5] Revert "ubifs: Fix out-of-bounds memory access caused by abnormal value of node_len" Zhihao Cheng
2020-06-16  7:11 ` [PATCH RFC 3/5] ubifs: Pass node length in all node dumping callers Zhihao Cheng
2020-06-16  7:11 ` [PATCH RFC 4/5] ubifs: ubifs_dump_sleb: Remove unused function Zhihao Cheng
2020-06-16  7:11 ` [PATCH RFC 5/5] ubifs: ubifs_dump_node: Dump all branches of the index node Zhihao Cheng
2020-10-19  3:05 ` [PATCH RFC 0/5] ubifs: Prevent memory oob accessing while dumping node Zhihao Cheng
2020-10-31 21:13   ` Richard Weinberger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).