Re: [PATCH nf] netfilter: conntrack: fix false CRC32c mismatch using paged skb

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Davide,

On Thu, May 18, 2017 at 06:01:43PM +0200, Davide Caratti wrote:
> sctp_compute_cksum() implementation assumes that at least the SCTP header
> is in the linear part of skb: modify conntrack error callback to avoid
> false CRC32c mismatch, if the transport header is partially/entirely paged.

I guess you considered this, but I would like to know the reason for
this approach. Why not fix this from sctp_compute_cksum()? I mean, I
can see other spots in the kernel tree that may be affected by this?
Or is it that you're only observing this from a path that is specific
of conntrack?

> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
>  net/netfilter/nf_conntrack_proto_sctp.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
> index 13875d5..1c5b14a 100644
> --- a/net/netfilter/nf_conntrack_proto_sctp.c
> +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> @@ -512,16 +512,19 @@ static int sctp_error(struct net *net, struct nf_conn *tpl, struct sk_buff *skb,
>  		      u8 pf, unsigned int hooknum)
>  {
>  	const struct sctphdr *sh;
> -	struct sctphdr _sctph;
>  	const char *logmsg;
>  
> -	sh = skb_header_pointer(skb, dataoff, sizeof(_sctph), &_sctph);
> -	if (!sh) {
> +	if (skb->len < dataoff + sizeof(struct sctphdr)) {
>  		logmsg = "nf_ct_sctp: short packet ";
>  		goto out_invalid;
>  	}
>  	if (net->ct.sysctl_checksum && hooknum == NF_INET_PRE_ROUTING &&
>  	    skb->ip_summed == CHECKSUM_NONE) {
> +		if (!skb_make_writable(skb, dataoff + sizeof(struct sctphdr))) {
> +			logmsg = "nf_ct_sctp: failed to read header ";
> +			goto out_invalid;
> +		}
> +		sh = (const struct sctphdr *)(skb->data + dataoff);
>  		if (sh->checksum != sctp_compute_cksum(skb, dataoff)) {
>  			logmsg = "nf_ct_sctp: bad CRC ";
>  			goto out_invalid;
> -- 
> 2.7.4
>