Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Wenwen Wang <wang6495@umn.edu>
To:     Wenwen Wang <wang6495@umn.edu>
Cc:     Kangjie Lu <kjlu@umn.edu>,
        Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Yehezkel Bernat <YehezkelShB@gmail.com>,
        linux-kernel@vger.kernel.org (open list)
Subject: [PATCH] thunderbolt: Fix a missing-check bug
Date:   Wed, 17 Oct 2018 09:00:29 -0500
Message-Id: <1539784829-1159-1-git-send-email-wang6495@umn.edu>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

In tb_cfg_copy(), the header of the received control package, which is in
the buffer 'pkg->buffer', is firstly parsed through parse_header() to make
sure the header is in the expected format. In parse_header(), the header is
actually checked by invoking check_header(), which checks the 'unknown'
field of the header and the response route acquired through
'tb_cfg_get_route(header)'. If there is no error in this checking process,
the package, including the header, is then copied to 'req->response' in
tb_cfg_copy() through memcpy() and the parsing result is saved to
'req->result'.

The problem here is that the whole parsing and checking process is
conducted directly on the buffer 'pkg->buffer', which is actually a DMA
region and allocated through dma_pool_alloc() in tb_ctl_pkg_alloc(). Given
that the DMA region can also be accessed directly by a device at any time,
it is possible that a malicious device can race to modify the package data
after the parsing and checking process but before memcpy() is invoked in
tb_cfg_copy(). Through this way, the attacker can bypass the parsing and
checking process and inject malicious data. This can potentially cause
undefined behavior of the kernel and introduce unexpected security risk.

This patch firstly copies the header of the package to a stack variable
'hdr' and performs the parsing and checking process on 'hdr'. If there is
no error in this process, 'hdr' is then used to rewrite the header in
'req->response' after memcpy(). This way, the above issue can be avoided.

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

diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c
index 37a7f4c..ae4cd61 100644
--- a/drivers/thunderbolt/ctl.c
+++ b/drivers/thunderbolt/ctl.c
@@ -166,10 +166,9 @@ tb_cfg_request_find(struct tb_ctl *ctl, struct ctl_pkg *pkg)
 
 
 static int check_header(const struct ctl_pkg *pkg, u32 len,
-			enum tb_cfg_pkg_type type, u64 route)
+			enum tb_cfg_pkg_type type, u64 route,
+			const struct tb_cfg_header *hdr)
 {
-	struct tb_cfg_header *header = pkg->buffer;
-
 	/* check frame, TODO: frame flags */
 	if (WARN(len != pkg->frame.size,
 			"wrong framesize (expected %#x, got %#x)\n",
@@ -183,12 +182,12 @@ static int check_header(const struct ctl_pkg *pkg, u32 len,
 		return -EIO;
 
 	/* check header */
-	if (WARN(header->unknown != 1 << 9,
-			"header->unknown is %#x\n", header->unknown))
+	if (WARN(hdr->unknown != 1 << 9,
+			"hdr->unknown is %#x\n", hdr->unknown))
 		return -EIO;
-	if (WARN(route != tb_cfg_get_route(header),
+	if (WARN(route != tb_cfg_get_route(hdr),
 			"wrong route (expected %llx, got %llx)",
-			route, tb_cfg_get_route(header)))
+			route, tb_cfg_get_route(hdr)))
 		return -EIO;
 	return 0;
 }
@@ -215,14 +214,15 @@ static int check_config_address(struct tb_cfg_address addr,
 	return 0;
 }
 
-static struct tb_cfg_result decode_error(const struct ctl_pkg *response)
+static struct tb_cfg_result decode_error(const struct ctl_pkg *response,
+					 const struct tb_cfg_header *hdr)
 {
 	struct cfg_error_pkg *pkg = response->buffer;
 	struct tb_cfg_result res = { 0 };
-	res.response_route = tb_cfg_get_route(&pkg->header);
+	res.response_route = tb_cfg_get_route(hdr);
 	res.response_port = 0;
 	res.err = check_header(response, sizeof(*pkg), TB_CFG_PKG_ERROR,
-			       tb_cfg_get_route(&pkg->header));
+			       tb_cfg_get_route(hdr), hdr);
 	if (res.err)
 		return res;
 
@@ -237,17 +237,17 @@ static struct tb_cfg_result decode_error(const struct ctl_pkg *response)
 }
 
 static struct tb_cfg_result parse_header(const struct ctl_pkg *pkg, u32 len,
-					 enum tb_cfg_pkg_type type, u64 route)
+					 enum tb_cfg_pkg_type type, u64 route,
+					 const struct tb_cfg_header *hdr)
 {
-	struct tb_cfg_header *header = pkg->buffer;
 	struct tb_cfg_result res = { 0 };
 
 	if (pkg->frame.eof == TB_CFG_PKG_ERROR)
-		return decode_error(pkg);
+		return decode_error(pkg, hdr);
 
 	res.response_port = 0; /* will be updated later for cfg_read/write */
-	res.response_route = tb_cfg_get_route(header);
-	res.err = check_header(pkg, len, type, route);
+	res.response_route = tb_cfg_get_route(hdr);
+	res.err = check_header(pkg, len, type, route, hdr);
 	return res;
 }
 
@@ -753,13 +753,18 @@ static bool tb_cfg_match(const struct tb_cfg_request *req,
 
 static bool tb_cfg_copy(struct tb_cfg_request *req, const struct ctl_pkg *pkg)
 {
+	struct tb_cfg_header hdr;
 	struct tb_cfg_result res;
 
+	hdr = *(struct tb_cfg_header *)pkg->buffer;
+
 	/* Now make sure it is in expected format */
 	res = parse_header(pkg, req->response_size, req->response_type,
-			   tb_cfg_get_route(req->request));
-	if (!res.err)
+			   tb_cfg_get_route(req->request), &hdr);
+	if (!res.err) {
 		memcpy(req->response, pkg->buffer, req->response_size);
+		*(struct tb_cfg_header *)req->response = hdr;
+	}
 
 	req->result = res;
 
-- 
2.7.4