[PATCH] net: check skb partial checksum offset after trim

[PATCH] net: check skb partial checksum offset after trim

[Vasily Averin]

syzkaller reproduces BUG_ON in skb_checksum_help():
tun creates skb with big partial checksum area and small ip packet inside,
then ip_rcv() decreases skb size of below length of checksummed area,
then checksum_tg() called via netfilter hook detects incorrect skb:

        offset = skb_checksum_start_offset(skb);
        BUG_ON(offset >= skb_headlen(skb));

This patch drops CHEKSUM_PARTIAL mark when skb is trimmed below
size of checksummed area.
Link: https://syzkaller.appspot.com/bug?id=b419a5ca95062664fe1a60b764621eb4526e2cd0
Reported-by: syzbot+7010af67ced6105e5ab6@syzkaller.appspotmail.com
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 include/linux/skbuff.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a828cf9..0a9545d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3419,9 +3419,18 @@ static inline void *skb_push_rcsum(struct sk_buff *skb, unsigned int len)
 
 static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
 {
+	int ret;
+
 	if (likely(len >= skb->len))
 		return 0;
-	return pskb_trim_rcsum_slow(skb, len);
+	ret = pskb_trim_rcsum_slow(skb, len);
+	if (!ret && (skb->ip_summed == CHECKSUM_PARTIAL)) {
+		int offset = skb_checksum_start_offset(skb) + skb->csum_offset;
+
+		if (offset + sizeof(__sum16) > skb_headlen(skb))
+			skb->ip_summed = CHECKSUM_NONE;
+	}
+	return ret;
 }
 
 static inline int __skb_trim_rcsum(struct sk_buff *skb, unsigned int len)
-- 
1.8.3.1

Re: [PATCH] net: check skb partial checksum offset after trim

[Vasily Averin]

Originally it was reported on Ubuntu 4.14 kernel,
then I've reproduced it on upstream 5.10-rc7.
If I'm right the problem is quite old and should 
affect all maintained stable kernels too.

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?

Thank you,
	Vasily Averin

On 12/11/20 6:00 PM, Vasily Averin wrote:
> syzkaller reproduces BUG_ON in skb_checksum_help():
> tun creates skb with big partial checksum area and small ip packet inside,
> then ip_rcv() decreases skb size of below length of checksummed area,
> then checksum_tg() called via netfilter hook detects incorrect skb:
> 
>         offset = skb_checksum_start_offset(skb);
>         BUG_ON(offset >= skb_headlen(skb));
> 
> This patch drops CHEKSUM_PARTIAL mark when skb is trimmed below
> size of checksummed area.
> Link: https://syzkaller.appspot.com/bug?id=b419a5ca95062664fe1a60b764621eb4526e2cd0
> Reported-by: syzbot+7010af67ced6105e5ab6@syzkaller.appspotmail.com
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  include/linux/skbuff.h | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index a828cf9..0a9545d 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3419,9 +3419,18 @@ static inline void *skb_push_rcsum(struct sk_buff *skb, unsigned int len)
>  
>  static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
>  {
> +	int ret;
> +
>  	if (likely(len >= skb->len))
>  		return 0;
> -	return pskb_trim_rcsum_slow(skb, len);
> +	ret = pskb_trim_rcsum_slow(skb, len);
> +	if (!ret && (skb->ip_summed == CHECKSUM_PARTIAL)) {
> +		int offset = skb_checksum_start_offset(skb) + skb->csum_offset;
> +
> +		if (offset + sizeof(__sum16) > skb_headlen(skb))
> +			skb->ip_summed = CHECKSUM_NONE;
> +	}
> +	return ret;
>  }
>  
>  static inline int __skb_trim_rcsum(struct sk_buff *skb, unsigned int len)
> 

Re: [PATCH] net: check skb partial checksum offset after trim

[Vasily Averin]

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.

Thank you,
	Vasily Averin

Re: [PATCH] net: check skb partial checksum offset after trim

[Willem de Bruijn]

On Sat, Dec 12, 2020 at 5:01 AM Vasily Averin <vvs@virtuozzo.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.

It is not ideal to have to add branches in the common path for these
obscure bad packets from virtio/tuntap/af_packet. We try to avoid that
with more strict validation at the source in virtio_net_hdr_to_skb.
Evidently syzbot again found a way past again.

If this is a packet with gso_type and checksum offload, we know the
accepted protocols and can validate the offset. If gso_type is none,
however, no such assumptions can be made. All we could do is try to
dissect and if a known protocol and valid th_off, compare that to the
checksum fields passed by userspace.

So that path is certainly more complex than your fix, which works as well.

Re: [PATCH] net: check skb partial checksum offset after trim

[Vasily Averin]

On 12/13/20 2:49 AM, Willem de Bruijn wrote:
> On Sat, Dec 12, 2020 at 5:01 AM Vasily Averin <vvs@virtuozzo.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?

Thank you,
	Vasily Averin

Re: [PATCH] net: check skb partial checksum offset after trim

[Willem de Bruijn]

On Sun, Dec 13, 2020 at 2:37 PM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> On 12/13/20 2:49 AM, Willem de Bruijn wrote:
> > On Sat, Dec 12, 2020 at 5:01 AM Vasily Averin <vvs@virtuozzo.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.

Instead of adding a workaround in the not path, I thought about
converting the two checks in skb_checksum_help

  BUG_ON(offset >= skb_headlen(skb));
  BUG_ON(offset + sizeof(__sum16) > skb_headlen(skb));

to normal error paths and return EINVAL. But most callers, including
this one (checksum_tg), don't check the return value to drop the
packet.

