Re: tip -ENOBOOT - bisected to locking/refcounts, x86/asm: Implement fast refcount overflow protection
From: Kees Cook
Date: Fri Sep 01 2017 - 13:12:53 EST
On Fri, Sep 1, 2017 at 6:09 AM, Mike Galbraith <efault@xxxxxx> wrote:
> On Fri, 2017-09-01 at 08:57 +0200, Mike Galbraith wrote:
>> On Thu, 2017-08-31 at 11:45 -0700, Kees Cook wrote:
>> > On Thu, Aug 31, 2017 at 10:19 AM, Mike Galbraith <efault@xxxxxx> wrote:
>> > > On Thu, 2017-08-31 at 10:00 -0700, Kees Cook wrote:
>> > >>
>> > >> Oh! So it's gcc-version sensitive? That's alarming. Is this mapping correct:
>> > >>
>> > >> 4.8.5: WARN, eventual kernel hang
>> > >> 6.3.1, 7.0.1: WARN, but continues working
>> > >
>> > > Yeah, that's correct. I find that troubling, simply because this gcc
>> > > version has been through one hell of a lot of kernels with me. Yeah, I
>> > > know, that doesn't exempt it from having bugs, but color me suspicious.
>> >
>> > I still can't hit this with a 4.8.5 build. :(
>> >
>> > With _RATELIMIT removed, this should, in theory, report whatever goes
>> > negative first...
>>
>> I applied the other patch you posted, and built with gcc-6.3.1 to
>> remove the gcc-4.8.5 aspect. Look below the resulting splat.
>
> Grr, that one has a in6_dev_getx() line missing for the first
> increment, where things go pear shaped.
>
> With that added, looking at counter both before, and after incl, with a
> trace_printk() in the exception handler showing it doing its saturate
> thing, irqs disabled across the whole damn refcount_inc(), and even
> booting box nr_cpus=1 for extra credit...
>
> HTH can that first refcount_inc() get there?
>
> # tracer: nop
> #
> # _-----=> irqs-off
> # / _----=> need-resched
> # | / _---=> hardirq/softirq
> # || / _--=> preempt-depth
> # ||| / delay
> # TASK-PID CPU# |||| TIMESTAMP FUNCTION
> # | | | |||| | |
> systemd-1 [000] d..1 1.937284: in6_dev_getx: PRE refs.counter:3
> systemd-1 [000] d..1 1.937295: ex_handler_refcount: *(int *)regs->cx = -1073741824
> systemd-1 [000] d..1 1.937296: in6_dev_getx: POST refs.counter:-1073741824
O_o
Can you paste the disassembly of in6_dev_getx? I can't understand how
we're landing in the exception handler.
> systemd-1 [000] d..1 1.937296: in6_dev_getx: PRE refs.counter:-1073741824
> systemd-1 [000] d..1 1.937297: ex_handler_refcount: *(int *)regs->cx = -1073741824
> systemd-1 [000] d..1 1.937297: in6_dev_getx: POST refs.counter:-1073741824
> systemd-1 [000] d..1 1.937297: in6_dev_getx: PRE refs.counter:-1073741824
> systemd-1 [000] d..1 1.937298: ex_handler_refcount: *(int *)regs->cx = -1073741824
> systemd-1 [000] d..1 1.937299: in6_dev_getx: POST refs.counter:-1073741824
>
> ---
> arch/x86/include/asm/refcount.h | 14 ++++++++++++++
> arch/x86/mm/extable.c | 1 +
> include/net/addrconf.h | 12 ++++++++++++
> net/ipv6/route.c | 6 +++---
> 4 files changed, 30 insertions(+), 3 deletions(-)
>
> --- a/arch/x86/include/asm/refcount.h
> +++ b/arch/x86/include/asm/refcount.h
> @@ -55,6 +55,20 @@ static __always_inline void refcount_inc
> : : "cc", "cx");
> }
>
> +static __always_inline void refcount_inc_x(refcount_t *r)
> +{
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + trace_printk("PRE refs.counter:%d\n", r->refs.counter);
> + asm volatile(LOCK_PREFIX "incl %0\n\t"
> + REFCOUNT_CHECK_LT_ZERO
> + : [counter] "+m" (r->refs.counter)
> + : : "cc", "cx");
Does this need an explicit "memory" added to the clobbers line here?
This isn't present in the atomic_inc() implementation, but maybe
something confuses gcc in this case into ignoring the "+m" marking?
> + trace_printk("POST refs.counter:%d\n", r->refs.counter);
> + local_irq_restore(flags);
> +}
> +
> static __always_inline void refcount_dec(refcount_t *r)
> {
> asm volatile(LOCK_PREFIX "decl %0\n\t"
> --- a/arch/x86/mm/extable.c
> +++ b/arch/x86/mm/extable.c
> @@ -45,6 +45,7 @@ bool ex_handler_refcount(const struct ex
> {
> /* First unconditionally saturate the refcount. */
> *(int *)regs->cx = INT_MIN / 2;
> + trace_printk("*(int *)regs->cx = %d\n", *(int *)regs->cx);
Just for fun, can you print out *(int *)regs->cx before the assignment too?
>
> /*
> * Strictly speaking, this reports the fixup destination, not
> --- a/include/net/addrconf.h
> +++ b/include/net/addrconf.h
> @@ -321,6 +321,18 @@ static inline struct inet6_dev *in6_dev_
> return idev;
> }
>
> +static inline struct inet6_dev *in6_dev_getx(const struct net_device *dev)
> +{
> + struct inet6_dev *idev;
> +
> + rcu_read_lock();
> + idev = rcu_dereference(dev->ip6_ptr);
> + if (idev)
> + refcount_inc_x(&idev->refcnt);
> + rcu_read_unlock();
> + return idev;
> +}
> +
> static inline struct neigh_parms *__in6_dev_nd_parms_get_rcu(const struct net_device *dev)
> {
> struct inet6_dev *idev = __in6_dev_get(dev);
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -4041,12 +4041,12 @@ void __init ip6_route_init_special_entri
> * the loopback reference in rt6_info will not be taken, do it
> * manually for init_net */
> init_net.ipv6.ip6_null_entry->dst.dev = init_net.loopback_dev;
> - init_net.ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev);
> + init_net.ipv6.ip6_null_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev);
> #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> init_net.ipv6.ip6_prohibit_entry->dst.dev = init_net.loopback_dev;
> - init_net.ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev);
> + init_net.ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev);
> init_net.ipv6.ip6_blk_hole_entry->dst.dev = init_net.loopback_dev;
> - init_net.ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(init_net.loopback_dev);
> + init_net.ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_getx(init_net.loopback_dev);
> #endif
> }
>
-Kees
--
Kees Cook
Pixel Security