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

[Xin Long]

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);