netdev - Re: [PATCH v5 net-next 6/7] ip6tlvs: Add netlink interface

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]

Date:   Mon, 7 Oct 2019 14:39:45 +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 6/7] ip6tlvs: Add netlink interface

On Thu, Oct 03, 2019 at 02:58:03PM -0700, Tom Herbert wrote:
> From: Tom Herbert <tom@...ntonium.net>
> 
> Add a netlink interface to manage the TX TLV parameters. Managed
> parameters include those for validating and sending TLVs being sent
> such as alignment, TLV ordering, length limits, etc.
> 
> Signed-off-by: Tom Herbert <tom@...bertland.com>

Hi Tom,

I am wondering if you considered including extack support in this code.

...

> diff --git a/include/uapi/linux/ipeh.h b/include/uapi/linux/ipeh.h
> index dbf0728..bac36a7 100644
> --- a/include/uapi/linux/ipeh.h
> +++ b/include/uapi/linux/ipeh.h
> @@ -21,4 +21,33 @@ enum {
>  	IPEH_TLV_PERM_MAX = IPEH_TLV_PERM_NO_CHECK
>  };
>  
> +/* NETLINK_GENERIC related info for IP TLVs */
> +
> +enum {
> +	IPEH_TLV_ATTR_UNSPEC,
> +	IPEH_TLV_ATTR_TYPE,			/* u8, > 1 */
> +	IPEH_TLV_ATTR_ORDER,			/* u16 */
> +	IPEH_TLV_ATTR_ADMIN_PERM,		/* u8, perm value */
> +	IPEH_TLV_ATTR_USER_PERM,		/* u8, perm value */

My reading of struct tlv_tx_params is that admin_perm and user_perm are
2-bit entities whose valid values are currently 0, 1 and 2. Perhaps that
would be worth noting here in keeping with restrictions noted for other
attributes.

> +	IPEH_TLV_ATTR_CLASS,			/* u8, 3 bit flags */
> +	IPEH_TLV_ATTR_ALIGN_MULT,		/* u8, 1 to 16 */
> +	IPEH_TLV_ATTR_ALIGN_OFF,		/* u8, 0 to 15 */
> +	IPEH_TLV_ATTR_MIN_DATA_LEN,		/* u8 (option data length) */
> +	IPEH_TLV_ATTR_MAX_DATA_LEN,		/* u8 (option data length) */
> +	IPEH_TLV_ATTR_DATA_LEN_MULT,		/* u8, 1 to 16 */
> +	IPEH_TLV_ATTR_DATA_LEN_OFF,		/* u8, 0 to 15 */
> +
> +	__IPEH_TLV_ATTR_MAX,
> +};
> +
> +#define IPEH_TLV_ATTR_MAX              (__IPEH_TLV_ATTR_MAX - 1)

...

> diff --git a/net/ipv6/exthdrs_common.c b/net/ipv6/exthdrs_common.c
> index feaa4a6..e3c1f33 100644
> --- a/net/ipv6/exthdrs_common.c
> +++ b/net/ipv6/exthdrs_common.c
> @@ -454,6 +454,244 @@ int __ipeh_tlv_unset(struct tlv_param_table *tlv_param_table,

...

> +int ipeh_tlv_nl_cmd_set(struct tlv_param_table *tlv_param_table,
> +			struct genl_family *tlv_nl_family,
> +			struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct tlv_params new_params;
> +	struct tlv_proc *tproc;
> +	unsigned char type;
> +	unsigned int v;
> +	int retv = -EINVAL;
> +
> +	if (!info->attrs[IPEH_TLV_ATTR_TYPE])
> +		return -EINVAL;
> +
> +	type = nla_get_u8(info->attrs[IPEH_TLV_ATTR_TYPE]);
> +	if (type < 2)
> +		return -EINVAL;
> +
> +	rcu_read_lock();
> +
> +	/* Base new parameters on existing ones */
> +	tproc = ipeh_tlv_get_proc_by_type(tlv_param_table, type);
> +	new_params = tproc->params;
> +
> +	if (info->attrs[IPEH_TLV_ATTR_ORDER]) {
> +		v = nla_get_u16(info->attrs[IPEH_TLV_ATTR_ORDER]);
> +		new_params.t.preferred_order = v;
> +	}
> +
> +	if (info->attrs[IPEH_TLV_ATTR_ADMIN_PERM]) {
> +		v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_ADMIN_PERM]);
> +		if (v > IPEH_TLV_PERM_MAX)
> +			goto out;
> +		new_params.t.admin_perm = v;
> +	}
> +
> +	if (info->attrs[IPEH_TLV_ATTR_USER_PERM]) {
> +		v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_USER_PERM]);
> +		if (v > IPEH_TLV_PERM_MAX)
> +			goto out;
> +		new_params.t.user_perm = v;
> +	}
> +
> +	if (info->attrs[IPEH_TLV_ATTR_CLASS]) {
> +		v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_CLASS]);
> +		if (!v || (v & ~IPEH_TLV_CLASS_FLAG_MASK))
> +			goto out;
> +		new_params.t.class = v;
> +	}
> +
> +	if (info->attrs[IPEH_TLV_ATTR_ALIGN_MULT]) {
> +		v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_ALIGN_MULT]);
> +		if (v > 16 || v < 1)
> +			goto out;
> +		new_params.t.align_mult = v - 1;
> +	}
> +
> +	if (info->attrs[IPEH_TLV_ATTR_ALIGN_OFF]) {
> +		v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_ALIGN_OFF]);
> +		if (v > 15)
> +			goto out;
> +		new_params.t.align_off = v;
> +	}
> +
> +	if (info->attrs[IPEH_TLV_ATTR_MAX_DATA_LEN])
> +		new_params.t.max_data_len =
> +		    nla_get_u8(info->attrs[IPEH_TLV_ATTR_MAX_DATA_LEN]);
> +
> +	if (info->attrs[IPEH_TLV_ATTR_MIN_DATA_LEN])
> +		new_params.t.min_data_len =
> +		    nla_get_u8(info->attrs[IPEH_TLV_ATTR_MIN_DATA_LEN]);
> +
> +	if (info->attrs[IPEH_TLV_ATTR_DATA_LEN_MULT]) {
> +		v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_DATA_LEN_MULT]);
> +		if (v > 16 || v < 1)
> +			goto out;
> +		new_params.t.data_len_mult = v - 1;
> +	}

Is some sanity checking warranted for the min/max data len values.
f.e. that min <= max ?

> +
> +	if (info->attrs[IPEH_TLV_ATTR_DATA_LEN_OFF]) {
> +		v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_DATA_LEN_OFF]);
> +		if (v > 15)
> +			goto out;
> +		new_params.t.data_len_off = v;
> +	}
> +
> +	retv = ipeh_tlv_set_params(tlv_param_table, type, &new_params);
> +
> +out:
> +	rcu_read_unlock();
> +	return retv;
> +}
> +EXPORT_SYMBOL(ipeh_tlv_nl_cmd_set);

...