[PATCH v2 0/6] Add support for Control-Flow Integrity

[PATCH v2 0/6] Add support for Control-Flow Integrity

[Daniele Buono]

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.
* 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. The decorator is automatically expanded to an attribute
  asking clang to disable cfi-icall on the function.
  This should simplify tracking of sensitive function, compared to
  keeping the list in a separate file
  I tentatively added myself as maintainer of a new include file defined for
  that purpose, in case a maintainer is considered needed.
* Updated patch to work with the new build system based on meson
* Split LTO and CFI options. Now LTO can be used independently of CFI.
  LTO uses the meson option to build and can now work, in general, with
  any linker (ld, gold, lld). LTO with meson works fine with clang >=6
  and requires the use of LLVM's ar to handle shared libraries with
  intermediate code (selectable by setting the environment variable
  AR to llvm-ar-xx).
* Introduce a small patch for the linker script used by fuzzing targets,
  so that it works properly with both bfd and lld >=12
* Disable a couple of warning check that trigger errors with clang >= 11 
* Add additional checks for fuzzing and LTO. At the moment, only LLVM's
  lld linker v12 is able to support fuzzing and LTO, because of a bug in the
  bfd linker when handling --wrap with LTO. Therefore, automatically
  select lld if fuzzing and LTO are both enabled.
* Made sure that fuzzing works with LTO and CFI enabled.

-----
v1's cover letter starts here
-----

LLVM/Clang supports multiple types of forward-edge Control-Flow Integrity
(CFI), including runtime checks on indirect function calls.

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

*** 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

[Daniele Buono]

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 <dbuono@linux.vnet.ibm.com>
---
 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 v2 2/6] configure: avoid new clang 11+ warnings

[Daniele Buono]

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_iu                    status;
                              ^
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 <dbuono@linux.vnet.ibm.com>
---
 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

[Daniele Buono]

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 <dbuono@linux.vnet.ibm.com>
---
 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 the linker. This usually happens when mixing GNU and LLVM toolchain."
+  fi
+
+  #### All good, add the flags for CFI to our CFLAGS and LDFLAGS
+  # Flag needed both at compilation and at linking
+  QEMU_LDFLAGS="$QEMU_LDFLAGS $test_ldflag"
+  # Add -flto to CONFIGURE_*FLAGS since we need it in configure,
+  # but will be added by meson later
+  CONFIGURE_CFLAGS="$QEMU_CFLAGS -flto"
+  CONFIGURE_LDFLAGS="$QEMU_LDFLAGS -flto"
+fi
 # See if __attribute__((alias)) is supported.
 # This false for Xcode 9, but has been remedied for Xcode 10.
 # Unfortunately, travis uses Xcode 9 by default.
@@ -5532,6 +5622,43 @@ if test "$fuzzing" = "yes" && test -z "${LIB_FUZZING_ENGINE+xxx}"; then
     error_exit "Your compiler doesn't support -fsanitize=fuzzer"
     exit 1
   fi
+  # Make sure that the linker supports a custom linker script
+  # If LTO is enabled, switch linker to lld, since at the moment
+  # it is the only linker that works with lto and fuzzing:
+  # - gold does not support a custom script
+  # - bfd does not support wrapping functions with LTO
+  cat > $TMPC << EOF
+#include <stdlib.h>
+#include <stdio.h>
+void* __real_malloc(size_t size);
+void* __wrap_malloc(size_t size);
+
+void* __wrap_malloc(size_t size){
+  printf("Inside wrap_malloc\n");
+  return __real_malloc(size);
+}
+
+int main(int argc, char *argv[]) {
+  int *myint = (void*) malloc(sizeof(int));
+  *myint = 0;
+  return *myint;
+}
+EOF
+  extra_cflags="$CPU_CFLAGS -Werror"
+  extra_ldflags="-Wl,-T,${source_path}/tests/qtest/fuzz/fork_fuzz.ld"
+  extra_ldflags="${extra_ldflags} -Wl,--wrap,malloc"
+  if test "$lto" = "true"; then
+     extra_ldflags="${extra_ldflags} -fuse-ld=lld"
+  fi
+  if ! compile_prog "$extra_cflags" "$extra_ldflags"; then
+    error_exit "Your linker does not support our linker script"
+  fi
+  if ! do_run_filter "yes" "Inside wrap_malloc" ${TMPE} ""; then
+    error_exit "Your linker does not support our linker script"
+  fi
+  if test "$lto" = "true"; then
+     QEMU_LDFLAGS="${QEMU_LDFLAGS} -fuse-ld=lld"
+  fi
 fi
 
 # Thread sanitizer is, for now, much noisier than the other sanitizers;
