| Message ID | 20201105221905.1350-1-dbuono@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for Control-Flow Integrity | expand |
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(-) >
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(-) >> > >
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(-) > > > > > > >
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.