Re: [PATCH net v2] net: udp: fix handling of CHECKSUM_COMPLETE packets

[Maxim Mikityanskiy <maxtram95@gmail.com>]

Hi Sean,

I was analyzing this code and have some concerns. Please see the comments below.

On Wed, Oct 24, 2018 at 1:04 AM Sean Tranchetti <stranche@codeaurora.org> wrote:
>
> Current handling of CHECKSUM_COMPLETE packets by the UDP stack is
> incorrect for any packet that has an incorrect checksum value.
>
> udp4/6_csum_init() will both make a call to
> __skb_checksum_validate_complete() to initialize/validate the csum
> field when receiving a CHECKSUM_COMPLETE packet. When this packet
> fails validation, skb->csum will be overwritten with the pseudoheader
> checksum so the packet can be fully validated by software, but the
> skb->ip_summed value will be left as CHECKSUM_COMPLETE so that way
> the stack can later warn the user about their hardware spewing bad
> checksums. Unfortunately, leaving the SKB in this state can cause
> problems later on in the checksum calculation.
>
> Since the the packet is still marked as CHECKSUM_COMPLETE,
> udp_csum_pull_header() will SUBTRACT the checksum of the UDP header
> from skb->csum instead of adding it, leaving us with a garbage value
> in that field. Once we try to copy the packet to userspace in the
> udp4/6_recvmsg(), we'll make a call to skb_copy_and_csum_datagram_msg()
> to checksum the packet data and add it in the garbage skb->csum value
> to perform our final validation check.
>
> Since the value we're validating is not the proper checksum, it's possible
> that the folded value could come out to 0, causing us not to drop the
> packet. Instead, we believe that the packet was checksummed incorrectly
> by hardware since skb->ip_summed is still CHECKSUM_COMPLETE, and we attempt
> to warn the user with netdev_rx_csum_fault(skb->dev);
>
> Unfortunately, since this is the UDP path, skb->dev has been overwritten
> by skb->dev_scratch and is no longer a valid pointer, so we end up
> reading invalid memory.
>
> This patch addresses this problem in two ways:
>         1) Do not use the dev pointer when calling netdev_rx_csum_fault()
>            from skb_copy_and_csum_datagram_msg(). Since this gets called
>            from the UDP path where skb->dev has been overwritten, we have
>            no way of knowing if the pointer is still valid. Also for the
>            sake of consistency with the other uses of
>            netdev_rx_csum_fault(), don't attempt to call it if the
>            packet was checksummed by software.
>
>         2) Add better CHECKSUM_COMPLETE handling to udp4/6_csum_init().
>            If we receive a packet that's CHECKSUM_COMPLETE that fails
>            verification (i.e. skb->csum_valid == 0), check who performed
>            the calculation. It's possible that the checksum was done in
>            software by the network stack earlier (such as Netfilter's
>            CONNTRACK module), and if that says the checksum is bad,
>            we can drop the packet immediately instead of waiting until
>            we try and copy it to userspace. Otherwise, we need to
>            mark the SKB as CHECKSUM_NONE, since the skb->csum field
>            no longer contains the full packet checksum after the
>            call to __skb_checksum_validate_complete().
>
> Fixes: e6afc8ace6dd ("udp: remove headers from UDP packets before queueing")
> Fixes: c84d949057ca ("udp: copy skb->truesize in the first cache line")
> Cc: Sam Kumar <samanthakumar@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
> ---
>  net/core/datagram.c     |  5 +++--
>  net/ipv4/udp.c          | 20 ++++++++++++++++++--
>  net/ipv6/ip6_checksum.c | 20 ++++++++++++++++++--
>  3 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index 9aac0d6..df16493 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -808,8 +808,9 @@ int skb_copy_and_csum_datagram_msg(struct sk_buff *skb,
>                         return -EINVAL;
>                 }
>
> -               if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE))
> -                       netdev_rx_csum_fault(skb->dev);
> +               if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
> +                   !skb->csum_complete_sw)
> +                       netdev_rx_csum_fault(NULL);

