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]

Date:   Tue, 18 Apr 2023 09:59:57 +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 9:47 PM, Willem de Bruijn 写道:
> luwei (O) wrote:
>> 在 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.
> The header is pulled, but still present. Obviously something bogus gets
> written if the virtio_net_hdr configures csum offload with a bogus
> offset. But as long as the offset is zero or positive from skb->head,
> the checksum helper works as intended.

    OK, Thanks for your reply

>   
>>> 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