Re: [PATCH] meson: Stop if cfi is enabled with system slirp

[Daniele Buono]

On 3/4/2021 5:56 AM, Paolo Bonzini wrote:
> On 04/03/21 11:37, Daniel P. Berrangé wrote:
> > On Wed, Mar 03, 2021 at 09:59:38PM -0500, Daniele Buono wrote:
> > > For CFI, we need to compile slirp as a static library together with 
> > > qemu.
> > > This is because we register slirp functions as callbacks for QEMU 
> > > Timers.
> > > When using a system-wide shared libslirp, the type information for the
> > > callback is missing and the timer call produces a false positive with 
> > > CFI.
> >
> > Is there work being done, or at least an active plan, for fixing this ?
>
> Daniele, would this work (uncompiled even)?

Basically, yes it would work, but I'd be worried of the effectiveness of 
CFI. Timers in QEMU are one of the easiest ways to change control flow 
when you get arbitrary write permissions, and were used in at least a 
couple of VM escape demos last year. CFI as before would stop that 
attack vector. With this patch, we would leave the attack vector 
essentially open. More comments (and a couple of suggestions to make it 
harder) later.

> diff --git a/net/slirp.c b/net/slirp.c
> index be914c0be0..82e05d2c01 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -174,23 +174,42 @@ static int64_t net_slirp_clock_get_ns(void *opaque)
>       return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>   }
>
> +typedef struct SlirpTimer {
> +    QEMUTimer t;
> +    SlirpTimerCb cb;
> +    void *cb_opaque;
> +} SlirpTimer;
> +
> +static void slirp_timer_cb(void *opaque)
> +{
> +    SlirpTimer *st = opaque;
> +    st->cb(st->cb_opaque);
> +}

This call is still violating CFI (st->cb is a pointer in libslirp), but
now that we have a specific callback for slirp, we can easily add a
"QEMU_DISABLE_CFI" decorator to disable CFI only on `slirp_timer_cb`.

The problem is that an attack on the timer is as easy now as it was
without CFI. You just have to change the timer entry to call
slirp_timer_cb, and the opaque pointer to a SlirpTimer struct you
created, anywhere in memory, with a pointer to your own function and
your own parameters. The call to slirp_timer_cb is valid for CFI, while
the call to st->cb is not checked anymore.

I believe we can "fix" this by manually (i.e. in the code) making sure
that st->cb is a valid callback. I think there's a limited number of
(possibly only one right now) callbacks that slirp will use on a timer.
We could create a R/O array that contains the pointers to the allowed
functions, and here check that st->cb is a pointer to one of those.

A more generic alternative could be to try to use dladdr, to resolve the
pointer to a symbol, and make sure that st->cb points to a symbol in 
libslirp. Not sure it will work (we are not opening libslirp with
dlopen), and definitely heavy from a performance point of view, but
would be probably a solution generic enough for all possible future
cases.

I can try to provide RFC patches for both cases, if you guys are 
interested, to see how the code would look like.

> +
>   static void *net_slirp_timer_new(SlirpTimerCb cb,
>                                    void *cb_opaque, void *opaque)
>   {
> -    return timer_new_full(NULL, QEMU_CLOCK_VIRTUAL,
> -                          SCALE_MS, QEMU_TIMER_ATTR_EXTERNAL,
> -                          cb, cb_opaque);
> +    SlirpTimer *st = g_new(SlirpTimer, 1);
> +    st->cb = cb;
> +    st->cb_opaque = cb_opaque;
> +    timer_init_full(&st->t, NULL, QEMU_CLOCK_VIRTUAL,
> +                    SCALE_MS, QEMU_TIMER_ATTR_EXTERNAL,
> +                    slirp_timer_cb, st);
> +    return st;
>   }
>
>   static void net_slirp_timer_free(void *timer, void *opaque)
>   {
> -    timer_free(timer);
> +    SlirpTimer *st = timer;
> +    timer_del(&st->t);
> +    g_free(st);
>   }
>
>   static void net_slirp_timer_mod(void *timer, int64_t expire_timer,
>                                   void *opaque)
>   {
> -    timer_mod(timer, expire_timer);
> +    SlirpTimer *st = timer;
> +    timer_mod(&st->t, expire_timer);
>   }
>
>   static void net_slirp_register_poll_fd(int fd, void *opaque)