svn commit: r337776 - head/sys/netinet6

[Bjoern A. Zeeb]

On 30 Aug 2018, at 22:00, Kristof Provost wrote:

> On 14 Aug 2018, at 19:17, Jonathan T. Looney wrote:
>> Author: jtl
>> Date: Tue Aug 14 17:17:37 2018
>> New Revision: 337776
>> URL: https://svnweb.freebsd.org/changeset/base/337776
>>
>> Log:
>>   Improve IPv6 reassembly performance by hashing fragments into 
>> buckets.
>>
>>   Currently, all IPv6 fragment reassembly queues are kept in a flat
>>   linked list. This has a number of implications. Two significant
>>   implications are: all reassembly operations share a common lock,
>>   and it is possible for the linked list to grow quite large.
>>
>>   Improve IPv6 reassembly performance by hashing fragments into 
>> buckets,
>>   each of which has its own lock. Calculate the hash key using a 
>> Jenkins
>>   hash with a random seed.
>>
>>   Reviewed by:	jhb
>>   Security:	FreeBSD-SA-18:10.ip
>>   Security:	CVE-2018-6923
>>
>> Modified:
>>   head/sys/netinet6/frag6.c
>>
>> Modified: head/sys/netinet6/frag6.c
>> ==============================================================================
>> --- head/sys/netinet6/frag6.c	Tue Aug 14 17:15:47 2018	(r337775)
>> +++ head/sys/netinet6/frag6.c	Tue Aug 14 17:17:37 2018	(r337776)
>> @@ -157,12 +176,13 @@ frag6_input(struct mbuf **mp, int *offp, int 
>> proto)
>>  	struct mbuf *m = *mp, *t;
>>  	struct ip6_hdr *ip6;
>>  	struct ip6_frag *ip6f;
>> -	struct ip6q *q6;
>> +	struct ip6q *head, *q6;
>>  	struct ip6asfrag *af6, *ip6af, *af6dwn;
>>  	struct in6_ifaddr *ia;
>>  	int offset = *offp, nxt, i, next;
>>  	int first_frag = 0;
>>  	int fragoff, frgpartlen;	/* must be larger than u_int16_t */
>> +	uint32_t hash, hashkey[sizeof(struct in6_addr) * 2 + 1], *hashkeyp;
>
> I’m pretty sure you didn’t mean for the hashkey to be 1028 bytes 
> long.
>
>>  	struct ifnet *dstifp;
>>  	u_int8_t ecn, ecn0;
>>  #ifdef RSS
>> @@ -231,7 +251,16 @@ frag6_input(struct mbuf **mp, int *offp, int 
>> proto)
>>  		return (ip6f->ip6f_nxt);
>>  	}
>>
>> -	IP6Q_LOCK();
>> +	hashkeyp = hashkey;
>> +	memcpy(hashkeyp, &ip6->ip6_src, sizeof(struct in6_addr));
>> +	hashkeyp += sizeof(struct in6_addr) / sizeof(*hashkeyp);
>> +	memcpy(hashkeyp, &ip6->ip6_dst, sizeof(struct in6_addr));
>> +	hashkeyp += sizeof(struct in6_addr) / sizeof(*hashkeyp);
>> +	*hashkeyp = ip6f->ip6f_ident;
>> +	hash = jenkins_hash32(hashkey, nitems(hashkey), V_ip6q_hashseed);
>
> Especially combined with this, because it means we hash a lot more 
> than the src/dst dress and ident.
> We end up hashing random stack garbage, so the hash isn’t guaranteed 
> to be the same every time.
> So we end up not always being able to reassemble the packet.
>
> It’s especially fun when you consider that a Dtrace fbt:::entry 
> probe will leave a consistent stack state, so when you try to probe 
> frag6_input() the problem magically goes away.
>
> This broke the sys.netpfil.pf.fragmentation.v6 test.
>
> I’ve done this, which fixes the problem:
>
> 	diff --git a/sys/netinet6/frag6.c b/sys/netinet6/frag6.c
> 	index 0f30801540a..e1f2b3f5842 100644
> 	--- a/sys/netinet6/frag6.c
> 	+++ b/sys/netinet6/frag6.c
> 	@@ -218,7 +218,9 @@ frag6_input(struct mbuf **mp, int *offp, int 
> proto)
> 	        int offset = *offp, nxt, i, next;
> 	        int first_frag = 0;
> 	        int fragoff, frgpartlen;        /* must be larger than 
> u_int16_t */
> 	-       uint32_t hash, hashkey[sizeof(struct in6_addr) * 2 + 1], 
> *hashkeyp;
> 	+       uint32_t hashkey[(sizeof(struct in6_addr) * 2 + 
> sizeof(u_int32_t)) /

Can we actually make it the size of the field rather than uint32_t (not 
u_int32_t)?  I guess not easily but at least change the type spelling 
and leave a comment what it is?


> 	+           sizeof(uint32_t)];
> 	+       uint32_t hash, *hashkeyp;
> 	        struct ifnet *dstifp;
> 	        u_int8_t ecn, ecn0;
> 	 #ifdef RSS
>
> Regards,
> Kristof