[kernel-hardening] Re: Conversion from atomic_t to refcount_t: summary of issues

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Dec 20, 2016 at 12:55:02PM +0200, Liljestrand Hans wrote:

> For reference, I've listed here the places that were causing "increment
> on 0" WARNs on my previous boot (temporarily allowed inc on 0 to make
> boot possible). These seem to be mostly related to resource reuse, but
> we haven't yet to looked in detail on how to deal with them.
> 
> fs/ext4/mballoc.c:3399          ext4_mb_use_preallocated
>         Seems to have separate tracking of destruction

This one seems particularly daft, since afaict all pa_count usage is
under pa_lock. No need for it to be atomic. Also, that code is weird, it
has separate pa_free and pa_deleted state.

This should definitely not be converted to refcount_t until its
sanitized.

> net/ipv4/fib_semantics.c:994    fib_create_info

This one I'm not sure how its not broken.

It does something like:

		ofi = fib_find_info(fi);
		if (ofi) {
			// use ofi, free fi
		}

		spin_lock_hb();
		atomic_inc(&fi->fib_clntref);
		// insert fi
		spin_unlock_hb();


If that races against itself, both instances can fail to find an
existing matching fi, and both will insert fi, resulting in a duplicate
fi.

Also, note that at the point of atomic_inc(), fi isn't in fact published
and therefore this need not be an atomic operation.

I could of course miss something subtle, since I only read part of this
code. In any case, even if that were somehow incorrect, I think you can
init fib_clntref with 1 and make it work with that.

> net/ipv4/devinet.c:233          inetdev_init

This seems to use atomic_inc before the object is published, and could
therefore simply use atomic_set().

> net/ipv4/tcp_ipv4.c:1793        inet_sk_rx_dst_set

That needs more context to be evaluated. Also seems very dodgy code in
any case. We need the caller of this function that calls it on 0.

> net/ipv4/route.c:2153:          __ip_route_output_key_hash

need more context, there's not in fact an atomic_ in that function, and
its a giant function.

> net/ipv6/ip6_fib.c:949          fib6_add

Can't make sense of this :/



Didn't look at the rest, but going by the above blindly converting to
refcount_t without prior cleanups isn't a good idea.