[v3] can: gw: ensure DLC boundaries after CAN frame modification
diff mbox series

Message ID 20190104145526.28492-1-mkl@pengutronix.de
State Accepted
Delegated to: David Miller
Headers show
Series
  • [v3] can: gw: ensure DLC boundaries after CAN frame modification
Related show

Commit Message

Marc Kleine-Budde Jan. 4, 2019, 2:55 p.m. UTC
From: Oliver Hartkopp <socketcan@hartkopp.net>

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
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Hello,

I've removed the else from dlc length check. Keeps the code and the
patch more readable.

Marc

Changes since v2:
- add newline after goto (Tnx, Oliver)
Changes since v1:
- remove else from dlc length check.

 net/can/gw.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

Comments

David Miller Jan. 4, 2019, 9:41 p.m. UTC | #1
From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Fri,  4 Jan 2019 15:55:26 +0100

> From: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> 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
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Marc, do you want me to apply this directly to my net tree?

Thanks.
Marc Kleine-Budde Jan. 5, 2019, 10:56 a.m. UTC | #2
On 1/4/19 10:41 PM, David Miller wrote:
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Date: Fri,  4 Jan 2019 15:55:26 +0100
> 
>> From: Oliver Hartkopp <socketcan@hartkopp.net>
>>
>> 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
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> Marc, do you want me to apply this directly to my net tree?

No, I'll send a pull request.

Thanks,
Marc
Oliver Hartkopp Jan. 7, 2019, 12:14 p.m. UTC | #3
Hello Marc,

On 1/5/19 11:56 AM, Marc Kleine-Budde wrote:
> On 1/4/19 10:41 PM, David Miller wrote:
>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>> Date: Fri,  4 Jan 2019 15:55:26 +0100
>>
>> Marc, do you want me to apply this directly to my net tree?
> 
> No, I'll send a pull request.
> 
> Thanks,
> Marc

I was still on vacation last week an my environment at home was not that 
amused - but I really tried to fix this CVE as fast as possible in the 
best way we could.

Now the patch is nearly 70h hanging around and 48h since you announced 
to send a pull request.

I don't get it. Wouldn't it been better when you just wrote one sentence 
so that the patch could go upstream already on Saturday?

Regards,
Oliver
Oliver Hartkopp Jan. 7, 2019, 12:51 p.m. UTC | #4
Hello Dave,

the boss of Marc just send me a private mail that Marc is currently ill.
I hope he'll get well soon.

Can you please take this patch directly then?

Many thanks,
Oliver

On 1/7/19 1:14 PM, Oliver Hartkopp wrote:
> Hello Marc,
> 
> On 1/5/19 11:56 AM, Marc Kleine-Budde wrote:
>> On 1/4/19 10:41 PM, David Miller wrote:
>>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Date: Fri,  4 Jan 2019 15:55:26 +0100
>>>
>>> Marc, do you want me to apply this directly to my net tree?
>>
>> No, I'll send a pull request.
>>
>> Thanks,
>> Marc
> 
> I was still on vacation last week an my environment at home was not that 
> amused - but I really tried to fix this CVE as fast as possible in the 
> best way we could.
> 
> Now the patch is nearly 70h hanging around and 48h since you announced 
> to send a pull request.
> 
> I don't get it. Wouldn't it been better when you just wrote one sentence 
> so that the patch could go upstream already on Saturday?
> 
> Regards,
> Oliver
>
David Miller Jan. 7, 2019, 1:17 p.m. UTC | #5
From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Mon, 7 Jan 2019 13:51:22 +0100

> the boss of Marc just send me a private mail that Marc is currently
> ill.
> I hope he'll get well soon.

I'm sorry to hear that and hope he gets well soon as well.

> Can you please take this patch directly then?

Yes, I will.

Patch
diff mbox series

diff --git a/net/can/gw.c b/net/can/gw.c
index faa3da88a127..53859346dc9a 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -416,13 +416,29 @@  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)
+		/* get available space for the processed CAN frame type */
+		int max_len = nskb->len - offsetof(struct can_frame, data);
+
+		/* 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;
+
 			(*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
+		}
+
+		if (gwj->mod.csumfunc.xor) {
+			if (cf->can_dlc > 8)
+				goto out_delete;
 
-		if (gwj->mod.csumfunc.xor)
 			(*gwj->mod.csumfunc.xor)(cf, &gwj->mod.csum.xor);
+		}
 	}
 
 	/* clear the skb timestamp if not configured the other way */
@@ -434,6 +450,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)