Re: [PATCH] coroutine: resize pool periodically instead of limiting size
Stefan, the patch looks great.
Thank you for debugging the performance issue that was happening with
SafeStack.
On 9/2/2021 4:10 AM, Stefan Hajnoczi wrote:
On Wed, Sep 01, 2021 at 05:09:23PM +0100, Stefan Hajnoczi wrote:
It was reported that enabling SafeStack reduces IOPS significantly
(>25%) with the following fio benchmark on virtio-blk using a NVMe host
block device:
# fio --rw=randrw --bs=4k --iodepth=64 --runtime=1m --direct=1 \
--filename=/dev/vdb --name=job1 --ioengine=libaio --thread \
--group_reporting --numjobs=16 --time_based \
--output=/tmp/fio_result
Serge Guelton and I found that SafeStack is not really at fault, it just
increases the cost of coroutine creation. This fio workload exhausts the
coroutine pool and coroutine creation becomes a bottleneck. Previous
work by Honghao Wang also pointed to excessive coroutine creation.
Creating new coroutines is expensive due to allocating new stacks with
mmap(2) and mprotect(2). Currently there are thread-local and global
pools that recycle old Coroutine objects and their stacks but the
hardcoded size limit of 64 for thread-local pools and 128 for the global
pool is insufficient for the fio benchmark shown above.
This patch changes the coroutine pool algorithm to a simple thread-local
pool without a size limit. Threads periodically shrink the pool down to
a size sufficient for the maximum observed number of coroutines.
This is a very simple algorithm. Fancier things could be done like
keeping a minimum number of coroutines around to avoid latency when a
new coroutine is created after a long period of inactivity. Another
thought is to stop the timer when the pool size is zero for power saving
on threads that aren't using coroutines. However, I'd rather not add
bells and whistles unless they are really necessary.
I agree we should aim at something that is as simple as possible.
However keeping a minumum number of coroutines could be useful to avoid
performance regressions from the previous version.
I wouldn't say restore the global pool - that's way too much trouble -
but keeping the old "pool size" configuration parameter as the miniumum
pool size could be a good idea to maintain the previous performance in
cases where the dynamic pool shrinks too much.
The global pool is removed by this patch. It can help to hide the fact
that local pools are easily exhausted, but it's doesn't fix the root
cause. I don't think there is a need for a global pool because QEMU's
threads are long-lived, so let's keep things simple.
Performance of the above fio benchmark is as follows:
Before After
IOPS 60k 97k
Memory usage varies over time as needed by the workload:
VSZ (KB) RSS (KB)
Before fio 4705248 843128
During fio 5747668 (+ ~100 MB) 849280
After fio 4694996 (- ~100 MB) 845184
This confirms that coroutines are indeed being freed when no longer
needed.
Thanks to Serge Guelton for working on identifying the bottleneck with
me!
Reported-by: Tingting Mao
Cc: Serge Guelton
Cc: Honghao Wang
Cc: Paolo Bonzini
Cc: Daniele Buono
Signed-off-by: Stefan Hajnoczi
---
include/qemu/coroutine-pool-timer.h | 36 +
include/qemu/coroutine.h| 7
iothread.c | 6 +++
util/coroutine-pool-timer.c | 35
util/main-loop.c| 5 +++
util/qemu-coroutine.c | 62 ++---
util/meson.build| 1 +
7 files changed, 119 insertions(+), 33 deletions(-)
create mode 100644 include/qemu/coroutine-pool-timer.h
create mode 100644 util/coroutine-pool-timer.c
Adding Andrew and Jenifer in case they have thoughts on improving QEMU's
coroutine pool algorithm.
diff --git a/include/qemu/coroutine-pool-timer.h
b/include/qemu/coroutine-pool-timer.h
new file mode 100644
index 00..c0b520ce99
--- /dev/null
+++ b/include/qemu/coroutine-pool-timer.h
@@ -0,0 +1,36 @@
+/*
+ * QEMU coroutine pool timer
+ *
+ * Copyright (c) 2021 Red Hat, Inc.
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+#ifndef COROUTINE_POOL_TIMER_H
+#define COROUTINE_POOL_TIMER_H
+
+#include "qemu/osdep.h"
+#include "block/aio.h"
+
+/**
+ * A timer that periodically resizes this thread's coroutine pool, freeing
+ * memory if there are too many unused coroutines.
+ *
+ * Threads that make heavy use of coroutines should use this. Failure to resize
+ * the coroutine pool can lead to large amounts of memory sitting idle and
+ * never being used after the first time.
+ */
+typedef struct {
+QEMUTimer *timer;
+} CoroutinePoolTimer;
+
+/* Call this before the thread runs the AioContext */
+void coroutine_pool_timer_init(CoroutinePoolTimer *pt, AioContext *ctx);
+
+/* Cal
Re: [PATCH] gitlab-ci.d/buildtest: Mark the aarch64 and ppc64-s390x CFI jobs as manual
I agree, making these manual tasks until we find a fix for this is the only solution I can think of too. Daniele On 7/28/2021 3:51 AM, Thomas Huth wrote: These two jobs are currently failing very often - the linker seems to get killed due to out-of-memory problems. Since apparently nobody has currently an idea how to fix that nicely, let's mark the jobs as manual for the time being until someone comes up with a proper fix. Signed-off-by: Thomas Huth --- .gitlab-ci.d/buildtest.yml | 12 1 file changed, 12 insertions(+) diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml index 63f1903f07..3537c6f1a1 100644 --- a/.gitlab-ci.d/buildtest.yml +++ b/.gitlab-ci.d/buildtest.yml @@ -416,6 +416,12 @@ build-cfi-aarch64: expire_in: 2 days paths: - build + rules: +# FIXME: This job is often failing, likely due to out-of-memory problems in +# the constraint containers of the shared runners. Thus this is marked as +# manual until the situation has been solved. +- when: manual + allow_failure: true check-cfi-aarch64: extends: .native_test_job_template @@ -452,6 +458,12 @@ build-cfi-ppc64-s390x: expire_in: 2 days paths: - build + rules: +# FIXME: This job is often failing, likely due to out-of-memory problems in +# the constraint containers of the shared runners. Thus this is marked as +# manual until the situation has been solved. +- when: manual + allow_failure: true check-cfi-ppc64-s390x: extends: .native_test_job_template
Re: [PULL v2 04/15] gitlab-ci.yml: Add jobs to test CFI flags
Hi Philippe, I'm looking at the public QEMU pipelines and it seems that that job usually takes between 50 and 55 minutes, but there are higher spikes at 56, 57 and one where it failed at 1h. We could perhaps set the timeout a bit higher, like 1h 10m, to not terminate the outliers immediately? The job you linked was almost over, there were just about 20-ish tests to be linked, so it was probably next to completion. On 3/22/2021 9:39 AM, Philippe Mathieu-Daudé wrote: +build-cfi-x86_64: + <<: *native_build_job_definition + needs: + - job: amd64-fedora-container + variables: +LD_JOBS: 1 +AR: llvm-ar +IMAGE: fedora +CONFIGURE_ARGS: --cc=clang --cxx=clang++ --enable-cfi --enable-cfi-debug + --enable-safe-stack --enable-slirp=git +TARGETS: x86_64-softmmu +MAKE_CHECK_ARGS: check-build + artifacts: +expire_in: 2 days +paths: + - build FYI this job is timeouting: ERROR: Job failed: execution took longer than 1h0m0s seconds https://gitlab.com/qemu-project/qemu/-/jobs/1112829128
Re: [PATCH] meson: Stop if cfi is enabled with system slirp
On 3/5/2021 12:18 PM, Paolo Bonzini wrote: 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. Hi Paolo, sounds like a good plan! I'll start working on this. Regards, Daniele
Re: [PATCH] meson: Stop if cfi is enabled with system slirp
On 3/8/2021 6:19 AM, Daniel P. Berrangé wrote: My concern is that libslirp is just showing us one known example of the problem. QEMU links to many more external libraries, which might exhibit similar issues. If we need to rebuild all the dependancies with CFI too, to be confident that the combined work will operate correctly, then this is quite a significant implication. Overall I think this is going to be a problem for the changes of distros adopting the use of CFI, especially if they're not using CLang as their toolchain. In my opinion, there's no need to rebuild everything with CFI. There will be libraries that will benefit more from CFI, such as libslirp IMHO. But that still doesn't even mean that we need a CFI-enabled version to operate correctly. From a functional point of view, there are plenty of ways to have a CFI- enabled binary work with shared libraries that do not support CFI (or cross-dso CFI). From a security point of view it will be a trade-off. So I think we should study it on a per-library case to find out the best way forward. I believe in most cases, an approach like the one discussed with Paolo will be more than enough to get a good security level in QEMU, especially if the feature provided by the library is not used at runtime. Regards, Daniel
Re: [PATCH] meson: Stop if cfi is enabled with system slirp
On 3/4/2021 5:37 AM, Daniel P. Berrangé wrote: Is there work being done, or at least an active plan, for fixing this ? Distros generally won't want to static link slirp to QEMU when there is a shared slirp available. It increases the security burden to maintain slirp twice, especially as slirp has a history of CVEs. IOW, the inability to use shared slirp may well prevent CFI from being used in distros. Daniel, Adoption is a very good point. We don't want to have multiple versions of the same library hanging around the O.S., unless strictly necessary. The problem (if I wear my security hat) is that, as you pointed out, slirp is known to have a history of CVEs, and it also rely heavily on callbacks and function pointers. So it would be one of the best candidates for CFI support. A (long-term) solution could be to compile libslirp as a shared library, WITH Control-Flow Integrity. Clang does have an experimental support for Cross-DSO CFI. However, it is not viable at the moment because: 1. It is still considered Experimental 2. It is not compatible with pointer type generalization (which we need because of Glib and other uses in QEMU). Cross-DSO CFI also have some performance implications but I think that would be a very small price to pay, and only in corner-case conditions. I don't want to bore anyone too much with the details of the implementation... Yet. I'd be happy to explain the Cross-DSO mechanism implemented by Clang if it is considered interesting here. The details can also be found here: https://clang.llvm.org/docs/ControlFlowIntegrity.html#shared-library-support And https://clang.llvm.org/docs/ControlFlowIntegrityDesign.html (Section "Shared library support") I think this would be the best long-term solution to improve security because it would allow to use CFI virtually on every library we consider security-sensitive, but not on the others. But it would require some help and work from/to the Clang community. In the short term, we should work out something similar to Paolo's approach. I'll add a few comments to his email.
Re: [PATCH] meson: Stop if cfi is enabled with system slirp
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(>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(>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(>t, expire_timer);
}
static void net_slirp_register_poll_fd(int fd, void *opaque)
[PATCH v3 2/2] gitlab-ci.yml: Add jobs to test CFI flags
QEMU has had options to enable control-flow integrity features for a few months now. Add two sets of build/check/acceptance jobs to ensure the binary produced is working fine. The three sets allow testing of x86_64 binaries for x86_64, s390x, ppc64 and aarch64 targets Signed-off-by: Daniele Buono --- .gitlab-ci.yml | 119 + 1 file changed, 119 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 814f51873f..7b1f25c92e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -483,6 +483,125 @@ clang-user: --extra-cflags=-fsanitize=undefined --extra-cflags=-fno-sanitize-recover=undefined MAKE_CHECK_ARGS: check-unit check-tcg +# Set LD_JOBS=1 because this requires LTO and ld consumes a large amount of memory. +# On gitlab runners, default value sometimes end up calling 2 lds concurrently and +# triggers an Out-Of-Memory error +# +# Since slirp callbacks are used in QEMU Timers, slirp needs to be compiled together +# with QEMU and linked as a static library to avoid false positives in CFI checks. +# This can be accomplished by using -enable-slirp=git, which avoids the use of +# a system-wide version of the library +# +# Split in three sets of build/check/acceptance to limit the execution time of each +# job +build-cfi-arm: + <<: *native_build_job_definition + needs: + - job: amd64-fedora-container + variables: +LD_JOBS: 1 +AR: llvm-ar +IMAGE: fedora +CONFIGURE_ARGS: --cc=clang --cxx=clang++ --enable-cfi --enable-cfi-debug + --enable-safe-stack --enable-slirp=git +TARGETS: aarch64-softmmu +MAKE_CHECK_ARGS: check-build + artifacts: +expire_in: 2 days +paths: + - build + +check-cfi-arm: + <<: *native_test_job_definition + needs: +- job: build-cfi-arm + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check + +acceptance-cfi-arm: + <<: *native_test_job_definition + needs: +- job: build-cfi-arm + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check-acceptance + <<: *acceptance_definition + +build-cfi-ibm: + <<: *native_build_job_definition + needs: + - job: amd64-fedora-container + variables: +LD_JOBS: 1 +AR: llvm-ar +IMAGE: fedora +CONFIGURE_ARGS: --cc=clang --cxx=clang++ --enable-cfi --enable-cfi-debug + --enable-safe-stack --enable-slirp=git +TARGETS: ppc64-softmmu s390x-softmmu +MAKE_CHECK_ARGS: check-build + artifacts: +expire_in: 2 days +paths: + - build + +check-cfi-ibm: + <<: *native_test_job_definition + needs: +- job: build-cfi-ibm + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check + +acceptance-cfi-ibm: + <<: *native_test_job_definition + needs: +- job: build-cfi-ibm + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check-acceptance + <<: *acceptance_definition + +build-cfi-intel: + <<: *native_build_job_definition + needs: + - job: amd64-fedora-container + variables: +LD_JOBS: 1 +AR: llvm-ar +IMAGE: fedora +CONFIGURE_ARGS: --cc=clang --cxx=clang++ --enable-cfi --enable-cfi-debug + --enable-safe-stack --enable-slirp=git +TARGETS: x86_64-softmmu +MAKE_CHECK_ARGS: check-build + artifacts: +expire_in: 2 days +paths: + - build + +check-cfi-intel: + <<: *native_test_job_definition + needs: +- job: build-cfi-intel + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check + +acceptance-cfi-intel: + <<: *native_test_job_definition + needs: +- job: build-cfi-intel + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check-acceptance + <<: *acceptance_definition + tsan-build: <<: *native_build_job_definition variables: -- 2.30.0
[PATCH v3 1/2] gitlab-ci.yml: Allow custom # of parallel linkers
Define a new variable LD_JOBS, that can be used to select
the maximum number of linking jobs to be executed in parallel.
If the variable is not defined, maintain the default given by
make -j
Currently, make parallelism at build time is based on the number
of cpus available.
This doesn't work well with LTO at linking, because with LTO the
linker has to load in memory all the intermediate object files
for optimization.
The end result is that, if the gitlab runner happens to run two
linking processes at the same time, the job will fail with an
out-of-memory error,
This patch leverages the ability to maintain high parallelism at
compile time, but limit the number of linkers executed in parallel.
Signed-off-by: Daniele Buono
---
.gitlab-ci.yml | 4
1 file changed, 4 insertions(+)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 8b6d495288..814f51873f 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -27,6 +27,10 @@ include:
else
../configure --enable-werror $CONFIGURE_ARGS ;
fi || { cat config.log meson-logs/meson-log.txt && exit 1; }
+- if test -n "$LD_JOBS";
+ then
+meson configure . -Dbackend_max_links="$LD_JOBS" ;
+ fi || exit 1;
- make -j"$JOBS"
- if test -n "$MAKE_CHECK_ARGS";
then
--
2.30.0
[PATCH v3 0/2] gitlab-ci.yml: Add jobs to test CFI
For a few months now QEMU has had options to enable compiler-based control-flow integrity if built with clang. While this feature has a low maintenance, It's probably still better to add tests to the CI environment to check that an update doesn't break it. The patchset allow gitlab testing of: * --enable-cfi: forward-edge cfi (function pointers) * --enable-safe-stack: backward-edge cfi (return pointers) As an added benefit, this also inherently tests LTO. The first patch allows a custom selection for linker parallelism. Currently, make parallelism at build time is based on the number of cpus available. This doesn't work well with LTO at linking, because the linker has to load in memory all the intermediate object files for optimization. If the gitlab runner happens to run two linking processes at the same time, the job will fail with an out-of-memory error, The patch leverages the ability to maintain high parallelism at compile time, but limit the number of linkers executed in parallel. The second patch introduces the ci/cd jobs in the gitlab pipeline. To maintain a limited number of short jobs, Daniel suggested to only test targets where KVM is available. This restricted the jobs to x86_64, ppc64, aarch64 and s390x. To keep the jobs under 1 hour, I created three chains of build -> check -> acceptance jobs, divided by architecture vendor (Intel, ARM, IBM). For build, we have to select --enable-slirp=git, because CFI needs a statically linked version of slirp, with CFI information. More info on this can be found in a comment in .gitlab-ci.yml, or on a patch for mason currently in ML: https://www.mail-archive.com/qemu-devel@nongnu.org/msg787636.html Test runs of the full pipeline are here (cfi-ci-v3 branch): https://gitlab.com/dbuono/qemu/-/pipelines/264484574 v3: - Restricted the targets to x86_64, ppc64, aarch64 and s390x, under suggestion from Daniel. v2: - More details in the code about the issue of using system-wide slirp - Use meson to only limit linker parallelism instead of forcing no parallelism on the whole compilation process. Daniele Buono (2): gitlab-ci.yml: Allow custom # of parallel linkers gitlab-ci.yml: Add jobs to test CFI flags .gitlab-ci.yml | 123 + 1 file changed, 123 insertions(+) -- 2.30.0
[PATCH] meson: Stop if cfi is enabled with system slirp
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.
With this patch, meson will stop if CFI is enabled with system-wide slirp
Signed-off-by: Daniele Buono
---
meson.build | 12
1 file changed, 12 insertions(+)
diff --git a/meson.build b/meson.build
index f3db83e974..e1ec5020ac 100644
--- a/meson.build
+++ b/meson.build
@@ -1569,6 +1569,18 @@ if have_system
endif
endif
+# 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.
+#
+# Now that slirp_opt has been defined, check if the selected slirp is
compatible
+# with control-flow integrity.
+if get_option('cfi') and slirp_opt == 'system'
+ error('Control-Flow Integrity is not compatible with system-wide slirp.' \
+ + ' Please configure with --enable-slirp=git')
+endif
+
fdt = not_found
fdt_opt = get_option('fdt')
if have_system
--
2.30.0
Re: [PATCH v2 0/2] gitlab-ci.yml: Add jobs to test CFI
On 3/2/2021 11:40 AM, Daniel P. Berrangé wrote: The CFI protection is something I'd say is relevant to virtualization use cases, not to emulation use cases https://qemu-project.gitlab.io/qemu/system/security.html IOW, the targets that are important to test are the ones where KVM is available. So that's s390x, ppc, x86, mips, and arm. I think we can probably ignore mips as that's fairly niche. We can also reasonably limit ourselves to only test the 64-bit variants of the target, on the basis that 32-bit is increasingly legacy/niche too. So that gives us ppc64le, x86_64, aarch64 and s390x as the targets we should get CI coverage for CFI. Thanks Daniel, I'll start working on a V3 that only contains those 4 targets, probably in two sets of build/check/acceptance to maintain the jobs below the hour mark. These would still be x86 binaries that are not testing KVM, however, because of the capabilities of the shared gitlab runners. I see that there's some work from Cleber Rosa to allow running custom jobs on aarch64 and s390x systems. I think that, when the infrastructure is ready, having a KVM-based CFI test there would help a lot in terms of coverage for those architectures.
Re: [PATCH v2 0/2] gitlab-ci.yml: Add jobs to test CFI
On 3/2/2021 10:38 AM, Daniel P. Berrangé wrote: Is this scenario going to upset CFI, or is it happy that 'void *' is compatible with 'mytype *', and ok with the intermediate casts to/from GCallback ? This is a valid scenario. LLVM does offer the ability of considering all pointer types compatible, and it is being enabled in QEMU. So void* is compatible to any type* and that would not be considered a fault. Intermediate casts are also fine since you are just passing the pointer but not using it. The check will happen only when the function is called, at which point it was cast back to something compatible.
Re: [PATCH v2 0/2] gitlab-ci.yml: Add jobs to test CFI
On 3/2/2021 5:30 AM, Daniel P. Berrangé wrote: On Mon, Mar 01, 2021 at 03:39:42PM -0500, Daniele Buono wrote: Hi Daniel, On 3/1/2021 10:08 AM, Daniel P. Berrangé wrote: What are the unique failure scenarios for CFI that these jobs are likely to expose ? Is it likely that we'll have cases where CFI succeeds in say, x86_64 target, but fails in aarch64 target ? For CFI to fail (even if it shouldn't) you'll need code that is calling a function pointer that was not well defined at compile time. Although unlikely, that could happen everywhere in the code. What does "was not well defined" mean here ? At high level, the compiler creates metadata for every function. Before jumping to a function pointer, it makes sure that the pointer and the pointee have matching types. Not well defined means one of these two cases: 1. The function has a different type than the pointer -> Most likely an error 2. The function was not available at compile time so the compiler could not create the related metadata -> Most likely a false positive. Thanks, Daniele
Re: [PATCH v2 0/2] gitlab-ci.yml: Add jobs to test CFI
Hi Daniel, On 3/1/2021 10:08 AM, Daniel P. Berrangé wrote: What are the unique failure scenarios for CFI that these jobs are likely to expose ? Is it likely that we'll have cases where CFI succeeds in say, x86_64 target, but fails in aarch64 target ? For CFI to fail (even if it shouldn't) you'll need code that is calling a function pointer that was not well defined at compile time. Although unlikely, that could happen everywhere in the code. So by just testing one (or few) targets we are are not covering the code in the TCG frontend used to compile the target ISA to tcg ops, which should be the in target/, and the architecture-dependent code in linux-user. That code seems unlikely (at least to me) to cause a false positive with CFI. Examples that I've seen in QEMU so far are: - Calling code that was JIT-ed at runtime - Callbacks to functions that were loaded from shared libraries - Signal Handlers And none of those should happen there IMHO. But you know, corner cases are still possible, and it's quite difficult to predict what new code may bring. We could also consider always testing one or two targets, and keep an optional job to test them all when deemed necessary. I'm thinking for example full testing when code in target/ or linux-user/ is considered, or for testing pre-release code. Would be nice to have this automated but I am not sure that's possible. Regards, Daniele
Re: [PATCH v2 0/2] gitlab-ci.yml: Add jobs to test CFI
Hi Daniel, On 3/1/2021 5:06 AM, Daniel P. Berrangé wrote: On Fri, Feb 26, 2021 at 10:21:06AM -0500, Daniele Buono wrote: Build jobs are on the longer side (about 2h and 20m), but I thought it would be better to just have 6 large jobs than tens of smaller ones. IMHO that is a not viable. Our longest job today is approx 60 minutes, and that is already painfully long when developers are repeatedly testing their patch series to find and fix bugs before posting them for review. I can perhaps get through 5-6 test cycles in a day. If we have a 2 hour 20 min job, then I'll get 2-3 test cycles a day. I don't want to see any new jobs added which increase the longest job execution time. We want to reduce our max job time if anything. I totally understand the argument. We could build two targets per job. That would create build jobs that take 40 to 60-ish minutes. If that's the case, however, I would not recommend testing all the possible targets but limit them to what is considered a set of most common targets. I have an example of the resulting pipeline here: https://gitlab.com/dbuono/qemu/-/pipelines/258983262 I selected intel, power, arm and s390 as "common" targets. Would something like this be a viable alternative? Perhaps after due thinking of what targets should be tested? Regards, Daniel
[PATCH v2 1/2] gitlab-ci.yml: Allow custom # of parallel linkers
Define a new variable LD_JOBS, that can be used to select
the maximum number of linking jobs to be executed in parallel.
If the variable is not defined, maintain the default given by
make -j
Signed-off-by: Daniele Buono
---
.gitlab-ci.yml | 4
1 file changed, 4 insertions(+)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 8b6d495288..814f51873f 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -27,6 +27,10 @@ include:
else
../configure --enable-werror $CONFIGURE_ARGS ;
fi || { cat config.log meson-logs/meson-log.txt && exit 1; }
+- if test -n "$LD_JOBS";
+ then
+meson configure . -Dbackend_max_links="$LD_JOBS" ;
+ fi || exit 1;
- make -j"$JOBS"
- if test -n "$MAKE_CHECK_ARGS";
then
--
2.30.0
[PATCH v2 2/2] gitlab-ci.yml: Add jobs to test CFI flags
QEMU has had options to enable control-flow integrity features for a few months now. Add two sets of build/check/acceptance jobs to ensure the binary produced is working fine. The two sets allow testing of x86_64 binaries for every target that is not deprecated. Signed-off-by: Daniele Buono --- .gitlab-ci.yml | 93 ++ 1 file changed, 93 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 814f51873f..15641abe63 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -483,6 +483,99 @@ clang-user: --extra-cflags=-fsanitize=undefined --extra-cflags=-fno-sanitize-recover=undefined MAKE_CHECK_ARGS: check-unit check-tcg +# Set LD_JOBS=1 because this requires LTO and ld consumes a large amount of memory. +# On gitlab runners, default value sometimes end up calling 2 lds concurrently and +# triggers an Out-Of-Memory error +# +# Since slirp callbacks are used in QEMU Timers, slirp needs to be compiled together +# with QEMU and linked as a static library to avoid false positives in CFI checks. +# This can be accomplished by using -enable-slirp=git, which avoids the use of +# a system-wide version of the library +# +# Split in two sets of build/check/acceptance because a single build job for every +# target creates an artifact archive too big to be uploaded +build-cfi-set1: + <<: *native_build_job_definition + needs: + - job: amd64-fedora-container + variables: +LD_JOBS: 1 +AR: llvm-ar +IMAGE: fedora +CONFIGURE_ARGS: --cc=clang --cxx=clang++ --enable-cfi --enable-cfi-debug + --enable-safe-stack --enable-slirp=git +TARGETS: aarch64-softmmu arm-softmmu alpha-softmmu i386-softmmu ppc-softmmu + ppc64-softmmu riscv32-softmmu riscv64-softmmu s390x-softmmu sparc-softmmu + sparc64-softmmu x86_64-softmmu + aarch64-linux-user aarch64_be-linux-user arm-linux-user i386-linux-user + ppc64-linux-user ppc64le-linux-user s390x-linux-user x86_64-linux-user +MAKE_CHECK_ARGS: check-build + timeout: 2h 30m + artifacts: +expire_in: 2 days +paths: + - build + +check-cfi-set1: + <<: *native_test_job_definition + needs: +- job: build-cfi-set1 + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check + +acceptance-cfi-set1: + <<: *native_test_job_definition + needs: +- job: build-cfi-set1 + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check-acceptance + <<: *acceptance_definition + +build-cfi-set2: + <<: *native_build_job_definition + needs: + - job: amd64-fedora-container + variables: +LD_JOBS: 1 +AR: llvm-ar +IMAGE: fedora +CONFIGURE_ARGS: --cc=clang --cxx=clang++ --enable-cfi --enable-cfi-debug + --enable-safe-stack --enable-slirp=git +TARGETS: avr-softmmu cris-softmmu hppa-softmmu m68k-softmmu + microblaze-softmmu microblazeel-softmmu mips-softmmu mips64-softmmu + mips64el-softmmu mipsel-softmmu moxie-softmmu nios2-softmmu or1k-softmmu + rx-softmmu sh4-softmmu sh4eb-softmmu tricore-softmmu xtensa-softmmu + xtensaeb-softmmu +MAKE_CHECK_ARGS: check-build + timeout: 2h 30m + artifacts: +expire_in: 2 days +paths: + - build + +check-cfi-set2: + <<: *native_test_job_definition + needs: +- job: build-cfi-set2 + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check + +acceptance-cfi-set2: + <<: *native_test_job_definition + needs: +- job: build-cfi-set2 + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check-acceptance + <<: *acceptance_definition + tsan-build: <<: *native_build_job_definition variables: -- 2.30.0
[PATCH v2 0/2] gitlab-ci.yml: Add jobs to test CFI
For a few months now QEMU has had options to enable compiler-based control-flow integrity if built with clang. While this feature has a low maintenance, It's probably still better to add tests to the CI environment to check that an update doesn't break it. The patchset allow gitlab testing of: * --enable-cfi: forward-edge cfi (function pointers) * --enable-safe-stack: backward-edge cfi (return pointers) As an added benefit, this also inherently tests LTO. The first patch allows a custom selection for linker parallelism. Currently, make parallelism at build time is based on the number of cpus available. This doesn't work well with LTO at linking, because the linker has to load in memory all the intermediate object files for optimization. If the gitlab runner happens to run two linking processes at the same time, the job will fail with an out-of-memory error, The patch leverages the ability to maintain high parallelism at compile time, but limit the number of linkers executed in parallel. The second patch introduces the ci/cd jobs in the gitlab pipeline. My original intention was to create a single chain of build -> check -> acceptance, with all the targets compiled by default. Unfortunately, the resulting artifact is too big and won't be uploaded. So I split the test in two chains, that should cover all non-deprecated targets as of today. Build jobs are on the longer side (about 2h and 20m), but I thought it would be better to just have 6 large jobs than tens of smaller ones. For build, we have to select --enable-slirp=git, because CFI needs a statically linked version of slirp, with CFI information. More info on this can be found in a comment in .gitlab-ci.yml. Test runs of the full pipeline are here (cfi-ci-v2 branch): https://gitlab.com/dbuono/qemu/-/pipelines/261311362 v2: - More details in the code about the issue of using system-wide slirp - Use meson to only limit linker parallelism instead of forcing no parallelism on the whole compilation process. Daniele Buono (2): gitlab-ci.yml: Allow custom # of parallel linkers gitlab-ci.yml: Add jobs to test CFI flags .gitlab-ci.yml | 97 ++ 1 file changed, 97 insertions(+) -- 2.30.0
Re: [PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism
On 2/24/2021 2:44 AM, Paolo Bonzini wrote: On 23/02/21 20:34, Daniele Buono wrote: This works, but setting this value to 1 for everybody seems a bit too restrictive. While the gitlab ci runners don't have enough memory for this, that's not necessarily true for every build platform, and linking multiple targets in parallel with LTO can result in a big save in time, so I'd prefer a customizable way. How about adding a flag `--max-ld-procs` to configure to manually set backend_max_links? Another possibility is to invoke "meson configure build -Dbackend_max_links=1" after configure. I like this, I'll send a v2 soon where I replace this patch with one just for linking. Daniele
Re: [PATCH 2/2] gitlab-ci.yml: Add jobs to test CFI flags
On 2/23/2021 3:11 AM, Paolo Bonzini wrote: On 23/02/21 00:01, Daniele Buono wrote: +# Set JOBS=1 because this requires LTO and ld consumes a large amount of memory. +# On gitlab runners, default JOBS of 2 sometimes end up calling 2 lds concurrently +# and triggers an Out-Of-Memory error Does it make sense to test only one target instead? I'd prefer grouping multiple targets per job so that the number of jobs doesn't explode, and stopping ninja from linking in parallel does solve the issue. There's also the issue that tests are also compiled here so you may end up with two linkers anyway. However the chance that this will end up in an out-of-memory error is quite smaller (possibly zero) since tests don't link that many object files together. +# Because of how slirp is used in QEMU, we need to have CFI also on libslirp. +# System-wide version in fedora is not compiled with CFI so we recompile it using +# -enable-slirp=git Can you explain what you mean, and perhaps add a check or warning for incompatible settings? Certainly. The issue here is that there is a function in libslirp that is used as callbacks for QEMU Timers: ra_timer_handler (There may be others, but of this one I'm sure because I traced it). This is not an issue when you compile slirp with qemu, since the whole library now has CFI informations and is statically linked in the QEMU binary. It becomes an issue if you are dynamically linking a system-wide libslirp, as it happens on Fedora. I'd be happy to add a check on configure/meson that ends the configure step with an error when this happens, but that would technically be an independent patch that I'd work on in parallel to this one. I would prefer to not automatically select the git-based libslirp because that may go unnoticed when configuring. Paolo
Re: [PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism
This works, but setting this value to 1 for everybody seems a bit too restrictive. While the gitlab ci runners don't have enough memory for this, that's not necessarily true for every build platform, and linking multiple targets in parallel with LTO can result in a big save in time, so I'd prefer a customizable way. How about adding a flag `--max-ld-procs` to configure to manually set backend_max_links? This would also allow setting it up to any specific number above 1, which looking at the Makefile seems to not be possible now: because of how the -j flag is passed from make to ninja, a compilation is either sequential or parallel based on #cpus On 2/23/2021 3:12 AM, Paolo Bonzini wrote: On 23/02/21 00:01, Daniele Buono wrote: Currently, make parallelism at build time is defined as #cpus+1. Some build jobs may need (or benefit from) a different number. An example is builds with LTO where, because of the huge demand of memory at link time, gitlab runners fails if two linkers are run concurrently This patch retains the default value of #cpus+1 but allows setting the "JOBS" variable to a different number when applying the template As I just found out, you can add -Dbackend_max_links=1 to the meson command line instead if LTO is enabled. Paolo
[PATCH 0/2] gitlab-ci.yml: Add jobs to test CFI
For a few months now QEMU has had options to enable compiler-based control-flow integrity if built with clang. While this feature has a low maintenance, It's probably still better to add tests to the CI environment to check that an update doesn't break it. As an added benefit, this also inherently tests LTO. The patch allow gitlab testing of: * --enable-cfi: forward-edge cfi (function pointers) * --enable-safe-stack: backward-edge cfi (return pointers) My original intention was to create a single chain of build -> check -> acceptance, with all the targets compiled by default. Unfortunately, the resulting artifact is too big and won't be uploaded. So I split the test in two chains, that should cover all non-deprecated targets as of today. I also had to add a small patch to allow a custom selection for make parallelism. This is because the gitlab runner nodes only have ~3.5GB of ram, and with the default parallelism (2), in some cases two ld instances will start working on two binaries and exaust the memory. By only forcing one make job at a time, this is avoided. Test runs of the full pipeline are here (cfi-ci branch): https://gitlab.com/dbuono/qemu/-/pipelines/259931154 Daniele Buono (2): gitlab-ci.yml: Allow custom make parallelism gitlab-ci.yml: Add jobs to test CFI flags .gitlab-ci.yml | 94 +- 1 file changed, 93 insertions(+), 1 deletion(-) -- 2.30.0
[PATCH 2/2] gitlab-ci.yml: Add jobs to test CFI flags
QEMU has had options to enable control-flow integrity features for a few months now. Add two sets of build/check/acceptance jobs to ensure the binary produced is working fine. The two sets allow testing of x86_64 binaries for every target that is not deprecated. Signed-off-by: Daniele Buono --- .gitlab-ci.yml | 92 ++ 1 file changed, 92 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5c198f05d4..f2fea8e2eb 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -479,6 +479,98 @@ clang-user: --extra-cflags=-fsanitize=undefined --extra-cflags=-fno-sanitize-recover=undefined MAKE_CHECK_ARGS: check-unit check-tcg +# Set JOBS=1 because this requires LTO and ld consumes a large amount of memory. +# On gitlab runners, default JOBS of 2 sometimes end up calling 2 lds concurrently +# and triggers an Out-Of-Memory error +# +# Because of how slirp is used in QEMU, we need to have CFI also on libslirp. +# System-wide version in fedora is not compiled with CFI so we recompile it using +# -enable-slirp=git +# +# Split in two sets of build/check/acceptance because a single build job for every +# target creates an artifact archive too big to be uploaded +build-cfi-set1: + <<: *native_build_job_definition + needs: + - job: amd64-fedora-container + variables: +JOBS: 1 +AR: llvm-ar +IMAGE: fedora +CONFIGURE_ARGS: --cc=clang --cxx=clang++ --enable-cfi --enable-cfi-debug + --enable-safe-stack --enable-slirp=git +TARGETS: aarch64-softmmu arm-softmmu alpha-softmmu i386-softmmu ppc-softmmu + ppc64-softmmu riscv32-softmmu riscv64-softmmu s390x-softmmu sparc-softmmu + sparc64-softmmu x86_64-softmmu + aarch64-linux-user aarch64_be-linux-user arm-linux-user i386-linux-user + ppc64-linux-user ppc64le-linux-user s390x-linux-user x86_64-linux-user +MAKE_CHECK_ARGS: check-build + timeout: 3h + artifacts: +expire_in: 2 days +paths: + - build + +check-cfi-set1: + <<: *native_test_job_definition + needs: +- job: build-cfi-set1 + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check + +acceptance-cfi-set1: + <<: *native_test_job_definition + needs: +- job: build-cfi-set1 + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check-acceptance + <<: *acceptance_definition + +build-cfi-set2: + <<: *native_build_job_definition + needs: + - job: amd64-fedora-container + variables: +JOBS: 1 +AR: llvm-ar +IMAGE: fedora +CONFIGURE_ARGS: --cc=clang --cxx=clang++ --enable-cfi --enable-cfi-debug + --enable-safe-stack --enable-slirp=git +TARGETS: avr-softmmu cris-softmmu hppa-softmmu m68k-softmmu + microblaze-softmmu microblazeel-softmmu mips-softmmu mips64-softmmu + mips64el-softmmu mipsel-softmmu moxie-softmmu nios2-softmmu or1k-softmmu + rx-softmmu sh4-softmmu sh4eb-softmmu tricore-softmmu xtensa-softmmu + xtensaeb-softmmu +MAKE_CHECK_ARGS: check-build + timeout: 3h + artifacts: +expire_in: 2 days +paths: + - build + +check-cfi-set2: + <<: *native_test_job_definition + needs: +- job: build-cfi-set2 + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check + +acceptance-cfi-set2: + <<: *native_test_job_definition + needs: +- job: build-cfi-set2 + artifacts: true + variables: +IMAGE: fedora +MAKE_CHECK_ARGS: check-acceptance + <<: *acceptance_definition + tsan-build: <<: *native_build_job_definition variables: -- 2.30.0
[PATCH 1/2] gitlab-ci.yml: Allow custom make parallelism
Currently, make parallelism at build time is defined as #cpus+1.
Some build jobs may need (or benefit from) a different number.
An example is builds with LTO where, because of the huge demand
of memory at link time, gitlab runners fails if two linkers are
run concurrently
This patch retains the default value of #cpus+1 but allows setting
the "JOBS" variable to a different number when applying the template
Signed-off-by: Daniele Buono
---
.gitlab-ci.yml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 8b6d495288..5c198f05d4 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -17,7 +17,7 @@ include:
stage: build
image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
before_script:
-- JOBS=$(expr $(nproc) + 1)
+- JOBS=${JOBS:-$(expr $(nproc) + 1)}
script:
- mkdir build
- cd build
--
2.30.0
Re: [PATCH 0/1] tests/acceptance/boot_linux: Switch to Fedora 32
On 1/28/2021 3:19 PM, Wainer dos Santos Moschetta wrote: Hi, On 1/26/21 10:09 PM, Daniele Buono wrote: Local acceptance tests run with "make check-acceptance" are now showing some cases canceled like the following: (01/39) tests/acceptance/boot_linux.py:BootLinuxX8664.test_pc_i440fx_tcg: CANCEL: Failed to download/prepare boot image (0.25 s) Turns out, every full-vm test in boot_linux.py is trying to use a Fedora 31 cloud image and is failing, with Avocado refusing to download it, presumably because Fedora 31 is EOL. This patch moves to Fedora 32, which is still supported. And seem to work fine While ago it was discussed about updating the Fedora version which, in my opinion, ended up without a conclusion. Please see the complete thread in: https://www.mail-archive.com/qemu-devel@nongnu.org/msg763986.html Oops, didn't notice the previous thread. Apologies for the duplicate! I'm CC'ing Daniel Berrrangé so that, perhaps, we could resume the discussion. Thanks! - Wainer The script has the image checksums hardcoded. I downloaded and verified the checksums with the Fedora 32 GPG key, but I would feel more confident if someone else wants to verify it too. The checksum files are here: https://download-ib01.fedoraproject.org/pub/fedora-secondary/releases/32/Cloud/ppc64le/images/Fedora-Cloud-32-1.6-ppc64le-CHECKSUM https://download-ib01.fedoraproject.org/pub/fedora-secondary/releases/32/Cloud/s390x/images/Fedora-Cloud-32-1.6-s390x-CHECKSUM https://download-ib01.fedoraproject.org/pub/fedora/linux/releases/32/Cloud/aarch64/images/Fedora-Cloud-32-1.6-aarch64-CHECKSUM https://download-ib01.fedoraproject.org/pub/fedora/linux/releases/32/Cloud/x86_64/images/Fedora-Cloud-32-1.6-x86_64-CHECKSUM and the GPG keys are available here: https://getfedora.org/en/security/ NOTE: I tried moving to Fedora 33, but the aarch64 VM cannot boot properly. May be worth investigating but I have no experience with ARM so I'll leave that to someone else, if interested. Daniele Buono (1): tests/acceptance/boot_linux: Switch to Fedora 32 tests/acceptance/boot_linux.py | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-)
[PATCH 0/1] tests/acceptance/boot_linux: Switch to Fedora 32
Local acceptance tests run with "make check-acceptance" are now showing some cases canceled like the following: (01/39) tests/acceptance/boot_linux.py:BootLinuxX8664.test_pc_i440fx_tcg: CANCEL: Failed to download/prepare boot image (0.25 s) Turns out, every full-vm test in boot_linux.py is trying to use a Fedora 31 cloud image and is failing, with Avocado refusing to download it, presumably because Fedora 31 is EOL. This patch moves to Fedora 32, which is still supported. And seem to work fine The script has the image checksums hardcoded. I downloaded and verified the checksums with the Fedora 32 GPG key, but I would feel more confident if someone else wants to verify it too. The checksum files are here: https://download-ib01.fedoraproject.org/pub/fedora-secondary/releases/32/Cloud/ppc64le/images/Fedora-Cloud-32-1.6-ppc64le-CHECKSUM https://download-ib01.fedoraproject.org/pub/fedora-secondary/releases/32/Cloud/s390x/images/Fedora-Cloud-32-1.6-s390x-CHECKSUM https://download-ib01.fedoraproject.org/pub/fedora/linux/releases/32/Cloud/aarch64/images/Fedora-Cloud-32-1.6-aarch64-CHECKSUM https://download-ib01.fedoraproject.org/pub/fedora/linux/releases/32/Cloud/x86_64/images/Fedora-Cloud-32-1.6-x86_64-CHECKSUM and the GPG keys are available here: https://getfedora.org/en/security/ NOTE: I tried moving to Fedora 33, but the aarch64 VM cannot boot properly. May be worth investigating but I have no experience with ARM so I'll leave that to someone else, if interested. Daniele Buono (1): tests/acceptance/boot_linux: Switch to Fedora 32 tests/acceptance/boot_linux.py | 11 +-- 1 file changed, 5 insertions(+), 6 deletions(-) -- 2.30.0
[PATCH 1/1] tests/acceptance/boot_linux: Switch to Fedora 32
Some acceptance test require a full VM image.
The tests were using Fedora 31, which is now EOL.
Probably as a consequence, the Avocado framework is not
able to download such images anymore.
Switch to Fedora 32, which is still supported.
Signed-off-by: Daniele Buono
---
tests/acceptance/boot_linux.py | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index 1da4a53d6a..b954f95f0b 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -48,7 +48,7 @@ class BootLinuxBase(Test):
image_arch = 'ppc64le'
try:
boot = vmimage.get(
-'fedora', arch=image_arch, version='31',
+'fedora', arch=image_arch, version='32',
checksum=self.chksum,
algorithm='sha256',
cache_dir=self.cache_dirs[0],
@@ -111,7 +111,7 @@ class BootLinuxX8664(BootLinux):
:avocado: tags=arch:x86_64
"""
-chksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
+chksum = '423a4ce32fa32c50c11e3d3ff392db97a762533b81bef9d00599de518a7469c8'
def test_pc_i440fx_tcg(self):
"""
@@ -160,8 +160,7 @@ class BootLinuxAarch64(BootLinux):
:avocado: tags=machine:virt
:avocado: tags=machine:gic-version=2
"""
-
-chksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
+chksum = 'b367755c664a2d7a26955bbfff985855adfa2ca15e908baf15b4b176d68d3967'
def add_common_args(self):
self.vm.add_args('-bios',
@@ -217,7 +216,7 @@ class BootLinuxPPC64(BootLinux):
:avocado: tags=arch:ppc64
"""
-chksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
+chksum = 'dd989a078d641713c55720ba3e4320b204ade6954e2bfe4570c8058dc36e2e5d'
def test_pseries_tcg(self):
"""
@@ -235,7 +234,7 @@ class BootLinuxS390X(BootLinux):
:avocado: tags=arch:s390x
"""
-chksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
+chksum = '93e49b98fa016797a6864a95cbb7beaec86ffd61dbcd42a951158ae732dae1ec'
@skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
def test_s390_ccw_virtio_tcg(self):
--
2.30.0
Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus
On 1/14/2021 3:17 AM, Marc-André Lureau wrote:
Any update on this patch?
thanks
Hi,
I had been waiting for a feedback on my previous message.
I don't know the UAS subsystem that well, but can't find where the
variable-sized field that is causing the issue is actually used.
If it helps, I can send an RFC for converting
struct UASStatus {
uint32_t stream;
uas_iustatus;
uint32_t length;
QTAILQ_ENTRY(UASStatus) next;
};
to
struct UASStatus {
uint32_t stream;
uas_iu*status;
uint32_t length;
QTAILQ_ENTRY(UASStatus) next;
};
And discuss it at that point.
Thanks,
Daniele
Re: [PATCH] decodetree: Open files with encoding='utf-8'
I had a similar issue in the past with the acceptance tests. Some VMs send UTF-8 output in their console and the acceptance test script would bail out if the locale was not UTF-8. I sent a patch on the ml but it probably got lost: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg06086.html I can re-spin it if you guys are interested On 1/8/2021 10:16 AM, Philippe Mathieu-Daudé wrote: When decodetree.py was added in commit 568ae7efae7, QEMU was using Python 2 which happily reads UTF-8 files in text mode. Python 3 requires either UTF-8 locale or an explicit encoding passed to open(). Now that Python 3 is required, explicit UTF-8 encoding for decodetree sources. This fixes: $ /usr/bin/python3 scripts/decodetree.py test.decode Traceback (most recent call last): File "scripts/decodetree.py", line 1397, in main() File "scripts/decodetree.py", line 1308, in main parse_file(f, toppat) File "scripts/decodetree.py", line 994, in parse_file for line in f: File "/usr/lib/python3.6/encodings/ascii.py", line 26, in decode return codecs.ascii_decode(input, self.errors)[0] UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 80: ordinal not in range(128) Reported-by: Peter Maydell Signed-off-by: Philippe Mathieu-Daudé --- scripts/decodetree.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/decodetree.py b/scripts/decodetree.py index 47aa9caf6d1..fa40903cff1 100644 --- a/scripts/decodetree.py +++ b/scripts/decodetree.py @@ -1304,7 +1304,7 @@ def main(): for filename in args: input_file = filename -f = open(filename, 'r') +f = open(filename, 'r', encoding='utf-8') parse_file(f, toppat) f.close()
[PATCH v4 4/5] configure,meson: support Control-Flow Integrity
This patch adds a flag to enable/disable control flow integrity checks
on indirect function calls.
This feature only allows indirect function calls at runtime to functions
with compatible signatures.
This feature is only provided by LLVM/Clang, and depends on link-time
optimization which is currently supported only with LLVM/Clang >= 6.0
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.
All the checks are performed in meson.build. configure is only used to
forward the flags to meson
Signed-off-by: Daniele Buono
---
configure | 21 -
meson.build | 45 +
meson_options.txt | 4
3 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/configure b/configure
index fee118518b..c4e5d92167 100755
--- a/configure
+++ b/configure
@@ -400,6 +400,8 @@ coroutine=""
coroutine_pool=""
debug_stack_usage="no"
crypto_afalg="no"
+cfi="disabled"
+cfi_debug="disabled"
seccomp=""
glusterfs=""
glusterfs_xlator_opt="no"
@@ -1180,6 +1182,16 @@ for opt do
;;
--disable-safe-stack) safe_stack="no"
;;
+ --enable-cfi)
+ cfi="enabled";
+ lto="true";
+ ;;
+ --disable-cfi) cfi="disabled"
+ ;;
+ --enable-cfi-debug) cfi_debug="enabled"
+ ;;
+ --disable-cfi-debug) cfi_debug="disabled"
+ ;;
--disable-curses) curses="disabled"
;;
--enable-curses) curses="enabled"
@@ -1760,6 +1772,13 @@ disabled with --disable-FEATURE, default is enabled if
available:
sparse sparse checker
safe-stack SafeStack Stack Smash Protection. Depends on
clang/llvm >= 3.7 and requires coroutine backend ucontext.
+ cfi Enable Control-Flow Integrity for indirect function calls.
+ In case of a cfi violation, QEMU is terminated with SIGILL
+ Depends on lto and is incompatible with modules
+ Automatically enables Link-Time Optimization (lto)
+ cfi-debug In case of a cfi violation, a message containing the line
that
+ triggered the error is written to stderr. After the error,
+ QEMU is still terminated with SIGILL
gnutls GNUTLS cryptography support
nettle nettle cryptography support
@@ -7020,7 +7039,7 @@ NINJA=$ninja $meson setup \
-Diconv=$iconv -Dcurses=$curses -Dlibudev=$libudev\
-Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
-Dvhost_user_blk_server=$vhost_user_blk_server \
--Db_lto=$lto \
+-Db_lto=$lto -Dcfi=$cfi -Dcfi_debug=$cfi_debug \
$cross_arg \
"$PWD" "$source_path"
diff --git a/meson.build b/meson.build
index ebd1c690e0..e1ae6521e0 100644
--- a/meson.build
+++ b/meson.build
@@ -773,6 +773,48 @@ elif get_option('vhost_user_blk_server').disabled() or not
have_system
have_vhost_user_blk_server = false
endif
+if get_option('cfi').enabled()
+ cfi_flags=[]
+ # Check for dependency on LTO
+ if not get_option('b_lto')
+error('Selected Control-Flow Integrity but LTO is disabled')
+ endif
+ if config_host.has_key('CONFIG_MODULES')
+error('Selected Control-Flow Integrity is not compatible with modules')
+ endif
+ # Check for cfi flags. CFI requires LTO so we can't use
+ # get_supported_arguments, but need a more complex "compiles" which allows
+ # custom arguments
+ if cc.compiles('int main () { return 0; }', name: '-fsanitize=cfi-icall',
+ args: ['-flto', '-fsanitize=cfi-icall'] )
+cfi_flags += '-fsanitize=cfi-icall'
+ else
+error('-fsanitize=cfi-icall is not supported by the compiler')
+ endif
+ if cc.compiles('int main () { return 0; }',
+ name: '-fsanitize-cfi-icall-generalize-pointers',
+ args: ['-flto', '-fsanitize=cfi-icall',
+'-fsanitize-cfi-icall-generalize-pointers'] )
+cfi_flags += '-fsanitize-cfi-icall-generalize-pointers'
+ else
+error('-fsanitize-cfi-icall-generalize-pointers is not supported by the
compiler')
+ endif
+ if get_option('cfi_debug').enabled()
+if cc.compiles('int main () { return 0; }',
+ name: '-fno-sanitize-trap=cfi-icall',
+ args: ['-flto', '-fsanitize=cfi-icall',
+ '-fno-sanitize-trap=cfi-icall'] )
+ cfi_flags += '-fno-sanitize-trap=cfi-icall'
+else
+ error('-fno-sanitize-trap=cfi-icall is not supported by the compiler')
+endif
+ endif
+ add_project_
[PATCH v4 5/5] docs: Add CFI Documentation
Document how to compile with CFI and how to maintain CFI-safe code Signed-off-by: Daniele Buono --- docs/devel/control-flow-integrity.rst | 137 ++ 1 file changed, 137 insertions(+) create mode 100644 docs/devel/control-flow-integrity.rst diff --git a/docs/devel/control-flow-integrity.rst b/docs/devel/control-flow-integrity.rst new file mode 100644 index 00..ec54d16a42 --- /dev/null +++ b/docs/devel/control-flow-integrity.rst @@ -0,0 +1,137 @@ + +Control-Flow Integrity (CFI) + + +This document describes the current control-flow integrity (CFI) mechanism in +QEMU. How it can be enabled, its benefits and deficiencies, and how it affects +new and existing code in QEMU + +Basics +-- + +CFI is a hardening technique that focusing on guaranteeing that indirect +function calls have not been altered by an attacker. +The type used in QEMU is a forward-edge control-flow integrity that ensures +function calls performed through function pointers, always call a "compatible" +function. A compatible function is a function with the same signature of the +function pointer declared in the source code. + +This type of CFI is entirely compiler-based and relies on the compiler knowing +the signature of every function and every function pointer used in the code. +As of now, the only compiler that provides support for CFI is Clang. + +CFI is best used on production binaries, to protect against unknown attack +vectors. + +In case of a CFI violation (i.e. call to a non-compatible function) QEMU will +terminate abruptly, to stop the possible attack. + +Building with CFI +- + +NOTE: CFI requires the use of link-time optimization. Therefore, when CFI is +selected, LTO will be automatically enabled. + +To build with CFI, the minimum requirement is Clang 6+. If you +are planning to also enable fuzzing, then Clang 11+ is needed (more on this +later). + +Given the use of LTO, a version of AR that supports LLVM IR is required. +The easies way of doing this is by selecting the AR provided by LLVM:: + + AR=llvm-ar-9 CC=clang-9 CXX=lang++-9 /path/to/configure --enable-cfi + +CFI is enabled on every binary produced. + +If desired, an additional flag to increase the verbosity of the output in case +of a CFI violation is offered (``--enable-debug-cfi``). + +Using QEMU built with CFI +- + +A binary with CFI will work exactly like a standard binary. In case of a CFI +violation, the binary will terminate with an illegal instruction signal. + +Incompatible code with CFI +-- + +As mentioned above, CFI is entirely compiler-based and therefore relies on +compile-time knowledge of the code. This means that, while generally supported +for most code, some specific use pattern can break CFI compatibility, and +create false-positives. The two main patterns that can cause issues are: + +* Just-in-time compiled code: since such code is created at runtime, the jump + to the buffer containing JIT code will fail. + +* Libraries loaded dynamically, e.g. with dlopen/dlsym, since the library was + not known at compile time. + +Current areas of QEMU that are not entirely compatible with CFI are: + +1. TCG, since the idea of TCG is to pre-compile groups of instructions at + runtime to speed-up interpretation, quite similarly to a JIT compiler + +2. TCI, where the interpreter has to interpret the generic *call* operation + +3. Plugins, since a plugin is implemented as an external library + +4. Modules, since they are implemented as an external library + +5. Directly calling signal handlers from the QEMU source code, since the + signal handler may have been provided by an external library or even plugged + at runtime. + +Disabling CFI for a specific function +- + +If you are working on function that is performing a call using an +incompatible way, as described before, you can selectively disable CFI checks +for such function by using the decorator ``QEMU_DISABLE_CFI`` at function +definition, and add an explanation on why the function is not compatible +with CFI. An example of the use of ``QEMU_DISABLE_CFI`` is provided here:: + + /* +* Disable CFI checks. +* TCG creates binary blobs at runtime, with the transformed code. +* A TB is a blob of binary code, created at runtime and called with an +* indirect function call. Since such function did not exist at compile time, +* the CFI runtime has no way to verify its signature and would fail. +* TCG is not considered a security-sensitive part of QEMU so this does not +* affect the impact of CFI in environment with high security requirements +*/ + QEMU_DISABLE_CFI + static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock *itb) + +NOTE: CFI needs to be disabled at the **caller** function, (i.e. a compa
[PATCH v4 1/5] configure,meson: add option to enable LTO
This patch allows to compile QEMU with link-time optimization (LTO).
Compilation with LTO is handled directly by meson. This patch only
adds the option in configure and forwards the request to meson
Tested with all major versions of clang from 6 to 12
Signed-off-by: Daniele Buono
---
configure | 7 +++
meson.build | 1 +
2 files changed, 8 insertions(+)
diff --git a/configure b/configure
index 18c26e0389..fee118518b 100755
--- a/configure
+++ b/configure
@@ -242,6 +242,7 @@ host_cc="cc"
audio_win_int=""
libs_qga=""
debug_info="yes"
+lto="false"
stack_protector=""
safe_stack=""
use_containers="yes"
@@ -1167,6 +1168,10 @@ for opt do
;;
--disable-werror) werror="no"
;;
+ --enable-lto) lto="true"
+ ;;
+ --disable-lto) lto="false"
+ ;;
--enable-stack-protector) stack_protector="yes"
;;
--disable-stack-protector) stack_protector="no"
@@ -1751,6 +1756,7 @@ disabled with --disable-FEATURE, default is enabled if
available:
module-upgrades try to load modules from alternate paths for upgrades
debug-tcg TCG debugging (default is disabled)
debug-info debugging information
+ lto Enable Link-Time Optimization.
sparse sparse checker
safe-stack SafeStack Stack Smash Protection. Depends on
clang/llvm >= 3.7 and requires coroutine backend ucontext.
@@ -7014,6 +7020,7 @@ NINJA=$ninja $meson setup \
-Diconv=$iconv -Dcurses=$curses -Dlibudev=$libudev\
-Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
-Dvhost_user_blk_server=$vhost_user_blk_server \
+-Db_lto=$lto \
$cross_arg \
"$PWD" "$source_path"
diff --git a/meson.build b/meson.build
index e3386196ba..ebd1c690e0 100644
--- a/meson.build
+++ b/meson.build
@@ -2044,6 +2044,7 @@ summary_info += {'gprof enabled':
config_host.has_key('CONFIG_GPROF')}
summary_info += {'sparse enabled':sparse.found()}
summary_info += {'strip binaries':get_option('strip')}
summary_info += {'profiler': config_host.has_key('CONFIG_PROFILER')}
+summary_info += {'link-time optimization (LTO)': get_option('b_lto')}
summary_info += {'static build': config_host.has_key('CONFIG_STATIC')}
if targetos == 'darwin'
summary_info += {'Cocoa support': config_host.has_key('CONFIG_COCOA')}
--
2.17.1
[PATCH v4 3/5] check-block: enable iotests with cfi-icall
cfi-icall is a form of Control-Flow Integrity for indirect function
calls implemented by llvm. It is enabled with a -fsanitize flag.
iotests are currently disabled when -fsanitize options is used, with the
exception of SafeStack.
This patch implements a generic filtering mechanism to allow iotests
with a set of known-to-be-safe -fsanitize option. Then marks SafeStack
and the new options used for cfi-icall safe for iotests
Signed-off-by: Daniele Buono
---
tests/check-block.sh | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/tests/check-block.sh b/tests/check-block.sh
index f6b1bda7b9..fb4c1baae9 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -21,14 +21,18 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ;
then
exit 0
fi
-# Disable tests with any sanitizer except for SafeStack
-CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
-SANITIZE_FLAGS=""
-#Remove all occurrencies of -fsanitize=safe-stack
-for i in ${CFLAGS}; do
-if [ "${i}" != "-fsanitize=safe-stack" ]; then
-SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
+# Disable tests with any sanitizer except for specific ones
+SANITIZE_FLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
+ALLOWED_SANITIZE_FLAGS="safe-stack cfi-icall"
+#Remove all occurrencies of allowed Sanitize flags
+for j in ${ALLOWED_SANITIZE_FLAGS}; do
+TMP_FLAGS=${SANITIZE_FLAGS}
+SANITIZE_FLAGS=""
+for i in ${TMP_FLAGS}; do
+if ! echo ${i} | grep -q "${j}" 2>/dev/null; then
+SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
fi
+done
done
if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
# Have a sanitize flag that is not allowed, stop
--
2.17.1
[PATCH v4 2/5] cfi: Initial support for cfi-icall in QEMU
LLVM/Clang, supports runtime checks for forward-edge Control-Flow
Integrity (CFI).
CFI on indirect function calls (cfi-icall) ensures that, in indirect
function calls, the function called is of the right signature for the
pointer type defined at compile time.
For this check to work, the code must always respect the function
signature when using function pointer, the function must be defined
at compile time, and be compiled with link-time optimization.
This rules out, for example, shared libraries that are dynamically loaded
(given that functions are not known at compile time), and code that is
dynamically generated at run-time.
This patch:
1) Introduces the CONFIG_CFI flag to support cfi in QEMU
2) Introduces a decorator to allow the definition of "sensitive"
functions, where a non-instrumented function may be called at runtime
through a pointer. The decorator will take care of disabling cfi-icall
checks on such functions, when cfi is enabled.
3) Marks functions currently in QEMU that exhibit such behavior,
in particular:
- The function in TCG that calls pre-compiled TBs
- The function in TCI that interprets instructions
- Functions in the plugin infrastructures that jump to callbacks
- Functions in util that directly call a signal handler
Signed-off-by: Daniele Buono
Acked-by: Alex Bennée env_ptr;
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index c76281f354..c87c242063 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -243,4 +243,16 @@ extern void QEMU_NORETURN QEMU_ERROR("code path is
reachable")
#define qemu_build_not_reached() g_assert_not_reached()
#endif
+#ifdef CONFIG_CFI
+/*
+ * If CFI is enabled, use an attribute to disable cfi-icall on the following
+ * function
+ */
+#define QEMU_DISABLE_CFI __attribute__((no_sanitize("cfi-icall")))
+#else
+/* If CFI is not enabled, use an empty define to not change the behavior */
+#define QEMU_DISABLE_CFI
+#endif
+
+
#endif /* COMPILER_H */
diff --git a/plugins/core.c b/plugins/core.c
index 51bfc94787..87b823bbc4 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -31,6 +31,7 @@
#include "tcg/tcg-op.h"
#include "trace/mem-internal.h" /* mem_info macros */
#include "plugin.h"
+#include "qemu/compiler.h"
struct qemu_plugin_cb {
struct qemu_plugin_ctx *ctx;
@@ -90,6 +91,12 @@ void plugin_unregister_cb__locked(struct qemu_plugin_ctx
*ctx,
}
}
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
static void plugin_vcpu_cb__simple(CPUState *cpu, enum qemu_plugin_event ev)
{
struct qemu_plugin_cb *cb, *next;
@@ -111,6 +118,12 @@ static void plugin_vcpu_cb__simple(CPUState *cpu, enum
qemu_plugin_event ev)
}
}
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
static void plugin_cb__simple(enum qemu_plugin_event ev)
{
struct qemu_plugin_cb *cb, *next;
@@ -128,6 +141,12 @@ static void plugin_cb__simple(enum qemu_plugin_event ev)
}
}
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
static void plugin_cb__udata(enum qemu_plugin_event ev)
{
struct qemu_plugin_cb *cb, *next;
@@ -325,6 +344,12 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
dyn_cb->f.generic = cb;
}
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
void qemu_plugin_tb_trans_cb(CPUState *cpu, struct qemu_plugin_tb *tb)
{
struct qemu_plugin_cb *cb, *next;
@@ -339,6 +364,12 @@ void qemu_plugin_tb_trans_cb(CPUState *cpu, struct
qemu_plugin_tb *tb)
}
}
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
void
qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1, uint64_t a2,
uint64_t a3, uint64_t a4, uint64_t a5,
@@ -358,6 +389,12 @@ qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num,
uint64_t a1, uint64_t a2,
}
}
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
void qemu_plugin_vcpu_syscall_ret(CPUState *cpu, int64_t num, int64_t ret)
{
struct qemu_plugin_cb *cb, *next;
diff --git a/plugins/loader.c b/plugins/loader.c
index 8ac5dbc20f..fd491961de 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -32,6 +32,7 @@
#ifndef CONFIG_USER_ONLY
#include "hw/boards.h"
#endif
+#include "qemu/compiler.h"
#include "plugin.h"
@@ -150,6 +151,12 @@ static uint64_t xorshift64star(uint64_t x)
[PATCH v4 0/5] Add support for Control-Flow Integrity
This patch adds supports for Control-Flow Integrity checks on indirect function calls. Requires the use of clang, and link-time optimizations Since it's been a month, and some of the patches are being merged independently, I thought of rebasing, retesting and sending an updated version. Also, added a documentation in docs/devel to explain CFI and how to handle CFI-sensitive code. Changes in v4: - Removed patches to avoid clang warnings, since they are being merged independently and are not really necessary for CFI - Added documentation in docs/devel to explain how to compile with CFI, and how to disable CFI for incompatible functions Changes in v3: - clang 11+ warnings are now handled directly at the source, instead of disabling specific warnings for the whole code. Some more work may be needed here to polish the patch, I would kindly ask for a review from the corresponding maintainers - Remove configure-time checks for toolchain compatibility with LTO. - the decorator to disable cfi checks on functions has been renamed and moved to include/qemu/compiler.h - configure-time checks for cfi support and dependencies has been moved from configure to meson Link to v3: https://www.mail-archive.com/qemu-devel@nongnu.org/msg757930.html Link to v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg753675.html Link to v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg718786.html Daniele Buono (5): configure,meson: add option to enable LTO cfi: Initial support for cfi-icall in QEMU check-block: enable iotests with cfi-icall configure,meson: support Control-Flow Integrity docs: Add CFI Documentation accel/tcg/cpu-exec.c | 11 +++ configure | 26 + docs/devel/control-flow-integrity.rst | 137 ++ include/qemu/compiler.h | 12 +++ meson.build | 46 + meson_options.txt | 4 + plugins/core.c| 37 +++ plugins/loader.c | 7 ++ tcg/tci.c | 7 ++ tests/check-block.sh | 18 ++-- util/main-loop.c | 11 +++ util/oslib-posix.c| 11 +++ 12 files changed, 320 insertions(+), 7 deletions(-) create mode 100644 docs/devel/control-flow-integrity.rst -- 2.17.1
Re: [PATCH v3 1/9] fuzz: Make fork_fuzz.ld compatible with LLVM's LLD
Thanks Alex,
do you think you could also give it a try linking with LLD?
just add --extra-ldflags="-fuse-ld=lld"
I do see some small differences when moving from BFD ro LLD, but they
should not be of importance. The position of the data.fuzz* is kept.
size -A on qemu-fuzz-i386, LTO DISABLED:
BFD
section size addr
[...]
.got10704 29849128
.data 1160800 29859840
__sancov_pcs 3362992 31020640
.data.fuzz_start 210187 34385920
.data.fuzz_ordered 211456 34596352
.bss 9659608 34807808
.comment 225 0
[...]
BFD
section size addr
[...]
.got 816 27824632
.got.plt 9992 27825448
.data 1160808 27839536
.data.fuzz_start 210187 29003776
.data.fuzz_ordered 211456 29214208
.data.fuzz_end 0 29425664
.tm_clone_table 0 29425664
__sancov_pcs 3362992 29425664
.bss 9659624 32788672
I tried running the fuzzer and didn't seem to have any issues, but I
haven't tried a test-build with OSS-Fuzz. Is there a info somewhere
on how to do that?
Thanks,
Daniele
On 11/6/2020 9:50 AM, Alexander Bulekov wrote:
On 201105 1718, Daniele Buono wrote:
LLVM's linker, LLD, supports the keyword "INSERT AFTER", starting with
version 11.
However, when multiple sections are defined in the same "INSERT AFTER",
they are added in a reversed order, compared to BFD's LD.
This patch makes fork_fuzz.ld generic enough to work with both linkers.
Each section now has its own "INSERT AFTER" keyword, so proper ordering is
defined between the sections added.
Hi Daniele,
Good to know that LLVM now has support for "INSERT AFTER" :)
I compared the resulting symbols between __FUZZ_COUNTERS_{START,END}
(after linking with BFD) before/after this patch, and they look good. I
also ran a test-build with OSS-Fuzz container and confirmed that the
resulting binary also had proper symbols.
Reviewed-by: Alexander Bulekov
Tested-by: Alexander Bulekov
Thanks
Signed-off-by: Daniele Buono
---
tests/qtest/fuzz/fork_fuzz.ld | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/fuzz/fork_fuzz.ld b/tests/qtest/fuzz/fork_fuzz.ld
index bfb667ed06..cfb88b7fdb 100644
--- a/tests/qtest/fuzz/fork_fuzz.ld
+++ b/tests/qtest/fuzz/fork_fuzz.ld
@@ -16,6 +16,11 @@ SECTIONS
/* Lowest stack counter */
*(__sancov_lowest_stack);
}
+}
+INSERT AFTER .data;
+
+SECTIONS
+{
.data.fuzz_ordered :
{
/*
@@ -34,6 +39,11 @@ SECTIONS
*/
*(.bss._ZN6fuzzer3TPCE);
}
+}
+INSERT AFTER .data.fuzz_start;
+
+SECTIONS
+{
.data.fuzz_end : ALIGN(4K)
{
__FUZZ_COUNTERS_END = .;
@@ -43,4 +53,4 @@ SECTIONS
* Don't overwrite the SECTIONS in the default linker script. Instead insert
the
* above into the default script
*/
-INSERT AFTER .data;
+INSERT AFTER .data.fuzz_ordered;
--
2.17.1
Re: [PATCH v3 0/9] Add support for Control-Flow Integrity
Hi Alex, Yeah I assumed it was an older version because the errors triggered by clang11 stop the compilation. I checked again and for oss-fuzz, you disable failing on warnings. So again, these patches are not directly connected to CFI and therefore could land independently. On 11/6/2020 9:58 AM, Alexander Bulekov wrote: I think oss-fuzz is using a bleeding edge version of Clang, so that might not be a problem. Here is the oss-fuzz build-log from earlier today: https://oss-fuzz-build-logs.storage.googleapis.com/log-1747e14f-6b87-43e0-96aa-07ea159e7eb2.txt ... Step #4: C compiler for the host machine: clang (clang 12.0.0 "clang version 12.0.0 (https://github.com/llvm/llvm-project.git c9f69ee7f94cfefc373c3c6cae08e51b11e6d3c2)") Step #4: C linker for the host machine: clang ld.bfd 2.26.1 Step #4: Host machine cpu family: x86_64 ... Yeah I assumed it was an older version because the errors triggered by clang11 stop the compilation. I checked again and for oss-fuzz, you disable failing on warnings. So again, these patches are not directly connected to CFI and therefore could land independently.
Re: [PATCH-for-5.2? v3 5/9] scsi: fix overflow in scsi_disk_new_request_dump
Hi Philippe, Since you have a patch upcoming, may I drop this patch from this set? Daniele On 11/9/2020 8:26 AM, Philippe Mathieu-Daudé wrote: I did it. Will post later as this is 6.0 material.
Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus
Hi Philippe,
On 11/6/2020 9:28 AM, Philippe Mathieu-Daudé wrote:
On 11/5/20 11:18 PM, Daniele Buono wrote:
The UASStatus data structure has a variable sized field inside of type uas_iu,
that however is not placed at the end of the data structure.
This placement triggers a warning with clang 11, and while not a bug right now,
(the status is never a uas_iu_command, which is the variable-sized case),
it could become one in the future.
The problem is uas_iu_command::add_cdb, indeed.
../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized
type 'uas_iu' not at the end of a struct or class is a GNU extension
[-Werror,-Wgnu-variable-sized-type-not-at-end]
If possible remove the "../qemu-base/" as it does not provide
any useful information.
Sure, will do at the next cycle
uas_iustatus;
^
1 error generated.
Fix this by moving uas_iu at the end of the struct
Your patch silents the warning, but the problem is the same.
It would be safer/cleaner to make 'status' a pointer on the heap IMO.
I'm thinking of moving 'status' in a pointer with the following code
changes:
UASStatus is allocated in `usb_uas_alloc_status`, which currently does
not take a type or size for the union field. I'm thinking of adding
requested size for the status, like this:
static UASStatus *usb_uas_alloc_status(UASDevice *uas, uint8_t id,
uint16_t tag, size_t size);
and the common call would be
usb_uas_alloc_status([...],sizeof(uas_iu));
Also we'd need a double free when the object is freed. Right now
it's handled in the code when the object is not used anymore with a
`g_free(st);`.
I'd have to replace it with
`g_free(st->status); g_free(st);`. Would you suggest doing it place
or by adding a usb_uas_dealloc_status() function?
---
However, I am confused by the use of that variable-lenght field.
I'm looking at what seems the only place where a command is
parsed, in `usb_uas_handle_data`.
uas_iu iu;
[...]
switch (p->ep->nr) {
case UAS_PIPE_ID_COMMAND:
length = MIN(sizeof(iu), p->iov.size);
usb_packet_copy(p, , length);
[...]
break;
[...]
It would seem that the copy is limited to at most sizeof(uas_iu),
so even if we had anything in add_cdb[], that wouldn't be copied
here?
Is this intended?
Signed-off-by: Daniele Buono
---
hw/usb/dev-uas.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index cec071d96c..5ef3f4fec9 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -154,9 +154,9 @@ struct UASRequest {
struct UASStatus {
uint32_t stream;
-uas_iustatus;
uint32_t length;
QTAILQ_ENTRY(UASStatus) next;
+uas_iustatus;
};
/* - */
Re: [PATCH v3 0/9] Add support for Control-Flow Integrity
Hi Cornelia, I don't have a real preference either way. So if it is acceptable to have the clang11+ patches separated and handled by the maintainers for the proper subsystem, I'd say whatever the maintainers prefer. In my opinion, the patches for clang11+ support may be merged separately. I'm saying this because, from my tests, the only feature that needs clang11+ to compile with Control-Flow Integrity is fuzzing. However, the main way we're fuzzing QEMU is through OSSfuzz, and I don't think their infrastructure is using a compiler that new, so we wouldn't be able to enable it anyway. (Alex can chip in to confirm this) On the other hand, if someone is looking for temporary support in-house, they can just add -Wno-[...] as extra-cflags until the additional patches land. (Assuming CFI lands before the clang11+ patches). Regards, Daniele On 11/6/2020 7:47 AM, Cornelia Huck wrote: On Thu, 5 Nov 2020 17:18:56 -0500 Daniele Buono wrote: This patch adds supports for Control-Flow Integrity checks on indirect function calls. Requires the use of clang, and link-time optimizations Changes in v3: - clang 11+ warnings are now handled directly at the source, instead of disabling specific warnings for the whole code. Some more work may be needed here to polish the patch, I would kindly ask for a review from the corresponding maintainers Process question :) Would you prefer to have this series merged in one go, or should maintainers pick the patches for their subsystem? - Remove configure-time checks for toolchain compatibility with LTO. - the decorator to disable cfi checks on functions has been renamed and moved to include/qemu/compiler.h - configure-time checks for cfi support and dependencies has been moved from configure to meson Link to v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg753675.html Link to v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg718786.html Daniele Buono (9): fuzz: Make fork_fuzz.ld compatible with LLVM's LLD s390x: fix clang 11 warnings in cpu_models.c hw/usb: reorder fields in UASStatus s390x: Avoid variable size warning in ipl.h scsi: fix overflow in scsi_disk_new_request_dump configure,meson: add option to enable LTO cfi: Initial support for cfi-icall in QEMU check-block: enable iotests with cfi-icall configure/meson: support Control-Flow Integrity accel/tcg/cpu-exec.c | 11 + configure | 26 hw/s390x/ipl.h| 4 +-- hw/scsi/scsi-disk.c | 4 +++ hw/usb/dev-uas.c | 2 +- include/qemu/compiler.h | 12 + meson.build | 46 +++ meson_options.txt | 4 +++ plugins/core.c| 37 plugins/loader.c | 7 ++ target/s390x/cpu_models.c | 8 +++--- tcg/tci.c | 7 ++ tests/check-block.sh | 18 -- tests/qtest/fuzz/fork_fuzz.ld | 12 - util/main-loop.c | 11 + util/oslib-posix.c| 11 + 16 files changed, 205 insertions(+), 15 deletions(-)
[PATCH v3 3/9] hw/usb: reorder fields in UASStatus
The UASStatus data structure has a variable sized field inside of type uas_iu,
that however is not placed at the end of the data structure.
This placement triggers a warning with clang 11, and while not a bug right now,
(the status is never a uas_iu_command, which is the variable-sized case),
it could become one in the future.
../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized
type 'uas_iu' not at the end of a struct or class is a GNU extension
[-Werror,-Wgnu-variable-sized-type-not-at-end]
uas_iustatus;
^
1 error generated.
Fix this by moving uas_iu at the end of the struct
Signed-off-by: Daniele Buono
---
hw/usb/dev-uas.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index cec071d96c..5ef3f4fec9 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -154,9 +154,9 @@ struct UASRequest {
struct UASStatus {
uint32_t stream;
-uas_iustatus;
uint32_t length;
QTAILQ_ENTRY(UASStatus) next;
+uas_iustatus;
};
/* - */
--
2.17.1
[PATCH v3 9/9] configure,meson: support Control-Flow Integrity
This patch adds a flag to enable/disable control flow integrity checks
on indirect function calls.
This feature only allows indirect function calls at runtime to functions
with compatible signatures.
This feature is only provided by LLVM/Clang, and depends on link-time
optimization which is currently supported only with LLVM/Clang >= 6.0
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.
All the checks are performed in meson.build. configure is only used to
forward the flags to meson
Signed-off-by: Daniele Buono
---
configure | 21 -
meson.build | 45 +
meson_options.txt | 4
3 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/configure b/configure
index 7115655fe4..020c5a6aff 100755
--- a/configure
+++ b/configure
@@ -399,6 +399,8 @@ coroutine=""
coroutine_pool=""
debug_stack_usage="no"
crypto_afalg="no"
+cfi="disabled"
+cfi_debug="disabled"
seccomp=""
glusterfs=""
glusterfs_xlator_opt="no"
@@ -1179,6 +1181,16 @@ for opt do
;;
--disable-safe-stack) safe_stack="no"
;;
+ --enable-cfi)
+ cfi="enabled";
+ lto="true";
+ ;;
+ --disable-cfi) cfi="disabled"
+ ;;
+ --enable-cfi-debug) cfi_debug="enabled"
+ ;;
+ --disable-cfi-debug) cfi_debug="disabled"
+ ;;
--disable-curses) curses="disabled"
;;
--enable-curses) curses="enabled"
@@ -1753,6 +1765,13 @@ disabled with --disable-FEATURE, default is enabled if
available:
sparse sparse checker
safe-stack SafeStack Stack Smash Protection. Depends on
clang/llvm >= 3.7 and requires coroutine backend ucontext.
+ cfi Enable Control-Flow Integrity for indirect function calls.
+ In case of a cfi violation, QEMU is terminated with SIGILL
+ Depends on lto and is incompatible with modules
+ Automatically enables Link-Time Optimization (lto)
+ cfi-debug In case of a cfi violation, a message containing the line
that
+ triggered the error is written to stderr. After the error,
+ QEMU is still terminated with SIGILL
gnutls GNUTLS cryptography support
nettle nettle cryptography support
@@ -6997,7 +7016,7 @@ NINJA=$ninja $meson setup \
-Dcapstone=$capstone -Dslirp=$slirp -Dfdt=$fdt \
-Diconv=$iconv -Dcurses=$curses -Dlibudev=$libudev\
-Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
--Db_lto=$lto \
+-Db_lto=$lto -Dcfi=$cfi -Dcfi_debug=$cfi_debug \
$cross_arg \
"$PWD" "$source_path"
diff --git a/meson.build b/meson.build
index 99c7ab1d38..49a301888e 100644
--- a/meson.build
+++ b/meson.build
@@ -751,6 +751,48 @@ statx_test = '''
has_statx = cc.links(statx_test)
+if get_option('cfi').enabled()
+ cfi_flags=[]
+ # Check for dependency on LTO
+ if not get_option('b_lto')
+error('Selected Control-Flow Integrity but LTO is disabled')
+ endif
+ if config_host.has_key('CONFIG_MODULES')
+error('Selected Control-Flow Integrity is not compatible with modules')
+ endif
+ # Check for cfi flags. CFI requires LTO so we can't use
+ # get_supported_arguments, but need a more complex "compiles" which allows
+ # custom arguments
+ if cc.compiles('int main () { return 0; }', name: '-fsanitize=cfi-icall',
+ args: ['-flto', '-fsanitize=cfi-icall'] )
+cfi_flags += '-fsanitize=cfi-icall'
+ else
+error('-fsanitize=cfi-icall is not supported by the compiler')
+ endif
+ if cc.compiles('int main () { return 0; }',
+ name: '-fsanitize-cfi-icall-generalize-pointers',
+ args: ['-flto', '-fsanitize=cfi-icall',
+'-fsanitize-cfi-icall-generalize-pointers'] )
+cfi_flags += '-fsanitize-cfi-icall-generalize-pointers'
+ else
+error('-fsanitize-cfi-icall-generalize-pointers is not supported by the
compiler')
+ endif
+ if get_option('cfi_debug').enabled()
+if cc.compiles('int main () { return 0; }',
+ name: '-fno-sanitize-trap=cfi-icall',
+ args: ['-flto', '-fsanitize=cfi-icall',
+ '-fno-sanitize-trap=cfi-icall'] )
+ cfi_flags += '-fno-sanitize-trap=cfi-icall'
+else
+ error('-fno-sanitize-trap=cfi-icall is not supported by the compiler')
+endif
+ endif
+ add_project_
[PATCH v3 5/9] scsi: fix overflow in scsi_disk_new_request_dump
scsi_disk_new_request_dump is used to dump the content of a scsi request
for tracing. It does that by decoding the command to get the size of the
command buffer, and then printing the content of such buffer on a string.
When using gcc with link-time optimizations, it warns that the argument of
malloc may be too large.
In function 'scsi_disk_new_request_dump',
inlined from 'scsi_new_request' at
../qemu-cfi-v3/hw/scsi/scsi-disk.c:2588:9:
../qemu-cfi-v3/hw/scsi/scsi-disk.c:2562:17: warning: argument 1 value
'18446744073709551612' exceeds maximum object size 9223372036854775807
[-Walloc-size-larger-than=]
line_buffer = g_malloc(len * 5 + 1);
^
../qemu-cfi-v3/hw/scsi/scsi-disk.c: In function 'scsi_new_request':
/usr/include/glib-2.0/glib/gmem.h:78:10: note: in a call to allocation function
'g_malloc' declared here
gpointer g_malloc (gsize n_bytes) G_GNUC_MALLOC G_GNUC_ALLOC_SIZE(1);
len is a signed integer filled up by scsi_cdb_length which can return -1
if it can't decode the command. In this case, g_malloc would probably fail.
However, an unknown command here is a possibility, and since this is used for
tracing, we should try to print the command anyway, for debugging purposes.
Since knowing the size of the command in the buffer is impossible (could not
decode the command), only print the header by setting len=1 if scsi_cdb_length
returned -1
Signed-off-by: Daniele Buono
---
If we had a way to know the (maximum) size of the buffer, we could
alternatively dump the whole buffer, instead of dumping only the
first byte. Not sure if this can be done, nor if it is considered
a better option.
We could also produce an error instead/in addition to just dumping
the buffer, if the command cannot be decoded.
hw/scsi/scsi-disk.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e859534eaf..d70dfdd9dc 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2559,6 +2559,10 @@ static void scsi_disk_new_request_dump(uint32_t lun,
uint32_t tag, uint8_t *buf)
int len = scsi_cdb_length(buf);
char *line_buffer, *p;
+if (len < 0) {
+len = 1;
+}
+
line_buffer = g_malloc(len * 5 + 1);
for (i = 0, p = line_buffer; i < len; i++) {
--
2.17.1
[PATCH v3 8/9] check-block: enable iotests with cfi-icall
cfi-icall is a form of Control-Flow Integrity for indirect function
calls implemented by llvm. It is enabled with a -fsanitize flag.
iotests are currently disabled when -fsanitize options is used, with the
exception of SafeStack.
This patch implements a generic filtering mechanism to allow iotests
with a set of known-to-be-safe -fsanitize option. Then marks SafeStack
and the new options used for cfi-icall safe for iotests
Signed-off-by: Daniele Buono
---
tests/check-block.sh | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/tests/check-block.sh b/tests/check-block.sh
index f6b1bda7b9..fb4c1baae9 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -21,14 +21,18 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ;
then
exit 0
fi
-# Disable tests with any sanitizer except for SafeStack
-CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
-SANITIZE_FLAGS=""
-#Remove all occurrencies of -fsanitize=safe-stack
-for i in ${CFLAGS}; do
-if [ "${i}" != "-fsanitize=safe-stack" ]; then
-SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
+# Disable tests with any sanitizer except for specific ones
+SANITIZE_FLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
+ALLOWED_SANITIZE_FLAGS="safe-stack cfi-icall"
+#Remove all occurrencies of allowed Sanitize flags
+for j in ${ALLOWED_SANITIZE_FLAGS}; do
+TMP_FLAGS=${SANITIZE_FLAGS}
+SANITIZE_FLAGS=""
+for i in ${TMP_FLAGS}; do
+if ! echo ${i} | grep -q "${j}" 2>/dev/null; then
+SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
fi
+done
done
if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
# Have a sanitize flag that is not allowed, stop
--
2.17.1
[PATCH v3 2/9] s390x: fix clang 11 warnings in cpu_models.c
There are void * pointers that get casted to enums, in cpu_models.c
Such casts can result in a small integer type and are caught as
warnings with clang, starting with version 11:
Clang 11 finds a bunch of spots in the code that trigger this new warnings:
../qemu-base/target/s390x/cpu_models.c:985:21: error: cast to smaller integer
type 'S390Feat' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
S390Feat feat = (S390Feat) opaque;
^
../qemu-base/target/s390x/cpu_models.c:1002:21: error: cast to smaller integer
type 'S390Feat' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
S390Feat feat = (S390Feat) opaque;
^
../qemu-base/target/s390x/cpu_models.c:1036:27: error: cast to smaller integer
type 'S390FeatGroup' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
S390FeatGroup group = (S390FeatGroup) opaque;
^~
../qemu-base/target/s390x/cpu_models.c:1057:27: error: cast to smaller integer
type 'S390FeatGroup' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast]
S390FeatGroup group = (S390FeatGroup) opaque;
^~
4 errors generated.
Avoid this warning by casting the pointer to uintptr_t first.
Signed-off-by: Daniele Buono
---
target/s390x/cpu_models.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 461e0b8f4a..b5abff8bef 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -986,7 +986,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
static void get_feature(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
-S390Feat feat = (S390Feat) opaque;
+S390Feat feat = (S390Feat) (uintptr_t) opaque;
S390CPU *cpu = S390_CPU(obj);
bool value;
@@ -1003,7 +1003,7 @@ static void get_feature(Object *obj, Visitor *v, const
char *name,
static void set_feature(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
-S390Feat feat = (S390Feat) opaque;
+S390Feat feat = (S390Feat) (uintptr_t) opaque;
DeviceState *dev = DEVICE(obj);
S390CPU *cpu = S390_CPU(obj);
bool value;
@@ -1037,7 +1037,7 @@ static void set_feature(Object *obj, Visitor *v, const
char *name,
static void get_feature_group(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
-S390FeatGroup group = (S390FeatGroup) opaque;
+S390FeatGroup group = (S390FeatGroup) (uintptr_t) opaque;
const S390FeatGroupDef *def = s390_feat_group_def(group);
S390CPU *cpu = S390_CPU(obj);
S390FeatBitmap tmp;
@@ -1058,7 +1058,7 @@ static void get_feature_group(Object *obj, Visitor *v,
const char *name,
static void set_feature_group(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
-S390FeatGroup group = (S390FeatGroup) opaque;
+S390FeatGroup group = (S390FeatGroup) (uintptr_t) opaque;
const S390FeatGroupDef *def = s390_feat_group_def(group);
DeviceState *dev = DEVICE(obj);
S390CPU *cpu = S390_CPU(obj);
--
2.17.1
[PATCH v3 6/9] configure,meson: add option to enable LTO
This patch allows to compile QEMU with link-time optimization (LTO).
Compilation with LTO is handled directly by meson. This patch only
adds the option in configure and forwards the request to meson
Tested with all major versions of clang from 6 to 12
Signed-off-by: Daniele Buono
---
configure | 7 +++
meson.build | 1 +
2 files changed, 8 insertions(+)
diff --git a/configure b/configure
index 2c3c69f118..7115655fe4 100755
--- a/configure
+++ b/configure
@@ -242,6 +242,7 @@ host_cc="cc"
audio_win_int=""
libs_qga=""
debug_info="yes"
+lto="false"
stack_protector=""
safe_stack=""
use_containers="yes"
@@ -1166,6 +1167,10 @@ for opt do
;;
--disable-werror) werror="no"
;;
+ --enable-lto) lto="true"
+ ;;
+ --disable-lto) lto="false"
+ ;;
--enable-stack-protector) stack_protector="yes"
;;
--disable-stack-protector) stack_protector="no"
@@ -1744,6 +1749,7 @@ disabled with --disable-FEATURE, default is enabled if
available:
module-upgrades try to load modules from alternate paths for upgrades
debug-tcg TCG debugging (default is disabled)
debug-info debugging information
+ lto Enable Link-Time Optimization.
sparse sparse checker
safe-stack SafeStack Stack Smash Protection. Depends on
clang/llvm >= 3.7 and requires coroutine backend ucontext.
@@ -6991,6 +6997,7 @@ NINJA=$ninja $meson setup \
-Dcapstone=$capstone -Dslirp=$slirp -Dfdt=$fdt \
-Diconv=$iconv -Dcurses=$curses -Dlibudev=$libudev\
-Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
+-Db_lto=$lto \
$cross_arg \
"$PWD" "$source_path"
diff --git a/meson.build b/meson.build
index 39ac5cf6d8..99c7ab1d38 100644
--- a/meson.build
+++ b/meson.build
@@ -2023,6 +2023,7 @@ summary_info += {'gprof enabled':
config_host.has_key('CONFIG_GPROF')}
summary_info += {'sparse enabled':sparse.found()}
summary_info += {'strip binaries':get_option('strip')}
summary_info += {'profiler': config_host.has_key('CONFIG_PROFILER')}
+summary_info += {'link-time optimization (LTO)': get_option('b_lto')}
summary_info += {'static build': config_host.has_key('CONFIG_STATIC')}
if targetos == 'darwin'
summary_info += {'Cocoa support': config_host.has_key('CONFIG_COCOA')}
--
2.17.1
[PATCH v3 4/9] s390x: Avoid variable size warning in ipl.h
S390IPLState contains two IplParameterBlock, which may in turn have
either a IPLBlockPV or a IplBlockFcp, both ending with a variable
sized field (an array).
This causes a warning with clang 11 or greater, which checks that
variable sized type are only allocated at the end of the struct:
In file included from ../qemu-cfi-v3/target/s390x/diag.c:21:
../qemu-cfi-v3/hw/s390x/ipl.h:161:23: error: field 'iplb' with variable sized
type 'IplParameterBlock' (aka 'union IplParameterBlock') not at the end of a
struct or class is a GNU extension
[-Werror,-Wgnu-variable-sized-type-not-at-end]
IplParameterBlock iplb;
^
../qemu-cfi-v3/hw/s390x/ipl.h:162:23: error: field 'iplb_pv' with variable
sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at the end
of a struct or class is a GNU extension
[-Werror,-Wgnu-variable-sized-type-not-at-end]
IplParameterBlock iplb_pv;
In this case, however, the warning is a false positive, because
IPLBlockPV and IplBlockFcp are allocated in a union wrapped at 4K,
making the union non-variable sized.
Fix the warning by turning the two variable sized arrays into arrays
of size 0. This avoids the compiler error and should produce the
same code.
Signed-off-by: Daniele Buono
---
There is the possibility of removing IplBlockFcp from
IplParameterBlock, since it is not actually used.
This would also allow to entirely remove the definition of
IplBlockFcp, but we may want to keep it for completeness.
hw/s390x/ipl.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 9e90169695..dfc6dfd89c 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -32,7 +32,7 @@ struct IPLBlockPV {
uint32_t num_comp; /* 0x74 */
uint64_t pv_header_addr;/* 0x78 */
uint64_t pv_header_len; /* 0x80 */
-struct IPLBlockPVComp components[];
+struct IPLBlockPVComp components[0];
} QEMU_PACKED;
typedef struct IPLBlockPV IPLBlockPV;
@@ -63,7 +63,7 @@ struct IplBlockFcp {
uint64_t br_lba;
uint32_t scp_data_len;
uint8_t reserved6[260];
-uint8_t scp_data[];
+uint8_t scp_data[0];
} QEMU_PACKED;
typedef struct IplBlockFcp IplBlockFcp;
--
2.17.1
[PATCH v3 1/9] fuzz: Make fork_fuzz.ld compatible with LLVM's LLD
LLVM's linker, LLD, supports the keyword "INSERT AFTER", starting with
version 11.
However, when multiple sections are defined in the same "INSERT AFTER",
they are added in a reversed order, compared to BFD's LD.
This patch makes fork_fuzz.ld generic enough to work with both linkers.
Each section now has its own "INSERT AFTER" keyword, so proper ordering is
defined between the sections added.
Signed-off-by: Daniele Buono
---
tests/qtest/fuzz/fork_fuzz.ld | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/fuzz/fork_fuzz.ld b/tests/qtest/fuzz/fork_fuzz.ld
index bfb667ed06..cfb88b7fdb 100644
--- a/tests/qtest/fuzz/fork_fuzz.ld
+++ b/tests/qtest/fuzz/fork_fuzz.ld
@@ -16,6 +16,11 @@ SECTIONS
/* Lowest stack counter */
*(__sancov_lowest_stack);
}
+}
+INSERT AFTER .data;
+
+SECTIONS
+{
.data.fuzz_ordered :
{
/*
@@ -34,6 +39,11 @@ SECTIONS
*/
*(.bss._ZN6fuzzer3TPCE);
}
+}
+INSERT AFTER .data.fuzz_start;
+
+SECTIONS
+{
.data.fuzz_end : ALIGN(4K)
{
__FUZZ_COUNTERS_END = .;
@@ -43,4 +53,4 @@ SECTIONS
* Don't overwrite the SECTIONS in the default linker script. Instead insert
the
* above into the default script
*/
-INSERT AFTER .data;
+INSERT AFTER .data.fuzz_ordered;
--
2.17.1
[PATCH v3 7/9] cfi: Initial support for cfi-icall in QEMU
LLVM/Clang, supports runtime checks for forward-edge Control-Flow
Integrity (CFI).
CFI on indirect function calls (cfi-icall) ensures that, in indirect
function calls, the function called is of the right signature for the
pointer type defined at compile time.
For this check to work, the code must always respect the function
signature when using function pointer, the function must be defined
at compile time, and be compiled with link-time optimization.
This rules out, for example, shared libraries that are dynamically loaded
(given that functions are not known at compile time), and code that is
dynamically generated at run-time.
This patch:
1) Introduces the CONFIG_CFI flag to support cfi in QEMU
2) Introduces a decorator to allow the definition of "sensitive"
functions, where a non-instrumented function may be called at runtime
through a pointer. The decorator will take care of disabling cfi-icall
checks on such functions, when cfi is enabled.
3) Marks functions currently in QEMU that exhibit such behavior,
in particular:
- The function in TCG that calls pre-compiled TBs
- The function in TCI that interprets instructions
- Functions in the plugin infrastructures that jump to callbacks
- Functions in util that directly call a signal handler
Signed-off-by: Daniele Buono
Acked-by: Alex Bennée for plugins
---
accel/tcg/cpu-exec.c| 11 +++
include/qemu/compiler.h | 12
plugins/core.c | 37 +
plugins/loader.c| 7 +++
tcg/tci.c | 7 +++
util/main-loop.c| 11 +++
util/oslib-posix.c | 11 +++
7 files changed, 96 insertions(+)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 58aea605d8..ffe0e1e3fb 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -26,6 +26,7 @@
#include "exec/exec-all.h"
#include "tcg/tcg.h"
#include "qemu/atomic.h"
+#include "qemu/compiler.h"
#include "sysemu/qtest.h"
#include "qemu/timer.h"
#include "qemu/rcu.h"
@@ -144,6 +145,16 @@ static void init_delay_params(SyncClocks *sc, const
CPUState *cpu)
#endif /* CONFIG USER ONLY */
/* Execute a TB, and fix up the CPU state afterwards if necessary */
+/*
+ * Disable CFI checks.
+ * TCG creates binary blobs at runtime, with the transformed code.
+ * A TB is a blob of binary code, created at runtime and called with an
+ * indirect function call. Since such function did not exist at compile time,
+ * the CFI runtime has no way to verify its signature and would fail.
+ * TCG is not considered a security-sensitive part of QEMU so this does not
+ * affect the impact of CFI in environment with high security requirements
+ */
+QEMU_DISABLE_CFI
static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock
*itb)
{
CPUArchState *env = cpu->env_ptr;
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index c76281f354..c87c242063 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -243,4 +243,16 @@ extern void QEMU_NORETURN QEMU_ERROR("code path is
reachable")
#define qemu_build_not_reached() g_assert_not_reached()
#endif
+#ifdef CONFIG_CFI
+/*
+ * If CFI is enabled, use an attribute to disable cfi-icall on the following
+ * function
+ */
+#define QEMU_DISABLE_CFI __attribute__((no_sanitize("cfi-icall")))
+#else
+/* If CFI is not enabled, use an empty define to not change the behavior */
+#define QEMU_DISABLE_CFI
+#endif
+
+
#endif /* COMPILER_H */
diff --git a/plugins/core.c b/plugins/core.c
index 51bfc94787..87b823bbc4 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -31,6 +31,7 @@
#include "tcg/tcg-op.h"
#include "trace/mem-internal.h" /* mem_info macros */
#include "plugin.h"
+#include "qemu/compiler.h"
struct qemu_plugin_cb {
struct qemu_plugin_ctx *ctx;
@@ -90,6 +91,12 @@ void plugin_unregister_cb__locked(struct qemu_plugin_ctx
*ctx,
}
}
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
static void plugin_vcpu_cb__simple(CPUState *cpu, enum qemu_plugin_event ev)
{
struct qemu_plugin_cb *cb, *next;
@@ -111,6 +118,12 @@ static void plugin_vcpu_cb__simple(CPUState *cpu, enum
qemu_plugin_event ev)
}
}
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
static void plugin_cb__simple(enum qemu_plugin_event ev)
{
struct qemu_plugin_cb *cb, *next;
@@ -128,6 +141,12 @@ static void plugin_cb__simple(enum qemu_plugin_event ev)
}
}
+/*
+ * Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information
+ */
+QEMU_DISABLE_CFI
static void plugin_cb__udata(enum qemu_plugin
[PATCH v3 0/9] Add support for Control-Flow Integrity
This patch adds supports for Control-Flow Integrity checks on indirect function calls. Requires the use of clang, and link-time optimizations Changes in v3: - clang 11+ warnings are now handled directly at the source, instead of disabling specific warnings for the whole code. Some more work may be needed here to polish the patch, I would kindly ask for a review from the corresponding maintainers - Remove configure-time checks for toolchain compatibility with LTO. - the decorator to disable cfi checks on functions has been renamed and moved to include/qemu/compiler.h - configure-time checks for cfi support and dependencies has been moved from configure to meson Link to v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg753675.html Link to v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg718786.html Daniele Buono (9): fuzz: Make fork_fuzz.ld compatible with LLVM's LLD s390x: fix clang 11 warnings in cpu_models.c hw/usb: reorder fields in UASStatus s390x: Avoid variable size warning in ipl.h scsi: fix overflow in scsi_disk_new_request_dump configure,meson: add option to enable LTO cfi: Initial support for cfi-icall in QEMU check-block: enable iotests with cfi-icall configure/meson: support Control-Flow Integrity accel/tcg/cpu-exec.c | 11 + configure | 26 hw/s390x/ipl.h| 4 +-- hw/scsi/scsi-disk.c | 4 +++ hw/usb/dev-uas.c | 2 +- include/qemu/compiler.h | 12 + meson.build | 46 +++ meson_options.txt | 4 +++ plugins/core.c| 37 plugins/loader.c | 7 ++ target/s390x/cpu_models.c | 8 +++--- tcg/tci.c | 7 ++ tests/check-block.sh | 18 -- tests/qtest/fuzz/fork_fuzz.ld | 12 - util/main-loop.c | 11 + util/oslib-posix.c| 11 + 16 files changed, 205 insertions(+), 15 deletions(-) -- 2.17.1
Re: [PATCH v2 3/6] configure: add option to enable LTO
On 10/28/2020 5:35 AM, Alex Bennée wrote: Breakage in both system and linux-user emulation probably points at something in the instruction decode being broken. Shame we don't have a working risu setup for sparc64 to give the instruction handling a proper work out. This is what I'm thinking too. Interesting bit is that sparc32 seem to work fine, and it should be the same codebase. I played a bit with a couple of days but couldn't isolate the faulty instruction. But I'd be happy to work on this issue with someone, perhaps from the sparc maintainers, to see if we can find out what's happening
Re: [PATCH v2 3/6] configure: add option to enable LTO
If LTO is enabled with the wrong linker/ar: - with the checks, it will exit at configure with an error. I can change this in a warning and disabling LTO if preferred. - without the checks compilation will fail If LTO is enabled with the wrong compiler (e.g. old gcc), you may get a bunch of warnings at compile time, and a binary that won't pass some of the tests in make check. On 10/28/2020 2:44 AM, Paolo Bonzini wrote: On 27/10/20 21:42, Daniele Buono wrote: Ok, no problem. I can definitely disable the check on GCC. Paolo, would you like me to disable checks on AR/linker for lto too? If so, should I add some of this information on a document, perhaps docs/devel/lto.rst, so it is written somewhere for future uses? I am not sure of the effects. Does it simply effectively disable LTO or is it something worse? I'll look into the SCSI issue. Paolo
Re: [PATCH v2 3/6] configure: add option to enable LTO
Ok, no problem. I can definitely disable the check on GCC.
Paolo, would you like me to disable checks on AR/linker for lto too?
If so, should I add some of this information on a document, perhaps
docs/devel/lto.rst, so it is written somewhere for future uses?
--
Btw, using lto with gcc I found another interesting warning here
(adding scsi maintainer so they can chip in on the solution):
In function 'scsi_disk_new_request_dump',
inlined from 'scsi_new_request' at
../qemu-cfi-v3/hw/scsi/scsi-disk.c:2588:9:
../qemu-cfi-v3/hw/scsi/scsi-disk.c:2562:17: warning: argument 1 value
'18446744073709551612' exceeds maximum object size 9223372036854775807
[-Walloc-size-larger-than=]
line_buffer = g_malloc(len * 5 + 1);
^
../qemu-cfi-v3/hw/scsi/scsi-disk.c: In function 'scsi_new_request':
/usr/include/glib-2.0/glib/gmem.h:78:10: note: in a call to allocation
function 'g_malloc' declared here
gpointer g_malloc (gsize n_bytes) G_GNUC_MALLOC
G_GNUC_ALLOC_SIZE(1);
This seems like a bug to me. len is a signed integer filled up by
scsi_cdb_length which can return -1 if it can't decode the command.
What would probably happen is that we try a g_malloc with something too
big and that would fail. However, scsi_disk_new_request_dump is used for
tracing and:
a) I believe an unknown command here is a possibility, and is
handled by the caller - scsi_new_request - that has the following:
command = buf[0];
ops = scsi_disk_reqops_dispatch[command];
if (!ops) {
ops = _disk_emulate_reqops;
}
so a termination here on the malloc is probably not desired.
b) In the tracing, we should probably print the content of the buffer
anyway, so that the unknown command can be debugged. However, I don't
know what size I should use here.
I'm thinking either 1, to print just the command header in the buffer,
or the max size of the buffer, which I am not sure how to get.
Ideas or you prefer having an initial patch and then discuss it there?
On 10/27/2020 11:17 AM, Daniel P. Berrangé wrote:
On Tue, Oct 27, 2020 at 10:57:14AM -0400, Daniele Buono wrote:
In terms of ar and linker, if you don't have the right mix it will just
stop at link time with an error.
In terms of using gcc the errors may be a bit more subtle, similar to
what Daniel mentioned. Succesfully compiling but then showing issues at
runtime or in the test suite.
I'm using ubuntu 18.04 and the stock compiler (based on gcc 7.5) issues
a bunch of warnings but compile succesfully with LTO.
However, the tcg binary for sparc64 is broken. System-wide emulation
stops in OpenFirmware with an exception. User emulation triggers a
segmentation fault in some of the test cases. If I compile QEMU with
--enable-debug the tests magically work.
I briefly tested with gcc-9 and that seemed to work ok, buy your mileage
may vary
This why we shouldn't artificially block use of LTO with GCC in
the configure script. It blocks completely legitimate usage of
LTO with GCC versions where it works.
The user can detect if their version of GCC is broken by running the
test suite during their build process, which is best practice already,
and actually testing the result.
On 10/26/2020 11:50 AM, Daniel P. Berrangé wrote:
On Mon, Oct 26, 2020 at 10:51:43AM +0100, Paolo Bonzini wrote:
On 23/10/20 22:06, Daniele Buono wrote:
This patch allows to compile QEMU with link-time optimization (LTO).
Compilation with LTO is handled directly by meson. This patch adds checks
in configure to make sure the toolchain supports LTO.
Currently, allow LTO only with clang, since I have found a couple of issues
with gcc-based LTO.
In case fuzzing is enabled, automatically switch to llvm's linker (lld).
The standard bfd linker has a bug where function wrapping (used by the fuzz*
targets) is used in conjunction with LTO.
Tested with all major versions of clang from 6 to 12
Signed-off-by: Daniele Buono
What are the problems like if you have GCC or you ar/linker are not up
to the job? I wouldn't mind omitting the tests since this has to be
enabled explicitly by the user.
We temporarily disabled LTO in Fedora rawhide due to GCC bugs causing
wierd test suite asserts. Those were pre-release versions of GCC/binutils
though. I've just tested again and LTO works correctly, so I've enabled
LTO once again.
Regards,
Daniel
Regards,
Daniel
Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings
So what I would do for the warning in IplParameterBlock is the following (if I got the comments right): - Remove IplBlockFcp from IplParameterBlock - Keep IPLBlockPV and, in its declaration, use struct IPLBlockPVComp components[0]; Now for the IplBlockFcp struct declaration, it does not seem to be used anywhere now. I could either keep it as it was before (with the variable-size array) or remove it entirely. I guess this is more a question for the maintainers, what is your preference here? Daniele On 10/27/2020 7:38 AM, Cornelia Huck wrote: On Tue, 27 Oct 2020 12:26:21 +0100 Thomas Huth wrote: On 26/10/2020 16.12, Paolo Bonzini wrote: On 26/10/20 16:03, Daniele Buono wrote: Hi Paolo, I reorganized UASStatus to put uas_iu at the end and it works fine. Unfortunately, this uncovered another part of the code with a similar issue (variable sized type not at the end of the struct), here: In file included from ../qemu-cfi-v3/target/s390x/diag.c:21: ../qemu-cfi-v3/hw/s390x/ipl.h:161:23: error: field 'iplb' with variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] IplParameterBlock iplb; ^ ../qemu-cfi-v3/hw/s390x/ipl.h:162:23: error: field 'iplb_pv' with variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] IplParameterBlock iplb_pv; My understanding is that each of these IplParameterBlock may contain either a IPLBlockPV or a IplBlockFcp, which both end with a variable sized field (an array). Adding maintainers of s390x to see if they have a suggested solution to avoid disabling the warning. This one seems okay because the union constrains the size to 4K. If "[0]" is enough to shut up the compiler, I'd say do that. The "IplBlockFcp fcp" part seems to be completely unused, so I think you could even remove that IplBlockFcp struct. For IPLBlockPV I agree with Paolo, it's likely easiest to use [0] for that struct. The fcp block had probably been added for completeness' sake, but we do not support list-directed IPL anyway. Not sure if we actually want it, as we use a different mechanism for IPLing from SCSI devices. So yes, maybe we should just drop it.
Re: [PATCH v2 3/6] configure: add option to enable LTO
In terms of ar and linker, if you don't have the right mix it will just stop at link time with an error. In terms of using gcc the errors may be a bit more subtle, similar to what Daniel mentioned. Succesfully compiling but then showing issues at runtime or in the test suite. I'm using ubuntu 18.04 and the stock compiler (based on gcc 7.5) issues a bunch of warnings but compile succesfully with LTO. However, the tcg binary for sparc64 is broken. System-wide emulation stops in OpenFirmware with an exception. User emulation triggers a segmentation fault in some of the test cases. If I compile QEMU with --enable-debug the tests magically work. I briefly tested with gcc-9 and that seemed to work ok, buy your mileage may vary On 10/26/2020 11:50 AM, Daniel P. Berrangé wrote: On Mon, Oct 26, 2020 at 10:51:43AM +0100, Paolo Bonzini wrote: On 23/10/20 22:06, Daniele Buono wrote: This patch allows to compile QEMU with link-time optimization (LTO). Compilation with LTO is handled directly by meson. This patch adds checks in configure to make sure the toolchain supports LTO. Currently, allow LTO only with clang, since I have found a couple of issues with gcc-based LTO. In case fuzzing is enabled, automatically switch to llvm's linker (lld). The standard bfd linker has a bug where function wrapping (used by the fuzz* targets) is used in conjunction with LTO. Tested with all major versions of clang from 6 to 12 Signed-off-by: Daniele Buono What are the problems like if you have GCC or you ar/linker are not up to the job? I wouldn't mind omitting the tests since this has to be enabled explicitly by the user. We temporarily disabled LTO in Fedora rawhide due to GCC bugs causing wierd test suite asserts. Those were pre-release versions of GCC/binutils though. I've just tested again and LTO works correctly, so I've enabled LTO once again. Regards, Daniel
Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings
Using an array of length 0 seems to be enough to avoid the warning Will use that solution in v3. Thanks, Daniele On 10/26/2020 11:12 AM, Paolo Bonzini wrote: On 26/10/20 16:03, Daniele Buono wrote: Hi Paolo, I reorganized UASStatus to put uas_iu at the end and it works fine. Unfortunately, this uncovered another part of the code with a similar issue (variable sized type not at the end of the struct), here: In file included from ../qemu-cfi-v3/target/s390x/diag.c:21: ../qemu-cfi-v3/hw/s390x/ipl.h:161:23: error: field 'iplb' with variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] IplParameterBlock iplb; ^ ../qemu-cfi-v3/hw/s390x/ipl.h:162:23: error: field 'iplb_pv' with variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] IplParameterBlock iplb_pv; My understanding is that each of these IplParameterBlock may contain either a IPLBlockPV or a IplBlockFcp, which both end with a variable sized field (an array). Adding maintainers of s390x to see if they have a suggested solution to avoid disabling the warning. This one seems okay because the union constrains the size to 4K. If "[0]" is enough to shut up the compiler, I'd say do that. Paolo
Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings
Hi Paolo, I reorganized UASStatus to put uas_iu at the end and it works fine. Unfortunately, this uncovered another part of the code with a similar issue (variable sized type not at the end of the struct), here: In file included from ../qemu-cfi-v3/target/s390x/diag.c:21: ../qemu-cfi-v3/hw/s390x/ipl.h:161:23: error: field 'iplb' with variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] IplParameterBlock iplb; ^ ../qemu-cfi-v3/hw/s390x/ipl.h:162:23: error: field 'iplb_pv' with variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] IplParameterBlock iplb_pv; My understanding is that each of these IplParameterBlock may contain either a IPLBlockPV or a IplBlockFcp, which both end with a variable sized field (an array). Adding maintainers of s390x to see if they have a suggested solution to avoid disabling the warning. On 10/26/2020 5:50 AM, Paolo Bonzini wrote: On 23/10/20 22:06, Daniele Buono wrote: 1 error generated. The data structure is UASStatus, which must end with a QTAILQ_ENTRY, so I believe we cannot have uas_iu at the end. Since this is a gnu extension but CLANG supports it, just add -Wno-gnu-variable-sized-type-not-at-end This is potentially a real bug, in this case it works only because UASStatus's packet is never uas_iu_command (which has the variable sized type). The QTAILQ_ENTRY need not be at the end, please rearrange UASStatus's field so that the "usb_ui status" field is the last. Thanks, Paolo
Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings
On 10/24/2020 1:17 AM, Thomas Huth wrote: Compiling all code with -Wno-void-pointer-to-enum-cast sounds like the wrong approach to me, since this might hide some real bugs in other spots instead. Could you please try to cast the value through (uintptr_t) first, e.g. : S390Feat feat = (S390Feat)(uintptr_t) opaque; It's a little bit ugly, but still better than to disable the warning globally, I think. Thomas Hi Thomas, I did a quick check and casting to a uintptr_t seem to be a working solution to the issue. I will fix this in v3 Thanks, Daniele
Re: [PATCH v2 0/6] Add support for Control-Flow Integrity
On 10/23/2020 4:33 PM, Eric Blake wrote: On 10/23/20 3:06 PM, Daniele Buono wrote: v2: Several months (and structural changes in QEMU) have passed since v1. While the spirit of the patch is similar, the implementation is changed in multiple points, and should address most if not all the comments received in v1. 5) Most of the logic to enable CFI goes in the configure, since it's just a matter of checking for dependencies and incompatible options. However, I had to disable CFI checks for a few TCG functions. This can only be done through a blacklist file. I added a file in the root of QEMU, called cfi-blacklist.txt for such purpose. I am open to suggestions on where the file should go, and I am willing to become the maintainer of it, if deemed necessary. In the meantime, we have commits like: commit b199c682f1f0aaee22b2170a5fb885250057eec2 Author: Philippe Mathieu-Daudé Date: Thu Sep 10 09:01:31 2020 +0200 target/i386/kvm: Rename host_tsx_blacklisted() as host_tsx_broken() In order to use inclusive terminology, rename host_tsx_blacklisted() as host_tsx_broken(). which may help you in coming up with a more appropriate name for the new file. MAINTAINERS | 5 + accel/tcg/cpu-exec.c | 9 ++ configure | 214 ++ include/qemu/sanitizers.h | 22 meson.build | 3 + plugins/core.c| 25 plugins/loader.c | 5 + tcg/tci.c | 5 + tests/check-block.sh | 18 +-- tests/qtest/fuzz/fork_fuzz.ld | 12 +- util/main-loop.c | 9 ++ util/oslib-posix.c| 9 ++ 12 files changed, 328 insertions(+), 8 deletions(-) create mode 100644 include/qemu/sanitizers.h although I don't see a new file by that name here, so perhaps the v1 overview is now stale? Correct, the v1 overview is stale on that regard. V2 is not using a "broken" file anymore. CFI is now disabled by using an attribute directly on the code. From the v2 overview: * Instead of disabling CFI in specific functions by using a filter file, disable cfi by using a new decorator to be prefixed to the function definition. Beside the removal of a non-inclusive term, I believe this is a better way to track functions, since it is directly inside the code so everyone working on those functions will see it immediately. It's safer with regards of function naming changes and, hopefully, this will make maintaining cfi easier.
[PATCH v2 5/6] check-block: enable iotests with cfi-icall
cfi-icall is a form of Control-Flow Integrity for indirect function
calls implemented by llvm. It is enabled with a -fsanitize flag.
iotests are currently disabled when -fsanitize options is used, with the
exception of SafeStack.
This patch implements a generic filtering mechanism to allow iotests
with a set of known-to-be-safe -fsanitize option. Then marks SafeStack
and the new options used for cfi-icall safe for iotests
Signed-off-by: Daniele Buono
---
tests/check-block.sh | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/tests/check-block.sh b/tests/check-block.sh
index f6b1bda7b9..fb4c1baae9 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -21,14 +21,18 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ;
then
exit 0
fi
-# Disable tests with any sanitizer except for SafeStack
-CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
-SANITIZE_FLAGS=""
-#Remove all occurrencies of -fsanitize=safe-stack
-for i in ${CFLAGS}; do
-if [ "${i}" != "-fsanitize=safe-stack" ]; then
-SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
+# Disable tests with any sanitizer except for specific ones
+SANITIZE_FLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
+ALLOWED_SANITIZE_FLAGS="safe-stack cfi-icall"
+#Remove all occurrencies of allowed Sanitize flags
+for j in ${ALLOWED_SANITIZE_FLAGS}; do
+TMP_FLAGS=${SANITIZE_FLAGS}
+SANITIZE_FLAGS=""
+for i in ${TMP_FLAGS}; do
+if ! echo ${i} | grep -q "${j}" 2>/dev/null; then
+SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
fi
+done
done
if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
# Have a sanitize flag that is not allowed, stop
--
2.17.1
[PATCH v2 4/6] cfi: Initial support for cfi-icall in QEMU
LLVM/Clang, supports runtime checks for forward-edge Control-Flow
Integrity (CFI).
CFI on indirect function calls (cfi-icall) ensures that, in indirect
function calls, the function called is of the right signature for the
pointer type defined at compile time.
For this check to work, the code must always respect the function
signature when using function pointer, the function must be defined
at compile time, and be compiled with link-time optimization.
This rules out, for example, shared libraries that are dynamically loaded
(given that functions are not known at compile time), and code that is
dynamically generated at run-time.
This patch:
1) Introduces the CONFIG_CFI flag to support cfi in QEMU
2) Introduces a decorator to allow the definition of "sensitive"
functions, where a non-instrumented function may be called at runtime
through a pointer. The decorator will take care of disabling cfi-icall
checks on such functions, when cfi is enabled.
3) Marks functions currently in QEMU that exhibit such behavior,
in particular:
- The function in TCG that calls pre-compiled TBs
- The function in TCI that interprets instructions
- Functions in the plugin infrastructures that jump to callbacks
- Functions in util that directly call a signal handler
4) Add a new section in MAINTAINERS with me as a maintainer for
include/qemu/sanitizers.h, in case a maintainer is deemed
necessary for this feature
Signed-off-by: Daniele Buono
---
MAINTAINERS | 5 +
accel/tcg/cpu-exec.c | 9 +
include/qemu/sanitizers.h | 22 ++
plugins/core.c| 25 +
plugins/loader.c | 5 +
tcg/tci.c | 5 +
util/main-loop.c | 9 +
util/oslib-posix.c| 9 +
8 files changed, 89 insertions(+)
create mode 100644 include/qemu/sanitizers.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 6a197bd358..93b2b52b88 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3094,6 +3094,11 @@ S: Maintained
F: hw/semihosting/
F: include/hw/semihosting/
+Control Flow Integrity
+M: Daniele Buono
+S: Maintained
+F: include/qemu/sanitizers.h
+
Build and test automation
-
Build and test automation
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 58aea605d8..efa01c51db 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -26,6 +26,7 @@
#include "exec/exec-all.h"
#include "tcg/tcg.h"
#include "qemu/atomic.h"
+#include "qemu/sanitizers.h"
#include "sysemu/qtest.h"
#include "qemu/timer.h"
#include "qemu/rcu.h"
@@ -144,6 +145,14 @@ static void init_delay_params(SyncClocks *sc, const
CPUState *cpu)
#endif /* CONFIG USER ONLY */
/* Execute a TB, and fix up the CPU state afterwards if necessary */
+/* Disable CFI checks.
+ * TCG creates binary blobs at runtime, with the transformed code.
+ * A TB is a blob of binary code, created at runtime and called with an
+ * indirect function call. Since such function did not exist at compile time,
+ * the CFI runtime has no way to verify its signature and would fail.
+ * TCG is not considered a security-sensitive part of QEMU so this does not
+ * affect the impact of CFI in environment with high security requirements */
+__disable_cfi__
static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, TranslationBlock
*itb)
{
CPUArchState *env = cpu->env_ptr;
diff --git a/include/qemu/sanitizers.h b/include/qemu/sanitizers.h
new file mode 100644
index 00..31e404d58d
--- /dev/null
+++ b/include/qemu/sanitizers.h
@@ -0,0 +1,22 @@
+/*
+ * Decorators to disable sanitizers on specific functions
+ *
+ * Copyright IBM Corp., 2020
+ *
+ * Author:
+ * Daniele Buono
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifdef CONFIG_CFI
+/* If CFI is enabled, use an attribute to disable cfi-icall on the following
+ * function */
+#define __disable_cfi__ __attribute__((no_sanitize("cfi-icall")))
+#else
+/* If CFI is not enabled, use an empty define to not change the behavior */
+#define __disable_cfi__
+#endif
+
diff --git a/plugins/core.c b/plugins/core.c
index 51bfc94787..9b712ca4ac 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -31,6 +31,7 @@
#include "tcg/tcg-op.h"
#include "trace/mem-internal.h" /* mem_info macros */
#include "plugin.h"
+#include "qemu/sanitizers.h"
struct qemu_plugin_cb {
struct qemu_plugin_ctx *ctx;
@@ -90,6 +91,10 @@ void plugin_unregister_cb__locked(struct qemu_plugin_ctx
*ctx,
}
}
+/* Disable CFI checks.
+ * The callback function has been loaded from an external library so we do not
+ * have type information */
+__disable_cfi__
static void plugin_vcpu_cb__simple(CPUState *cpu, enum qemu_plugin_event ev)
{
struct qemu_plugin_cb *cb, *next;
[PATCH v2 6/6] configure: add support for Control-Flow Integrity
This patch adds a flag to enable/disable control flow integrity checks
on indirect function calls.
This feature only allows indirect function calls at runtime to functions
with compatible signatures.
This feature is only provided by LLVM/Clang, and depends on link-time
optimization which is currently supported only with LLVM/Clang >= 6.0
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.
Signed-off-by: Daniele Buono
---
configure | 84 +
meson.build | 2 ++
2 files changed, 86 insertions(+)
diff --git a/configure b/configure
index e964040522..f996c4462e 100755
--- a/configure
+++ b/configure
@@ -272,6 +272,8 @@ debug_info="yes"
lto="false"
stack_protector=""
safe_stack=""
+cfi="no"
+cfi_debug="no"
use_containers="yes"
gdb_bin=$(command -v "gdb-multiarch" || command -v "gdb")
@@ -1199,6 +1201,16 @@ for opt do
;;
--disable-safe-stack) safe_stack="no"
;;
+ --enable-cfi)
+ cfi="yes" ;
+ lto="true" ;
+ ;;
+ --disable-cfi) cfi="no"
+ ;;
+ --enable-cfi-debug) cfi_debug="yes"
+ ;;
+ --disable-cfi-debug) cfi_debug="no"
+ ;;
--disable-curses) curses="disabled"
;;
--enable-curses) curses="enabled"
@@ -1772,6 +1784,13 @@ disabled with --disable-FEATURE, default is enabled if
available:
sparse sparse checker
safe-stack SafeStack Stack Smash Protection. Depends on
clang/llvm >= 3.7 and requires coroutine backend ucontext.
+ cfi Enable Control-Flow Integrity for indirect function calls.
+ In case of a cfi violation, QEMU is terminated with SIGILL
+ Depends on lto and is incompatible with modules
+ Automatically enables Link-Time Optimization (lto)
+ cfi-debug In case of a cfi violation, a message containing the line
that
+ triggered the error is written to stderr. After the error,
+ QEMU is still terminated with SIGILL
gnutls GNUTLS cryptography support
nettle nettle cryptography support
@@ -5312,6 +5331,64 @@ EOF
CONFIGURE_CFLAGS="$QEMU_CFLAGS -flto"
CONFIGURE_LDFLAGS="$QEMU_LDFLAGS -flto"
fi
+
+
+# cfi (Control Flow Integrity)
+
+if test "$cfi" = "yes"; then
+ # Compiler/Linker Flags that needs to be added for cfi:
+ # -fsanitize=cfi-icall to enable control-flow integrity checks on
+ #indirect function calls.
+ # -fsanitize-cfi-icall-generalize-pointers to allow indirect function calls
+ #with pointers of a different type (i.e. pass a void* to a
+ #function that expects a char*). Used in some spots in QEMU,
+ #with compile-time type checks done by macros
+ # -fno-sanitize-trap=cfi-icall, when debug is enabled, to display the
+ #position in the code that triggered a CFI violation
+
+ # Make sure that LTO is enabled
+ if test "$lto" != "true"; then
+error_exit "Control Flow Integrity requires Link-Time Optimization (LTO)"
+ fi
+
+ test_cflag="-fsanitize=cfi-icall -fsanitize-cfi-icall-generalize-pointers"
+ test_ldflag="-fsanitize=cfi-icall"
+
+ if test "$cfi_debug" = "yes"; then
+# Disable the default trap mechanism so that a error message is displayed
+# when a CFI violation happens. The code is still terminated after the
+# message
+test_cflag="${test_cflag} -fno-sanitize-trap=cfi-icall"
+test_ldflag="${test_ldflag} -fno-sanitize-trap=cfi-icall"
+ fi
+
+ # Check that cfi is supported.
+ cat > $TMPC << EOF
+int main(int argc, char *argv[]) {
+ return 0;
+}
+EOF
+ # Manually add -flto because even if is enabled, flags for it will be
+ # set up later by meson
+ if ! compile_prog "-Werror $test_cflag" "$test_ldflag"; then
+error_exit "Control Flow Integrity is not supported by your compiler"
+ fi
+
+ # Check for incompatible options
+ if test "$modules" = "yes"; then
+error_exit "Control Flow Integrity is not compatible with modules"
+ fi
+
+ All good, add the flags for CFI to our CFLAGS and LDFLAGS
+ # Flag needed both at compilation and at linking
+ QEMU_CFLAGS="$QEMU_CFLAGS $test_cflag"
+ QEMU_LDFLAGS="$QEMU_LDFLAGS $test_ldflag"
+else
+ if test "$cfi_debug" = "
[PATCH v2 2/6] configure: avoid new clang 11+ warnings
Clang 11 finds a couple of spots in the code that trigger new warnings: ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized type 'uas_iu' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] uas_iustatus; ^ 1 error generated. The data structure is UASStatus, which must end with a QTAILQ_ENTRY, so I believe we cannot have uas_iu at the end. Since this is a gnu extension but CLANG supports it, just add -Wno-gnu-variable-sized-type-not-at-end to remove the warning. ../qemu-base/target/s390x/cpu_models.c:985:21: error: cast to smaller integer type 'S390Feat' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] S390Feat feat = (S390Feat) opaque; ^ ../qemu-base/target/s390x/cpu_models.c:1002:21: error: cast to smaller integer type 'S390Feat' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] S390Feat feat = (S390Feat) opaque; ^ ../qemu-base/target/s390x/cpu_models.c:1036:27: error: cast to smaller integer type 'S390FeatGroup' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] S390FeatGroup group = (S390FeatGroup) opaque; ^~ ../qemu-base/target/s390x/cpu_models.c:1057:27: error: cast to smaller integer type 'S390FeatGroup' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] S390FeatGroup group = (S390FeatGroup) opaque; ^~ 4 errors generated. These are void * that get casted to enums, which are (or can be) smaller than a 64bit pointer. A code reorg may be better on the long term, but for now will fix this adding -Wno-void-pointer-to-enum-cast Signed-off-by: Daniele Buono --- configure | 2 ++ 1 file changed, 2 insertions(+) diff --git a/configure b/configure index e6754c1e87..9dc05cfb8a 100755 --- a/configure +++ b/configure @@ -2000,6 +2000,8 @@ add_to nowarn_flags -Wno-shift-negative-value add_to nowarn_flags -Wno-string-plus-int add_to nowarn_flags -Wno-typedef-redefinition add_to nowarn_flags -Wno-tautological-type-limit-compare +add_to nowarn_flags -Wno-gnu-variable-sized-type-not-at-end +add_to nowarn_flags -Wno-void-pointer-to-enum-cast add_to nowarn_flags -Wno-psabi gcc_flags="$warn_flags $nowarn_flags" -- 2.17.1
[PATCH v2 3/6] configure: add option to enable LTO
This patch allows to compile QEMU with link-time optimization (LTO).
Compilation with LTO is handled directly by meson. This patch adds checks
in configure to make sure the toolchain supports LTO.
Currently, allow LTO only with clang, since I have found a couple of issues
with gcc-based LTO.
In case fuzzing is enabled, automatically switch to llvm's linker (lld).
The standard bfd linker has a bug where function wrapping (used by the fuzz*
targets) is used in conjunction with LTO.
Tested with all major versions of clang from 6 to 12
Signed-off-by: Daniele Buono
---
configure | 128
meson.build | 1 +
2 files changed, 129 insertions(+)
diff --git a/configure b/configure
index 9dc05cfb8a..e964040522 100755
--- a/configure
+++ b/configure
@@ -76,6 +76,7 @@ fi
TMPB="qemu-conf"
TMPC="${TMPDIR1}/${TMPB}.c"
TMPO="${TMPDIR1}/${TMPB}.o"
+TMPA="${TMPDIR1}/lib${TMPB}.a"
TMPCXX="${TMPDIR1}/${TMPB}.cxx"
TMPE="${TMPDIR1}/${TMPB}.exe"
TMPTXT="${TMPDIR1}/${TMPB}.txt"
@@ -180,6 +181,32 @@ compile_prog() {
$LDFLAGS $CONFIGURE_LDFLAGS $QEMU_LDFLAGS $local_ldflags
}
+do_run_filter() {
+# Run a generic program, capturing its output to the log,
+# but also filtering the output with grep.
+# Returns the return value of grep.
+# First argument is the filter string.
+# Second argument is binary to execute.
+local filter="$1"
+local filter_pattern=""
+if test "$filter" = "yes"; then
+ shift
+ filter_pattern="$1"
+fi
+shift
+local program="$1"
+shift
+echo $program $@ >> config.log
+$program $@ >> config.log 2>&1 || return $?
+if test "$filter" = "yes"; then
+ $program $@ 2>&1 | grep "${filter_pattern}" >> /dev/null || return $?
+fi
+}
+
+create_library() {
+ do_run_filter "no" "$ar" -rc${1} $TMPA $TMPO
+}
+
# symbolically link $1 to $2. Portable version of "ln -sf".
symlink() {
rm -rf "$2"
@@ -242,6 +269,7 @@ host_cc="cc"
audio_win_int=""
libs_qga=""
debug_info="yes"
+lto="false"
stack_protector=""
safe_stack=""
use_containers="yes"
@@ -1159,6 +1187,10 @@ for opt do
;;
--disable-werror) werror="no"
;;
+ --enable-lto) lto="true"
+ ;;
+ --disable-lto) lto="false"
+ ;;
--enable-stack-protector) stack_protector="yes"
;;
--disable-stack-protector) stack_protector="no"
@@ -1735,6 +1767,8 @@ disabled with --disable-FEATURE, default is enabled if
available:
module-upgrades try to load modules from alternate paths for upgrades
debug-tcg TCG debugging (default is disabled)
debug-info debugging information
+ lto Enable Link-Time Optimization.
+ Depends on clang/llvm >=6.0
sparse sparse checker
safe-stack SafeStack Stack Smash Protection. Depends on
clang/llvm >= 3.7 and requires coroutine backend ucontext.
@@ -5222,6 +5256,62 @@ if test "$plugins" = "yes" &&
fi
+# lto (Link-Time Optimization)
+
+if test "$lto" = "true"; then
+ # Test compiler/ar/linker support for lto.
+ # compilation with lto is handled by meson. Just make sure that compiler
+ # support is fully functional, and add additional compatibility flags
+ # if necessary.
+
+ if ! echo | $cc -dM -E - | grep __clang__ > /dev/null 2>&1 ; then
+# LTO with GCC and other compilers is not tested, and possibly broken
+error_exit "QEMU only supports LTO with CLANG"
+ fi
+
+ # Check that lto is supported.
+ # Need to check for:
+ # - Valid compiler, that supports lto flags
+ # - Valid ar, able to support intermediate code
+ # - Valid linker, able to support intermediate code
+
+ Check for a valid *ar* for link-time optimization.
+ # Test it by creating a static library and linking it
+ # Compile an object first
+ cat > $TMPC << EOF
+int fun(int val);
+
+int fun(int val) {
+return val;
+}
+EOF
+ if ! compile_object "-Werror -flto"; then
+error_exit "LTO is not supported by your compiler"
+ fi
+ # Create a library out of it
+ if ! create_library "s" ; then
+error_exit "LTO is not supported by ar. This usually happens when mixing
GNU and LLVM toolchain."
+ fi
+ # Now create a binary using the library
+ cat > $TMPC << EOF
+int fun(int val);
+
+int main(int argc, char *argv[]) {
+ return fun(0);
+}
+EOF
+ if ! compile_prog "-Werror" "$test_ldflag -flto ${TMPA}"; then
+error_exit "LTO is not supported by ar or t
[PATCH v2 0/6] Add support for Control-Flow Integrity
with Clang v9. There is also a possible performance issue, since for every indirect function call, and additional address check is performed, followed by an additional indirect call to the trampoline function. However, especially in the case of KVM-based virtualization, the impact should be minimal, since indirect function pointers should be used mostly for device emulation. I used Kata Container's metrics tests since that is a simple, reproducible set of tests to stress storage and network between VMs, and run a Lifecycle test to measure VM startup times under a specific workload. A full report is available here [4]. The difference between LLVM with and without CFI is generally low. Sometimes CFI is actually offering better performance, which may be explained by having a different binary layout because of LTO. Lifecycle and network do not seem to be affected much. With storage, the situation is a bit more variable, but the oscillations seem to be more related to the benchmark variability than the CFI overhead. I also run a quick check-acceptance on full system VMs with and without CFI, the results are at [4] and show comparable results, with CFI slightly outperforming the default binary produced by LLVM. [1] Mehdi Talbi and Paul Fariello. VM escape - QEMU Case Study [2] Nelson Elhage. Virtunoid: Breaking out of KVM [3] Marco Grassi and Kira. Vulnerability Discovery and Exploitation of Virtualization Solutions for Cloud Computing and Desktops [4] https://github.com/dbuono/QEMU-CFI-Performance *** BLURB HERE *** Daniele Buono (6): fuzz: Make fork_fuzz.ld compatible with LLVM's LLD configure: avoid new clang 11+ warnings configure: add option to enable LTO cfi: Initial support for cfi-icall in QEMU check-block: enable iotests with cfi-icall configure: add support for Control-Flow Integrity MAINTAINERS | 5 + accel/tcg/cpu-exec.c | 9 ++ configure | 214 ++ include/qemu/sanitizers.h | 22 meson.build | 3 + plugins/core.c| 25 plugins/loader.c | 5 + tcg/tci.c | 5 + tests/check-block.sh | 18 +-- tests/qtest/fuzz/fork_fuzz.ld | 12 +- util/main-loop.c | 9 ++ util/oslib-posix.c| 9 ++ 12 files changed, 328 insertions(+), 8 deletions(-) create mode 100644 include/qemu/sanitizers.h -- 2.17.1
[PATCH v2 1/6] fuzz: Make fork_fuzz.ld compatible with LLVM's LLD
LLVM's linker, LLD, supports the keyword "INSERT AFTER", starting with
version 11.
However, when multiple sections are defined in the same "INSERT AFTER",
they are added in a reversed order, compared to BFD's LD.
This patch makes fork_fuzz.ld generic enough to work with both linkers.
Each section now has its own "INSERT AFTER" keyword, so proper ordering is
defined between the sections added.
Signed-off-by: Daniele Buono
---
tests/qtest/fuzz/fork_fuzz.ld | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/fuzz/fork_fuzz.ld b/tests/qtest/fuzz/fork_fuzz.ld
index bfb667ed06..cfb88b7fdb 100644
--- a/tests/qtest/fuzz/fork_fuzz.ld
+++ b/tests/qtest/fuzz/fork_fuzz.ld
@@ -16,6 +16,11 @@ SECTIONS
/* Lowest stack counter */
*(__sancov_lowest_stack);
}
+}
+INSERT AFTER .data;
+
+SECTIONS
+{
.data.fuzz_ordered :
{
/*
@@ -34,6 +39,11 @@ SECTIONS
*/
*(.bss._ZN6fuzzer3TPCE);
}
+}
+INSERT AFTER .data.fuzz_start;
+
+SECTIONS
+{
.data.fuzz_end : ALIGN(4K)
{
__FUZZ_COUNTERS_END = .;
@@ -43,4 +53,4 @@ SECTIONS
* Don't overwrite the SECTIONS in the default linker script. Instead insert
the
* above into the default script
*/
-INSERT AFTER .data;
+INSERT AFTER .data.fuzz_ordered;
--
2.17.1
Re: [PATCH 2/2] configure: add support for Control-Flow Integrity
Yes, Something like that, probably with a small python script. On 8/10/2020 5:33 PM, Alexander Bulekov wrote: On 200810 2139, Paolo Bonzini wrote: On 10/08/20 21:01, Daniele Buono wrote: So I'm thinking of adding a check in configure. If gold is the linker, automatically create (somehow, still working on it) the full link script by obtaining the default bfd script and add the required parts. Would that work for you? Maybe even do it unconditionally? I agree. I can try a respin of my compiler-rt/libFuzzer patches to add a built-in fork-server to libFuzzer, so we can avoid the linker-script madness altogether. Don't know how soon I can get to this, but I do think it is worth another try. TIL about these differences between ld.bfd and ld.gold. So the idea is to use something like: "ld --verbose | grep -n ".*:" | grep -A1 "\s.data\s" | tail -n1" and insert the existing linker-script before that line? Thanks -Alex Paolo
Re: [PATCH 2/2] configure: add support for Control-Flow Integrity
Hi Alex, Paolo Hitting a small issue while adding support for lto (and therefore cfi) to the fuzzer. The fuzzer requires a modified linker script to place all of the stuff that needs to persist across fuzzing runs into a contiguous section of memory. It does that by inserting three new sections after the .data section of the binary. Instead of rewriting a full linker script, it's using the *INSERT* keyword to add this to the default linker script. Now, LTO with LLVM requires to use the gold linker, which does not have a default linker script and therefore does not support the *INSERT* keyword. This can be fixed by taking the default script from the bdf linker, adding the additional sections and ask gold to use the full script. However, I don't like the idea of replacing the small, self-contained script snippet that is currently used, with a full script (which is probably also dependent on the bfd/os version). So I'm thinking of adding a check in configure. If gold is the linker, automatically create (somehow, still working on it) the full link script by obtaining the default bfd script and add the required parts. Would that work for you? Cheers, Daniele On 7/2/2020 11:43 AM, Daniele Buono wrote: Hey Alex! I agree, in most cases (possibly all of them), a wrong indirect function call will end up with something that is catched by ASan or UBSan. This way, however, you may catch it earlier and it may make debug easier (especially with --enable-cfi-debug which is printing an error with the calling and, most times, the called function). UBSan does have a similar feature, -fsanitize=function, but unfortunately it works only on C++ code, and therefore is not good in the QEMU case. And, of course, it will also be used to weed out missing functions in the CFI filter. On 7/2/2020 9:38 AM, Alexander Bulekov wrote: Can't wait to try this out! On 200702 1459, Paolo Bonzini wrote: On 02/07/20 14:50, Daniele Buono wrote: I also wonder if this is something that could be put in the fuzzing environment. It would probably also help in finding coding error in corner cases quicker. Yes, fuzzing and tests/docker/test-debug should enable CFI. Also, tests/docker/test-clang should enable LTO. Paolo I believe CFI is most-useful as an active defensive measure. I can't think of many cases where a fuzzer/test could raise a CFI alert that wouldn't also be caught by something like canaries, ASan or UBsan, though maybe I'm missing something. Maybe testing/fuzzing with CFI could be useful to weed out any errors due to e.g. an incomplete cfi-blacklist.txt -Alex
[PATCH 0/1] tests/acceptance: set VM socket encoding to UTF-8
Some of the acceptance tests were failing for me with an error while decoding characters coming from the VMs running for the test. The error looked like this: (30/36) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_nios2_10m50: ERROR: 'ascii' codec can't decode byte 0xc2 in position 36: ordinal not in range(128) (1.29 s) The very same test is running fine on GitLab CI An investigation of the issue shows that the socket we are using between QEMU and AVOCADO is using the default encoding of the host OS. In my case, locale was set to plain ASCII with LANG=C. Setting it to a UTF-8 locale fixed the issue, but a safer approach should be to just tell Python to always use a UTF-8 charset for the socket. This is what the patch does. Daniele Buono (1): tests/acceptance: set VM socket encoding to UTF-8 tests/acceptance/avocado_qemu/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.26.2
[PATCH 1/1] tests/acceptance: set VM socket encoding to UTF-8
The OS of some VM used by acceptance tests sends messages in UTF-8
encoding.
The socket used to communicate between the VM and the host by the
AVOCADO framework uses the default host encoding. This is not generally
a problem since most distributions default to UTF. However, if for some
reason the host is using a plain ASCII encoding, the VM test will fail
with an error. This patch explicitly sets the encoding to be UTF-8.
Signed-off-by: Daniele Buono
---
tests/acceptance/avocado_qemu/__init__.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/acceptance/avocado_qemu/__init__.py
b/tests/acceptance/avocado_qemu/__init__.py
index 77d1c1d9ff..5f7ef0a2f2 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -73,7 +73,7 @@ def _console_interaction(test, success_message,
failure_message,
assert not keep_sending or send_string
if vm is None:
vm = test.vm
-console = vm.console_socket.makefile()
+console = vm.console_socket.makefile(encoding='UTF-8')
console_logger = logging.getLogger('console')
while True:
if send_string:
--
2.26.2
Re: [PATCH 2/2] configure: add support for Control-Flow Integrity
On 7/2/2020 5:52 AM, Daniel P. Berrangé wrote:
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 ?
Hi Daniel,
I gave it some thought and studied more of clang's options. It is
possible to disable cfi on specific functions by using an __attribute__
keyword, instead of providing a list in an external file.
In terms of maintaining, this is much better since we are removing the
need to update the list. I would suggest defining a macro,
__disable_cfi__, that can be prepended to a function. The macro would
expand to nothing if cfi is disabled, or to the proper attribute if it
is enabled. Here's example code snippet
/* Disable CFI checks.
* The callback function has been loaded from an external library so we
do not
* have type information */
__disable_cfi__
void qemu_plugin_vcpu_syscall_ret(CPUState *cpu, int64_t num, int64_t ret)
{
...
}
This would take care of renaming and removal of functions that need cfi.
It would also probably be beneficial to the developers since they can
see immediately that the function they are working with needs to have
CFI checks disabled, and why.
If you think this is a better approach, I'll submit v2 with this
approach instead of the external function list.
For new code, however, the best thing is proper education and testing.
I'll work on a document for docs/devel to detail what it is and how to
make code cfi-safe.
A good approach should be to test code changes with CFI enabled. CFI is,
after all, a sanitizer and therefore it makes sense to use it also
during development, together with ASan, UBSan and the likes.
Unfortunately, since it works only with clang, I don't think this can
ever be a hard requirement.
Daniele
Re: [PATCH 2/2] configure: add support for Control-Flow Integrity
Hey Alex! I agree, in most cases (possibly all of them), a wrong indirect function call will end up with something that is catched by ASan or UBSan. This way, however, you may catch it earlier and it may make debug easier (especially with --enable-cfi-debug which is printing an error with the calling and, most times, the called function). UBSan does have a similar feature, -fsanitize=function, but unfortunately it works only on C++ code, and therefore is not good in the QEMU case. And, of course, it will also be used to weed out missing functions in the CFI filter. On 7/2/2020 9:38 AM, Alexander Bulekov wrote: Can't wait to try this out! On 200702 1459, Paolo Bonzini wrote: On 02/07/20 14:50, Daniele Buono wrote: I also wonder if this is something that could be put in the fuzzing environment. It would probably also help in finding coding error in corner cases quicker. Yes, fuzzing and tests/docker/test-debug should enable CFI. Also, tests/docker/test-clang should enable LTO. Paolo I believe CFI is most-useful as an active defensive measure. I can't think of many cases where a fuzzer/test could raise a CFI alert that wouldn't also be caught by something like canaries, ASan or UBsan, though maybe I'm missing something. Maybe testing/fuzzing with CFI could be useful to weed out any errors due to e.g. an incomplete cfi-blacklist.txt -Alex
Re: [PATCH 2/2] configure: add support for Control-Flow Integrity
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 --- 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 00..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 *, en
Re: [PATCH 2/2] configure: add support for Control-Flow Integrity
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 --- 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 00..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. So yes, there is risk, but IMHO it's not as high as in seccomp. I think with a bit of education, it would be easy to spot red flags in new code. As for education/testing... I can definitely work on a doc to be put in docs/devel. Testing for CFI violations may be more difficult, however if a test code that exercises it is written in tests/, compiling QEMU with CFI and running the test should be sufficient to hit the violation. I also wonder if this is something that could be put in the fuzzing environment. It would probably also help in finding coding error in corner cases quicker. Regards, Daniele
Re: [PATCH 2/2] configure: add support for Control-Flow Integrity
On 7/2/2020 5:45 AM, Paolo Bonzini wrote:
On 02/07/20 07:49, 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
Can you split this option in two parts, --enable-lto to enable link-time
optimization (perhaps also for GCC) and --enable-cfi which implies
--enable-lto?
This is because in the future we are considering switching to Meson,
where LTO support is built-in; having a separate switch would make it
easier to find the relevant code.
Paolo
Sure, that shouldn't be a big deal.
Thanks,
Daniele
---
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 00..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
diff --git a/configure b/configure
index 4a22dcd563..86fb0f390c 100755
--- a/configure
+++ b/configure
@@ -27,6 +27,7 @@ fi
TMPB="qemu-conf"
TMPC="${TMPDIR1}/${TMPB}.c"
TMPO="${TMPDIR1}/${TMPB}.o"
+TMPA="${TMPDIR1}/lib${TMPB}.a"
TMPCXX="${TMPDIR1}/${TMPB}.cxx"
TMPE="${TMPDIR1}/${TMPB}.exe"
TMPMO="${TMPDIR1}/${TMPB}.mo"
@@ -134,6 +135,43 @@ compile_prog() {
do_cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $QEMU_LDFLAGS $local_ldflags
}
+do_run() {
+# Run a generic program, capturing its output to the log.
+# First argument is binary to execute.
+local program="$1"
+shift
+echo $program $@ >> config.log
+$program $@ >> config.log 2>&1 || return $?
+}
+
+do_run_filter() {
+# Run a generic program, capturing its output to the log,
+# but also filtering the output with grep.
+# Returns the return value of grep.
+# First argument is the filter string.
+# Second argument is binary to execute.
+local filter="$1"
+shift
+local program="$1"
+shift
+echo $program $@ >> config.log
+$program $@ >> config.log 2>&1
+$program $@ 2>&1 | grep ${filter} >> /dev/null || return $?
+
+}
+
+create_library() {
+ do_run "$ar" -rc${1} $TMPA $TMPO
+}
+
+create_index() {
+ do_run "$ranlib" $TMPA
+}
+
+find_library_symbol() {
+ do_run_filter ${1} "$nm" $TMPA
+}
+
# symbolically link $1 to $2. Portable version of "ln -sf".
symlink() {
rm -rf "$2"
@@ -306,6 +344,8 @@ libs_tools=""
audio_win_int=""
libs_qga=""
debug_info="yes"
+cfi="no"
+cfi_debug="no"
stack_protector=""
safe_stack=""
use_containers="yes"
@@ -1285,6 +1325,14 @@ for opt do
;;
--disable-werror) werror="no"
;;
+ --enable-cfi) cfi="yes"
+ ;;
+ --disabl
[PATCH 2/2] configure: add support for Control-Flow Integrity
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
---
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 00..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
diff --git a/configure b/configure
index 4a22dcd563..86fb0f390c 100755
--- a/configure
+++ b/configure
@@ -27,6 +27,7 @@ fi
TMPB="qemu-conf"
TMPC="${TMPDIR1}/${TMPB}.c"
TMPO="${TMPDIR1}/${TMPB}.o"
+TMPA="${TMPDIR1}/lib${TMPB}.a"
TMPCXX="${TMPDIR1}/${TMPB}.cxx"
TMPE="${TMPDIR1}/${TMPB}.exe"
TMPMO="${TMPDIR1}/${TMPB}.mo"
@@ -134,6 +135,43 @@ compile_prog() {
do_cc $QEMU_CFLAGS $local_cflags -o $TMPE $TMPC $QEMU_LDFLAGS $local_ldflags
}
+do_run() {
+# Run a generic program, capturing its output to the log.
+# First argument is binary to execute.
+local program="$1"
+shift
+echo $program $@ >> config.log
+$program $@ >> config.log 2>&1 || return $?
+}
+
+do_run_filter() {
+# Run a generic program, capturing its output to the log,
+# but also filtering the output with grep.
+# Returns the return value of grep.
+# First argument is the filter string.
+# Second argument is binary to execute.
+local filter="$1"
+shift
+local program="$1"
+shift
+echo $program $@ >> config.log
+$program $@ >> config.log 2>&1
+$program $@ 2>&1 | grep ${filter} >> /dev/null || return $?
+
+}
+
+create_library() {
+ do_run "$ar" -rc${1} $TMPA $TMPO
+}
+
+create_index() {
+ do_run "$ranlib" $TMPA
+}
+
+find_library_symbol() {
+ do_run_filter ${1} "$nm" $TMPA
+}
+
# symbolically link $1 to $2. Portable version of "ln -sf".
symlink() {
rm -rf "$2"
@@ -306,6 +344,8 @@ libs_tools=""
audio_win_int=""
libs_qga=""
debug_info="yes"
+cfi="no"
+cfi_debug="no"
stack_protector=""
safe_stack=""
use_containers="yes"
@@ -1285,6 +1325,14 @@ for opt do
;;
--disable-werror) werror="no"
;;
+ --enable-cfi) cfi="yes"
+ ;;
+ --disable-cfi) cfi="no"
+ ;;
+ --enable-cfi-debug) cfi_debug="yes"
+ ;;
+ --disable-cfi-debug) cfi_debug="no"
+ ;;
--enable-stack-protector) stack_protector="yes"
;;
--disable-stack-protector) stack_protector="no"
@@ -1838,6 +1886,10 @@ disabled with --disable-FEATURE, default is enabled if
available:
module-upgrades try to load modules from alternate paths for upgrades
debug-tcg TCG debugging (default is disabled)
debug-info
[PATCH 1/2] check-block: enable iotests with cfi-icall
cfi-icall is a form of Control-Flow Integrity for indirect function
calls implemented by llvm. It is enabled with a -fsanitize flag.
iotests are currently disabled when -fsanitize options is used, with the
exception of SafeStack.
This patch implements a generic filtering mechanism to allow iotests
with a set of known-to-be-safe -fsanitize option. Then mark SafeStack
and the new options used for cfi-icall safe for iotests
Signed-off-by: Daniele Buono
---
tests/check-block.sh | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/tests/check-block.sh b/tests/check-block.sh
index 8e29c868e5..7691213bd9 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -21,14 +21,18 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ;
then
exit 0
fi
-# Disable tests with any sanitizer except for SafeStack
-CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
-SANITIZE_FLAGS=""
-#Remove all occurrencies of -fsanitize=safe-stack
-for i in ${CFLAGS}; do
-if [ "${i}" != "-fsanitize=safe-stack" ]; then
-SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
+# Disable tests with any sanitizer except for specific ones
+SANITIZE_FLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
+ALLOWED_SANITIZE_FLAGS="safe-stack cfi-icall sanitize-blacklist"
+#Remove all occurrencies of allowed Sanitize flags
+for j in ${ALLOWED_SANITIZE_FLAGS}; do
+TMP_FLAGS=${SANITIZE_FLAGS}
+SANITIZE_FLAGS=""
+for i in ${TMP_FLAGS}; do
+if ! echo ${i} | grep -q "${j}" 2>/dev/null; then
+SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
fi
+done
done
if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
# Have a sanitize flag that is not allowed, stop
--
2.26.2
[PATCH 0/2] Add support for Control-Flow Integrity
LLVM/Clang, starting from v3.9, supports runtime checks for forward-edge Control-Flow Integrity (CFI). CFI on indirect function calls can have a huge impact in enhancing QEMU security, by significantly limiting one of the most used attack vectors for VM Escape. Attacks demonstrated in [1],[2] and [3] will, at some point, change a function pointer in a QEMU data structure. At high level, LLVM's implementation relies on compile-time information to create a range of consecutive trampolines for "compatible functions". At runtime, if the pointer is not in the valid range, it is assumed that the control flow was hijacked, and the process is terminated with an "Illegal Instruction" exception. CAVEATS: 1) For this CFI check to work, the code must always respect the function signature when using function pointer. While this is generally true in QEMU, there are a few instances where pointers are handled as generic void* from the caller. Since this is a common approach, Clang offer a flag to relax pointer checks and consider all pointer types to be compatible. 2) Since CFI relies on compile-time information, it requires using link-time optimization (LTO) to support CFI across compilation units. This adds a requirement for the gold linker, and LLVM's versions of static libraries tools (ar, ranlib, nm). 3) CFI checks cannot be performed on shared libraries (given that functions are not known at compile time). This means that indirect function calls will fail if the function pointer belong to a shared library. This does not seem to be a big issue for a standard QEMU deployment today, but QEMU modules won't be able to work when CFI is enabled. There is a way to allow shared library pointers, but it is experimental in LLVM, requires some work and reduces performance and security. For these reasons, I think it's best to start with this version, and discuss an extension for modules later. 4) CFI cannot be fully applied to TCG. The purpose of TCG is to transform code on-the-fly from one ISA to another. In doing so, binary blobs of executable code are created and called with function pointers. Since the code did not exist at compile time, runtime CFI checks find such functions illegal. To allow the code to keep running, CFI checks are not performed in the core function of TCG/TCI, and in the code that implements TCG plugins. This does not affect QEMU when executed with KVM, and all the device emulation code is always protected, even when using TCG. 5) Most of the logic to enable CFI goes in the configure, since it's just a matter of checking for dependencies and incompatible options. However, I had to disable CFI checks for a few TCG functions. This can only be done through a blacklist file. I added a file in the root of QEMU, called cfi-blacklist.txt for such purpose. I am open to suggestions on where the file should go, and I am willing to become the maintainer of it, if deemed necessary. PERFORMANCE: Enabling CFI creates a larger binary, which may be an issue in some use cases. However, the increase is not exceptionally large. On my Ubuntu system, with default options, I see an increase of stripped size from 14MiB to 15.3MiB when enabling CFI with Clang v9. There is also a possible performance issue, since for every indirect function call, and additional address check is performed, followed by an additional indirect call to the trampoline function. However, especially in the case of KVM-based virtualization, the impact should be minimal, since indirect function pointers should be used mostly for device emulation. I used Kata Container's metrics tests since that is a simple, reproducible set of tests to stress storage and network between VMs, and run a Lifecycle test to measure VM startup times under a specific workload. A full report is available here [4]. The difference between LLVM with and without CFI is generally low. Sometimes CFI is actually offering better performance, which may be explained by having a different binary layout because of LTO. Lifecycle and network do not seem to be affected much. With storage, the situation is a bit more variable, but the oscillations seem to be more related to the benchmark variability than the CFI overhead. I also run a quick check-acceptance on full system VMs with and without CFI, the results are at [4] and show comparable results, with CFI slightly outperforming the default binary produced by LLVM. [1] Mehdi Talbi and Paul Fariello. VM escape - QEMU Case Study [2] Nelson Elhage. Virtunoid: Breaking out of KVM [3] Marco Grassi and Kira. Vulnerability Discovery and Exploitation of Virtualization Solutions for Cloud Computing and Desktops [4] https://github.com/dbuono/QEMU-CFI-Performance Daniele Buono (2): check-block: enable iotests with cfi-icall configure: add support for Control-Flow Integrity cfi-blacklist.txt| 27 +++ configure| 177 +++ tests/check-block.sh
Re: [PATCH v2 0/4] Add support for SafeStack
Ping? On 5/29/2020 4:51 PM, Daniele Buono wrote: LLVM supports SafeStack instrumentation to protect against stack buffer overflows, since version 3.7 From https://clang.llvm.org/docs/SafeStack.html: "It works by separating the program stack into two distinct regions: the safe stack and the unsafe stack. The safe stack stores return addresses, register spills, and local variables that are always accessed in a safe way, while the unsafe stack stores everything else. This separation ensures that buffer overflows on the unsafe stack cannot be used to overwrite anything on the safe stack." Unfortunately, the use of two stack regions does not cope well with QEMU's coroutines. The second stack region is not properly set up with both ucontext and sigaltstack, so multiple coroutines end up sharing the same memory area for the unsafe stack, causing undefined behaviors at runtime (and most iochecks to fail). This patch series fixes the implementation of the ucontext backend and make sure that sigaltstack is never used if the compiler is applying the SafeStack instrumentation. It also adds a configure flag to enable SafeStack, and enables iotests when SafeStack is used. Changes since v1: - CONFIG_SAFESTACK is now set up in configure, and not in the code - Added option for a --disable-safe-stack in configure - Configure checks if SafeStack is enabled by default in the compiler, and set the CONFIG_SAFESTACK accordingly - Updated some comments in the code and the commit log NOTE: I kept configure as Patch #3. The reason is that the code changes will not be enabled without the configure, making the code fully functional if only Patches #1 or #2 are applied. On the other hand, the configure patch will produce incorrect code if we request SafeStack and the other patches are not applied. Daniele Buono (4): coroutine: support SafeStack in ucontext backend coroutine: add check for SafeStack in sigaltstack configure: add flags to support SafeStack check-block: enable iotests with SafeStack configure| 73 include/qemu/coroutine_int.h | 5 +++ tests/check-block.sh | 12 +- util/coroutine-sigaltstack.c | 4 ++ util/coroutine-ucontext.c| 26 + 5 files changed, 119 insertions(+), 1 deletion(-)
[PATCH v2 4/4] check-block: enable iotests with SafeStack
SafeStack is a stack protection technique implemented in llvm. It is
enabled with a -fsanitize flag.
iotests are currently disabled when any -fsanitize option is used,
because such options tend to produce additional warnings and false
positives.
While common -fsanitize options are used to verify the code and not
added in production, SafeStack's main use is in production environments
to protect against stack smashing.
Since SafeStack does not print any warning or false positive, enable
iotests when SafeStack is the only -fsanitize option used.
This is likely going to be a production binary and we want to make sure
it works correctly.
Signed-off-by: Daniele Buono
---
tests/check-block.sh | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tests/check-block.sh b/tests/check-block.sh
index ad320c21ba..8e29c868e5 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -21,7 +21,17 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ;
then
exit 0
fi
-if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then
+# Disable tests with any sanitizer except for SafeStack
+CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
+SANITIZE_FLAGS=""
+#Remove all occurrencies of -fsanitize=safe-stack
+for i in ${CFLAGS}; do
+if [ "${i}" != "-fsanitize=safe-stack" ]; then
+SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
+fi
+done
+if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
+# Have a sanitize flag that is not allowed, stop
echo "Sanitizers are enabled ==> Not running the qemu-iotests."
exit 0
fi
--
2.26.2
[PATCH v2 3/4] configure: add flags to support SafeStack
This patch adds a flag to enable/disable the SafeStack instrumentation
provided by LLVM.
On enable, make sure that the compiler supports the flags, and that we
are using the proper coroutine implementation (coroutine-ucontext).
On disable, explicitly disable the option if it was enabled by default.
While SafeStack is supported only on Linux, NetBSD, FreeBSD and macOS,
we are not checking for the O.S. since this is already done by LLVM.
Signed-off-by: Daniele Buono
---
configure | 73 +++
1 file changed, 73 insertions(+)
diff --git a/configure b/configure
index b969dee675..260772b2d5 100755
--- a/configure
+++ b/configure
@@ -302,6 +302,7 @@ audio_win_int=""
libs_qga=""
debug_info="yes"
stack_protector=""
+safe_stack=""
use_containers="yes"
gdb_bin=$(command -v "gdb-multiarch" || command -v "gdb")
@@ -1275,6 +1276,10 @@ for opt do
;;
--disable-stack-protector) stack_protector="no"
;;
+ --enable-safe-stack) safe_stack="yes"
+ ;;
+ --disable-safe-stack) safe_stack="no"
+ ;;
--disable-curses) curses="no"
;;
--enable-curses) curses="yes"
@@ -1804,6 +1809,8 @@ disabled with --disable-FEATURE, default is enabled if
available:
debug-tcg TCG debugging (default is disabled)
debug-info debugging information
sparse sparse checker
+ safe-stack SafeStack Stack Smash Protection. Depends on
+ clang/llvm >= 3.7 and requires coroutine backend ucontext.
gnutls GNUTLS cryptography support
nettle nettle cryptography support
@@ -5517,6 +5524,67 @@ if test "$debug_stack_usage" = "yes"; then
fi
fi
+##
+# SafeStack
+
+
+if test "$safe_stack" = "yes"; then
+cat > $TMPC << EOF
+int main(int argc, char *argv[])
+{
+#if ! __has_feature(safe_stack)
+#error SafeStack Disabled
+#endif
+return 0;
+}
+EOF
+ flag="-fsanitize=safe-stack"
+ # Check that safe-stack is supported and enabled.
+ if compile_prog "-Werror $flag" "$flag"; then
+# Flag needed both at compilation and at linking
+QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+QEMU_LDFLAGS="$QEMU_LDFLAGS $flag"
+ else
+error_exit "SafeStack not supported by your compiler"
+ fi
+ if test "$coroutine" != "ucontext"; then
+error_exit "SafeStack is only supported by the coroutine backend ucontext"
+ fi
+else
+cat > $TMPC << EOF
+int main(int argc, char *argv[])
+{
+#if defined(__has_feature)
+#if __has_feature(safe_stack)
+#error SafeStack Enabled
+#endif
+#endif
+return 0;
+}
+EOF
+if test "$safe_stack" = "no"; then
+ # Make sure that safe-stack is disabled
+ if ! compile_prog "-Werror" ""; then
+# SafeStack was already enabled, try to explicitly remove the feature
+flag="-fno-sanitize=safe-stack"
+if ! compile_prog "-Werror $flag" "$flag"; then
+ error_exit "Configure cannot disable SafeStack"
+fi
+QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+QEMU_LDFLAGS="$QEMU_LDFLAGS $flag"
+ fi
+else # "$safe_stack" = ""
+ # Set safe_stack to yes or no based on pre-existing flags
+ if compile_prog "-Werror" ""; then
+safe_stack="no"
+ else
+safe_stack="yes"
+if test "$coroutine" != "ucontext"; then
+ error_exit "SafeStack is only supported by the coroutine backend
ucontext"
+fi
+ fi
+fi
+fi
##
# check if we have open_by_handle_at
@@ -6611,6 +6679,7 @@ echo "sparse enabled$sparse"
echo "strip binaries$strip_opt"
echo "profiler $profiler"
echo "static build $static"
+echo "safe stack$safe_stack"
if test "$darwin" = "yes" ; then
echo "Cocoa support $cocoa"
fi
@@ -8195,6 +8264,10 @@ if test "$ccache_cpp2" = "yes"; then
echo "export CCACHE_CPP2=y" >> $config_host_mak
fi
+if test "$safe_stack" = "yes"; then
+ echo "CONFIG_SAFESTACK=y" >> $config_host_mak
+fi
+
# If we're using a separate build tree, set it up now.
# DIRS are directories which we simply mkdir in the build tree;
# LINKS are things to symlink back into the source tree
--
2.26.2
[PATCH v2 1/4] coroutine: support SafeStack in ucontext backend
LLVM's SafeStack instrumentation does not yet support programs that make
use of the APIs in ucontext.h
With the current implementation of coroutine-ucontext, the resulting
binary is incorrect, with different coroutines sharing the same unsafe
stack and producing undefined behavior at runtime.
This fix allocates an additional unsafe stack area for each coroutine,
and sets the new unsafe stack pointer before calling swapcontext() in
qemu_coroutine_new.
This is the only place where the pointer needs to be manually updated,
since sigsetjmp/siglongjmp are already instrumented by LLVM to properly
support SafeStack.
The additional stack is then freed in qemu_coroutine_delete.
Signed-off-by: Daniele Buono
---
include/qemu/coroutine_int.h | 5 +
util/coroutine-ucontext.c| 26 ++
2 files changed, 31 insertions(+)
diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index bd6b0468e1..1da148552f 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -28,6 +28,11 @@
#include "qemu/queue.h"
#include "qemu/coroutine.h"
+#ifdef CONFIG_SAFESTACK
+/* Pointer to the unsafe stack, defined by the compiler */
+extern __thread void *__safestack_unsafe_stack_ptr;
+#endif
+
#define COROUTINE_STACK_SIZE (1 << 20)
typedef enum {
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index bd593e61bc..9108eb1294 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -41,6 +41,11 @@ typedef struct {
Coroutine base;
void *stack;
size_t stack_size;
+#ifdef CONFIG_SAFESTACK
+/* Need an unsafe stack for each coroutine */
+void *unsafe_stack;
+size_t unsafe_stack_size;
+#endif
sigjmp_buf env;
#ifdef CONFIG_VALGRIND_H
@@ -140,6 +145,10 @@ Coroutine *qemu_coroutine_new(void)
co = g_malloc0(sizeof(*co));
co->stack_size = COROUTINE_STACK_SIZE;
co->stack = qemu_alloc_stack(>stack_size);
+#ifdef CONFIG_SAFESTACK
+co->unsafe_stack_size = COROUTINE_STACK_SIZE;
+co->unsafe_stack = qemu_alloc_stack(>unsafe_stack_size);
+#endif
co->base.entry_arg = _env; /* stash away our jmp_buf */
uc.uc_link = _uc;
@@ -160,6 +169,20 @@ Coroutine *qemu_coroutine_new(void)
/* swapcontext() in, siglongjmp() back out */
if (!sigsetjmp(old_env, 0)) {
start_switch_fiber(_stack_save, co->stack, co->stack_size);
+#ifdef CONFIG_SAFESTACK
+/*
+ * Before we swap the context, set the new unsafe stack
+ * The unsafe stack grows just like the normal stack, so start from
+ * the last usable location of the memory area.
+ * NOTE: we don't have to re-set the usp afterwards because we are
+ * coming back to this context through a siglongjmp.
+ * The compiler already wrapped the corresponding sigsetjmp call with
+ * code that saves the usp on the (safe) stack before the call, and
+ * restores it right after (which is where we return with siglongjmp).
+ */
+void *usp = co->unsafe_stack + co->unsafe_stack_size;
+__safestack_unsafe_stack_ptr = usp;
+#endif
swapcontext(_uc, );
}
@@ -192,6 +215,9 @@ void qemu_coroutine_delete(Coroutine *co_)
#endif
qemu_free_stack(co->stack, co->stack_size);
+#ifdef CONFIG_SAFESTACK
+qemu_free_stack(co->unsafe_stack, co->unsafe_stack_size);
+#endif
g_free(co);
}
--
2.26.2
[PATCH v2 2/4] coroutine: add check for SafeStack in sigaltstack
Current implementation of LLVM's SafeStack is not compatible with
code that uses an alternate stack created with sigaltstack().
Since coroutine-sigaltstack relies on sigaltstack(), it is not
compatible with SafeStack. The resulting binary is incorrect, with
different coroutines sharing the same unsafe stack and producing
undefined behavior at runtime.
In the future LLVM may provide a SafeStack implementation compatible with
sigaltstack(). In the meantime, if SafeStack is desired, the coroutine
implementation from coroutine-ucontext should be used.
As a safety check, add a control in coroutine-sigaltstack to throw a
preprocessor #error if SafeStack is enabled and we are trying to
use coroutine-sigaltstack to implement coroutines.
Signed-off-by: Daniele Buono
---
util/coroutine-sigaltstack.c | 4
1 file changed, 4 insertions(+)
diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index f6fc49a0e5..aade82afb8 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -30,6 +30,10 @@
#include "qemu-common.h"
#include "qemu/coroutine_int.h"
+#ifdef CONFIG_SAFESTACK
+#error "SafeStack is not compatible with code run in alternate signal stacks"
+#endif
+
typedef struct {
Coroutine base;
void *stack;
--
2.26.2
[PATCH v2 0/4] Add support for SafeStack
LLVM supports SafeStack instrumentation to protect against stack buffer overflows, since version 3.7 >From https://clang.llvm.org/docs/SafeStack.html: "It works by separating the program stack into two distinct regions: the safe stack and the unsafe stack. The safe stack stores return addresses, register spills, and local variables that are always accessed in a safe way, while the unsafe stack stores everything else. This separation ensures that buffer overflows on the unsafe stack cannot be used to overwrite anything on the safe stack." Unfortunately, the use of two stack regions does not cope well with QEMU's coroutines. The second stack region is not properly set up with both ucontext and sigaltstack, so multiple coroutines end up sharing the same memory area for the unsafe stack, causing undefined behaviors at runtime (and most iochecks to fail). This patch series fixes the implementation of the ucontext backend and make sure that sigaltstack is never used if the compiler is applying the SafeStack instrumentation. It also adds a configure flag to enable SafeStack, and enables iotests when SafeStack is used. Changes since v1: - CONFIG_SAFESTACK is now set up in configure, and not in the code - Added option for a --disable-safe-stack in configure - Configure checks if SafeStack is enabled by default in the compiler, and set the CONFIG_SAFESTACK accordingly - Updated some comments in the code and the commit log NOTE: I kept configure as Patch #3. The reason is that the code changes will not be enabled without the configure, making the code fully functional if only Patches #1 or #2 are applied. On the other hand, the configure patch will produce incorrect code if we request SafeStack and the other patches are not applied. Daniele Buono (4): coroutine: support SafeStack in ucontext backend coroutine: add check for SafeStack in sigaltstack configure: add flags to support SafeStack check-block: enable iotests with SafeStack configure| 73 include/qemu/coroutine_int.h | 5 +++ tests/check-block.sh | 12 +- util/coroutine-sigaltstack.c | 4 ++ util/coroutine-ucontext.c| 26 + 5 files changed, 119 insertions(+), 1 deletion(-) -- 2.26.2
Re: [PATCH 2/4] coroutine: Add check for SafeStack in sigalstack
Sorry, missed the question at the end of the email. Will change the commit and error message to explain better in v2. Similar to the ucontext, case, sigaltstack does not work out of the box because it requires a stack to be allocated by the user. I'll be honest, I didn't check the details of how sigaltstack is used in coroutine-sigaltstack. It is very possible that this coroutine version can also be adapted to run properly with SafeStack. coroutine_trampoline is probably the place where we can set the usp so that, when coroutine_bootstrap is called, it is done with the new unsafe stack. My initial focus was on the ucontext implementation since that is the default one and mostly used. For now, I am just blocking the user from using coroutine-sigaltstack with SafeStack. However, if you are interested, Ican try to add support to sigaltstack in the future. On 5/21/2020 5:49 AM, Stefan Hajnoczi wrote: On Wed, Apr 29, 2020 at 03:44:18PM -0400, Daniele Buono wrote: s/sigalstack/sigaltstack/ in the commit message. LLVM's SafeStack instrumentation cannot be used inside signal handlers that make use of sigaltstack(). Since coroutine-sigaltstack relies on sigaltstack(), it is not compatible with SafeStack. The resulting binary is incorrect, with different coroutines sharing the same unsafe stack and producing undefined behavior at runtime. To avoid this, we add a check in coroutine-sigaltstack that throws a preprocessor #error and interrupt the compilation if SafeStack is enabled. Signed-off-by: Daniele Buono --- util/coroutine-sigaltstack.c | 4 1 file changed, 4 insertions(+) diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c index f6fc49a0e5..b7cdc959f8 100644 --- a/util/coroutine-sigaltstack.c +++ b/util/coroutine-sigaltstack.c @@ -30,6 +30,10 @@ #include "qemu-common.h" #include "qemu/coroutine_int.h" +#ifdef CONFIG_SAFESTACK +#error "SafeStack does not work with sigaltstack's implementation" +#endif Neither the commit description nor the #error message explain why it doesn't work. Could it work in the future or is there a fundamental reason why it will never work? Stefan
Re: [PATCH 3/4] configure: add flag to enable SafeStack
Hi Stefan, a quick clarification on configure: right now, in configure, there's * "Advanced Options (experts only)" which usually don't have both enable and disable for each option, and * "Optional features, enabled with --enable-FEATURE and disabled with --disable-FEATURE, default is enabled if available:" How do you think SafeStack should be classified? * If we do it as Advanced option, we should probably force it disabled unless --enable-safe-stack is provided. In this case --disable-safe-stack is not really necessary. * If we do it as optional feature, I have two ways to handle the default: 1. enable/disable based on the compilation flags given to configure 2. enable every time the provided compiler supports it On 5/27/2020 7:12 AM, Stefan Hajnoczi wrote: On Fri, May 22, 2020 at 11:24:46AM -0400, Daniele Buono wrote: I would feel more confident by adding another check in configure to make sure that the user didn't enable SafeStack manually through other means, like manually setting the option through extra_cflags. What do you think? Sure, a compile_prog call could check if SafeStack is enable when it shouldn't be. This can be done together with a --disable option. Stefan
Re: [PATCH 4/4] check-block: Enable iotests with SafeStack
On 5/21/2020 5:59 AM, Stefan Hajnoczi wrote:
On Wed, Apr 29, 2020 at 03:44:20PM -0400, Daniele Buono wrote:
SafeStack is a stack protection technique implemented in llvm. It is
enabled with a -fsanitize flag.
iotests are currently disabled when any -fsanitize option is used.
Since SafeStack is useful on production environments, and its
implementation may break the binary, filter it out when the check is
performed, so that if SafeStack was the only -fsanitize option, iotests
are still performed.
I can't parse this sentence. What does "its implementation may break the
binary" mean? Do you mean it's worth running tests with SafeStack
enabled because it exposes failures that go unnoticed without SafeStack?
What I meant is that, without proper changes, SafeStack breaks
co-routines. Since they are heavily used in the io subsystem, this is
probably the best class of tests to make sure co-routines are working
fine with SafeStack.
I initially re-enabled the iotests for my internal testing.
Since I wasn't seeing any issue, I thought it would be useful to keep
running this to make sure future implementations of SafeStack won't
break co-routines again.
Signed-off-by: Daniele Buono
---
tests/check-block.sh | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tests/check-block.sh b/tests/check-block.sh
index ad320c21ba..8e29c868e5 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -21,7 +21,17 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ;
then
exit 0
fi
-if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then
+# Disable tests with any sanitizer except for SafeStack
+CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
+SANITIZE_FLAGS=""
+#Remove all occurrencies of -fsanitize=safe-stack
+for i in ${CFLAGS}; do
+if [ "${i}" != "-fsanitize=safe-stack" ]; then
+SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
+fi
+done
+if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
+# Have a sanitize flag that is not allowed, stop
echo "Sanitizers are enabled ==> Not running the qemu-iotests."
exit 0
fi
The commit that disabled check-block.sh with sanitizers said:
The sanitizers (especially the address sanitizer from Clang) are
sometimes printing out warnings or false positives - this spoils
the output of the iotests, causing some of the tests to fail.
It seems fine to allow SafeStack if check-block.sh currently passes with
it enabled. Does it pass and produce no extra output?
Yes, that was the idea. SafeStack should pass the tests without extra
output.
It did (pass) on my testing machine. However I don't remember if I did
the full (slow) check or only the partial one.
Will check again before I submit v2
Stefan
Re: [PATCH 3/4] configure: add flag to enable SafeStack
Hi Stefan, I would feel more confident by adding another check in configure to make sure that the user didn't enable SafeStack manually through other means, like manually setting the option through extra_cflags. What do you think? On 5/21/2020 5:52 AM, Stefan Hajnoczi wrote: On Wed, Apr 29, 2020 at 03:44:19PM -0400, Daniele Buono wrote: This patch adds a flag to enable the SafeStack instrumentation provided by LLVM. The checks make sure that the compiler supports the flags, and that we are using the proper coroutine implementation (coroutine-ucontext). While SafeStack is supported only on Linux, NetBSD, FreeBSD and macOS, we are not checking for the O.S. since this is already done by LLVM. Signed-off-by: Daniele Buono --- configure | 29 + 1 file changed, 29 insertions(+) Great, this can become Patch 1 and it can set CONFIG_SAFESTACK as mentioned in my earlier reply. diff --git a/configure b/configure index 23b5e93752..f37e4ae0bd 100755 --- a/configure +++ b/configure @@ -302,6 +302,7 @@ audio_win_int="" libs_qga="" debug_info="yes" stack_protector="" +safe_stack="no" The comment above this says: # Always add --enable-foo and --disable-foo command line args. Please add --disable-safe-stack.
Re: [PATCH 1/4] coroutine: support SafeStack in ucontext backend
Hi Stefan,
thank you so much for reviewing this.
See my answers below:
On 5/21/2020 5:44 AM, Stefan Hajnoczi wrote:
On Wed, Apr 29, 2020 at 03:44:17PM -0400, Daniele Buono wrote:
diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index bd6b0468e1..2ffd75ddbe 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -28,6 +28,12 @@
#include "qemu/queue.h"
#include "qemu/coroutine.h"
+#if defined(__has_feature) && __has_feature(safe_stack)
+#define CONFIG_SAFESTACK 1
Please perform this feature check in ./configure. That way
CONFIG_SAFESTACK will be defined alongside all the other CONFIG_* values
and be available to C and Makefiles via config-host.h and
config-host.mak.
Sure, will do this in v2.
@@ -160,6 +169,19 @@ Coroutine *qemu_coroutine_new(void)
/* swapcontext() in, siglongjmp() back out */
if (!sigsetjmp(old_env, 0)) {
start_switch_fiber(_stack_save, co->stack, co->stack_size);
+#ifdef CONFIG_SAFESTACK
+/*
+ * Before we swap the context, set the new unsafe stack
+ * The unsafe stack grows just like the normal stack, so start from
+ * the last usable location of the memory area.
+ * NOTE: we don't have to re-set it afterwards because sigsetjmp was
+ * called with the original usp. Since we are not coming back with a
+ * swapcontext, but with a siglongjmp, when we are back here we
+ * already have usp restored to the valid one for this function
I don't understand this comment. __safestack_unsafe_stack_ptr is a
thread-local variable, not a CPU register. How will siglongjmp()
automatically restore it?
Correct, setjmp/longjmp have no visibility of the unsafe stack. What I
meant is that it is not automatically restored by the longjmp itself,
but by code the compiler adds around the sigsetjmp.
Specifically, every sigsetjmp/sigjmp is intercepted by the compiler, the
current value of __safestack_unsafe_stack_ptr is saved on the normal
(safe) stack.
Right after the sigsetjmp call it is then restored.
I will change the comment to make it clearer.
In practice, this is what happens:
Original clang implementation in qemu_coroutine_new:
40130c: callq 4008d0 <__sigsetjmp@plt>
401311: cmp$0x0,%eax
401314: jne40132d
Clang Implementation with safestack:
4027a7: mov%rdx,-0x38(%rbp) <- Save unsafe ptr onto the safe stack
[...]
40289c: callq 401410 <__sigsetjmp@plt>
4028a1: mov0x201738(%rip),%rdi# 603fe0
<__safestack_unsafe_stack_ptr@@Base+0x603fe0>
4028a8: mov-0x38(%rbp),%rbx
4028ac: mov%rbx,%fs:(%rdi) <- Restore the unsafe ptr
4028b0: cmp$0x0,%eax
4028b3: jne4028d9
+ */
+void *usp = co->unsafe_stack + co->unsafe_stack_size;
+__safestack_unsafe_stack_ptr = usp;
+#endif
swapcontext(_uc, );
}
Re: [PATCH 0/4] Add support for SafeStack
Hello everybody, just pinging since it it's been a few days. On 5/5/2020 9:56 AM, Philippe Mathieu-Daudé wrote: On 5/5/20 3:31 PM, Daniel P. Berrangé wrote: On Tue, May 05, 2020 at 03:15:18PM +0200, Philippe Mathieu-Daudé wrote: +Alex & Daniel who keep track on CI stuff. On 4/29/20 9:44 PM, Daniele Buono wrote: LLVM supports SafeStack instrumentation to protect against stack buffer overflows, since version 3.7 From https://clang.llvm.org/docs/SafeStack.html: "It works by separating the program stack into two distinct regions: the safe stack and the unsafe stack. The safe stack stores return addresses, register spills, and local variables that are always accessed in a safe way, while the unsafe stack stores everything else. This separation ensures that buffer overflows on the unsafe stack cannot be used to overwrite anything on the safe stack." Unfortunately, the use of two stack regions does not cope well with QEMU's coroutines. The second stack region is not properly set up with both ucontext and sigaltstack, so multiple coroutines end up sharing the same memory area for the unsafe stack, causing undefined behaviors at runtime (and most iochecks to fail). This patch series fixes the implementation of the ucontext backend and make sure that sigaltstack is never used if the compiler is applying the SafeStack instrumentation. It also adds a configure flag to enable SafeStack, and enables iotests when SafeStack is used. This is an RFC mainly because of the low-level use of the SafeStack runtime. When running swapcontext(), we have to manually set the unsafe stack pointer to the new area allocated for the coroutine. LLVM does not allow this by using builtin, so we have to use implementation details that may change in the future. This patch has been tested briefly ( make check on an x86 system ) with clang v3.9, v4.0, v5.0, v6.0 Heavier testing, with make check-acceptance has been performed with clang v7.0 I noticed building using SafeStack is slower, and running with it is even slwer. It makes sense to have this integrated if we use it regularly. Do you have plan for this? Using public CI doesn't seem reasonable. The runtime behaviour is rather odd, given the docs they provide: "The performance overhead of the SafeStack instrumentation is less than 0.1% on average across a variety of benchmarks This is mainly because most small functions do not have any variables that require the unsafe stack and, hence, do not need unsafe stack frames to be created. The cost of creating unsafe stack frames for large functions is amortized by the cost of executing the function. In some cases, SafeStack actually improves the performance" I'm sorry I was testing this with a single core instead of all of them... Thanks for looking at the doc. Regards, Daniel
[PATCH v2 0/1] target-ppc: fix rlwimi, rlwinm, rlwnm for Clang-9
This patch fixes a compilation error with Clang v9 and higher in target/ppc/translate.c, on a comparison that is always true in PPC32 because of type sizes. More information about the issue are in the first version of the patch. v2, changed to avoid the nested ifdef/conditional solution of v1, and keep a code structure more similar to the original. Daniele Buono (1): target-ppc: fix rlwimi, rlwinm, rlwnm for Clang-9 target/ppc/translate.c | 24 +--- 1 file changed, 21 insertions(+), 3 deletions(-) -- 2.26.2
[PATCH v2 1/1] target-ppc: fix rlwimi, rlwinm, rlwnm for Clang-9
Starting with Clang v9, -Wtype-limits is implemented and triggers a
few "result of comparison is always true" errors when compiling PPC32
targets.
The comparisons seem to be necessary only on PPC64, since the
else branch in PPC32 only has a "g_assert_not_reached();" in all cases.
This patch restructures the code so that the actual if/else is done on a
local flag variable, that is set accordingly for PPC64, and always
true for PPC32.
Signed-off-by: Daniele Buono
---
target/ppc/translate.c | 24 +---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 807d14faaa..338529879f 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1882,6 +1882,7 @@ static void gen_rlwimi(DisasContext *ctx)
tcg_gen_deposit_tl(t_ra, t_ra, t_rs, sh, me - mb + 1);
} else {
target_ulong mask;
+bool mask_in_32b = true;
TCGv t1;
#if defined(TARGET_PPC64)
@@ -1890,8 +1891,13 @@ static void gen_rlwimi(DisasContext *ctx)
#endif
mask = MASK(mb, me);
+#if defined(TARGET_PPC64)
+if (mask > 0xu) {
+mask_in_32b = false;
+}
+#endif
t1 = tcg_temp_new();
-if (mask <= 0xu) {
+if (mask_in_32b) {
TCGv_i32 t0 = tcg_temp_new_i32();
tcg_gen_trunc_tl_i32(t0, t_rs);
tcg_gen_rotli_i32(t0, t0, sh);
@@ -1933,12 +1939,18 @@ static void gen_rlwinm(DisasContext *ctx)
tcg_gen_extract_tl(t_ra, t_rs, rsh, len);
} else {
target_ulong mask;
+bool mask_in_32b = true;
#if defined(TARGET_PPC64)
mb += 32;
me += 32;
#endif
mask = MASK(mb, me);
-if (mask <= 0xu) {
+#if defined(TARGET_PPC64)
+if (mask > 0xu) {
+mask_in_32b = false;
+}
+#endif
+if (mask_in_32b) {
if (sh == 0) {
tcg_gen_andi_tl(t_ra, t_rs, mask);
} else {
@@ -1973,6 +1985,7 @@ static void gen_rlwnm(DisasContext *ctx)
uint32_t mb = MB(ctx->opcode);
uint32_t me = ME(ctx->opcode);
target_ulong mask;
+bool mask_in_32b = true;
#if defined(TARGET_PPC64)
mb += 32;
@@ -1980,7 +1993,12 @@ static void gen_rlwnm(DisasContext *ctx)
#endif
mask = MASK(mb, me);
-if (mask <= 0xu) {
+#if defined(TARGET_PPC64)
+if (mask > 0xu) {
+mask_in_32b = false;
+}
+#endif
+if (mask_in_32b) {
TCGv_i32 t0 = tcg_temp_new_i32();
TCGv_i32 t1 = tcg_temp_new_i32();
tcg_gen_trunc_tl_i32(t0, t_rb);
--
2.26.2
Re: [PATCH 1/1] target-ppc: fix rlwimi, rlwinm, rlwnm for Clang-9
On 5/5/2020 1:08 AM, David Gibson wrote:
On Fri, May 01, 2020 at 03:09:13PM -0400, Daniele Buono wrote:
Starting with Clang v9, -Wtype-limits is implemented and triggers a
few "result of comparison is always true" errors when compiling PPC32
targets.
The comparisons seem to be necessary only on PPC64, since the
else branch in PPC32 only has a "g_assert_not_reached();" in all cases.
This patch restructures the code so that PPC32 does not execute the
check, while PPC64 works like before
Signed-off-by: Daniele Buono
Urgh. #ifdefs intertangled with if statements gets pretty ugly. But,
then, it's already pretty ugly, so, applied.
Agreed, it's very ugly. After I sent the patch I thought of an
alternative that looks like this:
bool mask_in_32b = true;
#if defined(TARGET_PPC64)
if (mask > 0xu)
mask_in_32b = false;
#endif
if (mask_in_32b) {
/* Original if-else untouched using mask_in_32b instead of mask*/
It does have an additional if, but with a bit of luck the compiler may
optimize it out (at least for the 32bit case).
If you think the final outcome may be better, let me know and I'll send
a patch v2 like that.
---
target/ppc/translate.c | 34 +++---
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 807d14faaa..9400fa2c7c 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1882,6 +1882,7 @@ static void gen_rlwimi(DisasContext *ctx)
tcg_gen_deposit_tl(t_ra, t_ra, t_rs, sh, me - mb + 1);
} else {
target_ulong mask;
+TCGv_i32 t0;
TCGv t1;
#if defined(TARGET_PPC64)
@@ -1891,20 +1892,20 @@ static void gen_rlwimi(DisasContext *ctx)
mask = MASK(mb, me);
t1 = tcg_temp_new();
+#if defined(TARGET_PPC64)
if (mask <= 0xu) {
-TCGv_i32 t0 = tcg_temp_new_i32();
+#endif
+t0 = tcg_temp_new_i32();
tcg_gen_trunc_tl_i32(t0, t_rs);
tcg_gen_rotli_i32(t0, t0, sh);
tcg_gen_extu_i32_tl(t1, t0);
tcg_temp_free_i32(t0);
-} else {
#if defined(TARGET_PPC64)
+} else {
tcg_gen_deposit_i64(t1, t_rs, t_rs, 32, 32);
tcg_gen_rotli_i64(t1, t1, sh);
-#else
-g_assert_not_reached();
-#endif
}
+#endif
tcg_gen_andi_tl(t1, t1, mask);
tcg_gen_andi_tl(t_ra, t_ra, ~mask);
@@ -1938,7 +1939,9 @@ static void gen_rlwinm(DisasContext *ctx)
me += 32;
#endif
mask = MASK(mb, me);
+#if defined(TARGET_PPC64)
if (mask <= 0xu) {
+#endif
if (sh == 0) {
tcg_gen_andi_tl(t_ra, t_rs, mask);
} else {
@@ -1949,15 +1952,13 @@ static void gen_rlwinm(DisasContext *ctx)
tcg_gen_extu_i32_tl(t_ra, t0);
tcg_temp_free_i32(t0);
}
-} else {
#if defined(TARGET_PPC64)
+} else {
tcg_gen_deposit_i64(t_ra, t_rs, t_rs, 32, 32);
tcg_gen_rotli_i64(t_ra, t_ra, sh);
tcg_gen_andi_i64(t_ra, t_ra, mask);
-#else
-g_assert_not_reached();
-#endif
}
+#endif
}
if (unlikely(Rc(ctx->opcode) != 0)) {
gen_set_Rc0(ctx, t_ra);
@@ -1972,6 +1973,9 @@ static void gen_rlwnm(DisasContext *ctx)
TCGv t_rb = cpu_gpr[rB(ctx->opcode)];
uint32_t mb = MB(ctx->opcode);
uint32_t me = ME(ctx->opcode);
+TCGv_i32 t0;
+TCGv_i32 t1;
+
target_ulong mask;
#if defined(TARGET_PPC64)
@@ -1980,9 +1984,11 @@ static void gen_rlwnm(DisasContext *ctx)
#endif
mask = MASK(mb, me);
+#if defined(TARGET_PPC64)
if (mask <= 0xu) {
-TCGv_i32 t0 = tcg_temp_new_i32();
-TCGv_i32 t1 = tcg_temp_new_i32();
+#endif
+t0 = tcg_temp_new_i32();
+t1 = tcg_temp_new_i32();
tcg_gen_trunc_tl_i32(t0, t_rb);
tcg_gen_trunc_tl_i32(t1, t_rs);
tcg_gen_andi_i32(t0, t0, 0x1f);
@@ -1990,17 +1996,15 @@ static void gen_rlwnm(DisasContext *ctx)
tcg_gen_extu_i32_tl(t_ra, t1);
tcg_temp_free_i32(t0);
tcg_temp_free_i32(t1);
-} else {
#if defined(TARGET_PPC64)
+} else {
TCGv_i64 t0 = tcg_temp_new_i64();
tcg_gen_andi_i64(t0, t_rb, 0x1f);
tcg_gen_deposit_i64(t_ra, t_rs, t_rs, 32, 32);
tcg_gen_rotl_i64(t_ra, t_ra, t0);
tcg_temp_free_i64(t0);
-#else
-g_assert_not_reached();
-#endif
}
+#endif
tcg_gen_andi_tl(t_ra, t_ra, mask);
[PATCH 0/1] target-ppc: fix rlwimi, rlwinm, rlwnm for Clang-9
Until Clang v8, -Wtype-limits was kept for GCC compatibility but had no
effect. With Clang v9, the flag is now implemented ( see
https://tinyurl.com/clang8-manual vs https://tinyurl.com/clang9-manual )
Starting with Clang v9, compiling with -Wtype-limits (default for QEMU)
triggers the following errors on the ppc-softmmu and ppc-linux-user
targets:
/root/qemu/target/ppc/translate.c:1894:18: error: result of comparison
'target_ulong' (aka 'unsigned int') <= 4294967295 is always true
[-Werror,-Wtautological-type-limit-compare]
if (mask <= 0xu) {
^ ~~~
/root/qemu/target/ppc/translate.c:1941:18: error: result of comparison
'target_ulong' (aka 'unsigned int') <= 4294967295 is always true
[-Werror,-Wtautological-type-limit-compare]
if (mask <= 0xu) {
^ ~~~
/root/qemu/target/ppc/translate.c:1983:14: error: result of comparison
'target_ulong' (aka 'unsigned int') <= 4294967295 is always true
[-Werror,-Wtautological-type-limit-compare]
if (mask <= 0xu) {
^ ~~~
The same error can be triggered with this small repro:
int main() {
unsigned int x = 1;
if ( x <= 0xu) return 1;
return 0;
}
$ gcc test.c -Wtype-limits
[nothing]
$ clang-9 test.c -Wtype-limits
test.c:3:12: warning: result of comparison 'unsigned int' <= 4294967295
is always true [-Wtautological-type-limit-compare]
if ( x <= 0xu)
~ ^ ~~~
1 warning generated.
$ clang-9 test.c -Wtype-limits -Wno-tautological-type-limit-compare
[nothing]
We could get away with the compilation error by adding the flag
"-Wno-tautological-type-limit-compare", but I think we should avoid that
to make sure the checks are applied to the rest of the code and warn us
for logical errors in the future.
Looking at the code, the comparison is only needed for PPC64, since
the else branch in PPC32 only has a "g_assert_not_reached();"
This patch restructures the code so that PPC32 always runs the "true"
branch.
I tried to keep the changes to a minimum, to make sure to not affect the
semantics of the instructions. However, considering the target
architecture, my testing has been limited. check-acceptance seems to be
able to properly start a few linux kernels succesfully, and make check
didn't complain on both ppc-softmmu and ppc64-softmmu.
Daniele Buono (1):
target-ppc: fix rlwimi, rlwinm, rlwnm for Clang-9
target/ppc/translate.c | 34 +++---
1 file changed, 19 insertions(+), 15 deletions(-)
--
2.26.2
[PATCH 1/1] target-ppc: fix rlwimi, rlwinm, rlwnm for Clang-9
Starting with Clang v9, -Wtype-limits is implemented and triggers a
few "result of comparison is always true" errors when compiling PPC32
targets.
The comparisons seem to be necessary only on PPC64, since the
else branch in PPC32 only has a "g_assert_not_reached();" in all cases.
This patch restructures the code so that PPC32 does not execute the
check, while PPC64 works like before
Signed-off-by: Daniele Buono
---
target/ppc/translate.c | 34 +++---
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 807d14faaa..9400fa2c7c 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1882,6 +1882,7 @@ static void gen_rlwimi(DisasContext *ctx)
tcg_gen_deposit_tl(t_ra, t_ra, t_rs, sh, me - mb + 1);
} else {
target_ulong mask;
+TCGv_i32 t0;
TCGv t1;
#if defined(TARGET_PPC64)
@@ -1891,20 +1892,20 @@ static void gen_rlwimi(DisasContext *ctx)
mask = MASK(mb, me);
t1 = tcg_temp_new();
+#if defined(TARGET_PPC64)
if (mask <= 0xu) {
-TCGv_i32 t0 = tcg_temp_new_i32();
+#endif
+t0 = tcg_temp_new_i32();
tcg_gen_trunc_tl_i32(t0, t_rs);
tcg_gen_rotli_i32(t0, t0, sh);
tcg_gen_extu_i32_tl(t1, t0);
tcg_temp_free_i32(t0);
-} else {
#if defined(TARGET_PPC64)
+} else {
tcg_gen_deposit_i64(t1, t_rs, t_rs, 32, 32);
tcg_gen_rotli_i64(t1, t1, sh);
-#else
-g_assert_not_reached();
-#endif
}
+#endif
tcg_gen_andi_tl(t1, t1, mask);
tcg_gen_andi_tl(t_ra, t_ra, ~mask);
@@ -1938,7 +1939,9 @@ static void gen_rlwinm(DisasContext *ctx)
me += 32;
#endif
mask = MASK(mb, me);
+#if defined(TARGET_PPC64)
if (mask <= 0xu) {
+#endif
if (sh == 0) {
tcg_gen_andi_tl(t_ra, t_rs, mask);
} else {
@@ -1949,15 +1952,13 @@ static void gen_rlwinm(DisasContext *ctx)
tcg_gen_extu_i32_tl(t_ra, t0);
tcg_temp_free_i32(t0);
}
-} else {
#if defined(TARGET_PPC64)
+} else {
tcg_gen_deposit_i64(t_ra, t_rs, t_rs, 32, 32);
tcg_gen_rotli_i64(t_ra, t_ra, sh);
tcg_gen_andi_i64(t_ra, t_ra, mask);
-#else
-g_assert_not_reached();
-#endif
}
+#endif
}
if (unlikely(Rc(ctx->opcode) != 0)) {
gen_set_Rc0(ctx, t_ra);
@@ -1972,6 +1973,9 @@ static void gen_rlwnm(DisasContext *ctx)
TCGv t_rb = cpu_gpr[rB(ctx->opcode)];
uint32_t mb = MB(ctx->opcode);
uint32_t me = ME(ctx->opcode);
+TCGv_i32 t0;
+TCGv_i32 t1;
+
target_ulong mask;
#if defined(TARGET_PPC64)
@@ -1980,9 +1984,11 @@ static void gen_rlwnm(DisasContext *ctx)
#endif
mask = MASK(mb, me);
+#if defined(TARGET_PPC64)
if (mask <= 0xu) {
-TCGv_i32 t0 = tcg_temp_new_i32();
-TCGv_i32 t1 = tcg_temp_new_i32();
+#endif
+t0 = tcg_temp_new_i32();
+t1 = tcg_temp_new_i32();
tcg_gen_trunc_tl_i32(t0, t_rb);
tcg_gen_trunc_tl_i32(t1, t_rs);
tcg_gen_andi_i32(t0, t0, 0x1f);
@@ -1990,17 +1996,15 @@ static void gen_rlwnm(DisasContext *ctx)
tcg_gen_extu_i32_tl(t_ra, t1);
tcg_temp_free_i32(t0);
tcg_temp_free_i32(t1);
-} else {
#if defined(TARGET_PPC64)
+} else {
TCGv_i64 t0 = tcg_temp_new_i64();
tcg_gen_andi_i64(t0, t_rb, 0x1f);
tcg_gen_deposit_i64(t_ra, t_rs, t_rs, 32, 32);
tcg_gen_rotl_i64(t_ra, t_ra, t0);
tcg_temp_free_i64(t0);
-#else
-g_assert_not_reached();
-#endif
}
+#endif
tcg_gen_andi_tl(t_ra, t_ra, mask);
--
2.26.2
[PATCH 3/4] configure: add flag to enable SafeStack
This patch adds a flag to enable the SafeStack instrumentation provided
by LLVM.
The checks make sure that the compiler supports the flags, and that we
are using the proper coroutine implementation (coroutine-ucontext).
While SafeStack is supported only on Linux, NetBSD, FreeBSD and macOS,
we are not checking for the O.S. since this is already done by LLVM.
Signed-off-by: Daniele Buono
---
configure | 29 +
1 file changed, 29 insertions(+)
diff --git a/configure b/configure
index 23b5e93752..f37e4ae0bd 100755
--- a/configure
+++ b/configure
@@ -302,6 +302,7 @@ audio_win_int=""
libs_qga=""
debug_info="yes"
stack_protector=""
+safe_stack="no"
use_containers="yes"
gdb_bin=$(command -v "gdb")
@@ -1275,6 +1276,8 @@ for opt do
;;
--disable-stack-protector) stack_protector="no"
;;
+ --enable-safe-stack) safe_stack="yes"
+ ;;
--disable-curses) curses="no"
;;
--enable-curses) curses="yes"
@@ -1774,6 +1777,8 @@ Advanced options (experts only):
--with-coroutine=BACKEND coroutine backend. Supported options:
ucontext, sigaltstack, windows
--enable-gcovenable test coverage analysis with gcov
+ --enable-safe-stack enable the SafeStack stack protection. Depends on
+ clang/llvm >= 3.7 and coroutine backend ucontext.
--gcov=GCOV use specified gcov [$gcov_tool]
--disable-blobs disable installing provided firmware blobs
--with-vss-sdk=SDK-path enable Windows VSS support in QEMU Guest Agent
@@ -5501,6 +5506,29 @@ if test "$debug_stack_usage" = "yes"; then
fi
fi
+##
+# Check if SafeStack is enabled and supported
+
+if test "$safe_stack" = "yes"; then
+ cat > $TMPC << EOF
+int main(int argc, char *argv[])
+{
+return 0;
+}
+EOF
+ flag="-fsanitize=safe-stack"
+ # Check that safe-stack is supported.
+ if compile_prog "-Werror $flag" ""; then
+# Flag needed both at compilation and at linking
+QEMU_CFLAGS="$QEMU_CFLAGS $flag"
+QEMU_LDFLAGS="$QEMU_LDFLAGS $flag"
+ else
+error_exit "SafeStack not supported by your compiler"
+ fi
+ if test "$coroutine" != "ucontext"; then
+error_exit "SafeStack is only supported by the coroutine backend ucontext"
+ fi
+fi
##
# check if we have open_by_handle_at
@@ -6595,6 +6623,7 @@ echo "sparse enabled$sparse"
echo "strip binaries$strip_opt"
echo "profiler $profiler"
echo "static build $static"
+echo "safe stack$safe_stack"
if test "$darwin" = "yes" ; then
echo "Cocoa support $cocoa"
fi
--
2.26.2
[PATCH 2/4] coroutine: Add check for SafeStack in sigalstack
LLVM's SafeStack instrumentation cannot be used inside signal handlers
that make use of sigaltstack().
Since coroutine-sigaltstack relies on sigaltstack(), it is not
compatible with SafeStack. The resulting binary is incorrect, with
different coroutines sharing the same unsafe stack and producing
undefined behavior at runtime.
To avoid this, we add a check in coroutine-sigaltstack that throws a
preprocessor #error and interrupt the compilation if SafeStack is
enabled.
Signed-off-by: Daniele Buono
---
util/coroutine-sigaltstack.c | 4
1 file changed, 4 insertions(+)
diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index f6fc49a0e5..b7cdc959f8 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -30,6 +30,10 @@
#include "qemu-common.h"
#include "qemu/coroutine_int.h"
+#ifdef CONFIG_SAFESTACK
+#error "SafeStack does not work with sigaltstack's implementation"
+#endif
+
typedef struct {
Coroutine base;
void *stack;
--
2.26.2
[PATCH 4/4] check-block: Enable iotests with SafeStack
SafeStack is a stack protection technique implemented in llvm. It is
enabled with a -fsanitize flag.
iotests are currently disabled when any -fsanitize option is used.
Since SafeStack is useful on production environments, and its
implementation may break the binary, filter it out when the check is
performed, so that if SafeStack was the only -fsanitize option, iotests
are still performed.
Signed-off-by: Daniele Buono
---
tests/check-block.sh | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tests/check-block.sh b/tests/check-block.sh
index ad320c21ba..8e29c868e5 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -21,7 +21,17 @@ if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ;
then
exit 0
fi
-if grep -q "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null ; then
+# Disable tests with any sanitizer except for SafeStack
+CFLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
+SANITIZE_FLAGS=""
+#Remove all occurrencies of -fsanitize=safe-stack
+for i in ${CFLAGS}; do
+if [ "${i}" != "-fsanitize=safe-stack" ]; then
+SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
+fi
+done
+if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
+# Have a sanitize flag that is not allowed, stop
echo "Sanitizers are enabled ==> Not running the qemu-iotests."
exit 0
fi
--
2.26.2
[PATCH 1/4] coroutine: support SafeStack in ucontext backend
LLVM's SafeStack instrumentation does not yet support programs that make
use of the APIs in ucontext.h
With the current implementation of coroutine-ucontext, the resulting
binary is incorrect, with different coroutines sharing the same unsafe
stack and producing undefined behavior at runtime.
This fix allocates an additional unsafe stack area for each coroutine,
and sets the new unsafe stack pointer before calling swapcontext() in
qemu_coroutine_new.
This is the only place where the pointer needs to be manually updated,
since sigsetjmp/siglongjmp are already instrumented by LLVM to properly
support SafeStack.
The additional stack is then freed in qemu_coroutine_delete.
Signed-off-by: Daniele Buono
---
include/qemu/coroutine_int.h | 6 ++
util/coroutine-ucontext.c| 25 +
2 files changed, 31 insertions(+)
diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index bd6b0468e1..2ffd75ddbe 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -28,6 +28,12 @@
#include "qemu/queue.h"
#include "qemu/coroutine.h"
+#if defined(__has_feature) && __has_feature(safe_stack)
+#define CONFIG_SAFESTACK 1
+/* Pointer to the unsafe stack, defined by the compiler */
+extern __thread void *__safestack_unsafe_stack_ptr;
+#endif
+
#define COROUTINE_STACK_SIZE (1 << 20)
typedef enum {
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index bd593e61bc..b79e9df9eb 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -41,6 +41,11 @@ typedef struct {
Coroutine base;
void *stack;
size_t stack_size;
+#ifdef CONFIG_SAFESTACK
+/* Need an unsafe stack for each coroutine */
+void *unsafe_stack;
+size_t unsafe_stack_size;
+#endif
sigjmp_buf env;
#ifdef CONFIG_VALGRIND_H
@@ -140,6 +145,10 @@ Coroutine *qemu_coroutine_new(void)
co = g_malloc0(sizeof(*co));
co->stack_size = COROUTINE_STACK_SIZE;
co->stack = qemu_alloc_stack(>stack_size);
+#ifdef CONFIG_SAFESTACK
+co->unsafe_stack_size = COROUTINE_STACK_SIZE;
+co->unsafe_stack = qemu_alloc_stack(>unsafe_stack_size);
+#endif
co->base.entry_arg = _env; /* stash away our jmp_buf */
uc.uc_link = _uc;
@@ -160,6 +169,19 @@ Coroutine *qemu_coroutine_new(void)
/* swapcontext() in, siglongjmp() back out */
if (!sigsetjmp(old_env, 0)) {
start_switch_fiber(_stack_save, co->stack, co->stack_size);
+#ifdef CONFIG_SAFESTACK
+/*
+ * Before we swap the context, set the new unsafe stack
+ * The unsafe stack grows just like the normal stack, so start from
+ * the last usable location of the memory area.
+ * NOTE: we don't have to re-set it afterwards because sigsetjmp was
+ * called with the original usp. Since we are not coming back with a
+ * swapcontext, but with a siglongjmp, when we are back here we
+ * already have usp restored to the valid one for this function
+ */
+void *usp = co->unsafe_stack + co->unsafe_stack_size;
+__safestack_unsafe_stack_ptr = usp;
+#endif
swapcontext(_uc, );
}
@@ -192,6 +214,9 @@ void qemu_coroutine_delete(Coroutine *co_)
#endif
qemu_free_stack(co->stack, co->stack_size);
+#ifdef CONFIG_SAFESTACK
+qemu_free_stack(co->unsafe_stack, co->unsafe_stack_size);
+#endif
g_free(co);
}
--
2.26.2
