[PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs

[PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs

[Oliver Hartkopp]

KMSAN sysbot detected a read access to an untinitialized value in the headroom
of an outgoing CAN related sk_buff. When using CAN sockets this area is filled
appropriately - but when using a packet socket this initialization is missing.

The problematic read access occurs in the CAN receive path which can only be
triggered when the sk_buff is sent through a (virtual) CAN interface. So we
check in the sending path whether we need to perform the missing
initializations.

Fixes: d3b58c47d330d ("can: replace timestamp as unique skb attribute")
Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 include/linux/can/dev.h | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 9b3c720a31b1..8f86e7a1f8e9 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -18,6 +18,7 @@
 #include <linux/can/error.h>
 #include <linux/can/led.h>
 #include <linux/can/netlink.h>
+#include <linux/can/skb.h>
 #include <linux/netdevice.h>
 
 /*
@@ -91,6 +92,37 @@ struct can_priv {
 #define get_can_dlc(i)		(min_t(__u8, (i), CAN_MAX_DLC))
 #define get_canfd_dlc(i)	(min_t(__u8, (i), CANFD_MAX_DLC))
 
+/* Check for outgoing skbs that have not been created by the CAN subsystem */
+static inline bool can_check_skb_headroom(struct net_device *dev,
+					  struct sk_buff *skb)
+{
+	/* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
+	if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
+		return true;
+
+	/* af_packet does not apply CAN skb specific settings */
+	if (skb->ip_summed == CHECKSUM_NONE) {
+
+		/* init headroom */
+		can_skb_prv(skb)->ifindex = dev->ifindex;
+		can_skb_prv(skb)->skbcnt = 0;
+
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+		/* preform proper loopback on capable devices */
+		if (dev->flags & IFF_ECHO)
+			skb->pkt_type = PACKET_LOOPBACK;
+		else
+			skb->pkt_type = PACKET_HOST;
+
+		skb_reset_mac_header(skb);
+		skb_reset_network_header(skb);
+		skb_reset_transport_header(skb);
+	}
+
+	return false;
+}
+
 /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
 static inline bool can_dropped_invalid_skb(struct net_device *dev,
 					  struct sk_buff *skb)
@@ -108,6 +140,9 @@ static inline bool can_dropped_invalid_skb(struct net_device *dev,
 	} else
 		goto inval_skb;
 
+	if (can_check_skb_headroom(dev, skb))
+		goto inval_skb;
+
 	return false;
 
 inval_skb:
-- 
2.20.1

Re: [PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs

[Marc Kleine-Budde]

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

On 12/7/19 7:34 PM, Oliver Hartkopp wrote:
> KMSAN sysbot detected a read access to an untinitialized value in the headroom
> of an outgoing CAN related sk_buff. When using CAN sockets this area is filled
> appropriately - but when using a packet socket this initialization is missing.
> 
> The problematic read access occurs in the CAN receive path which can only be
> triggered when the sk_buff is sent through a (virtual) CAN interface. So we
> check in the sending path whether we need to perform the missing
> initializations.
> 
> Fixes: d3b58c47d330d ("can: replace timestamp as unique skb attribute")
> Reported-by: syzbot+b02ff0707a97e4e79ebb@syzkaller.appspotmail.com
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/linux/can/dev.h | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 9b3c720a31b1..8f86e7a1f8e9 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -18,6 +18,7 @@
>  #include <linux/can/error.h>
>  #include <linux/can/led.h>
>  #include <linux/can/netlink.h>
> +#include <linux/can/skb.h>
>  #include <linux/netdevice.h>
>  
>  /*
> @@ -91,6 +92,37 @@ struct can_priv {
>  #define get_can_dlc(i)		(min_t(__u8, (i), CAN_MAX_DLC))
>  #define get_canfd_dlc(i)	(min_t(__u8, (i), CANFD_MAX_DLC))
>  
> +/* Check for outgoing skbs that have not been created by the CAN subsystem */
> +static inline bool can_check_skb_headroom(struct net_device *dev,
> +					  struct sk_buff *skb)

Do we want to have such a big function as a static inline?

> +{
> +	/* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
> +	if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
> +		return true;
> +
> +	/* af_packet does not apply CAN skb specific settings */
> +	if (skb->ip_summed == CHECKSUM_NONE) {

Is it possible to set the ip_summed via the packet socket or is it
always 0 (== CHECKSUM_NONE)?

> +

Please remove that empty line.

> +		/* init headroom */
> +		can_skb_prv(skb)->ifindex = dev->ifindex;
> +		can_skb_prv(skb)->skbcnt = 0;
> +
> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> +
> +		/* preform proper loopback on capable devices */
> +		if (dev->flags & IFF_ECHO)
> +			skb->pkt_type = PACKET_LOOPBACK;
> +		else
> +			skb->pkt_type = PACKET_HOST;
> +
> +		skb_reset_mac_header(skb);
> +		skb_reset_network_header(skb);
> +		skb_reset_transport_header(skb);
> +	}
> +
> +	return false;
> +}
> +
>  /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
>  static inline bool can_dropped_invalid_skb(struct net_device *dev,
>  					  struct sk_buff *skb)
> @@ -108,6 +140,9 @@ static inline bool can_dropped_invalid_skb(struct net_device *dev,
>  	} else
>  		goto inval_skb;
>  
> +	if (can_check_skb_headroom(dev, skb))

Can you rename the function, so that it's clear that returning false
means it's an invalid skb?

> +		goto inval_skb;
> +
>  	return false;
>  
>  inval_skb:
> 

Marc

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


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

Re: [PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs

[Oliver Hartkopp]

On 08/12/2019 11.48, Marc Kleine-Budde wrote:
> On 12/7/19 7:34 PM, Oliver Hartkopp wrote:
>> KMSAN sysbot detected a read access to an untinitialized value in the headroom
>> of an outgoing CAN related sk_buff. When using CAN sockets this area is filled
>> appropriately - but when using a packet socket this initialization is missing.


>> +/* Check for outgoing skbs that have not been created by the CAN subsystem */
>> +static inline bool can_check_skb_headroom(struct net_device *dev,
>> +					  struct sk_buff *skb)
> 
> Do we want to have such a big function as a static inline?
> 

No. Usually not. can_dropped_invalid_skb() has the same problem IMO.

This additional inline function approach was the only way to ensure 
simple backwards portability for stable kernels.

I would suggest to clean this up once this patch went into mainline.

>> +{
>> +	/* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
>> +	if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
>> +		return true;
>> +
>> +	/* af_packet does not apply CAN skb specific settings */
>> +	if (skb->ip_summed == CHECKSUM_NONE) {
> 
> Is it possible to set the ip_summed via the packet socket or is it
> always 0 (== CHECKSUM_NONE)?

af_packet only reads ip_summed in the receive path. It is not set when 
creating the outgoing skb where the struct skb is mainly zero'ed.

> 
>> +
> 
> Please remove that empty line.
> 

ok.

>> +		/* init headroom */
>> +		can_skb_prv(skb)->ifindex = dev->ifindex;
>> +		can_skb_prv(skb)->skbcnt = 0;
>> +
>> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
>> +
>> +		/* preform proper loopback on capable devices */
>> +		if (dev->flags & IFF_ECHO)
>> +			skb->pkt_type = PACKET_LOOPBACK;
>> +		else
>> +			skb->pkt_type = PACKET_HOST;
>> +
>> +		skb_reset_mac_header(skb);
>> +		skb_reset_network_header(skb);
>> +		skb_reset_transport_header(skb);
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
>>   static inline bool can_dropped_invalid_skb(struct net_device *dev,
>>   					  struct sk_buff *skb)
>> @@ -108,6 +140,9 @@ static inline bool can_dropped_invalid_skb(struct net_device *dev,
>>   	} else
>>   		goto inval_skb;
>>   
>> +	if (can_check_skb_headroom(dev, skb))
> 
> Can you rename the function, so that it's clear that returning false
> means it's an invalid skb?
> 

Returning 'false' is *good* like in can_dropped_invalid_skb(). What 
naming would you suggest?

Regards,
Oliver

Re: [PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs

[Marc Kleine-Budde]

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

On 12/8/19 12:34 PM, Oliver Hartkopp wrote:
> 
> 
> On 08/12/2019 11.48, Marc Kleine-Budde wrote:
>> On 12/7/19 7:34 PM, Oliver Hartkopp wrote:
>>> KMSAN sysbot detected a read access to an untinitialized value in the headroom
>>> of an outgoing CAN related sk_buff. When using CAN sockets this area is filled
>>> appropriately - but when using a packet socket this initialization is missing.
> 
> 
>>> +/* Check for outgoing skbs that have not been created by the CAN subsystem */
>>> +static inline bool can_check_skb_headroom(struct net_device *dev,
>>> +					  struct sk_buff *skb)
>>
>> Do we want to have such a big function as a static inline?
>>
> 
> No. Usually not. can_dropped_invalid_skb() has the same problem IMO.
> 
> This additional inline function approach was the only way to ensure 
> simple backwards portability for stable kernels.

A "normal" function called from the can_dropped_invalid_skb() works aswell.

> I would suggest to clean this up once this patch went into mainline.
> 
>>> +{
>>> +	/* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */
>>> +	if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))
>>> +		return true;
>>> +
>>> +	/* af_packet does not apply CAN skb specific settings */
>>> +	if (skb->ip_summed == CHECKSUM_NONE) {
>>
>> Is it possible to set the ip_summed via the packet socket or is it
>> always 0 (== CHECKSUM_NONE)?
> 
> af_packet only reads ip_summed in the receive path. It is not set when 
> creating the outgoing skb where the struct skb is mainly zero'ed.
> 
>>
>>> +
>>
>> Please remove that empty line.
>>
> 
> ok.
> 
>>> +		/* init headroom */
>>> +		can_skb_prv(skb)->ifindex = dev->ifindex;
>>> +		can_skb_prv(skb)->skbcnt = 0;
>>> +
>>> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
>>> +
>>> +		/* preform proper loopback on capable devices */
>>> +		if (dev->flags & IFF_ECHO)
>>> +			skb->pkt_type = PACKET_LOOPBACK;
>>> +		else
>>> +			skb->pkt_type = PACKET_HOST;
>>> +
>>> +		skb_reset_mac_header(skb);
>>> +		skb_reset_network_header(skb);
>>> +		skb_reset_transport_header(skb);
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>>>   /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
>>>   static inline bool can_dropped_invalid_skb(struct net_device *dev,
>>>   					  struct sk_buff *skb)
>>> @@ -108,6 +140,9 @@ static inline bool can_dropped_invalid_skb(struct net_device *dev,
>>>   	} else
>>>   		goto inval_skb;
>>>   
>>> +	if (can_check_skb_headroom(dev, skb))
>>
>> Can you rename the function, so that it's clear that returning false
>> means it's an invalid skb?
>>
> 
> Returning 'false' is *good* like in can_dropped_invalid_skb(). What 
> naming would you suggest?

Something like: can_skb_headroom_valid().

See v2 I just sent around.

Marc

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


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