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: <1540060698-17331-1-git-send-email-wang6495@umn.edu>
Date:   Sat, 20 Oct 2018 13:38:18 -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 icm_copy(), the packet id 'hdr->packet_id' is firstly compared against
'req->npackets'. If it is less than 'req->npackets', the received packet.
i.e., 'pkg->buffer', is then copied to 'req->response + offset' through
memcpy(). It is worth noting that 'offset' is also calculated based on
'hdr->packet_id'. The problem here is that both the check and the
calculation are conducted directly on 'pkg->buffer', which is actually a
DMA memory region. Given that a device can also access the DMA region at
any time, it is possible that a malicious device controlled by an attacker
can modify the packet id after the check. By doing so, the attacker can
supply comprised value into 'offset' and thus cause unexpected errors.

This patch firstly copies the header of the packet and performs the check
and the calculation on the copied version to fix the above issue. This
patch also rewrites the header in 'req->response + offset' using the
copied header to avoid a potential inconsistency issue.

Signed-off-by: Wenwen Wang <wang6495@....edu>
---
 drivers/thunderbolt/icm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 28fc4ce..3beec4b 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -186,15 +186,18 @@ static bool icm_match(const struct tb_cfg_request *req,

 static bool icm_copy(struct tb_cfg_request *req, const struct ctl_pkg *pkg)
 {
-	const struct icm_pkg_header *hdr = pkg->buffer;
+	struct icm_pkg_header hdr;

-	if (hdr->packet_id < req->npackets) {
-		size_t offset = hdr->packet_id * req->response_size;
+	memcpy(&hdr, pkg->buffer, sizeof(hdr));
+
+	if (hdr.packet_id < req->npackets) {
+		size_t offset = hdr.packet_id * req->response_size;

 		memcpy(req->response + offset, pkg->buffer, req->response_size);
+		(struct icm_pkg_header *)(req->response + offset) = hdr;
 	}

-	return hdr->packet_id == hdr->total_packets - 1;
+	return hdr.packet_id == hdr.total_packets - 1;
 }

 static int icm_request(struct tb *tb, const void *request, size_t request_size,
-- 
2.7.4