@@ -7018,6 +7145,7 @@ NINJA=$ninja $meson setup \
         -Dcapstone=$capstone -Dslirp=$slirp -Dfdt=$fdt \
         -Diconv=$iconv -Dcurses=$curses -Dlibudev=$libudev\
         -Ddocs=$docs -Dsphinx_build=$sphinx_build \
+        -Db_lto=$lto \
         $cross_arg \
         "$PWD" "$source_path"
 
diff --git a/meson.build b/meson.build
index 7627a0ae46..50e5c527df 100644
--- a/meson.build
+++ b/meson.build
@@ -1959,6 +1959,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 v2 4/6] cfi: Initial support for cfi-icall in QEMU

[Daniele Buono]

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 <dbuono@linux.vnet.ibm.com>
---
 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 <dbuono@linux.vnet.ibm.com>
+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 0000000000..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 <dbuono@linux.vnet.ibm.com>
+ *
+ * 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;
@@ -111,6 +116,10 @@ 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 */
+__disable_cfi__
 static void plugin_cb__simple(enum qemu_plugin_event ev)
 {
     struct qemu_plugin_cb *cb, *next;
@@ -128,6 +137,10 @@ 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 */
+__disable_cfi__
 static void plugin_cb__udata(enum qemu_plugin_event ev)
 {
     struct qemu_plugin_cb *cb, *next;
@@ -325,6 +338,10 @@ 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 */
+__disable_cfi__
 void qemu_plugin_tb_trans_cb(CPUState *cpu, struct qemu_plugin_tb *tb)
 {
     struct qemu_plugin_cb *cb, *next;
@@ -339,6 +356,10 @@ 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 */
+__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 +379,10 @@ 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 */
+__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..d193c5772e 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -32,6 +32,7 @@
 #ifndef CONFIG_USER_ONLY
 #include "hw/boards.h"
 #endif
+#include "qemu/sanitizers.h"
 
 #include "plugin.h"
 
@@ -150,6 +151,10 @@ static uint64_t xorshift64star(uint64_t x)
     return x * UINT64_C(2685821657736338717);
 }
 
+/* Disable CFI checks.
+ * The install and version functions have been loaded from an external library
+ * so we do not have type information */
+__disable_cfi__
 static int plugin_load(struct qemu_plugin_desc *desc, const qemu_info_t *info)
 {
     qemu_plugin_install_func_t install;
diff --git a/tcg/tci.c b/tcg/tci.c
index 82039fd163..b82cae3d24 100644
--- a/tcg/tci.c
+++ b/tcg/tci.c
@@ -31,6 +31,7 @@
 #include "tcg/tcg.h"           /* MAX_OPC_PARAM_IARGS */
 #include "exec/cpu_ldst.h"
 #include "tcg/tcg-op.h"
+#include "qemu/sanitizers.h"
 
 /* Marker for missing code. */
 #define TODO() \
@@ -475,6 +476,10 @@ static bool tci_compare64(uint64_t u0, uint64_t u1, TCGCond condition)
 #endif
 
 /* Interpret pseudo code in tb. */
+/* Disable CFI checks.
+ * One possible operation in the pseudo code is a call to binary code.
+ * Therefore, disable CFI checks in the interpreter function */
+__disable_cfi__
 uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
 {
     tcg_target_ulong regs[TCG_TARGET_NB_REGS];
diff --git a/util/main-loop.c b/util/main-loop.c
index 6470f8eae3..610ef26cb0 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -33,6 +33,7 @@
 #include "block/aio.h"
 #include "qemu/error-report.h"
 #include "qemu/queue.h"
+#include "qemu/sanitizers.h"
 
 #ifndef _WIN32
 #include <sys/wait.h>
@@ -44,6 +45,14 @@
  * use signalfd to listen for them.  We rely on whatever the current signal
  * handler is to dispatch the signals when we receive them.
  */
+/* Disable CFI checks.
+ * We are going to call a signal hander directly. Such handler may or may not
+ * have been defined in our binary, so there's no guarantee that the pointer
+ * used to set the handler is a cfi-valid pointer. Since the handlers are
+ * stored in kernel memory, changing the handler to an attacker-defined
+ * function requires being able to call a sigaction() syscall,
+ * which is not as easy as overwriting a pointer in memory. */
+__disable_cfi__
 static void sigfd_handler(void *opaque)
 {
     int fd = (intptr_t)opaque;
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index f15234b5c0..099c50acc1 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -39,6 +39,7 @@
 #include "qemu/thread.h"
 #include <libgen.h>
 #include "qemu/cutils.h"
+#include "qemu/sanitizers.h"
 
 #ifdef CONFIG_LINUX
 #include <sys/syscall.h>
@@ -773,6 +774,14 @@ void qemu_free_stack(void *stack, size_t sz)
     munmap(stack, sz);
 }
 
+/* Disable CFI checks.
+ * We are going to call a signal hander directly. Such handler may or may not
+ * have been defined in our binary, so there's no guarantee that the pointer
+ * used to set the handler is a cfi-valid pointer. Since the handlers are
+ * stored in kernel memory, changing the handler to an attacker-defined
+ * function requires being able to call a sigaction() syscall,
+ * which is not as easy as overwriting a pointer in memory. */
+__disable_cfi__
 void sigaction_invoke(struct sigaction *action,
                       struct qemu_signalfd_siginfo *info)
 {
-- 
2.17.1

[PATCH v2 5/6] check-block: enable iotests with cfi-icall

[Daniele Buono]

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 <dbuono@linux.vnet.ibm.com>
---
 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 6/6] configure: add support for Control-Flow Integrity

[Daniele Buono]

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 <dbuono@linux.vnet.ibm.com>
---
 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" = "yes"; then
+    error_exit "Cannot enable Control Flow Integrity debugging since CFI is not enabled"
+  fi
+fi
+
 # See if __attribute__((alias)) is supported.
 # This false for Xcode 9, but has been remedied for Xcode 10.
 # Unfortunately, travis uses Xcode 9 by default.
@@ -6972,6 +7049,13 @@ if test "$safe_stack" = "yes"; then
   echo "CONFIG_SAFESTACK=y" >> $config_host_mak
 fi
 
+if test "$cfi" = "yes"; then
+  echo "CONFIG_CFI=y" >> $config_host_mak
+  if test "$cfi_debug" = "yes"; then
+    echo "CONFIG_CFI_DEBUG=y" >> $config_host_mak
+  fi
+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
diff --git a/meson.build b/meson.build
index 50e5c527df..be74a232a0 100644
--- a/meson.build
+++ b/meson.build
@@ -2071,6 +2071,8 @@ if targetos == 'windows'
   summary_info += {'QGA MSI support':   config_host.has_key('CONFIG_QGA_MSI')}
 endif
 summary_info += {'seccomp support':   config_host.has_key('CONFIG_SECCOMP')}
+summary_info += {'cfi support':       config_host.has_key('CONFIG_CFI')}
+summary_info += {'cfi debug support': config_host.has_key('CONFIG_CFI_DEBUG')}
 summary_info += {'coroutine backend': config_host['CONFIG_COROUTINE_BACKEND']}
 summary_info += {'coroutine pool':    config_host['CONFIG_COROUTINE_POOL'] == '1'}
 summary_info += {'debug stack usage': config_host.has_key('CONFIG_DEBUG_STACK_USAGE')}
-- 
2.17.1

Re: [PATCH v2 0/6] Add support for Control-Flow Integrity

[Eric Blake]

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é <philmd@redhat.com>
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?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings

[Thomas Huth]

On 23/10/2020 22.06, Daniele Buono wrote:
> 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_iu                    status;
>                               ^
> 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

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


> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> ---
>  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"
> 

Re: [PATCH v2 0/6] Add support for Control-Flow Integrity

[Daniele Buono]

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é <philmd@redhat.com>
> 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.

Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings

[Daniele Buono]

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

[Daniel P. Berrangé]

On Fri, Oct 23, 2020 at 03:33:31PM -0500, 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é <philmd@redhat.com>
> 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.

Something like  cfi-exclude-list.txt or cfi-skip-list.txt seems reasonable


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

Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings

[Paolo Bonzini]

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 3/6] configure: add option to enable LTO

[Paolo Bonzini]

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 <dbuono@linux.vnet.ibm.com>

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.

Paolo

Re: [PATCH v2 4/6] cfi: Initial support for cfi-icall in QEMU

[Paolo Bonzini]

On 23/10/20 22:06, Daniele Buono wrote:
> +
> +#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

__disable_cfi__ is a reserved identifier, since it starts with two
underscores.  Please name it QEMU_DISABLE_CFI and put it in
include/qemu/compiler.h.

Thanks,

Paolo

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

[Paolo Bonzini]

On 23/10/20 22:06, Daniele Buono wrote:
> +
> +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"

Can you pass both options to the linker for simplicity?

Unless you need to add the flag to CONFIGURE_CFLAGS/CONFIGURE_LDFLAGS,
please do all the tests in meson instead, it's much simpler to do
something like

if get_option('cfi')
  cfi_flags=['-fsanitize=cfi-icall',
             '-fsanitize-cfi-icall-generalize-pointers']
  if get_option('cfi_debug')
    cfi_flags += 'fno-sanitize-trap=cfi-icall'
  endif
  if cc.get_supported_arguments(cfi_flags).length() != cfi_flags.length()
    error('...')
  endif
  add_project_arguments(cfi_flags, native: false, language: ['c', 'cpp',
'objc'])
)
  add_project_link_arguments(cfi_flags, native: false, language: ['c',
'cpp', 'objc'])
)
endif

> +  if test "$cfi_debug" = "yes"; then
> +    error_exit "Cannot enable Control Flow Integrity debugging since CFI is not enabled"
> +  fi
> +fi

Generally dependent options are ignored so you can remove this part.

Paolo

Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings

[Daniele Buono]

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

[Paolo Bonzini]

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 3/6] configure: add option to enable LTO

