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