netdev - Re: [PATCH v3] [net]: Fix skb->csum update in inet_proto_csum

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: <daf995db-37c6-a2f7-4d12-5c1a29e1c59b@iogearbox.net>
Date:   Wed, 22 Jan 2020 17:22:54 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Florian Westphal <fw@...len.de>
Cc:     Praveen Chaudhary <praveen5582@...il.com>, pablo@...filter.org,
        davem@...emloft.net, kadlec@...filter.org,
        netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Zhenggen Xu <zxu@...kedin.com>,
        Andy Stracner <astracner@...kedin.com>
Subject: Re: [PATCH v3] [net]: Fix skb->csum update in
 inet_proto_csum_replace16().

On 1/22/20 12:43 PM, Florian Westphal wrote:
> Daniel Borkmann <daniel@...earbox.net> wrote:
>>> @@ -449,9 +464,6 @@ void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb,
>>>    	if (skb->ip_summed != CHECKSUM_PARTIAL) {
>>>    		*sum = csum_fold(csum_partial(diff, sizeof(diff),
>>>    				 ~csum_unfold(*sum)));
>>> -		if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
>>> -			skb->csum = ~csum_partial(diff, sizeof(diff),
>>> -						  ~skb->csum);
>>
>> What is the technical rationale in removing this here but not in any of the
>> other inet_proto_csum_replace*() functions? You changelog has zero analysis
>> on why here but not elsewhere this change would be needed?
> 
> Right, I think it could be dropped everywhere BUT there is a major caveat:
> 
> At least for the nf_nat case ipv4 header manipulation (which uses the other
> helpers froum utils.c) will eventually also update iph->checksum field
> to account for the changed ip addresses.
> 
> And that update doesn't touch skb->csum.
> 
> So in a way the update of skb->csum in the other helpers indirectly account
> for later ip header checksum update.
> 
> At least that was my conclusion when reviewing the earlier incarnation
> of the patch.

Mainly asking because not inet_proto_csum_replace16() but the other ones are
exposed via BPF and they are all in no way fundamentally different to each
other, but my concern is that depending on how the BPF prog updates the csums
things could start to break. :/