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] [day] [month] [year] [list]

Date:   Mon, 9 Dec 2019 17:07:31 +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/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 |



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