Re: [PATCH 2/2] configure: add support for Control-Flow Integrity

[Daniel P . Berrangé]

On Thu, Jul 02, 2020 at 08:50:08AM -0400, Daniele Buono wrote:
> 
> 
> On 7/2/2020 5:52 AM, Daniel P. Berrangé wrote:
> > On Thu, Jul 02, 2020 at 01:49:48AM -0400, Daniele Buono wrote:
> > > This patch adds a flag to enable/disable control flow integrity checks
> > > on indirect function calls. This feature is only provided by LLVM/Clang
> > > v3.9 or higher, and only allows indirect function calls to functions
> > > with compatible signatures.
> > > 
> > > We also add an option to enable a debugging version of cfi, with verbose
> > > output in case of a CFI violation.
> > > 
> > > CFI on indirect function calls does not support calls to functions in
> > > shared libraries (since they were not known at compile time), and such
> > > calls are forbidden. QEMU relies on dlopen/dlsym when using modules,
> > > so we make modules incompatible with CFI.
> > > 
> > > We introduce a blacklist file, to disable CFI checks in a limited number
> > > of TCG functions.
> > > 
> > > The feature relies on link-time optimization (lto), which requires the
> > > use of the gold linker, and the LLVM versions of ar, ranlib and nm.
> > > This patch take care of checking that all the compiler toolchain
> > > dependencies are met.
> > > 
> > > Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> > > ---
> > >   cfi-blacklist.txt |  27 +++++++
> > >   configure         | 177 ++++++++++++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 204 insertions(+)
> > >   create mode 100644 cfi-blacklist.txt
> > > 
> > > diff --git a/cfi-blacklist.txt b/cfi-blacklist.txt
> > > new file mode 100644
> > > index 0000000000..bf804431a5
> > > --- /dev/null
> > > +++ b/cfi-blacklist.txt
> > > @@ -0,0 +1,27 @@
> > > +# List of functions that should not be compiled with Control-Flow 
> > > Integrity
> > > +
> > > +[cfi-icall]
> > > +# TCG creates binary blobs at runtime, with the transformed code.
> > > +# When it's time to execute it, the code is called with an indirect 
> > > function
> > > +# call. Since such function did not exist at compile time, the runtime 
> > > has no
> > > +# way to verify its signature. Disable CFI checks in the function that 
> > > calls
> > > +# the binary blob
> > > +fun:cpu_tb_exec
> > > +
> > > +# TCI (Tiny Compiler Interpreter) is an interpreter for TCG pseudo code.
> > > +# One possible operation in the pseudo code is a call to binary code.
> > > +# Therefore, disable CFI checks in the interpreter function
> > > +fun:tcg_qemu_tb_exec
> > > +
> > > +# TCG Plugins Callback Functions. The mechanism rely on opening external
> > > +# shared libraries at runtime and get pointers to functions in such 
> > > libraries
> > > +# Since these pointers are external to the QEMU binary, the runtime 
> > > cannot
> > > +# verify their signature. Disable CFI Checks in all the functions that 
> > > use
> > > +# such pointers.
> > > +fun:plugin_vcpu_cb__simple
> > > +fun:plugin_cb__simple
> > > +fun:plugin_cb__udata
> > > +fun:qemu_plugin_tb_trans_cb
> > > +fun:qemu_plugin_vcpu_syscall
> > > +fun:qemu_plugin_vcpu_syscall_ret
> > > +fun:plugin_load
> > 
> > The need to maintain this list of functions makes me feel very
> > uneasy.
> > 
> > How can we have any confidence that this list of functions is
> > accurate ? How will maintainers ensure that they correctly update
> > it as they are writing/changing code, and how will they test the
> > result ?
> > 
> > It feels like it has the same general maint problem as the original
> > seccomp code we used, where we were never confident we had added
> > the right exceptions to let QEMU run without crashing when users
> > tickled some feature we forgot about.
> > 
> > 
> > Regards,
> > Daniel
> > 
> 
> I agree with you that keeping that list updated is a daunting task. In my
> opinion, however, it is not as difficult as a seccomp filter, for the
> following reasons:
> 
> 1) Seccomp covers everything that runs in your process, including shared
> libraries that you have no control over. CFI covers only the code in the
> QEMU binary. So at least we don't have to guess what other libraries used by
> QEMU will or won't do during execution.
> 
> 2) With seccomp you have to filter behavior that, while admissible, should
> not happen in your code. CFI can be seen as a run-time type checking system;
> if the signature of the function is wrong, that is a coding error... in
> theory. In practice, there is a corner-case because the type checking
> doesn't know the signature of code loaded or written at run-time, and that
> is why you have to use a CFI filter.

Can you elaborate on this function signature rules here ? Is this referring
to scenarios where you cast between 2 different function prototypes ?

I'm wondering if this applies to the way GLib's main loop source callbacks
are registered.

eg the g_source_set_callback method takes a callback with a signature
of "GSourceFunc" which is

  gboolean (*)(void *)

but the way GSources are implemented means that each implementation gets
to define its own custom callback signature. So for example, in QIOChannel
we use

  int (*)(struct QIOChannel *, enum <anonymous>,  void *)

Thus, we always have an intentional bad function cast when invoking the
g_source_set_callback method.

GCC is able to warn about these if we add -Wcast-function-type, but we
don't do that because these bad casts are intentional.

eg

io/channel.c: In function ‘qio_channel_add_watch_full’:
io/channel.c:315:35: error: cast between incompatible function types from 
‘QIOChannelFunc’ {aka ‘int (*)(struct QIOChannel *, enum <anonymous>,  void 
*)’} to ‘gboolean (*)(void *)’ {aka ‘int (*)(void *)’} 
[-Werror=cast-function-type]
  315 |     g_source_set_callback(source, (GSourceFunc)func, user_data, notify);
      |                                   ^
io/channel.c: In function ‘qio_channel_wait’:
io/channel.c:507:27: error: cast between incompatible function types from 
‘gboolean (*)(QIOChannel *, GIOCondition,  void *)’ {aka ‘int (*)(struct 
QIOChannel *, enum <anonymous>,  void *)’} to ‘gboolean (*)(void *)’ {aka ‘int 
(*)(void *)’} [-Werror=cast-function-type]
  507 |                           (GSourceFunc)qio_channel_wait_complete,
      |                           ^


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|