netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] can: gw: ensure DLC boundaries after CAN frame modification
@ 2019-01-04  9:13 Oliver Hartkopp
  2019-01-04 10:31 ` Michal Kubecek
  2019-01-04 14:16 ` Marc Kleine-Budde
  0 siblings, 2 replies; 12+ messages in thread
From: Oliver Hartkopp @ 2019-01-04  9:13 UTC (permalink / raw)
  To: davem, netdev
  Cc: ieatmuttonchuan, meissner, mkubecek, linux-can, Oliver Hartkopp,
	linux-stable

Muyu Yu provided a POC where user root with CAP_NET_ADMIN can create a CAN
frame modification rule that makes the data length code a higher value than
the available CAN frame data size. In combination with a configured checksum
calculation where the result is stored relatively to the end of the data
(e.g. cgw_csum_xor_rel) the tail of the skb (e.g. frag_list pointer in
skb_shared_info) can be rewritten which finally can cause a system crash.

Michael Kubecek suggested to drop frames that have a DLC exceeding the
available space after the modification process and provided a patch that can
handle CAN FD frames too. Within this patch we also limit the length for the
checksum calculations to the maximum of Classic CAN data length (8).

CAN frames that are dropped by these additional checks are counted with the
CGW_DELETED counter which indicates misconfigurations in can-gw rules.

This fixes CVE-2019-3701.

Reported-by: Muyu Yu <ieatmuttonchuan@gmail.com>
Reported-by: Marcus Meissner <meissner@suse.de>
Suggested-by: Michal Kubecek <mkubecek@suse.cz>
Tested-by: Muyu Yu <ieatmuttonchuan@gmail.com>
Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: linux-stable <stable@vger.kernel.org> # >= v3.2
---
 net/can/gw.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index faa3da88a127..180c389af5b1 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -416,13 +416,31 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 	while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx])
 		(*gwj->mod.modfunc[modidx++])(cf, &gwj->mod);
 
-	/* check for checksum updates when the CAN frame has been modified */
+	/* Has the CAN frame been modified? */
 	if (modidx) {
-		if (gwj->mod.csumfunc.crc8)
-			(*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
+		/* get available space for the processed CAN frame type */
+		int max_len = nskb->len - offsetof(struct can_frame, data);
 
-		if (gwj->mod.csumfunc.xor)
-			(*gwj->mod.csumfunc.xor)(cf, &gwj->mod.csum.xor);
+		/* dlc may have changed, make sure it fits to the CAN frame */
+		if (cf->can_dlc > max_len)
+			goto out_delete;
+
+		/* check for checksum updates in classic CAN length only */
+		if (gwj->mod.csumfunc.crc8) {
+			if (cf->can_dlc > 8)
+				goto out_delete;
+			else
+				(*gwj->mod.csumfunc.crc8)
+					(cf, &gwj->mod.csum.crc8);
+		}
+
+		if (gwj->mod.csumfunc.xor) {
+			if (cf->can_dlc > 8)
+				goto out_delete;
+			else
+				(*gwj->mod.csumfunc.xor)
+					(cf, &gwj->mod.csum.xor);
+		}
 	}
 
 	/* clear the skb timestamp if not configured the other way */
@@ -434,6 +452,14 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 		gwj->dropped_frames++;
 	else
 		gwj->handled_frames++;
+
+	return;
+
+out_delete:
+	/* delete frame due to misconfiguration */
+	gwj->deleted_frames++;
+	kfree_skb(nskb);
+	return;
 }
 
 static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH] can: gw: ensure DLC boundaries after CAN frame modification
@ 2019-01-03 12:26 Oliver Hartkopp
  2019-01-03 14:01 ` Michal Kubecek
  0 siblings, 1 reply; 12+ messages in thread
From: Oliver Hartkopp @ 2019-01-03 12:26 UTC (permalink / raw)
  To: davem, netdev
  Cc: ieatmuttonchuan, meissner, linux-can, Oliver Hartkopp,
	linux-stable

The CAN frame modification rules allow bitwise logical operations which can
be also applied to the can_dlc field. Ensure the manipulation result to
maintain the can_dlc boundaries so that the CAN drivers do not accidently
write arbitrary content beyond the data registers in the CAN controllers
I/O mem when processing can-gw manipulated outgoing frames. When passing these
frames to user space this issue did not have any effect to the kernel or any
leaked data as we always strictly copy sizeof(struct can_frame) bytes.

Reported-by: Muyu Yu <ieatmuttonchuan@gmail.com>
Reported-by: Marcus Meissner <meissner@suse.de>
Tested-by: Muyu Yu <ieatmuttonchuan@gmail.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: linux-stable <stable@vger.kernel.org> # >= v3.2
---
 net/can/gw.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/can/gw.c b/net/can/gw.c
index faa3da88a127..9000d9b8a133 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -418,6 +418,10 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 
 	/* check for checksum updates when the CAN frame has been modified */
 	if (modidx) {
+		/* ensure DLC boundaries after the different mods */
+		if (cf->can_dlc > 8)
+			cf->can_dlc = 8;
+
 		if (gwj->mod.csumfunc.crc8)
 			(*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
 
-- 
2.19.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-01-04 14:16 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-04  9:13 [PATCH] can: gw: ensure DLC boundaries after CAN frame modification Oliver Hartkopp
2019-01-04 10:31 ` Michal Kubecek
2019-01-04 10:57   ` Oliver Hartkopp
2019-01-04 13:25     ` Test results - was " Oliver Hartkopp
2019-01-04 14:16 ` Marc Kleine-Budde
  -- strict thread matches above, loose matches on Subject: below --
2019-01-03 12:26 Oliver Hartkopp
2019-01-03 14:01 ` Michal Kubecek
2019-01-03 19:31   ` Oliver Hartkopp
2019-01-03 20:33     ` Michal Kubecek
2019-01-03 21:03       ` Oliver Hartkopp
2019-01-04  9:01         ` Michal Kubecek
2019-01-04  9:28           ` Oliver Hartkopp

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).