Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ubifs: Fix potentially out-of-bounds memory access in ubifs_dump_node
@ 2019-12-16 15:44 Liu Song
  2020-01-12 22:49 ` Richard Weinberger
  0 siblings, 1 reply; 3+ messages in thread
From: Liu Song @ 2019-12-16 15:44 UTC (permalink / raw)
  To: richard; +Cc: linux-mtd, linux-kernel, liu.song11

From: Liu Song <liu.song11@zte.com.cn>

In “ubifs_check_node”, when “node_len + offs> c-> leb_size”, then
goto label of "out_len". Then, in the following "ubifs_dump_node",
if inode type is "UBIFS_DATA_NODE", in "print_hex_dump", an
out-of-bounds access may occur due to the wrong "ch->len".
We encountered this problem in our environment. If "ch-> len" is
very large, it may even cause the kernel to crash.

There are three reasons to choose to remove "print_hex_dump".
1) As mentioned earlier, the exception "ch-> len" may cause an
exception in "print_hex_dump";
2) Data nodes are often large. When printing data content in
"print_hex_dump", a large amount of output will bring a high load
on the system, and may even cause a watchdog reset;
3) Even if there is a CRC check error, the stuff of file is difficult
to identify, and difficult to find the problem from a large amount of
output. We have already output the LEB and offset of the node. So we
can take the initiative to view the data of interest, instead of
printing it directly.

Signed-off-by: Liu Song <liu.song11@zte.com.cn>
---
 fs/ubifs/debug.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index 0f5a480fe264..e9fc6f0daec8 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -476,9 +476,6 @@ void ubifs_dump_node(const struct ubifs_info *c, const void *node)
 		pr_err("\tcompr_typ      %d\n",
 		       (int)le16_to_cpu(dn->compr_type));
 		pr_err("\tdata size      %d\n", dlen);
-		pr_err("\tdata:\n");
-		print_hex_dump(KERN_ERR, "\t", DUMP_PREFIX_OFFSET, 32, 1,
-			       (void *)&dn->data, dlen, 0);
 		break;
 	}
 	case UBIFS_TRUN_NODE:
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

* Re: [PATCH] ubifs: Fix potentially out-of-bounds memory access in ubifs_dump_node
  2019-12-16 15:44 [PATCH] ubifs: Fix potentially out-of-bounds memory access in ubifs_dump_node Liu Song
@ 2020-01-12 22:49 ` Richard Weinberger
  0 siblings, 0 replies; 3+ messages in thread
From: Richard Weinberger @ 2020-01-12 22:49 UTC (permalink / raw)
  To: Liu Song; +Cc: Richard Weinberger, linux-mtd, LKML, liu.song11

On Mon, Dec 16, 2019 at 4:45 PM Liu Song <fishland@aliyun.com> wrote:
>
> From: Liu Song <liu.song11@zte.com.cn>
>
> In “ubifs_check_node”, when “node_len + offs> c-> leb_size”, then
> goto label of "out_len". Then, in the following "ubifs_dump_node",
> if inode type is "UBIFS_DATA_NODE", in "print_hex_dump", an
> out-of-bounds access may occur due to the wrong "ch->len".
> We encountered this problem in our environment. If "ch-> len" is
> very large, it may even cause the kernel to crash.
>
> There are three reasons to choose to remove "print_hex_dump".
> 1) As mentioned earlier, the exception "ch-> len" may cause an
> exception in "print_hex_dump";
> 2) Data nodes are often large. When printing data content in
> "print_hex_dump", a large amount of output will bring a high load
> on the system, and may even cause a watchdog reset;
> 3) Even if there is a CRC check error, the stuff of file is difficult
> to identify, and difficult to find the problem from a large amount of
> output. We have already output the LEB and offset of the node. So we
> can take the initiative to view the data of interest, instead of
> printing it directly.

If UBIFS dumps data nodes due an error we are already in deep trouble
and having the content of bad data nodes can be helpful.
This feature helped me more than once to debug issues.

-- 
Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

* Re: [PATCH] ubifs: Fix potentially out-of-bounds memory access in ubifs_dump_node
       [not found] ` <202001131229371470661@zte.com.cn>
@ 2020-01-13  7:54   ` Richard Weinberger
  0 siblings, 0 replies; 3+ messages in thread
From: Richard Weinberger @ 2020-01-13  7:54 UTC (permalink / raw)
  To: liu song11; +Cc: Richard Weinberger, linux-mtd, linux-kernel, jiang xuexin

----- Ursprüngliche Mail -----
> Problems with storage devices are indeed a problem, But because the wrong
> "ch->len" causes the kernel to crash, this cost is too heavy. We should
> avoid kernel crashes due to such errors.
> 
> 
> Although a crc error was found in "ubifs_check_node", it is difficult to
> simply judge whether "ch->len" is reasonable, so I think we only need to know
> the _location_ of the error data node, and it is not necessary to present its
> contents together.

What we can try is optionally passing the buffer length to ubifs_dump_node().
If crc is bad but ch->len is within bounds we can still safely dump.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-16 15:44 [PATCH] ubifs: Fix potentially out-of-bounds memory access in ubifs_dump_node Liu Song
2020-01-12 22:49 ` Richard Weinberger
     [not found] <20191216154441.6648-1-fishland@aliyun.com, CAFLxGvyU=zh23vkYiAGRzyd4LGJodLwRRa1S03THAoSaSL=dGA@mail.gmail.com>
     [not found] ` <202001131229371470661@zte.com.cn>
2020-01-13  7:54   ` Richard Weinberger

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git