[PATCH v3 0/9] Add support for Control-Flow Integrity

[PATCH v3 0/9] Add support for Control-Flow Integrity

[Daniele Buono]

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

[PATCH v3 1/9] 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 v3 2/9] s390x: fix clang 11 warnings in cpu_models.c

[Daniele Buono]

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 <dbuono@linux.vnet.ibm.com>
---
 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 3/9] hw/usb: reorder fields in UASStatus

[Daniele Buono]

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_iu                    status;
                              ^
1 error generated.

Fix this by moving uas_iu at the end of the struct

Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
---
 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_iu                    status;
     uint32_t                  length;
     QTAILQ_ENTRY(UASStatus)   next;
+    uas_iu                    status;
 };
 
 /* --------------------------------------------------------------------- */
-- 
2.17.1

[PATCH v3 4/9] s390x: Avoid variable size warning in ipl.h

[Daniele Buono]

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 <dbuono@linux.vnet.ibm.com>
---
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 5/9] scsi: fix overflow in scsi_disk_new_request_dump

[Daniele Buono]

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 <dbuono@linux.vnet.ibm.com>
---
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 6/9] configure,meson: 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 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 <dbuono@linux.vnet.ibm.com>
---
 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 7/9] 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

Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
Acked-by: Alex Bennée <alex.bennee@linaro.org> 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_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)
     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
+ */
+QEMU_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..5d97b7c71c 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/compiler.h"
 
 /* Marker for missing code. */
 #define TODO() \
@@ -475,6 +476,12 @@ 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
+ */
+QEMU_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..6bfc7c46f5 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/compiler.h"
 
 #ifndef _WIN32
 #include <sys/wait.h>
@@ -44,6 +45,16 @@
  * 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.
+ */
+QEMU_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..f1e2801b11 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/compiler.h"
 
 #ifdef CONFIG_LINUX
 #include <sys/syscall.h>
@@ -773,6 +774,16 @@ 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.
+ */
+QEMU_DISABLE_CFI
 void sigaction_invoke(struct sigaction *action,
                       struct qemu_signalfd_siginfo *info)
 {
-- 
2.17.1

[PATCH v3 8/9] 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 v3 9/9] configure,meson: support 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.

All the checks are performed in meson.build. configure is only used to
forward the flags to meson

Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
---
 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_arguments(cfi_flags, native: false, language: ['c', 'cpp',
+                                                             'objc'])
+  add_project_link_arguments(cfi_flags, native: false, language: ['c', 'cpp',
+                                                                  'objc'])
+endif
+
 #################
 # config-host.h #
 #################
@@ -784,6 +826,7 @@ config_host_data.set('CONFIG_KEYUTILS', keyutils.found())
 config_host_data.set('CONFIG_GETTID', has_gettid)
 config_host_data.set('CONFIG_MALLOC_TRIM', has_malloc_trim)
 config_host_data.set('CONFIG_STATX', has_statx)
+config_host_data.set('CONFIG_CFI', get_option('cfi').enabled())
 config_host_data.set('QEMU_VERSION', '"@0@"'.format(meson.project_version()))
 config_host_data.set('QEMU_VERSION_MAJOR', meson.project_version().split('.')[0])
 config_host_data.set('QEMU_VERSION_MINOR', meson.project_version().split('.')[1])
@@ -2136,6 +2179,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':       get_option('cfi').enabled()}
+summary_info += {'cfi debug support': get_option('cfi_debug').enabled()}
 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')}
diff --git a/meson_options.txt b/meson_options.txt
index b4f1801875..582069c9ed 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -35,6 +35,10 @@ option('xen_pci_passthrough', type: 'feature', value: 'auto',
        description: 'Xen PCI passthrough support')
 option('tcg', type: 'feature', value: 'auto',
        description: 'TCG support')
