[PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Oliver Hartkopp]

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

Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Michal Kubecek]

On Thu, Jan 03, 2019 at 01:26:34PM +0100, Oliver Hartkopp wrote:
> 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);
>  

IMHO "8" should rather be "CAN_MAX_DLEN". I can see two problems with
your patch:

1. If I understand the code correctly, canfd_frame packets (which allow
larger lenth) are also processed by this code path.

2. Silently capping the DLC feels wrong, IMHO it would be cleaner to
drop the resulting packet if it's invalid.

This is the patch I came with:

From 8cc56bc825fa88803c08a8f85dc315bc112a8b05 Mon Sep 17 00:00:00 2001
From: Michal Kubecek <mkubecek@suse.cz>
Date: Thu, 3 Jan 2019 11:00:26 +0100
Subject: [PATCH] can: recheck dlc value of modified packet

CAN frame modification rules allow modification (set, and, or, xor) of DLC
(or payload length value). The new value is not checked against
CAN(FD)_MAX_DLEN so that indicated payload length may reach beyond actual
packet length.

As reported to security list, cgw_csum_xor_rel() with negative offset can
then rewrite e.g. frag_list pointer in skb_shared_info, crashing the
system. With unprivileged user namespaces, this can be exploited by any
regular user.

Rather than distinguishing between can_frame and canfd_frame, check if
resulting payload length does not reach beyond nskb->len which is what we
are actually interested in.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 net/can/gw.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/can/gw.c b/net/can/gw.c
index faa3da88a127..87b7043e3250 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -418,6 +418,15 @@ 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) {
+		int max_dlc = nskb->len - offsetof(struct can_frame, data);
+
+		/* dlc may have changed, check the new value */
+		if (cf->can_dlc > max_dlc) {
+			gwj->dropped_frames++;
+			kfree_skb(nskb);
+			return;
+		}
+
 		if (gwj->mod.csumfunc.crc8)
 			(*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
 
-- 
2.20.1

Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Michal Kubecek]

On Thu, Jan 03, 2019 at 01:26:34PM +0100, Oliver Hartkopp wrote:
> 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);
>  

IMHO "8" should rather be "CAN_MAX_DLEN". I can see two problems with
your patch:

1. If I understand the code correctly, canfd_frame packets (which allow
larger lenth) are also processed by this code path.

2. Silently capping the DLC feels wrong, IMHO it would be cleaner to
drop the resulting packet if it's invalid.

This is the patch I came with:

>From 8cc56bc825fa88803c08a8f85dc315bc112a8b05 Mon Sep 17 00:00:00 2001
From: Michal Kubecek <mkubecek@suse.cz>
Date: Thu, 3 Jan 2019 11:00:26 +0100
Subject: [PATCH] can: recheck dlc value of modified packet

CAN frame modification rules allow modification (set, and, or, xor) of DLC
(or payload length value). The new value is not checked against
CAN(FD)_MAX_DLEN so that indicated payload length may reach beyond actual
packet length.

As reported to security list, cgw_csum_xor_rel() with negative offset can
then rewrite e.g. frag_list pointer in skb_shared_info, crashing the
system. With unprivileged user namespaces, this can be exploited by any
regular user.

