Date:   Tue, 15 Dec 2020 08:42:56 +0300
From:   Vasily Averin <vvs@...tuozzo.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: check skb partial checksum offset after trim

On 12/15/20 12:07 AM, Willem de Bruijn wrote:
> On Mon, Dec 14, 2020 at 3:56 PM Jakub Kicinski <kuba@...nel.org> wrote:
>>
>> On Sun, 13 Dec 2020 20:59:54 -0500 Willem de Bruijn wrote:
>>> On Sun, Dec 13, 2020 at 2:37 PM Vasily Averin <vvs@...tuozzo.com> wrote:
>>>>>> On 12/11/20 6:37 PM, Vasily Averin wrote:
>>>>>>> It seems for me the similar problem can happen in __skb_trim_rcsum().
>>>>>>> Also I doubt that that skb_checksum_start_offset(skb) checks in
>>>>>>> __skb_postpull_rcsum() and skb_csum_unnecessary() are correct,
>>>>>>> becasue they do not guarantee that skb have correct CHECKSUM_PARTIAL.
>>>>>>> Could somebody confirm it?
>>>>>>
>>>>>> I've rechecked the code and I think now that other places are not affected,
>>>>>> i.e. skb_push_rcsum() only should be patched.
>>>>>
>>>>> Thanks for investigating this. So tun was able to insert a packet with
>>>>> csum_start + csum_off + 2 beyond the packet after trimming, using
>>>>> virtio_net_hdr.csum_...
>>>>>
>>>>> Any packet with an offset beyond the end of the packet is bogus
>>>>> really. No need to try to accept it by downgrading to CHECKSUM_NONE.
>>>>
>>>> Do you mean it's better to force pskb_trim_rcsum() to return -EINVAL instead?
>>>
>>> I would prefer to have more strict input validation in
>>> tun/virtio/packet (virtio_net_hdr_to_skb), rather than new checks in
>>> the hot path. But that is a larger change and not feasible
>>> unconditionally due to performance impact and likely some false
>>> positive drops. So out of scope here.
>>
>> Could you please elaborate? Is it the case that syzbot constructed some
>> extremely convoluted frame to trigger this?
> 
> Somewhat convoluted, yes. A packet with a checksum offset beyond the
> end of the ip packet.
> 
> skb_partial_csum_set (called from virtio_net_hdr_to_skb) verifies that
> the offsets are within the linear buffer passed from userspace, but
> without protocol parsing we don't know at that time that the offset is
> beyond the end of the packet.
> 
>> Otherwise the validation
>> at the source would work as well, no?
> 
> The problem with validation is two fold: it may add noticeable cost to
> the hot path and it may have false positives: packets that the flow
> dissector cannot fully dissect, but which are harmless and were
> previously accepted.
> 
> I do want to add such strict source validation based on flow
> dissection, but as an opt-in (sysctl) feature.
> 
>> Does it actually trigger upstream? The linked syzbot report is for 4.14
>> but from the commit description it sounds like the problem should repro
>> rather reliably.
> 
>>>From the description, I would assume so, too. Haven't tested.

Original syzkaller reproducer fails on upstream because it prepares
invalid iptable ruleset, new kernels have more strict validation of iptable rules. 
I've commented this call im originsl reproducer and set the CHECKSUM rule manually,
then run of corrected reproducer triggered BUG_ON in skb_checksum_help().
I've crashed upstream 5.10-rc7 kernel by this way and then validated patched kernel.
originally we got such problem on RHEL7-based kernel, so I think the problem 
affects all stable and actual distribution kernels. 

Thank you,	
    Vasily Averin

Powered by blists - more mailing lists