[Daniel P. Berrangé]

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 <dbuono@linux.vnet.ibm.com>
> 
> 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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings

[Daniele Buono]

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

[Peter Maydell]

On Mon, 26 Oct 2020 at 15:13, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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.

Do you mean converting a C flexible array member (declared as
"foo[]") into a gcc zero-length array (declared as "foo[0]") ?
That seems like it would be going backwards from the direction
we started in with commits f7795e4096d8bd1c and 880a7817c1a82
of preferring flexible arrays to the GNU extension, so it's
maybe not ideal, but I guess it's expedient.

thanks
-- PMM

Re: [PATCH v2 4/6] cfi: Initial support for cfi-icall in QEMU

[Alex Bennée]

Daniele Buono <dbuono@linux.vnet.ibm.com> writes:

> 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 <dbuono@linux.vnet.ibm.com>
> ---
>  MAINTAINERS               |  5 +++++
>  accel/tcg/cpu-exec.c      |  9 +++++++++
>  include/qemu/sanitizers.h | 22 ++++++++++++++++++++++
>  plugins/core.c            | 25 +++++++++++++++++++++++++
>  plugins/loader.c          |  5 +++++

With the changes Paolo suggested (QEMU_DISABLE_CFI and use compilers.h)
then for the plugin bits:

