netdev - Re: [PATCH 2/3] bpf: Track alignment of MAP pointers in verifier.

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: <20170515.113419.1265777991542814689.davem@davemloft.net>
Date:   Mon, 15 May 2017 11:34:19 -0400 (EDT)
From:   David Miller <davem@...emloft.net>
To:     daniel@...earbox.net
Cc:     ast@...com, alexei.starovoitov@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH 2/3] bpf: Track alignment of MAP pointers in verifier.

From: Daniel Borkmann <daniel@...earbox.net>
Date: Mon, 15 May 2017 15:10:02 +0200

>>> What are the semantics of using id here? In ptr_to_pkt, we have it,
>>> so that eventually, in find_good_pkt_pointers() we can match on id
>>> and update the range for all such regs with the same id. I'm just
>>> wondering as the side effect of this is that this makes state
>>> pruning worse.

Daniel, I looked at the state pruning for maps.  The situation is
quite interesting.

Once we have env->varlen_map_value_access (and load or store via a
PTR_TO_MAP_VALUE_ADJ pointer), the state pruning gets more strict, the
relevant tests are:

		if (memcmp(rold, rcur, sizeof(*rold)) == 0)
			continue;

		/* If the ranges were not the same, but everything else was and
		 * we didn't do a variable access into a map then we are a-ok.
		 */
		if (!varlen_map_access &&
		    memcmp(rold, rcur, offsetofend(struct bpf_reg_state, id)) == 0)
			continue;

The first memcmp() is not going to match any time we adjust any
component of a MAP pointer reg.  The offset, the alignment, anything.
That means any side effect whatsoever performed by check_pointer_add()
even if we changed the code to not modify reg->id

The second check elides:

	s64 min_value;
	u64 max_value;
	u32 min_align;
	u32 aux_off;
	u32 aux_off_align;

from the comparison but only if we haven't done a variable length
MAP access.

The only conclusion I can come to is that changing reg->id for
PTR_TO_MAP_VALUE_ADJ has no side effect for state pruning, unless we
perform PTR_TO_MAP_VALUE_ADJ register adjustments without ever
accessing the map via that pointer in the entire program.

I could add some new state to avoid the reg->id change, but given
the above I don't think that it is really necessary.