netdev - [PATCH] thunderbolt: xdomain: Avoid potential stack OOB read

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: <20211207063413.2698788-1-keescook@chromium.org>
Date:   Mon,  6 Dec 2021 22:34:13 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Andreas Noever <andreas.noever@...il.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Michael Jamet <michael.jamet@...el.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Yehezkel Bernat <YehezkelShB@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org, netdev@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: [PATCH] thunderbolt: xdomain: Avoid potential stack OOB read

tb_xdp_properties_changed_request() was calling tb_xdp_handle_error() with
a struct tb_xdp_properties_changed_response on the stack, which does not
have the "error" field present when cast to struct tb_xdp_error_response.
This was detected when building with -Warray-bounds:

drivers/thunderbolt/xdomain.c: In function 'tb_xdomain_properties_changed':
drivers/thunderbolt/xdomain.c:226:22: error: array subscript 'const struct tb_xdp_error_response[0]' is partly outside array bounds of 'struct tb_xdp_properties_changed_response[1]' [-Werror=array-bounds]
  226 |         switch (error->error) {
      |                 ~~~~~^~~~~~~
drivers/thunderbolt/xdomain.c:448:51: note: while referencing 'res'
  448 |         struct tb_xdp_properties_changed_response res;
      |                                                   ^~~

Add union containing struct tb_xdp_error_response to structures passed
to tb_xdp_handle_error(), so that the "error" field will be present.

Signed-off-by: Kees Cook <keescook@...omium.org>
---
 drivers/thunderbolt/tb_msgs.h | 47 ++++++++++++++++++++++-------------
 drivers/thunderbolt/xdomain.c | 16 +++++-------
 2 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/thunderbolt/tb_msgs.h b/drivers/thunderbolt/tb_msgs.h
index bcabfcb2fd03..fe1afa44c56d 100644
--- a/drivers/thunderbolt/tb_msgs.h
+++ b/drivers/thunderbolt/tb_msgs.h
@@ -535,15 +535,25 @@ struct tb_xdp_header {
 	u32 type;
 };
 
+struct tb_xdp_error_response {
+	struct tb_xdp_header hdr;
+	u32 error;
+};
+
 struct tb_xdp_uuid {
 	struct tb_xdp_header hdr;
 };
 
 struct tb_xdp_uuid_response {
-	struct tb_xdp_header hdr;
-	uuid_t src_uuid;
-	u32 src_route_hi;
-	u32 src_route_lo;
+	union {
+		struct tb_xdp_error_response err;
+		struct {
+			struct tb_xdp_header hdr;
+			uuid_t src_uuid;
+			u32 src_route_hi;
+			u32 src_route_lo;
+		};
+	};
 };
 
 struct tb_xdp_properties {
@@ -555,13 +565,18 @@ struct tb_xdp_properties {
 };
 
 struct tb_xdp_properties_response {
-	struct tb_xdp_header hdr;
-	uuid_t src_uuid;
-	uuid_t dst_uuid;
-	u16 offset;
-	u16 data_length;
-	u32 generation;
-	u32 data[0];
+	union {
+		struct tb_xdp_error_response err;
+		struct {
+			struct tb_xdp_header hdr;
+			uuid_t src_uuid;
+			uuid_t dst_uuid;
+			u16 offset;
+			u16 data_length;
+			u32 generation;
+			u32 data[];
+		};
+	};
 };
 
 /*
@@ -580,7 +595,10 @@ struct tb_xdp_properties_changed {
 };
 
 struct tb_xdp_properties_changed_response {
-	struct tb_xdp_header hdr;
+	union {
+		struct tb_xdp_error_response err;
+		struct tb_xdp_header hdr;
+	};
 };
 
 enum tb_xdp_error {
@@ -591,9 +609,4 @@ enum tb_xdp_error {
 	ERROR_NOT_READY,
 };
 
-struct tb_xdp_error_response {
-	struct tb_xdp_header hdr;
-	u32 error;
-};
-
 #endif
diff --git a/drivers/thunderbolt/xdomain.c b/drivers/thunderbolt/xdomain.c
index eff32499610f..01d6b724ca51 100644
--- a/drivers/thunderbolt/xdomain.c
+++ b/drivers/thunderbolt/xdomain.c
@@ -214,16 +214,12 @@ static inline void tb_xdp_fill_header(struct tb_xdp_header *hdr, u64 route,
 	memcpy(&hdr->uuid, &tb_xdp_uuid, sizeof(tb_xdp_uuid));
 }
 
-static int tb_xdp_handle_error(const struct tb_xdp_header *hdr)
+static int tb_xdp_handle_error(const struct tb_xdp_error_response *res)
 {
-	const struct tb_xdp_error_response *error;
-
-	if (hdr->type != ERROR_RESPONSE)
+	if (res->hdr.type != ERROR_RESPONSE)
 		return 0;
 
-	error = (const struct tb_xdp_error_response *)hdr;
-
-	switch (error->error) {
+	switch (res->error) {
 	case ERROR_UNKNOWN_PACKET:
 	case ERROR_UNKNOWN_DOMAIN:
 		return -EIO;
@@ -257,7 +253,7 @@ static int tb_xdp_uuid_request(struct tb_ctl *ctl, u64 route, int retry,
 	if (ret)
 		return ret;
 
-	ret = tb_xdp_handle_error(&res.hdr);
+	ret = tb_xdp_handle_error(&res.err);
 	if (ret)
 		return ret;
 
@@ -329,7 +325,7 @@ static int tb_xdp_properties_request(struct tb_ctl *ctl, u64 route,
 		if (ret)
 			goto err;
 
-		ret = tb_xdp_handle_error(&res->hdr);
+		ret = tb_xdp_handle_error(&res->err);
 		if (ret)
 			goto err;
 
@@ -462,7 +458,7 @@ static int tb_xdp_properties_changed_request(struct tb_ctl *ctl, u64 route,
 	if (ret)
 		return ret;
 
-	return tb_xdp_handle_error(&res.hdr);
+	return tb_xdp_handle_error(&res.err);
 }
 
 static int
-- 
2.30.2