Here you are fixing skb_copy_and_csum_datagram_msg, but this is not
the only flow that leads to netdev_rx_csum_fault. This function is
also called from __skb_checksum_complete (also after using the screwed
up skb->csum), and there are two ways we can get to
__skb_checksum_complete:

1. Directly from skb_copy_and_csum_datagram_msg.

2. udpv6_recvmsg -> __udp_lib_checksum_complete -> __skb_checksum_complete:

if (copied < ulen || peeking ||
    (is_udplite && UDP_SKB_CB(skb)->partial_cov)) {
        checksum_valid = udp_skb_csum_unnecessary(skb) ||
                        !__udp_lib_checksum_complete(skb);
        if (!checksum_valid)
                goto csum_copy_err;
}

It looks to me, as if these flows may also lead to parsing
skb->dev_scratch as skb->dev.

>         }
>         return 0;
>  fault:
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index c32a4c1..f8183fd 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2120,8 +2120,24 @@ static inline int udp4_csum_init(struct sk_buff *skb, struct udphdr *uh,
>         /* Note, we are only interested in != 0 or == 0, thus the
>          * force to int.
>          */
> -       return (__force int)skb_checksum_init_zero_check(skb, proto, uh->check,
> -                                                        inet_compute_pseudo);
> +       err = (__force int)skb_checksum_init_zero_check(skb, proto, uh->check,
> +                                                       inet_compute_pseudo);
> +       if (err)
> +               return err;
> +
> +       if (skb->ip_summed == CHECKSUM_COMPLETE && !skb->csum_valid) {
> +               /* If SW calculated the value, we know it's bad */
> +               if (skb->csum_complete_sw)
> +                       return 1;
> +
> +               /* HW says the value is bad. Let's validate that.
> +                * skb->csum is no longer the full packet checksum,
> +                * so don't treat it as such.
> +                */
> +               skb_checksum_complete_unset(skb);
> +       }
> +
> +       return 0;
>  }
>
>  /* wrapper for udp_queue_rcv_skb tacking care of csum conversion and
> diff --git a/net/ipv6/ip6_checksum.c b/net/ipv6/ip6_checksum.c
> index 547515e..3777170 100644
> --- a/net/ipv6/ip6_checksum.c
> +++ b/net/ipv6/ip6_checksum.c
> @@ -88,8 +88,24 @@ int udp6_csum_init(struct sk_buff *skb, struct udphdr *uh, int proto)
>          * Note, we are only interested in != 0 or == 0, thus the
>          * force to int.
>          */
> -       return (__force int)skb_checksum_init_zero_check(skb, proto, uh->check,
> -                                                        ip6_compute_pseudo);
> +       err = (__force int)skb_checksum_init_zero_check(skb, proto, uh->check,
> +                                                       ip6_compute_pseudo);
> +       if (err)
> +               return err;
> +
> +       if (skb->ip_summed == CHECKSUM_COMPLETE && !skb->csum_valid) {
> +               /* If SW calculated the value, we know it's bad */
> +               if (skb->csum_complete_sw)
> +                       return 1;
> +
> +               /* HW says the value is bad. Let's validate that.
> +                * skb->csum is no longer the full packet checksum,
> +                * so don't treat is as such.
> +                */
> +               skb_checksum_complete_unset(skb);

If I understand it correctly, we'll have ip_summed == CHECKSUM_NONE
after this point, so __skb_checksum_complete won't call
netdev_rx_csum_fault anymore in case of bad hardware that calculates
checksums incorrectly.

> +       }
> +
> +       return 0;
>  }
>  EXPORT_SYMBOL(udp6_csum_init);
>

Are my concerns real, or am I missing anything? If my points are
valid, do you have ideas how it could be fixed?

Thanks and looking forward to your reply.