netdev - Re: [PATCH] can: ensure an initialized headroom in outgoing CAN sk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cc102c3b-d9d3-6447-7581-a36795259cc2@pengutronix.de>
Date:   Sun, 8 Dec 2019 11:48:14 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Oliver Hartkopp <socketcan@...tkopp.net>,
        linux-can@...r.kernel.org, dvyukov@...gle.com
Cc:     syzbot+b02ff0707a97e4e79ebb@...kaller.appspotmail.com,
        glider@...gle.com, syzkaller-bugs@...glegroups.com,
        netdev@...r.kernel.org, o.rempel@...gutronix.de,
        eric.dumazet@...il.com
Subject: Re: [PATCH] can: ensure an initialized headroom in outgoing CAN
 sk_buffs

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@...kaller.appspotmail.com
> Signed-off-by: Oliver Hartkopp <socketcan@...tkopp.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 |



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)