netdev - Re: [PATCH v2 net-next] ip6_tunnel: Adding support of mapping rules for MAP-E tunnel

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:   Tue, 04 Dec 2018 21:07:22 -0800 (PST)
From:   David Miller <davem@...emloft.net>
To:     felix.jia@...iedtelesis.co.nz
Cc:     blair.steven@...iedtelesis.co.nz, netdev@...r.kernel.org,
        sheena.mira-ato@...iedtelesis.co.nz, masakazu.asama@...il.com
Subject: Re: [PATCH v2 net-next] ip6_tunnel: Adding support of mapping
 rules for MAP-E tunnel

From: Felix Jia <felix.jia@...iedtelesis.co.nz>
Date: Mon,  3 Dec 2018 16:39:31 +1300

> +int
> +ip6_get_addrport(struct iphdr *iph, __be32 *saddr4, __be32 *daddr4,
> +		 __be16 *sport4, __be16 *dport4, __u8 *proto, int *icmperr)
> +{

This looks like something the flow dissector can do alreayd, please look into
utilizing that common piece of infrastructure instead of reimplementing it.

> +	u8 *ptr;
> +	struct iphdr *icmpiph = NULL;
> +	struct tcphdr *tcph, *icmptcph;
> +	struct udphdr *udph, *icmpudph;
> +	struct icmphdr *icmph, *icmpicmph;

Please always order local variables from longest to shortest line.

Please audit your entire submission for this problem.

> +static struct ip6_tnl_rule *ip6_tnl_rule_find(struct net_device *dev,
> +					      __be32 _dst)
> +{
> +	u32 dst = ntohl(_dst);
> +	struct ip6_rule_list *pos = NULL;
> +	struct ip6_tnl *t = netdev_priv(dev);
> +
> +	list_for_each_entry(pos, &t->rules.list, list) {
> +		int mask =
> +		    0xFFFFFFFF ^ ((1 << (32 - pos->data.ipv4_prefixlen)) - 1);
> +		if ((dst & mask) == ntohl(pos->data.ipv4_subnet.s_addr))
> +			return &pos->data;
> +	}
> +	return NULL;
> +}

How will this scale with large numbers of rules?

This rule facility seems to be designed in a way that sophisticated
(at least as fast as "O(log N)") lookup schemes aren't even possible,
and that even worse the ordering matters.