Acked-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings

[Thomas Huth]

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.

 Thomas

Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings

[Cornelia Huck]

On Tue, 27 Oct 2020 12:26:21 +0100
Thomas Huth <thuth@redhat.com> 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

[Daniele Buono]

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 <dbuono@linux.vnet.ibm.com>
>>
>> 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 3/6] configure: add option to enable LTO

[Daniel P. Berrangé]

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 <dbuono@linux.vnet.ibm.com>
> > > 
> > > 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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/6] configure: avoid new clang 11+ warnings

[Daniele Buono]

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 <thuth@redhat.com> 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

[Daniele Buono]

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 = &scsi_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 <dbuono@linux.vnet.ibm.com>
>>>>
>>>> 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 3/6] configure: add option to enable LTO

[Paolo Bonzini]

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

[Alex Bennée]

Daniele Buono <dbuono@linux.vnet.ibm.com> writes:

> 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.

sparc64-linux-user? I think that might be in a bit of a bit rotted state
- we had to disable running check-tcg on it in CI because of instability
so I wouldn't be surprised if messing around with LTO has dug up even
more gremlins.

> 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.

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.

>
> 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 <dbuono@linux.vnet.ibm.com>
>>>
>>> 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
>> 


-- 
Alex Bennée

Re: [PATCH v2 3/6] configure: add option to enable LTO

[Daniele Buono]

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

[Daniele Buono]

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

[Paolo Bonzini]

On 28/10/20 19:22, Daniele Buono wrote:
> 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.

I think both of these count as user error or compiler bug, which we
generally don't protect against.

There is one exception.  We check if the C++ compiler driver can link
object files produced by the C compiler driver; this issue arises if the
driver used for compilation (C) is GCC and the driver used for linking
(C++) is clang, because GCC and clang's sanitizer libraries are not
compatible with each other.

I think however that in this case the problem is not one of
compatibility, but just a broken install, so I think we can just ignore
and just forward b_lto.

Paolo