Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Patrick McHardy <kaber@trash.net>]

Harald Welte wrote:
> Sorry, forget that other patch.
> 
> The only way how we can thoroughly solve (and avoid) this problem, also
> for future cases, is to behave like 'real' TLV parsers (e.g. ASN1).
> 
> That is, for any kind of numeric values, we don't make an assumption
> that the attribute has a certain fixed size.  Instead, we derive the
> (u8/u16/u32/u64) size from the length of the attribute, i.e.
> NFA_PAYLOAD() is 1/2/4/8 bytes long.
> 
> This is a quick and dirty patch to demonstrate what I mean:
> 
> =====
> 
> diff --git a/net/ipv4/netfilter/ip_conntrack_netlink.c b/net/ipv4/netfilter/ip_conntrack_netlink.c
> --- a/net/ipv4/netfilter/ip_conntrack_netlink.c
> +++ b/net/ipv4/netfilter/ip_conntrack_netlink.c
> @@ -502,12 +502,13 @@ ctnetlink_parse_tuple_ip(struct nfattr *
>  }
>  
>  static const size_t cta_min_proto[CTA_PROTO_MAX] = {
> -	[CTA_PROTO_NUM-1]	= sizeof(u_int16_t),
> +	[CTA_PROTO_NUM-1]	= sizeof(u_int8_t),
>  	[CTA_PROTO_SRC_PORT-1]	= sizeof(u_int16_t),
>  	[CTA_PROTO_DST_PORT-1]	= sizeof(u_int16_t),
>  	[CTA_PROTO_ICMP_TYPE-1]	= sizeof(u_int8_t),
>  	[CTA_PROTO_ICMP_CODE-1]	= sizeof(u_int8_t),
>  	[CTA_PROTO_ICMP_ID-1]	= sizeof(u_int16_t),
> +	[CTA_PROTO-1]		= sizeof(u_int8_t),
>  };
>  
>  static inline int
> @@ -527,7 +528,18 @@ ctnetlink_parse_tuple_proto(struct nfatt
>  
>  	if (!tb[CTA_PROTO_NUM-1])
>  		return -EINVAL;
> -	tuple->dst.protonum = *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
> +
> +	switch (NFA_PAYLOAD(tb[CTA_PROTO_NUM-1]))
> +	case sizeof(u_int8_t):
> +		tuple->dst.protonum = 
> +				*(u_int8_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
> +		break;
> +	case sizeof(u_int16_t):
> +		tuple->dst.protonum = 
> +				*(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	proto = ip_conntrack_proto_find_get(tuple->dst.protonum);
> 
> =====
> 
> Obviously, this needs to be moved into a nfnetlink core funciton,
> something like a function nfattr_parse_number() that would then be
> called from all places that parse a number.
> 
> Userspace parsers (libnetfilter_conntrack) would have to introduce the
> same semantics.

That won't work for this case since usually u_int16_t's are encoded
in network byte order but in this case its always host byte order.

> It might be a bit too much overhead, I'm not really decided yet.  But in
> the end, if everybody plays according to that rule, we don't have any
> such issues in the future.
> 
> Comments?

I think just the first patch is fine. I really hope we don't find
more of these.