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]

Message-ID: <CAF=yD-K_-i1wCaRg4VqocMqL9m7OrcCy3AXVn4d8k7yXg6yz5g@mail.gmail.com>
Date:   Sat, 23 Oct 2021 22:41:06 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Cyril Strejc <cyril.strejc@...da.cz>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: multicast: calculate csum of looped-back and
 forwarded packets

On Sat, Oct 23, 2021 at 7:26 PM Cyril Strejc <cyril.strejc@...da.cz> wrote:
>
> On 10/22/21 9:08 PM, Willem de Bruijn wrote:
> >> 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.
>
> As You clarified, the receive path handles CHECKSUM_PARTIAL.
>
> There is a problem with CHECKSUM_NONE -- the case when TX checksum
> offload is not supported by a NIC. Kernel does not set
> CHECKSUM_UNNECESSARY with a correct value of csum_level when a packet
> is being prepared for transmission, but just set the CHECKSUM_NONE.
>
> >> 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?
> >
>
> Still we need some solution for the CHECKSUM_NONE case which triggers
> checksum validation.
>
> >> 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
> >>
> >
>
> Alternatively, we could solve the CHECKSUM_NONE case by a simple,
> practical and historical compatible "TX->RX translation" of ip_summed
> in dev_loopback_xmit(), which keeps CHECKSUM_PARTIAL and leaves
> __skb_checksum_validate_needed() as is:
>
>         if (skb->ip_summed == CHECKSUM_NONE)
>                 skb->ip_summed = CHECKSUM_UNNECESSARY;
>
> or:
>         if (skb->ip_summed != CHECKSUM_PARTIAL)
>                 skb->ip_summed = CHECKSUM_UNNECESSARY;

Based on the idea that these packets are fully checksummed, so even if
they loop to the tx path again with ip_summed CHECKSUM_UNNECESSARY,
they will not cause the bug that you originally reported?

Yes, that looks like a nice solution.

I wonder what the behavior is for unicast packets. As it makes sense
for the two to work the same. For instance, packets traveling over
veth_xmit to a local socket. Their ip_summed is not adjusted as far as
I know. If created with CHECKSUM_NONE, these will then incur an
unnecessary checksum validation, too. Such packets are built, e.g.,
for udp sockets with corking. They could benefit from a similar
solution. Not suggesting for this patch, to be clear.