netdev - Re: [Patch net] mlx4: set csum_complete_sw bit when fixing complete 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]

Date:	Mon, 27 Jun 2016 15:53:53 -0700
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Tom Herbert <tom@...bertland.com>
Cc:	Or Gerlitz <gerlitz.or@...il.com>,
	Linux Netdev List <netdev@...r.kernel.org>,
	Shani Michaeli <shanim@...lanox.com>,
	Tariq Toukan <tariqt@...lanox.com>,
	Yishai Hadas <yishaih@...lanox.com>,
	Carol L Soto <clsoto@...ibm.com>
Subject: Re: [Patch net] mlx4: set csum_complete_sw bit when fixing complete csum

On Mon, Jun 27, 2016 at 3:04 PM, Tom Herbert <tom@...bertland.com> wrote:
> On Mon, Jun 27, 2016 at 2:49 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
>> On Mon, Jun 27, 2016 at 2:47 PM, Tom Herbert <tom@...bertland.com> wrote:
>>> On Mon, Jun 27, 2016 at 2:44 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
>>>> On Mon, Jun 27, 2016 at 2:08 PM, Or Gerlitz <gerlitz.or@...il.com> wrote:
>>>>> On Mon, Jun 27, 2016 at 9:22 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
>>>>>> The stack doesn't trust the complete csum by hardware
>>>>>> even when it is correct.
>>>>>
>>>>> Can you explain that a little further?
>>>>
>>>> Sure, here is the code in __skb_checksum_complete():
>>>>
>>>>         /* skb->csum holds pseudo checksum */
>>>>         sum = csum_fold(csum_add(skb->csum, csum));
>>>>         if (likely(!sum)) {
>>>>                 if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
>>>>                     !skb->csum_complete_sw)
>>>>                         netdev_rx_csum_fault(skb->dev);
>>>>         }
>>>>
>>>> So when sum == 0, it means the checksum is correct. And
>>>> we already set ->ip_summed to CHECKSUM_COMPLETE
>>>> after check_csum(), and ->csum_complete_sw is initialized
>>>> to 0 when we allocate the skb. This is why we trigger
>>>> netdev_rx_csum_fault().
>>>>
>>> Yes, but this also means that the driver gave the stack a checksum
>>> complete value that was incorrect. That's an error.
>>
>> That is the whole purpose of commit f8c6455bb04b944edb69e,
>> isn't it?
>
> No. Unless you've uncovered some other bug, what is probably happening
> is that driver receives a packet with a checksum complete value. It
> records the value in the skbuff and marks it as CHECKSUM_COMPLETE.
> Subsequently, the stack tries to validate a transport layer checksum,
> and the validation fails (checksum does not sum to zero). The stack
> will then call __skb_checksum_complete from
> __skb_checksum_validate_complete. In this case the stack computes that
> transport checksum by hand and sees that transport checksum is valid--
> so that means that the original value in checksum complete was not
> correct, it is not set to the computed checksum of the whole packet.
> This is an important error because it catches issues where checksum is
> not correctly being pulled up.

I see, the comments in mlx4 driver said:

/* Although the stack expects checksum which doesn't include the pseudo
 * header, the HW adds it. To address that, we are subtracting the pseudo
 * header checksum from the checksum value provided by the HW.
 */

which seems imply it calculates a correct checksum for the whole
packet here, but the stack disagrees. Therefore skb->csum is not
still not what the stack expects.

Given skb_checksum_simple_validate() always pass a null pseudo
header, it looks like either the fix-up for pseudo header is not needed
at all for ICMP case, OR we need to call skb_checksum_validate()
for ICMPv4 case. Hmm...