| Message ID | 7080e8a3-6eaa-e9e1-afd8-b1eef38d1e89@virtuozzo.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: check skb partial checksum offset after trim | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 9536 this patch: 9536 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Unnecessary parentheses around 'skb->ip_summed == CHECKSUM_PARTIAL' |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 9648 this patch: 9648 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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) >
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
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.
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
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.
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.
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.
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
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)
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(-)