* [PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs
@ 2019-12-07 18:34 Oliver Hartkopp
2019-12-08 10:48 ` Marc Kleine-Budde
0 siblings, 1 reply; 4+ messages in thread
From: Oliver Hartkopp @ 2019-12-07 18:34 UTC (permalink / raw)
To: linux-can, dvyukov, mkl
Cc: syzbot+b02ff0707a97e4e79ebb, glider, syzkaller-bugs, netdev,
o.rempel, eric.dumazet, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs
2019-12-07 18:34 [PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs Oliver Hartkopp
@ 2019-12-08 10:48 ` Marc Kleine-Budde
2019-12-08 11:34 ` Oliver Hartkopp
0 siblings, 1 reply; 4+ messages in thread
From: Marc Kleine-Budde @ 2019-12-08 10:48 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can, dvyukov
Cc: syzbot+b02ff0707a97e4e79ebb, glider, syzkaller-bugs, netdev,
o.rempel, eric.dumazet
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs
2019-12-08 10:48 ` Marc Kleine-Budde
@ 2019-12-08 11:34 ` Oliver Hartkopp
2019-12-09 16:07 ` Marc Kleine-Budde
0 siblings, 1 reply; 4+ messages in thread
From: Oliver Hartkopp @ 2019-12-08 11:34 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can, dvyukov
Cc: syzbot+b02ff0707a97e4e79ebb, glider, syzkaller-bugs, netdev,
o.rempel, eric.dumazet
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs
2019-12-08 11:34 ` Oliver Hartkopp
@ 2019-12-09 16:07 ` Marc Kleine-Budde
0 siblings, 0 replies; 4+ messages in thread
From: Marc Kleine-Budde @ 2019-12-09 16:07 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can, dvyukov
Cc: syzbot+b02ff0707a97e4e79ebb, glider, syzkaller-bugs, netdev,
o.rempel, eric.dumazet
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-12-09 16:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-07 18:34 [PATCH] can: ensure an initialized headroom in outgoing CAN sk_buffs Oliver Hartkopp
2019-12-08 10:48 ` Marc Kleine-Budde
2019-12-08 11:34 ` Oliver Hartkopp
2019-12-09 16:07 ` Marc Kleine-Budde
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.