netdev - Re: [PATCH] net: multicast: calculate csum of looped-back and forwarded packets

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 linux-cve-announce 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:   Fri, 22 Oct 2021 15:07:18 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Cyril Strejc <cyril.strejc@...da.cz>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: multicast: calculate csum of looped-back and
 forwarded packets

On Tue, Oct 19, 2021 at 7:46 AM Cyril Strejc <cyril.strejc@...da.cz> wrote:
>
> During a testing of an user-space application which transmits UDP
> multicast datagrams and utilizes multicast routing to send the UDP
> datagrams out of defined network interfaces, I've found a multicast
> router does not fill-in UDP checksum into locally produced, looped-back
> and forwarded UDP datagrams, if an original output NIC the datagrams
> are sent to has UDP TX checksum offload enabled.
>
> The datagrams are sent malformed out of the NIC the datagrams have been
> forwarded to.
>
> It is because:
>
> 1. If TX checksum offload is enabled on an output NIC, UDP checksum
>    is not calculated by kernel and is not filled into skb data.
>
> 2. dev_loopback_xmit(), which is called solely by
>    ip_mc_finish_output(), sets skb->ip_summed = CHECKSUM_UNNECESSARY
>    unconditionally.
>
> 3. Since 35fc92a9 ("[NET]: Allow forwarding of ip_summed except
>    CHECKSUM_COMPLETE"), the ip_summed value is preserved during
>    forwarding.
>
> 4. If ip_summed != CHECKSUM_PARTIAL, checksum is not calculated during
>    a packet egress.
>
> We could fix this as follows:
>
> 1. Not set CHECKSUM_UNNECESSARY in dev_loopback_xmit(), because it
>    is just not true.

I think this is the right approach. The receive path has to be able to
handle packets looped from the transmit path with CHECKSUM_PARTIAL
set.

> 2. I assume, the original idea behind setting CHECKSUM_UNNECESSARY in
>    dev_loopback_xmit() is to prevent checksum validation of looped-back
>    local multicast packets. We can adjust
>    __skb_checksum_validate_needed() to handle this as the special case.
>
> Signed-off-by: Cyril Strejc <cyril.strejc@...da.cz>
> ---
>  include/linux/skbuff.h | 4 +++-
>  net/core/dev.c         | 1 -
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 841e2f0f5240..95aa0014c3d6 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -4048,7 +4048,9 @@ static inline bool __skb_checksum_validate_needed(struct sk_buff *skb,
>                                                   bool zero_okay,
>                                                   __sum16 check)
>  {
> -       if (skb_csum_unnecessary(skb) || (zero_okay && !check)) {
> +       if (skb_csum_unnecessary(skb) ||
> +           (zero_okay && !check) ||
> +           skb->pkt_type == PACKET_LOOPBACK) {

This should not be needed, as skb_csum_unnecessary already handles
CHECKSUM_PARTIAL?

        return ((skb->ip_summed == CHECKSUM_UNNECESSARY) ||
                skb->csum_valid ||
                (skb->ip_summed == CHECKSUM_PARTIAL &&
                 skb_checksum_start_offset(skb) >= 0));

>                 skb->csum_valid = 1;
>                 __skb_decr_checksum_unnecessary(skb);
>                 return false;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 7ee9fecd3aff..ba4a0994d97b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3906,7 +3906,6 @@ int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
>         skb_reset_mac_header(skb);
>         __skb_pull(skb, skb_network_offset(skb));
>         skb->pkt_type = PACKET_LOOPBACK;
> -       skb->ip_summed = CHECKSUM_UNNECESSARY;
>         WARN_ON(!skb_dst(skb));
>         skb_dst_force(skb);
>         netif_rx_ni(skb);
> --
> 2.25.1
>