netdev - RE: [RFC PATCH net-next 2/5] net: split skb_checksum

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:   Thu, 2 Feb 2017 16:55:10 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Davide Caratti' <dcaratti@...hat.com>,
        'Tom Herbert' <tom@...bertland.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: RE: [RFC PATCH net-next 2/5] net: split skb_checksum_help

From: Davide Caratti
> Sent: 02 February 2017 15:07
> > From: Tom Herbert
> > >
> > > Sent: 23 January 2017 21:00
> > ..
> > >
> > > skb_checksum_help is specific to the Internet checksum. For instance,
> > > CHECKSUM_COMPLETE can _only_ refer to Internet checksum calculation
> > > nothing else will work. Checksums and CRCs are very different things
> > > with very different processing. They are not interchangeable, have
> > > very different properties, and hence it is a mistake to try to shoe
> > > horn things so that they use a common infrastructure.
> > >
> 
> true, we don't need to test CHECKSUM_COMPLETE on skbs carrying SCTP.
> So maybe we can simply replace patches 2/5 and 3/5 with the smaller one at
> the bottom of this message.

I have to admit to not knowing exactly what the CHECKSUM_xxx flags actually mean.
I have a good idea about what the intention is though.

...
> On Tue, 2017-01-24 at 16:35 +0000, David Laight wrote:
> >
> > I can imagine horrid things happening if someone tries to encapsulate
> > SCTP/IP in UDP (or worse UDP/IP in SCTP).
> >
> > For UDP in UDP I suspect that CHECKSUM_COMPLETE on an inner UDP packet
> > allows the outer checksum be calculated by ignoring the inner packet
> > (since it sums to zero).
> > This just isn't true if SCTP is involved.
> > There are tricks to generate a crc of a longer packet, but they'd only
> > work for SCTP in SCTP.
> >
> > For non-encapsulated packets it is a different matter.
> 
> If we limit the scope to skbs having ip_summed equal to CHECKSUM_PARTIAL,
> like it's done in patch 4, we only need checksumming the packet starting
> from csum_start to its end, and copy the computed value to csum_offset.
> The difficult thing is discriminating skbs that need CRC32c, namely SCTP,
> from the rest of the traffic (that will likely be checksummed by
> skb_checksum_help).
...

I'm guessing that the SCTP code only sets CHECKSUM_PARTIAL (and doesn't
perform the checksum) if it somehow knows that the target interface
supports CRC32c checksums.

I'd put the onus on any such interface to perform the checksum (and
set CHECKSUM_COMPLETE (or is it UNNECESSARY?) before passing the 
message onto an interface that doesn't advertise CRC32 support.

You certainly don't want to have to go through all the ethernet drivers!

> 
> ------------------- 8< --------------------------
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -200,7 +200,8 @@
>  *accordingly. Note the there is no indication in the skbuff that the
>  *CHECKSUM_PARTIAL refers to an FCOE checksum, a driver that supports
>  *both IP checksum offload and FCOE CRC offload must verify which offload
> - *is configured for a packet presumably by inspecting packet headers.
> + *is configured for a packet presumably by inspecting packet headers; in
> + *case, skb_sctp_csum_help is provided to compute CRC on SCTP packets.
>  *
>  * E. Checksumming on output with GSO.
>  *
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ad5959e..fa9be6d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2580,6 +2580,42 @@ int skb_checksum_help(struct sk_buff *skb)
> }
> EXPORT_SYMBOL(skb_checksum_help);
> 
> +int skb_sctp_csum_help(struct sk_buff *skb)
> +{
> +	__le32 crc32c_csum;
> +	int ret = 0, offset;
> +
> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> +		goto out;
> +	if (unlikely(skb_is_gso(skb)))
> +		goto out;
> +	if (skb_has_shared_frag(skb)) {
> +		ret = __skb_linearize(skb);

I don't think you really want to linearize the packet.
...

	David