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  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:   Sun, 13 Dec 2020 20:59:54 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Vasily Averin <vvs@...tuozzo.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: check skb partial checksum offset after trim

On Sun, Dec 13, 2020 at 2:37 PM Vasily Averin <vvs@...tuozzo.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@...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.

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.

Powered by blists - more mailing lists