qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniele Buono
Subject: Re: [PATCH 2/2] configure: add support for Control-Flow Integrity
Date: Thu, 2 Jul 2020 11:02:42 -0400
User-agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0

On 7/2/2020 9:12 AM, Daniel P. Berrangé wrote:
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 ?

It is, partially. A CFI check, however, is done at the moment you call the function pointer. So if you cast a function pointer to a different type (say gboolean (*)(void *) ), but cast it back to the original type before you call it, it's going to be fine.

Chances are you are doing this anyway, since you really want to pass those parameters to the callback.

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

I think GLib's approach is clean and shouldn't be an issue with CFI. We are indeed passing the function with a wonky cast, but when the callback needs to be called back, that part is handled by a dispatcher function that we're writing in QEMU code, such as (io/channel-buffer.c:177)

static gboolean
qio_channel_buffer_source_dispatch(GSource *source,
                                   GSourceFunc callback,
                                   gpointer user_data)
{
    QIOChannelFunc func = (QIOChannelFunc)callback;
    QIOChannelBufferSource *bsource = (QIOChannelBufferSource *)source;

    return (*func)(QIO_CHANNEL(bsource->bioc),
                   ((G_IO_IN | G_IO_OUT) & bsource->condition),
                   user_data);
}

So the callback pointer is cast back to the original type, and CFI checks should be happy about it.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]