netdev - Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

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]

Date:   Sat, 5 May 2018 10:39:21 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        David Miller <davem@...emloft.net>
Subject: Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter
 to GSO segmentation

On Sat, May 5, 2018 at 3:01 AM, Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
> On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> From: Alexander Duyck <alexander.h.duyck@...el.com>
>>
>> This patch is meant to allow us to avoid having to recompute the checksum
>> from scratch and have it passed as a parameter.
>>
>> Instead of taking that approach we can take advantage of the fact that the
>> length that was used to compute the existing checksum is included in the
>> UDP header. If we cancel that out by adding the value XOR with 0xFFFF we
>> can then just add the new length in and fold that into the new result.
>>
>> I think this may be fixing a checksum bug in the original code as well
>> since the checksum that was passed included the UDP header in the checksum
>> computation, but then excluded it for the adjustment on the last frame. I
>> believe this may have an effect on things in the cases where the two differ
>> by bits that would result in things crossing the byte boundaries.
>
> The replacement code, below, subtracts original payload size then adds
> the new payload size. mss here excludes the udp header size.
>
>>                 /* last packet can be partial gso_size */
>> -               if (!seg->next)
>> -                       csum_replace2(&uh->check, htons(mss),
>> -                                     htons(seg->len - hdrlen - sizeof(*uh)));

That is my point. When you calculated your checksum you included the
UDP header in the calculation.

-       return __udp_gso_segment(gso_skb, features,
-                                udp_v4_check(sizeof(struct udphdr) + mss,
-                                             iph->saddr, iph->daddr, 0));

Basically the problem is in one spot you are adding the sizeof(struct
udphdr) + mss and then in another you are cancelling it out as mss and
trying to account for it by also dropping the UDP header from the
payload length of the value you are adding. That works in the cases
where the effect doesn't cause any issues with the byte ordering,
however I think when mss + 8 crosses a byte boundary it can lead to
issues since the calculation is done on a byte swapped value.