Re: [PATCH net] sctp: get sctphdr by offset in sctp_compute_cksum

[Neil Horman]

On Wed, Feb 27, 2019 at 08:53:26PM +0800, Xin Long wrote:
> On Tue, Feb 26, 2019 at 8:29 PM Neil Horman <nhorman@xxxxxxxxxxxxx> wrote:
> >
> > On Tue, Feb 26, 2019 at 12:15:54AM +0800, Xin Long wrote:
> > > On Mon, Feb 25, 2019 at 10:08 PM Neil Horman <nhorman@xxxxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Feb 25, 2019 at 09:20:44PM +0800, Xin Long wrote:
> > > > > On Mon, Feb 25, 2019 at 8:47 PM Neil Horman <nhorman@xxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Mon, Feb 25, 2019 at 07:25:37PM +0800, Xin Long wrote:
> > > > > > > sctp_hdr(skb) only works when skb->transport_header is set properly.
> > > > > > >
> > > > > > > But in the path of nf_conntrack_in: sctp_packet() -> sctp_error()
> > > > > > >
> > > > > > > skb->transport_header is not guaranteed to be right value for sctp.
> > > > > > > It will cause to fail to check the checksum for sctp packets.
> > > > > > >
> > > > > > > So fix it by using offset, which is always right in all places.
> > > > > > >
> > > > > > > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > > > > > > Reported-by: Li Shuang <shuali@xxxxxxxxxx>
> > > > > > > Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
> > > > > > > ---
> > > > > > >  include/net/sctp/checksum.h | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
> > > > > > > index 32ee65a..1c6e6c0 100644
> > > > > > > --- a/include/net/sctp/checksum.h
> > > > > > > +++ b/include/net/sctp/checksum.h
> > > > > > > @@ -61,7 +61,7 @@ static inline __wsum sctp_csum_combine(__wsum csum, __wsum csum2,
> > > > > > >  static inline __le32 sctp_compute_cksum(const struct sk_buff *skb,
> > > > > > >                                       unsigned int offset)
> > > > > > >  {
> > > > > > > -     struct sctphdr *sh = sctp_hdr(skb);
> > > > > > > +     struct sctphdr *sh = (struct sctphdr *)(skb->data + offset);
> > > > > > >       const struct skb_checksum_ops ops = {
> > > > > > >               .update  = sctp_csum_update,
> > > > > > >               .combine = sctp_csum_combine,
> > > > > > > --
> > > > > > > 2.1.0
> > > > > > >
> > > > > > >
> > > > > > Shouldn't you use skb_set_transport_header and skb_transport_header here?
> > > > > you mean:
> > > > > skb_set_transport_header(skb, offset);
> > > > > sh = sctp_hdr(skb);
> > > > > ?
> > > > >
> > > > > There's no place counting on here to set transport_header.
> > > > > It will be a kinda redundant job, yet skb is 'const'.
> > > > >
> > > > I'm not sure what you mean by "theres no place counting here".  We have the
> > > > transport header offset, and you're doing the exact same computation that that
> > > > function does.  It seems like we should use it in case the underlying
> > > > implementation changes.
> > > 1. skb_set_transport_header() and sctp_hdr() are like:
> > > skb->transport_header = skb->data - skb->head;
> > > skb->transport_header += offset
> > > sh = skb->head + skb->transport_header;
> > >
> > > 2. in this patch:
> > > sh = (struct sctphdr *)(skb->data + offset);  only
> > >
> > > I think the 2nd one is better.
> > >
> > > I feel it's weird to set transport_header here if it's only for
> > > sctp_hdr(skb) in here.
> > >
> > > As for "underlying implementation changes", I don't know exactly the case
> > > but there are quite a few places doing things like:
> > > *hdr = (struct *hdr *)(skb->data + hdroff);
> > >
> > > I'd think it's safe. no?
> > >
> > Safe, yes, it just doesn't seem right.  I know you've pointed out several places
> > below that rapidly compute transport offsets in a one-off fashion, but at this
> > same time, the other primary transports (tcp and udp), all seems to use the
> > transport header to do their work (linearizing as necessecary, which sctp also
> > does in sctp_rcv, at least in most cases).
> > > >
> > > > I understand what you are saying regarding the use of a const variable there,
> > > > but perhaps thats an argument for removing the const storage classifier.  Better
> > > > still, it would be good to figure out why all paths to this function don't
> > > > already set the transport header offset to begin with (addressing your redundant
> > > > comment)
> > > The issue was reported when going to nf_conntrack by br_netfilter's
> > > bridge-nf-call-iptables.
> > > As you can see on nf_conntrack_in() path, even iphdr is got by:
> > >    iph = skb_header_pointer(skb, nhoff, sizeof(_iph), &_iph);
> > > It's impossible to set skb->transport_header when we're not sure iphdr
> > > in linearized memory.
> > >
> > But if the skb isn't linearized, computing the transport header manually isn't
> > going to help you anyway.  You can see that in skb_header_pointer.  If the
> > offset they are trying to get to is outside the bounds of the length of the skb
> > (i.e. the fragmented case), it calls skb_copy_bits to linearize the needed
> > segment.  It seems we should be doing something simmilar.  In most cases we are
> > already linearized from sctp_rcv (possibly all, I need to think about that). All
> > I'm really saying is that by using the skb apis we insulate ourselves from
> > potential changes in how skbs might work in the future.  I'm not strictly bound
> > to setting the transport header, but we should definately be getting the
> > transport header via the skb utility functions wherever possible.
> Okay, I will change to fix it with the below patch if you agree.
> I've confirmed this won't affect netfilter.
> 
> diff --git a/net/netfilter/nf_conntrack_proto_sctp.c
> b/net/netfilter/nf_conntrack_proto_sctp.c
> index d53e3e7..6b53cd2 100644
> --- a/net/netfilter/nf_conntrack_proto_sctp.c
> +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> @@ -343,7 +343,9 @@ static bool sctp_error(struct sk_buff *skb,
>                         logmsg = "nf_ct_sctp: failed to read header ";
>                         goto out_invalid;
>                 }
> -               sh = (const struct sctphdr *)(skb->data + dataoff);
> +               /* sctp_compute_cksum() depends on correct transport header */
> +               skb_set_transport_header(skb, dataoff);
> +               sh = sctp_hdr(skb);
> 
Thank you, that looks much better to me.

Acked-by: Neil Horman <nhorman@xxxxxxxxxxxxx>