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:   Sun, 24 Oct 2021 01:26:08 +0200
From:   Cyril Strejc <cyril.strejc@...da.cz>
To:     willemdebruijn.kernel@...il.com
Cc:     cyril.strejc@...da.cz, davem@...emloft.net, kuba@...nel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH] net: multicast: calculate csum of looped-back and forwarded packets

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;