Date:   Thu, 3 Jan 2019 20:31:51 +0100
From:   Oliver Hartkopp <socketcan@...tkopp.net>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        ieatmuttonchuan@...il.com, meissner@...e.de,
        linux-can@...r.kernel.org, linux-stable <stable@...r.kernel.org>
Subject: Re: [PATCH] can: gw: ensure DLC boundaries after CAN frame
 modification

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@...e.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);
>   
> 

Powered by blists - more mailing lists