Given that, your approach sounds the most reasonable. I would still
drop these packets, as they are clearly bad and the only source of
badness we know is untrusted user input.

In that case, perhaps the test can move into pskb_trim_rcsum_slow,
below the CHECKSUM_COMPLETE branch.

[PATCH v2] net: drop bogus skb with CHECKSUM_PARTIAL and offset beyond end of trimmed packet

[Vasily Averin]

syzbot reproduces BUG_ON in skb_checksum_help():
tun creates (bogus) skb with huge partial-checksummed area and
small ip packet inside. Then ip_rcv trims the skb based on size
of internal ip packet, after that csum offset points beyond of
trimmed skb. Then checksum_tg() called via netfilter hook
triggers BUG_ON:

        offset = skb_checksum_start_offset(skb);
        BUG_ON(offset >= skb_headlen(skb));

To work around the problem this patch forces pskb_trim_rcsum_slow()
to return -EINVAL in described scenario. It allows its callers to
drop such kind of packets.

Link: https://syzkaller.appspot.com/bug?id=b419a5ca95062664fe1a60b764621eb4526e2cd0
Reported-by: syzbot+7010af67ced6105e5ab6@syzkaller.appspotmail.com
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
v2: drop bogus packets instead change its CHECKSUM_PARTIAL to CHECKSUM_NONE 

 net/core/skbuff.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e578544..fbadd93 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2011,6 +2011,12 @@ int pskb_trim_rcsum_slow(struct sk_buff *skb, unsigned int len)
 		skb->csum = csum_block_sub(skb->csum,
 					   skb_checksum(skb, len, delta, 0),
 					   len);
+	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		int hdlen = (len > skb_headlen(skb)) ? skb_headlen(skb) : len;
+		int offset = skb_checksum_start_offset(skb) + skb->csum_offset;
+
+		if (offset + sizeof(__sum16) > hdlen)
+			return -EINVAL;
 	}
 	return __pskb_trim(skb, len);
 }
-- 
1.8.3.1

Re: [PATCH] net: check skb partial checksum offset after trim

[Jakub Kicinski]

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@virtuozzo.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? Otherwise the validation
at the source would work as well, no?

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.

> Instead of adding a workaround in the not path, I thought about
> converting the two checks in skb_checksum_help
> 
>   BUG_ON(offset >= skb_headlen(skb));
>   BUG_ON(offset + sizeof(__sum16) > skb_headlen(skb));
> 
> to normal error paths and return EINVAL. But most callers, including
> this one (checksum_tg), don't check the return value to drop the
> packet.
> 
> Given that, your approach sounds the most reasonable. I would still
> drop these packets, as they are clearly bad and the only source of
> badness we know is untrusted user input.
> 
> In that case, perhaps the test can move into pskb_trim_rcsum_slow,
> below the CHECKSUM_COMPLETE branch.

Re: [PATCH v2] net: drop bogus skb with CHECKSUM_PARTIAL and offset beyond end of trimmed packet

[Willem de Bruijn]

On Mon, Dec 14, 2020 at 2:21 PM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> syzbot reproduces BUG_ON in skb_checksum_help():
> tun creates (bogus) skb with huge partial-checksummed area and
> small ip packet inside. Then ip_rcv trims the skb based on size
> of internal ip packet, after that csum offset points beyond of
> trimmed skb. Then checksum_tg() called via netfilter hook
> triggers BUG_ON:
>
>         offset = skb_checksum_start_offset(skb);
>         BUG_ON(offset >= skb_headlen(skb));
>
> To work around the problem this patch forces pskb_trim_rcsum_slow()
> to return -EINVAL in described scenario. It allows its callers to
> drop such kind of packets.
>
> Link: https://syzkaller.appspot.com/bug?id=b419a5ca95062664fe1a60b764621eb4526e2cd0
> Reported-by: syzbot+7010af67ced6105e5ab6@syzkaller.appspotmail.com
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
> v2: drop bogus packets instead change its CHECKSUM_PARTIAL to CHECKSUM_NONE

Thanks for revising.

As far as I can tell, this goes back to the original introduction of
that user interface to set checksum offload, so

Fixes: 296f96fcfc16 ("Net driver using virtio")

For next time, please also mark network fixes as [PATCH net]. With that

Acked-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH] net: check skb partial checksum offset after trim

[Willem de Bruijn]

On Mon, Dec 14, 2020 at 3:56 PM Jakub Kicinski <kuba@kernel.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@virtuozzo.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.

Re: [PATCH v2] net: drop bogus skb with CHECKSUM_PARTIAL and offset beyond end of trimmed packet

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Mon, 14 Dec 2020 22:07:39 +0300 you wrote:
> syzbot reproduces BUG_ON in skb_checksum_help():
> tun creates (bogus) skb with huge partial-checksummed area and
> small ip packet inside. Then ip_rcv trims the skb based on size
> of internal ip packet, after that csum offset points beyond of
> trimmed skb. Then checksum_tg() called via netfilter hook
> triggers BUG_ON:
> 
> [...]

Here is the summary with links:
  - [v2] net: drop bogus skb with CHECKSUM_PARTIAL and offset beyond end of trimmed packet
    https://git.kernel.org/netdev/net-next/c/54970a2fbb67

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] net: check skb partial checksum offset after trim

[Vasily Averin]

On 12/15/20 12:07 AM, Willem de Bruijn wrote:
> On Mon, Dec 14, 2020 at 3:56 PM Jakub Kicinski <kuba@kernel.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@virtuozzo.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