netdev - Re: [PATCH v5 net-next 4/7] ip6tlvs: Registration of TLV handlers and parameters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191006131830.nfiwl2ovexnxbdnh@netronome.com>
Date:   Sun, 6 Oct 2019 15:18:31 +0200
From:   Simon Horman <simon.horman@...ronome.com>
To:     Tom Herbert <tom@...bertland.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        Tom Herbert <tom@...ntonium.net>
Subject: Re: [PATCH v5 net-next 4/7] ip6tlvs: Registration of TLV handlers
 and parameters

On Thu, Oct 03, 2019 at 02:58:01PM -0700, Tom Herbert wrote:
> From: Tom Herbert <tom@...ntonium.net>
> 
> Create a single TLV parameter table that holds meta information for IPv6
> Hop-by-Hop and Destination TLVs. The data structure is composed of a 256
> element array of u8's (one entry for each TLV type to allow O(1)
> lookup). Each entry provides an offset into an array of TLV proc data
> structures which follows the array of u8s. The TLV proc data structure
> contains parameters and handler functions for receiving and transmitting
> TLVs. The zeroth element in the TLV proc array provides default
> parameters for TLVs.
> 
> A class attribute indicates the type of extension header in which the
> TLV may be used (e.g. Hop-by-Hop options, Destination options, or
> Destination options before the routing header).
> 
> Functions are defined to manipulate entries in the TLV parameter table.
> 
> * tlv_{set|unset}_proc set a TLV proc entry (ops and parameters)
> * tlv_{set|unset}_params set parameters only

Perhaps it would be worth mentioning that these will
be used by a following patch in the series.

> Receive TLV lookup and processing is modified to be a lookup in the TLV
> parameter table. An init table containing parameters for TLVs supported
> by the kernel is used to initialize the TLV table.
> 
> Signed-off-by: Tom Herbert <tom@...bertland.com>

Minor nit about types below, but overall this looks good to me.

Reviewed-by: Simon Horman <simon.horman@...ronome.com>

...

> diff --git a/include/net/ipeh.h b/include/net/ipeh.h
> index c1aa7b6..aaa2910 100644
> --- a/include/net/ipeh.h
> +++ b/include/net/ipeh.h

...

> +/* ipeh_tlv_get_proc_by_type assumes rcu_read_lock is held */
> +static inline struct tlv_proc *ipeh_tlv_get_proc_by_type(
> +		struct tlv_param_table *tlv_param_table, unsigned char type)
> +{
> +	struct tlv_param_table_data *tpt =
> +				rcu_dereference(tlv_param_table->data);
> +
> +	return &tpt->types[tpt->entries[type]].proc;
> +}
> +
> +/* ipeh_tlv_get_proc assumes rcu_read_lock is held */
> +static inline struct tlv_proc *ipeh_tlv_get_proc(
> +		struct tlv_param_table *tlv_param_table,
> +		const __u8 *tlv)
> +{
> +	return ipeh_tlv_get_proc_by_type(tlv_param_table, tlv[0]);

The type of tlv is const __u8 *, however,
1. the type of the 'type' parameter of ipeh_tlv_get_proc_by_type is
unsigned char; and
2. this function is passed a value of type const unsigned char * by
ipeh_parse_tlv().

I understand these are all one byte unsigned integer types.
But perhaps this could be made more consistent somehow.

> +}

...

> diff --git a/net/ipv6/exthdrs_common.c b/net/ipv6/exthdrs_common.c
> index 99a0911..43737fc 100644
> --- a/net/ipv6/exthdrs_common.c
> +++ b/net/ipv6/exthdrs_common.c

..

> @@ -221,26 +226,22 @@ bool ipeh_parse_tlv(const struct tlvtype_proc *procs, struct sk_buff *skb,
>  
>  			tlv_count++;
>  			if (tlv_count > max_count &&
> -			    parse_error(skb, off, IPEH_PARSE_ERR_OPT_TOOMANY))
> +			    !parse_error(skb, off, IPEH_PARSE_ERR_OPT_TOOMANY))
>  				goto bad;
>  
> -			for (curr = procs; curr->type >= 0; curr++) {
> -				if (curr->type == nh[off]) {
> -					/* type specific length/alignment
> -					 * checks will be performed in the
> -					 * func().
> -					 */
> -					if (curr->func(skb, off) == false)
> -						return false;
> -					break;
> -				}
> -			}
> -			if (curr->type < 0 &&
> -			    !parse_error(skb, off,
> +			curr = ipeh_tlv_get_proc(tlv_param_table, &nh[off]);
> +			if ((curr->params.r.class & class) && curr->ops.func) {
> +				/* Handler will apply additional checks to
> +				 * the TLV
> +				 */
> +				if (!curr->ops.func(class, skb, off))
> +					return false;
> +			} else if (!parse_error(skb, off,
>  					 disallow_unknowns ?
>  						IPEH_PARSE_ERR_OPT_UNK_DISALW :
> -						IPEH_PARSE_ERR_OPT_UNK))
> +						IPEH_PARSE_ERR_OPT_UNK)) {
>  				goto bad;
> +			}
>  
>  			padlen = 0;
>  			break;