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:
signature.asc
Description: OpenPGP digital signature