+option('cfi', type: 'feature', value: 'auto',
+       description: 'Control-Flow Integrity (CFI)')
+option('cfi_debug', type: 'feature', value: 'auto',
+       description: 'Verbose errors in case of CFI violation')
 
 option('cocoa', type : 'feature', value : 'auto',
        description: 'Cocoa user interface (macOS only)')
-- 
2.17.1

Re: [PATCH v3 0/9] Add support for Control-Flow Integrity

[Cornelia Huck]

On Thu,  5 Nov 2020 17:18:56 -0500
Daniele Buono <dbuono@linux.vnet.ibm.com> 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(-)
> 

Re: [PATCH v3 0/9] Add support for Control-Flow Integrity

[Daniele Buono]

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

Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus

[Philippe Mathieu-Daudé]

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.

>     uas_iu                    status;
>                               ^
> 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.

> 
> Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>
> ---
>  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_iu                    status;
>      uint32_t                  length;
>      QTAILQ_ENTRY(UASStatus)   next;
> +    uas_iu                    status;
>  };
>  
>  /* --------------------------------------------------------------------- */
> 

Re: [PATCH-for-5.2? v3 5/9] scsi: fix overflow in scsi_disk_new_request_dump

[Philippe Mathieu-Daudé]

On 11/5/20 11:19 PM, Daniele Buono wrote:
> 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 <dbuono@linux.vnet.ibm.com>
> ---
> 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++) {
> 

I think scsi_cdb_length() should always return >=1,
and scsi_req_parse_cdb() return if len <= 1.

Re: [PATCH-for-5.2? v3 5/9] scsi: fix overflow in scsi_disk_new_request_dump

[Philippe Mathieu-Daudé]

On 11/6/20 3:32 PM, Philippe Mathieu-Daudé wrote:
> On 11/5/20 11:19 PM, Daniele Buono wrote:
>> 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 <dbuono@linux.vnet.ibm.com>
>> ---
>> 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++) {
>>
> 
> I think scsi_cdb_length() should always return >=1,
> and scsi_req_parse_cdb() return if len <= 1.

Looking at how this works, scsi_req_new() shouldn't take
only a pointer to buffer without knowing its size...
We should add a buflen argument and propagate it.

Then we can check if scsi_cdb_length() <= buflen,
and dump buflen if unknown opcode.

Regards,

Phil.

Re: [PATCH v3 1/9] fuzz: Make fork_fuzz.ld compatible with LLVM's LLD

[Alexander Bulekov]

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 <alxndr@bu.edu>
Tested-by: Alexander Bulekov <alxndr@bu.edu>

Thanks

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

Re: [PATCH v3 0/9] Add support for Control-Flow Integrity

[Alexander Bulekov]

On 201106 0835, Daniele Buono wrote:
> 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)

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

I'm not sure what the status of LTO/LLD support is on oss-fuzz/libfuzzer. There
are some sparse mentions of lld/lto in the repo:
https://github.com/google/oss-fuzz/issues/933
https://github.com/google/oss-fuzz/pull/3597

I haven't found any projects actively using lld on oss-fuzz, but I might
not be grepping hard enough. I personally haven't tried building the
fuzzers with LTO yet, but it seems like a good idea. I'll try it out.

-Alex

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

Re: [PATCH v3 2/9] s390x: fix clang 11 warnings in cpu_models.c

[Cornelia Huck]

On Thu,  5 Nov 2020 17:18:58 -0500
Daniele Buono <dbuono@linux.vnet.ibm.com> wrote:

> 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 <dbuono@linux.vnet.ibm.com>
> ---
>  target/s390x/cpu_models.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v3 4/9] s390x: Avoid variable size warning in ipl.h

[Cornelia Huck]

On Thu,  5 Nov 2020 17:19:00 -0500
Daniele Buono <dbuono@linux.vnet.ibm.com> wrote:

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

We can easily do that in the future clean-up round, if we want that.

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

For now, this is a nicely small patch.

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH-for-5.2? v3 5/9] scsi: fix overflow in scsi_disk_new_request_dump

[Philippe Mathieu-Daudé]

