linux-kernel - [PATCH] thunderbolt: Fix a missing-check bug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <1540058151-17116-1-git-send-email-wang6495@umn.edu>
Date:   Sat, 20 Oct 2018 12:55:51 -0500
From:   Wenwen Wang <wang6495@....edu>
To:     Wenwen Wang <wang6495@....edu>
Cc:     Kangjie Lu <kjlu@....edu>,
        Andreas Noever <andreas.noever@...il.com>,
        Michael Jamet <michael.jamet@...el.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Yehezkel Bernat <YehezkelShB@...il.com>,
        linux-kernel@...r.kernel.org (open list)
Subject: [PATCH] thunderbolt: Fix a missing-check bug

In tb_ctl_rx_callback(), the checksum of the received control packet is
calculated on 'pkg->buffer' through tb_crc() and saved to 'crc32', Then,
'crc32' is compared with the received checksum to confirm the integrity of
the received packet. If the checksum does not match, the packet will be
dropped. In the following execution, 'pkg->buffer' will be copied through
req->copy() and processed if there is an active request and the packet is
what is expected.

The problem here is that the above checking process is performed directly
on the buffer 'pkg->buffer', which is actually a DMA region. Given that the
DMA region can also be accessed directly by a device at any time, it is
possible that a malicious device controlled by an attacker can race to
modify the content in 'pkg->buffer' after the checksum checking but before
req->copy(). By doing so, the attacker can inject malicious data, which can
cause undefined behavior of the kernel and introduce potential security
risk.

This patch allocates a new buffer 'buf' to hold the data in 'pkg->buffer'.
By performing the checking and copying on 'buf', rather than 'pkg->buffer',
the above issue can be avoided.

Signed-off-by: Wenwen Wang <wang6495@....edu>
---
 drivers/thunderbolt/ctl.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c
index 37a7f4c..9e40572 100644
--- a/drivers/thunderbolt/ctl.c
+++ b/drivers/thunderbolt/ctl.c
@@ -409,6 +409,8 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 	struct ctl_pkg *pkg = container_of(frame, typeof(*pkg), frame);
 	struct tb_cfg_request *req;
 	__be32 crc32;
+	void *pkg_buf = pkg->buffer;
+	void *buf = NULL;

 	if (canceled)
 		return; /*
@@ -422,6 +424,13 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 		goto rx;
 	}

+	buf = kzalloc(frame->size, GFP_KERNEL);
+	if (!buf)
+		goto rx;
+
+	memcpy(buf, pkg->buffer, frame->size);
+	pkg->buffer = buf;
+
 	frame->size -= 4; /* remove checksum */
 	crc32 = tb_crc(pkg->buffer, frame->size);
 	be32_to_cpu_array(pkg->buffer, pkg->buffer, frame->size / 4);
@@ -476,6 +485,10 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 	}

 rx:
+	if (buf) {
+		pkg->buffer = pkg_buf;
+		kfree(buf);
+	}
 	tb_ctl_rx_submit(pkg);
 }

-- 
2.7.4