netdev - Re: [PATCH] xfrm: Fix wraparound in xfrm_policy_addr

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: <20201230115517.iZlNGikD3bKtySfO@hankala.org>
Date:   Wed, 30 Dec 2020 12:06:23 +0000
From:   Visa Hankala <visa@...kala.org>
To:     Florian Westphal <fw@...len.de>
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] xfrm: Fix wraparound in xfrm_policy_addr_delta()

On Tue, Dec 29, 2020 at 05:01:27PM +0100, Florian Westphal wrote:
> Visa Hankala <visa@...kala.org> wrote:
> > Use three-way comparison for address elements to avoid integer
> > wraparound in the result of xfrm_policy_addr_delta().
> > 
> > This ensures that the search trees are built and traversed correctly
> > when the difference between compared address elements is larger
> > than INT_MAX.
> 
> Please provide an update to tools/testing/selftests/net/xfrm_policy.sh
> that shows that this is a problem.

I will do that in the next revision.

> >  	switch (family) {
> >  	case AF_INET:
> > -		if (sizeof(long) == 4 && prefixlen == 0)
> > -			return ntohl(a->a4) - ntohl(b->a4);
> > -		return (ntohl(a->a4) & ((~0UL << (32 - prefixlen)))) -
> > -		       (ntohl(b->a4) & ((~0UL << (32 - prefixlen))));
> > +		mask = ~0U << (32 - prefixlen);
> > +		ma = ntohl(a->a4) & mask;
> > +		mb = ntohl(b->a4) & mask;
> 
> This is suspicious.  Is prefixlen == 0 impossible?
> 
> If not, then after patch
> mask = ~0U << 32;
> 
> ... and function returns 0.

prefixlen == 0 is possible. However, I now realize that left shift
by 32 should be avoided with 32-bit integers.

With prefixlen == 0, there is only one equivalence class, so
returning 0 seems reasonable to me.

Is there a reason why the function has treated /0 prefix as /32
with IPv4? IPv6 does not have this treatment.