netdev - Re: [RFC iproute2] tc/u32: use standard flexible array

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] [day] [month] [year] [list]

Date: Fri, 9 Feb 2024 19:09:24 +0100
From: Phil Sutter <phil@....cc>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: netdev@...r.kernel.org
Subject: Re: [RFC iproute2] tc/u32: use standard flexible array

On Fri, Feb 09, 2024 at 09:06:17AM -0800, Stephen Hemminger wrote:
> The code to parse selectors was depending on C extension to implement
> the array of keys. This would cause warnings if built with clang.
> Instead use ISO C99 flexible array.
> 
> Also the maximum number of keys was hardcoded already in two places.
> 
> Signed-off-by: Stephen Hemminger <stephen@...workplumber.org>
> ---
>  tc/f_u32.c | 46 +++++++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/tc/f_u32.c b/tc/f_u32.c
> index 913ec1de435d..a6e699d53d24 100644
> --- a/tc/f_u32.c
> +++ b/tc/f_u32.c
> @@ -21,6 +21,8 @@
>  #include "utils.h"
>  #include "tc_util.h"
>  
> +#define SEL_MAX_KEYS	128
> +
>  static void explain(void)
>  {
>  	fprintf(stderr,
> @@ -129,7 +131,7 @@ static int pack_key(struct tc_u32_sel *sel, __u32 key, __u32 mask,
>  		}
>  	}
>  
> -	if (hwm >= 128)
> +	if (hwm >= SEL_MAX_KEYS)
>  		return -1;
>  	if (off % 4)
>  		return -1;
> @@ -1017,10 +1019,7 @@ static __u32 u32_hash_fold(struct tc_u32_key *key)
>  static int u32_parse_opt(struct filter_util *qu, char *handle,
>  			 int argc, char **argv, struct nlmsghdr *n)
>  {
> -	struct {
> -		struct tc_u32_sel sel;
> -		struct tc_u32_key keys[128];
> -	} sel = {};
> +	struct tc_u32_sel *sel;
>  	struct tcmsg *t = NLMSG_DATA(n);
>  	struct rtattr *tail;
>  	int sel_ok = 0, terminal_ok = 0;
> @@ -1037,12 +1036,18 @@ static int u32_parse_opt(struct filter_util *qu, char *handle,
>  	if (argc == 0)
>  		return 0;
>  
> +	sel = alloca(sizeof(*sel) + SEL_MAX_KEYS * sizeof(struct tc_u32_key));
> +	if (sel == NULL)
> +		return -1;
> +
> +	memset(sel, 0, sizeof(*sel) + SEL_MAX_KEYS * sizeof(struct tc_u32_key));

Maybe a wrapper would clean things up a bit (untested):

| static void *calloca(size_t nmemb, size_t size)
| {
| 	void *ret = alloca(nmemb * size);
| 
| 	if (ret)
| 		memset(ret, 0, nmemb * size);
| 
| 	return ret;
| }

[...]
> @@ -1122,17 +1127,21 @@ static int u32_parse_opt(struct filter_util *qu, char *handle,
>  		} else if (strcmp(*argv, "sample") == 0) {
>  			__u32 hash;
>  			unsigned int divisor = 0x100;
> -			struct {
> -				struct tc_u32_sel sel;
> -				struct tc_u32_key keys[4];
> -			} sel2 = {};
> +			struct tc_u32_sel *sel2;

Not directly related to your patch, but seeing this makes me wonder
where the code asserts parse_selector won't exceed the key space.
pack_key() itself only checks it won't exceed 128 keys.

>  
>  			NEXT_ARG();
> -			if (parse_selector(&argc, &argv, &sel2.sel, n)) {
> +
> +			sel2 = alloca(sizeof(*sel) + 4 * sizeof(struct tc_u32_key));
> +			if (sel2 == NULL)
> +				return -1;
> +
> +			memset(sel2, 0, sizeof(*sel2));

The sizeof(*sel2) is identical to sizeof(struct tc_u32_sel) and thus too
small. Another point for the wrapper suggested above, as it avoids these
typical mistakes.

Cheers, Phil