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

[Paolo Bonzini]

On 05/03/21 17:52, Daniele Buono wrote:
> > 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`.

Ok, so let's go with your patch for now.  Independently we could change 
libslirp to:

- add a separate callback (timer_new_v2?) in SlirpCb.  If it is set, we 
use it and pass an enum instead of the SlirpTimerCb cb.

- add a function slirp_timer_expired(enum SlirpTimerId timer_id, void 
*cb_opaque) that does the indirection but without passing around an 
arbitrary function pointer.

In 6.1 we will update the internal libslirp to a version that supports 
the new API, through a patch very similar to mine above.  By requiring 
that new version as the minimum supported one, enabling CFI will be 
possible even with system slirp.

You can open a slirp merge request at 
https://gitlab.freedesktop.org/slirp/libslirp.

> 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 understand better now the situation, thanks for taking the time to 
explain it.

Paolo