Rather than distinguishing between can_frame and canfd_frame, check if
resulting payload length does not reach beyond nskb->len which is what we
are actually interested in.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 net/can/gw.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/can/gw.c b/net/can/gw.c
index faa3da88a127..87b7043e3250 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -418,6 +418,15 @@ 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) {
+		int max_dlc = nskb->len - offsetof(struct can_frame, data);
+
+		/* dlc may have changed, check the new value */
+		if (cf->can_dlc > max_dlc) {
+			gwj->dropped_frames++;
+			kfree_skb(nskb);
+			return;
+		}
+
 		if (gwj->mod.csumfunc.crc8)
 			(*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
 
-- 
2.20.1

Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Oliver Hartkopp]

Hi Michal,

On 1/3/19 3:01 PM, Michal Kubecek wrote:
> On Thu, Jan 03, 2019 at 01:26:34PM +0100, Oliver Hartkopp wrote:
(..)
>>   	/* 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);
>>   
> 
> IMHO "8" should rather be "CAN_MAX_DLEN". I can see two problems with
> your patch:
> 
> 1. If I understand the code correctly, canfd_frame packets (which allow
> larger lenth) are also processed by this code path.

In fact the can-gw frame modification and checksum functionalities lack 
CAN FD support today.

If you take a look into the netlink API only struct can_frame's can be 
supplied for frame modifications - and so are the checks e.g. in 
cgw_chk_csum_parms().

The given patch fixes the problem as described in the commit message in 
all stable Linux versions since can-gw appeared in Linux 3.2.

Anyway your modification makes definitely sense, as it allows to process 
CAN FD frames in struct canfd_frame as long as only data is modified 
that is also available in a struct can_frame. AND - as a bonus - it 
should work for stable 3.2 too, when CAN FD was not even introduced. 
Good idea!

If it's ok for you I would like to re-send the patch together with the 
CVE number and would like to credit your suggestion in the text and with 
"Suggested-by:".

> As reported to security list, cgw_csum_xor_rel() with negative offset can
> then rewrite e.g. frag_list pointer in skb_shared_info, crashing the
> system. With unprivileged user namespaces, this can be exploited by any
> regular user.

This is wrong! First there is no negative offset, as can_dlc is a u8 
value. Therefore you can try to hit content in the tail of the skb only.
Second can-gw rules can only be configured by *root* and not by any 
regular user - and finally it is definitely not namespace related.

So the user root can configure a can-gw rule to shoot into the skb tail 
to kill the machine. I can imagine better things to do when I'm already 
root ;-)

Thanks & best regards,
Oliver

> 
> Rather than distinguishing between can_frame and canfd_frame, check if
> resulting payload length does not reach beyond nskb->len which is what we
> are actually interested in.
> 
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> ---
>   net/can/gw.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index faa3da88a127..87b7043e3250 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -418,6 +418,15 @@ 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) {
> +		int max_dlc = nskb->len - offsetof(struct can_frame, data);
> +
> +		/* dlc may have changed, check the new value */
> +		if (cf->can_dlc > max_dlc) {
> +			gwj->dropped_frames++;
> +			kfree_skb(nskb);
> +			return;
> +		}
> +
>   		if (gwj->mod.csumfunc.crc8)
>   			(*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
>   
> 

Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Michal Kubecek]

On Thu, Jan 03, 2019 at 08:31:51PM +0100, Oliver Hartkopp wrote:
> Hi Michal,
> 
> On 1/3/19 3:01 PM, Michal Kubecek wrote:
> > On Thu, Jan 03, 2019 at 01:26:34PM +0100, Oliver Hartkopp wrote:
> (..)
> > >   	/* 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);
> > 
> > IMHO "8" should rather be "CAN_MAX_DLEN". I can see two problems with
> > your patch:
> > 
> > 1. If I understand the code correctly, canfd_frame packets (which allow
> > larger lenth) are also processed by this code path.
> 
> In fact the can-gw frame modification and checksum functionalities lack CAN
> FD support today.
> 
> If you take a look into the netlink API only struct can_frame's can be
> supplied for frame modifications - and so are the checks e.g. in
> cgw_chk_csum_parms().
> 
> The given patch fixes the problem as described in the commit message in all
> stable Linux versions since can-gw appeared in Linux 3.2.
> 
> Anyway your modification makes definitely sense, as it allows to process CAN
> FD frames in struct canfd_frame as long as only data is modified that is
> also available in a struct can_frame. AND - as a bonus - it should work for
> stable 3.2 too, when CAN FD was not even introduced. Good idea!
> 
> If it's ok for you I would like to re-send the patch together with the CVE
> number and would like to credit your suggestion in the text and with
> "Suggested-by:".

OK

> > As reported to security list, cgw_csum_xor_rel() with negative offset can
> > then rewrite e.g. frag_list pointer in skb_shared_info, crashing the
> > system. With unprivileged user namespaces, this can be exploited by any
> > regular user.
> 
> This is wrong! First there is no negative offset, as can_dlc is a u8 value.
> Therefore you can try to hit content in the tail of the skb only.

I probably didn't use the right term. By "negative offset" I meant the
value of cgw_csum_xor::result_idx which, if negative, is interpreted as
an offset relative to can_dlc. If the (invalid) value of modified
can_dlc is sufficiently large (larger then actual nskb->len), userspace
can enforce writing past packet data.

See http://bugzilla.suse.com/show_bug.cgi?id=1120386 (comment 1) for an
example which can crash unfixed kernel by rewriting a pointer in skb
shared data which is later dereferenced when the skb is freed.

> Second can-gw rules can only be configured by *root* and not by any regular
> user - and finally it is definitely not namespace related.
> 
> So the user root can configure a can-gw rule to shoot into the skb tail to
> kill the machine. I can imagine better things to do when I'm already root

Sorry for the noise, I misread the code (and commit 90f62cf30a78) so
that I thought netlink_ns_capable() is used in net/can/gw.c; now I see
that it's netlink_capable() so that global CAP_NET_ADMIN is required
rather than namespace one and the bug cannot be exploited by a regular
user.

Michal Kubecek

Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Oliver Hartkopp]

Hi Michal,

On 1/3/19 9:33 PM, Michal Kubecek wrote:
> On Thu, Jan 03, 2019 at 08:31:51PM +0100, Oliver Hartkopp wrote:

>> Anyway your modification makes definitely sense, as it allows to process CAN
>> FD frames in struct canfd_frame as long as only data is modified that is
>> also available in a struct can_frame. AND - as a bonus - it should work for
>> stable 3.2 too, when CAN FD was not even introduced. Good idea!
>>
>> If it's ok for you I would like to re-send the patch together with the CVE
>> number and would like to credit your suggestion in the text and with
>> "Suggested-by:".
> 
> OK 

Thanks!

I have created a new patch that allows CAN FD frames to be handled 
within the possibilities a struct can_frame offers (so especially with 
can_id and can_dlc). As checksum calculation currently only supports 
'Classic CAN' CAN FD frames are dropped when the can_dlc becomes > 8.

Two other hints about the upcoming patch:

1. I'm still using the test "if (cf->can_dlc > 8)" with a proper comment 
as CAN_MAX_DLC and CAN_MAX_DLEN are not defined in Linux stable 3.2 - so 
that we can apply the patch to all versions of gw.c

2. The counter gwj->deleted_frames is increased when dropping *invalid* 
frames as this indicates a potential misconfiguration. 
gwj->dropped_frames is only used for allocation problems like OOM.

>>> As reported to security list, cgw_csum_xor_rel() with negative offset can
>>> then rewrite e.g. frag_list pointer in skb_shared_info, crashing the
>>> system. With unprivileged user namespaces, this can be exploited by any
>>> regular user.
>>
>> This is wrong! First there is no negative offset, as can_dlc is a u8 value.
>> Therefore you can try to hit content in the tail of the skb only.
> 
> I probably didn't use the right term. By "negative offset" I meant the
> value of cgw_csum_xor::result_idx which, if negative, is interpreted as
> an offset relative to can_dlc.

If result_idx is negative the position is calculated back from the 
receive dlc - so to some index between 0 and dlc.

> If the (invalid) value of modified
> can_dlc is sufficiently large (larger then actual nskb->len), userspace
> can enforce writing past packet data.

Yes. This hits the point.

> See http://bugzilla.suse.com/show_bug.cgi?id=1120386 (comment 1) for an
> example which can crash unfixed kernel by rewriting a pointer in skb
> shared data which is later dereferenced when the skb is freed.
> 
>> Second can-gw rules can only be configured by *root* and not by any regular
>> user - and finally it is definitely not namespace related.
>>
>> So the user root can configure a can-gw rule to shoot into the skb tail to
>> kill the machine. I can imagine better things to do when I'm already root
> 
> Sorry for the noise, I misread the code (and commit 90f62cf30a78) so
> that I thought netlink_ns_capable() is used in net/can/gw.c; now I see
> that it's netlink_capable() so that global CAP_NET_ADMIN is required
> rather than namespace one and the bug cannot be exploited by a regular
> user.

No problem :-)

Thanks for going through the code that deep!

Best regards,
Oliver

Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Michal Kubecek]

On Thu, Jan 03, 2019 at 10:03:47PM +0100, Oliver Hartkopp wrote:
> 1. I'm still using the test "if (cf->can_dlc > 8)" with a proper comment as
> CAN_MAX_DLC and CAN_MAX_DLEN are not defined in Linux stable 3.2 - so that
> we can apply the patch to all versions of gw.c

I may be biased as substantial part of my work consists of trying to
understand unfamiliar code written by other people but I believe
readability of the code is more important than avoiding a bit of work
with backport to older stable branches.

Michal Kubecek

Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Oliver Hartkopp]

Hi Michal,

On 1/4/19 10:01 AM, Michal Kubecek wrote:
> On Thu, Jan 03, 2019 at 10:03:47PM +0100, Oliver Hartkopp wrote:
>> 1. I'm still using the test "if (cf->can_dlc > 8)" with a proper comment as
>> CAN_MAX_DLC and CAN_MAX_DLEN are not defined in Linux stable 3.2 - so that
>> we can apply the patch to all versions of gw.c
> 
> I may be biased as substantial part of my work consists of trying to
> understand unfamiliar code written by other people but I believe
> readability of the code is more important than avoiding a bit of work
> with backport to older stable branches.

I am with you.
Please take a look into the new patch. I think either the comment and 
the commit message make it very clear where the value comes from.

E.g. in cgw_chk_csum_parms() we have tons of fixed constants that have 
been introduced when CAN FD was far away from thinking. I introduced 
CAN_MAX_DLC and CAN_MAX_DLEN myself when I started the CAN FD support in 
Linux. gw.c would need a rework for full CAN FD support - where these 
defines then definitely would be introduced.

In this single case the simple applying to stable kernels weighted more 
to me.

Regards,
Oliver

Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3706 bytes --]

On 1/4/19 10:13 AM, Oliver Hartkopp wrote:
> 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);

What about removing the else, then we can keep this in one line.
> +		}
> +
> +		if (gwj->mod.csumfunc.xor) {
> +			if (cf->can_dlc > 8)
> +				goto out_delete;
> +			else
> +				(*gwj->mod.csumfunc.xor)
> +					(cf, &gwj->mod.csum.xor);

same here

> +		}
>  	}
>  
>  	/* 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)
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Oliver Hartkopp]

On 1/4/19 11:31 AM, Michal Kubecek wrote:
> On Fri, Jan 04, 2019 at 10:13:53AM +0100, Oliver Hartkopp wrote:
>> 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
>>
> 
> Except for the "8" vs "CAN_MAX_DLEN" issue discussed in v1, looks good
> to me.

Thanks for the review!

Best regards,
Oliver

Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Michal Kubecek]

On Fri, Jan 04, 2019 at 10:13:53AM +0100, Oliver Hartkopp wrote:
> 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
> 

Except for the "8" vs "CAN_MAX_DLEN" issue discussed in v1, looks good
to me.

Michal Kubecek

[PATCH] can: gw: ensure DLC boundaries after CAN frame modification

[Oliver Hartkopp]

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