On 11/6/20 3:43 PM, Philippe Mathieu-Daudé wrote:
> On 11/6/20 3:32 PM, Philippe Mathieu-Daudé wrote:
>> On 11/5/20 11:19 PM, Daniele Buono wrote:
>>> 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 <dbuono@linux.vnet.ibm.com>
>>> ---
>>> 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++) {
>>>
>>
>> I think scsi_cdb_length() should always return >=1,
>> and scsi_req_parse_cdb() return if len <= 1.
> 
> Looking at how this works, scsi_req_new() shouldn't take
> only a pointer to buffer without knowing its size...
> We should add a buflen argument and propagate it.
> 
> Then we can check if scsi_cdb_length() <= buflen,
> and dump buflen if unknown opcode.

I did it. Will post later as this is 6.0 material.

> 
> Regards,
> 
> Phil.
> 
> 

Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus

[Daniele Buono]

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_iu                    status;
>>                                ^
>> 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, &iu, 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 <dbuono@linux.vnet.ibm.com>
>> ---
>>   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_iu                    status;
>>       uint32_t                  length;
>>       QTAILQ_ENTRY(UASStatus)   next;
>> +    uas_iu                    status;
>>   };
>>   
>>   /* --------------------------------------------------------------------- */
>>
> 

Re: [PATCH-for-5.2? v3 5/9] scsi: fix overflow in scsi_disk_new_request_dump

[Daniele Buono]

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 v3 0/9] Add support for Control-Flow Integrity

[Daniele Buono]

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 v3 1/9] fuzz: Make fork_fuzz.ld compatible with LLVM's LLD

[Daniele Buono]

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
[...]
.got                    10704   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 <alxndr@bu.edu>
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> 
> Thanks
> 
>> 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
>>
> 

Re: [PATCH v3 1/9] fuzz: Make fork_fuzz.ld compatible with LLVM's LLD

[Alexander Bulekov]

On 201119 1706, Daniele Buono wrote:
> 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
> [...]
> .got                    10704   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
> 

Hi Daniele,
Sorry for the late response. I tried linking the fuzzer with lld, and
everything looks good. 

OSS-Fuzz just runs scripts/oss-fuzz/build.sh with some environment
variables set (CC, CXX, CFLAGS, LIB_FUZZING_ENGINE ...). To get this to
work with that script, we would just need to pass the corresponding
arguments to ./configure and find a way to locate and specify
llvm-ar-{11,12,...}.

So far I haven't noticed too much of a performance increase with -flto,
but I need to run some more tests. We probably spend too much time in
the kernel (fork()-ing for each input, etc) for the gains to show up.
-Alex

> 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 <alxndr@bu.edu>
> > Tested-by: Alexander Bulekov <alxndr@bu.edu>
> > 
> > Thanks
> > 
> > > 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
> > > 
> > 

Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2759 bytes --]

Hi

On Thu, Nov 19, 2020 at 8:19 PM Daniele Buono <dbuono@linux.vnet.ibm.com>
wrote:

> 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_iu                    status;
> >>                                ^
> >> 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, &iu, 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?
>
>
Any update on this patch?
thanks


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3564 bytes --]

Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus

[Daniele Buono]

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_iu                    status;
     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-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus

[Philippe Mathieu-Daudé]

On 11/19/20 5:16 PM, Daniele Buono wrote:
> 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_iu                    status;
>>>                                ^
>>> 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, &iu, 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?

Gerd, do you know?

Re: [PATCH-for-5.2? v3 3/9] hw/usb: reorder fields in UASStatus

[Gerd Hoffmann]

  Hi,

> > 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?
> 
> Gerd, do you know?

Don't remember, it's been a few years ago ...

Given that neither add_cdb nor add_cdb_length fields are checked
anywhere in the code it is probably save to just comment out the
add_cdb field.  Should we ever need to look at add_cdb at some
point in the future we can figure some better way deal with it,
in the simplest case just give it a fixed size based on the needs.

take care,
  Gerd