netdev - Re: 答复: [PATCH net] net: Add check for csum_start in skb_partial_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: <47fca2c7-db7c-0265-d724-38dffc62debe@huawei.com>
Date:   Sat, 15 Apr 2023 10:18:49 +0800
From:   "luwei (O)" <luwei32@...wei.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Eric Dumazet <edumazet@...gle.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "asml.silence@...il.com" <asml.silence@...il.com>,
        "imagedong@...cent.com" <imagedong@...cent.com>,
        "brouer@...hat.com" <brouer@...hat.com>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "jbenc@...hat.com" <jbenc@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: 答复: [PATCH net] net: Add check for csum_start in skb_partial_csum_set()


在 2023/4/15 1:20 AM, Willem de Bruijn 写道:
> Willem de Bruijn wrote:
>> luwei (O) wrote:
>>> yes, here is the vnet_hdr:
>>>
>>>      flags: 3
>>>      gso_type: 3
>>>      hdr_len: 23
>>>      gso_size: 58452
>>>      csum_start: 5
>>>      csum_offset: 16
>>>
>>> and the packet:
>>>
>>> | vnet_hdr | mac header | network header | data ... |
>>>
>>>    memcpy((void*)0x20000200,
>>>           "\x03\x03\x02\x00\x54\xe4\x05\x00\x10\x00\x80\x00\x00\x53\xcc\x9c\x2b"
>>>           "\x19\x3b\x00\x00\x00\x89\x4f\x08\x03\x83\x81\x04",
>>>           29);
>>>    *(uint16_t*)0x200000c0 = 0x11;
>>>    *(uint16_t*)0x200000c2 = htobe16(0);
>>>    *(uint32_t*)0x200000c4 = r[3];
>>>    *(uint16_t*)0x200000c8 = 1;
>>>    *(uint8_t*)0x200000ca = 0;
>>>    *(uint8_t*)0x200000cb = 6;
>>>    memset((void*)0x200000cc, 170, 5);
>>>    *(uint8_t*)0x200000d1 = 0;
>>>    memset((void*)0x200000d2, 0, 2);
>>>    syscall(__NR_sendto, r[1], 0x20000200ul, 0xe45ful, 0ul, 0x200000c0ul, 0x14ul);
>> Thanks. So this can happen whenever a packet is injected into the tx
>> path with a virtio_net_hdr.
>>
>> Even if we add bounds checking for the link layer header in pf_packet,
>> it can still point to the network header.
>>
>> If packets are looped to the tx path, skb_pull is common if a packet
>> traverses tunnel devices. But csum_start does not directly matter in
>> the rx path (CHECKSUM_PARTIAL is just seen as CHECKSUM_UNNECESSARY).
>> Until it is forwarded again to the tx path.
>>
>> So the question is which code calls skb_checksum_start_offset on the
>> tx path. Clearly, skb_checksum_help. Also a lot of drivers. Which
>> may cast the signed int return value to an unsigned. Even an u8 in
>> the first driver I spotted (alx).
>>
>> skb_postpull_rcsum anticipates a negative return value, as do other
>> core functions. So it clearly allowed in certain cases. We cannot
>> just bound it.
>>
>> Summary after a long story: an initial investigation, but I don't have
>> a good solution so far. Maybe others have a good suggestiong based on
>> this added context.
> Specific to skb_checksum_help, it appears that skb_checksum will
> work with negative offset just fine.

       In this case maybe not, since it checksums from within the mac 
header, and the mac header

        will be stripped when the rx path checks the checksum.

>
> Perhaps the only issue is that the WARN_ON_ONCE compares signed to
> unsigned, and thus incorrectly interprets a negative offset as
>   >= skb_headlen(skb)
> .

-- 
Best Regards,
Lu Wei