netdev - Re: [PATCH net-next v13 06/15] p4tc: add P4 data types

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: <ea2ad0773c91709094764474bf46825c146d741b.camel@redhat.com>
Date: Tue, 26 Mar 2024 15:47:52 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Jamal Hadi Salim <jhs@...atatu.com>, netdev@...r.kernel.org
Cc: deb.chatterjee@...el.com, anjali.singhai@...el.com, 
 namrata.limaye@...el.com, tom@...anda.io, mleitner@...hat.com, 
 Mahesh.Shirshyad@....com, Vipin.Jain@....com, tomasz.osinski@...el.com, 
 jiri@...nulli.us, xiyou.wangcong@...il.com, davem@...emloft.net, 
 edumazet@...gle.com, kuba@...nel.org, vladbu@...dia.com, horms@...nel.org, 
 khalidm@...dia.com, toke@...hat.com, daniel@...earbox.net,
 victor@...atatu.com,  pctammela@...atatu.com, bpf@...r.kernel.org
Subject: Re: [PATCH net-next v13  06/15] p4tc: add P4 data types

On Mon, 2024-03-25 at 10:28 -0400, Jamal Hadi Salim wrote:
> +static int p4t_s32_validate(struct p4tc_type *container, void *value,
> +			    u16 bitstart, u16 bitend,
> +			    struct netlink_ext_ack *extack)
> +{
> +	s32 minsz = S32_MIN, maxsz = S32_MAX;
> +	s32 *val = value;
> +
> +	if (val && (*val > maxsz || *val < minsz)) {
> +		NL_SET_ERR_MSG_MOD(extack, "S32 value out of range");

I'm sorry for the additional questions/points below which could/should
have been flagged earlier.

Out of sheer ignorance IDK if a single P4 command could use multiple
types, possibly use NL_SET_ERR_MSG_FMT_MOD() and specify the bogus
value/range.

The same point/question has a few other instances below.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[...]

> +static int p4t_be32_validate(struct p4tc_type *container, void *value,
> +			     u16 bitstart, u16 bitend,
> +			     struct netlink_ext_ack *extack)
> +{
> +	size_t container_maxsz = U32_MAX;
> +	__be32 *val_u32 = value;
> +	__u32 val = 0;
> +	size_t maxval;
> +	int ret;
> +
> +	ret = p4t_validate_bitpos(bitstart, bitend, 31, 31, extack);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (value)
> +		val = be32_to_cpu(*val_u32);
> +
> +	maxval = GENMASK(bitend, 0);
> +	if (val && (val > container_maxsz || val > maxval)) {

The first condition 'val > container_maxsz' is a bit confusing
(unneeded), as 'val' type is u32 and 'container_maxsz' is U32_MAX


> +		NL_SET_ERR_MSG_MOD(extack, "BE32 value out of range");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[...]

> +static int p4t_u16_validate(struct p4tc_type *container, void *value,
> +			    u16 bitstart, u16 bitend,
> +			    struct netlink_ext_ack *extack)
> +{
> +	u16 container_maxsz = U16_MAX;
> +	u16 *val = value;
> +	u16 maxval;
> +	int ret;
> +
> +	ret = p4t_validate_bitpos(bitstart, bitend, 15, 15, extack);
> +	if (ret < 0)
> +		return ret;
> +
> +	maxval = GENMASK(bitend, 0);
> +	if (val && (*val > container_maxsz || *val > maxval)) {

Mutatis mutandis, same thing here

> +		NL_SET_ERR_MSG_MOD(extack, "U16 value out of range");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct p4tc_type_mask_shift *
> +p4t_u16_bitops(u16 bitsiz, u16 bitstart, u16 bitend,
> +	       struct netlink_ext_ack *extack)
> +{
> +	struct p4tc_type_mask_shift *mask_shift;
> +	u16 mask = GENMASK(bitend, bitstart);
> +	u16 *cmask;
> +
> +	mask_shift = kzalloc(sizeof(*mask_shift), GFP_KERNEL);

(Not specifically related to _this_ allocation) I'm wondering if the
allocations in this file should GFP_KERNEL_ACCOUNT? If I read correctly
the user-space can (and is expected to) create an quite large number of
instances of this structs (???)

[...]
> +void __p4tc_type_host_write(const struct p4tc_type_ops *ops,
> +			    struct p4tc_type *container,
> +			    struct p4tc_type_mask_shift *mask_shift, void *sval,
> +			    void *dval)
> +{
> +	#define HWRITE(cops) \
> +	do { \
> +		if (ops == &(cops)) \
> +			return (cops).host_write(container, mask_shift, sval, \
> +						 dval); \
> +	} while (0)
> +
> +	HWRITE(u8_ops);
> +	HWRITE(u16_ops);
> +	HWRITE(u32_ops);
> +	HWRITE(u64_ops);
> +	HWRITE(u128_ops);
> +	HWRITE(s16_ops);
> +	HWRITE(s32_ops);
> +	HWRITE(be16_ops);
> +	HWRITE(be32_ops);
> +	HWRITE(mac_ops);
> +	HWRITE(ipv4_ops);
> +	HWRITE(bool_ops);
> +	HWRITE(dev_ops);
> +	HWRITE(key_ops);

possibly

#undef HWRITE

?

Otherwise LGTM!


Cheers,

Paolo