netdev - Re: [RFC] ndo_validate_skb: Let the netdev check a valid skb content

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:	Thu, 07 Jan 2010 18:50:35 +0100
From:	Oliver Hartkopp <oliver@...tkopp.net>
To:	David Miller <davem@...emloft.net>
CC:	netdev@...r.kernel.org, Wolfgang Grandegger <wg@...ndegger.com>
Subject: Re: [RFC] ndo_validate_skb: Let the netdev check a valid skb content

David Miller wrote:
> From: Oliver Hartkopp <oliver@...tkopp.net>
> Date: Wed, 23 Dec 2009 20:23:27 +0100
> 
>> @@ -2034,6 +2035,14 @@ int dev_queue_xmit(struct sk_buff *skb)
>>  			goto out_kfree_skb;
>>  	}
>>  
>> +	/* If the device offers a function to validate the skb content, let
>> +	 * it check the skb and return an error to the caller if it fails.
>> +	 */ 
>> +	if (ops->ndo_validate_skb && ops->ndo_validate_skb(skb)) {
>> +		rc = -EINVAL;
>> +		goto out_kfree_skb;
>> +	}
>> +
> 
> To me this is as valuable as no error at all and simply having
> the driver drop the frame.  Which is what we do now.  I can
> sniff at the receiver to see that the device never transmitted
> the frame.
> 
> Is this getting this generic -EINVAL back so important that it's
> worth adding the new if() test for the NULL method to every packet
> that traverses the machine?  Keep in mind we can route at rates
> exceeding 1 million packets per second, so ever cycle you add
> here really matters.
> 
> If you want to place the hooks still, put them out of the fast
> path, which is probably in AF_PACKET.  We can extend this to
> call the validation function from other "untrusted" sources.
> 
> But do realize that you're not saving anything, things like
> queueing disciplines and traffic classifiers in the qdisc
> layer can modify any part of the packet however they wish.
> 
> So even if the CAN protocol layer itself emitted a valid
> frame, the qdisc layer can "corrupt" it.

I see. Thanks for your detailed answer!

Indeed silently drop the invalid CAN frame inside the driver (and increase
dev->stats.tx_dropped) looks like the best solution to me now.

If people try to push rubbish into the device, tx_dropped is an appropriate
indication.

Thanks,
Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html