netdev - Re: [PATCH net-next v2] af-packet: new flag to indicate all csums are good

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:   Tue, 2 Jun 2020 10:29:19 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Victor Julien <victor@...iniac.net>
Cc:     Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Eric Dumazet <edumazet@...gle.com>,
        Mao Wenan <maowenan@...wei.com>, Arnd Bergmann <arnd@...db.de>,
        Neil Horman <nhorman@...driver.com>, linux-doc@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Alexander Drozdov <al.drozdov@...il.com>,
        Tom Herbert <tom@...bertland.com>
Subject: Re: [PATCH net-next v2] af-packet: new flag to indicate all csums are good

On Tue, Jun 2, 2020 at 4:05 AM Victor Julien <victor@...iniac.net> wrote:
>
> Introduce a new flag (TP_STATUS_CSUM_UNNECESSARY) to indicate
> that the driver has completely validated the checksums in the packet.
>
> The TP_STATUS_CSUM_UNNECESSARY flag differs from TP_STATUS_CSUM_VALID
> in that the new flag will only be set if all the layers are valid,
> while TP_STATUS_CSUM_VALID is set as well if only the IP layer is valid.

transport, not ip checksum.

But as I understand it drivers set CHECKSUM_COMPLETE if they fill in
skb->csum over the full length of the packet. This does not
necessarily imply that any of the checksum fields in the packet are
valid yet (see also skb->csum_valid). Protocol code later compares
checksum fields against this using __skb_checksum_validate_complete and friends.

But packet sockets may be called before any of this, however. So I wonder
how valid the checksum really is right now when setting TP_STATUS_CSUM_VALID.
I assume it's correct, but don't fully understand where the validation
has taken place..

Similar to commit 682f048bd494 ("af_packet: pass checksum validation
status to the user"), please update tpacket_rcv and packet_rcv.

Note also that net-next is currently closed.




>  for convenience there are also the following defines::
>
> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
> index 3d884d68eb30..76a5c762e2e0 100644
> --- a/include/uapi/linux/if_packet.h
> +++ b/include/uapi/linux/if_packet.h
> @@ -113,6 +113,7 @@ struct tpacket_auxdata {
>  #define TP_STATUS_BLK_TMO              (1 << 5)
>  #define TP_STATUS_VLAN_TPID_VALID      (1 << 6) /* auxdata has valid tp_vlan_tpid */
>  #define TP_STATUS_CSUM_VALID           (1 << 7)
> +#define TP_STATUS_CSUM_UNNECESSARY     (1 << 8)
>
>  /* Tx ring - header status */
>  #define TP_STATUS_AVAILABLE          0
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 29bd405adbbd..94e213537646 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2215,10 +2215,13 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
>
>         if (skb->ip_summed == CHECKSUM_PARTIAL)
>                 status |= TP_STATUS_CSUMNOTREADY;
> -       else if (skb->pkt_type != PACKET_OUTGOING &&
> -                (skb->ip_summed == CHECKSUM_COMPLETE ||
> -                 skb_csum_unnecessary(skb)))
> -               status |= TP_STATUS_CSUM_VALID;
> +       else if (skb->pkt_type != PACKET_OUTGOING) {
> +               if (skb->ip_summed == CHECKSUM_UNNECESSARY)
> +                       status |= TP_STATUS_CSUM_UNNECESSARY | TP_STATUS_CSUM_VALID;
> +               else if (skb->ip_summed == CHECKSUM_COMPLETE ||
> +                        skb_csum_unnecessary(skb))
> +                       status |= TP_STATUS_CSUM_VALID;
> +       }
>
>         if (snaplen > res)
>                 snaplen = res;
> --
> 2.17.1
>