by David Brazdil

[permalink] [raw]

Subject: Re: [PATCH v3 2/9] KVM: arm64: Add a buffer that can pass UBSan data from hyp/nVHE to kernel

On Fri, Jan 15, 2021 at 05:18:23PM +0000, Elena Petrova wrote:
> From: George Popescu <[email protected]>
>
> Share a buffer between the kernel and the hyp/nVHE code by using the
> macros from kvm_debug_buffer.h.
> The hyp/nVHE code requires a write index which counts how many elements
> have been writtens inside the buffer and the kernel requires a read
> index which counts how many elements have been read from the buffer.
> The write index and the buffer are shared with the kernel in read-only.
>
> The kvm_debug_buffer_ind returns the reading and writing points of the
> circular buffer and updates the reading index.
>
> Data collected from UBSan handlers inside hyp/nVHE is stored in the
> kvm_ubsan_buffer.
> This buffer stores only UBSan data because it should not be preoccupied
> by other mechanisms data structures and functionalities.
>
> Also, for the moment the buffer is mapped inside .bss, where both the kernel
> and the hyp/nVHE code have Read/Write rights, but in the future this will change
> and the kernel will not be able to acess hyp/nVHE's .bss. At that point the buffer
> will only need to be mapped in order for this patch to work.
>
> Signed-off-by: Elena Petrova <[email protected]>
> ---
> arch/arm64/include/asm/assembler.h | 11 +++++++
> arch/arm64/include/asm/kvm_debug_buffer.h | 36 ++++++++++++++++++++
> arch/arm64/include/asm/kvm_host.h | 8 ++++-
> arch/arm64/include/asm/kvm_ubsan.h | 14 ++++++++
> arch/arm64/kvm/Makefile | 2 ++
> arch/arm64/kvm/arm.c | 9 +++++
> arch/arm64/kvm/hyp/nvhe/host.S | 4 +++
> arch/arm64/kvm/hyp/nvhe/ubsan.c | 23 +++++++++++++
> arch/arm64/kvm/kvm_ubsan_buffer.c | 40 +++++++++++++++++++++++
> 9 files changed, 146 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/include/asm/kvm_debug_buffer.h
> create mode 100644 arch/arm64/include/asm/kvm_ubsan.h
> create mode 100644 arch/arm64/kvm/kvm_ubsan_buffer.c
>
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index bf125c591116..ebc18a8a0e1f 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -258,6 +258,17 @@ alternative_endif
> ldr \dst, [\dst, \tmp]
> .endm
>
> + /*
> + * @sym: The name of the per-cpu variable
> + * @reg: value to store
> + * @tmp1: scratch register
> + * @tmp2: scratch register
> + */
> + .macro str_this_cpu sym, reg, tmp1, tmp2
> + adr_this_cpu \tmp1, \sym, \tmp2
> + str \reg, [\tmp1]
> + .endm
> +
> /*
> * vma_vm_mm - get mm pointer from vma pointer (vma->vm_mm)
> */
> diff --git a/arch/arm64/include/asm/kvm_debug_buffer.h b/arch/arm64/include/asm/kvm_debug_buffer.h
> new file mode 100644
> index 000000000000..e5375c2cff1a
> --- /dev/null
> +++ b/arch/arm64/include/asm/kvm_debug_buffer.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2020 Google LLC
> + * Author: George Popescu <[email protected]>
> + */
> +
> +#include <linux/percpu-defs.h>
> +
> +
> +#define KVM_DEBUG_BUFFER_SIZE 1000
nit: unused?

> +
> +#ifdef __KVM_NVHE_HYPERVISOR__
> +#define DEFINE_KVM_DEBUG_BUFFER(type_name, buffer_name, write_ind, size)\
> + DEFINE_PER_CPU(type_name, buffer_name)[size]; \
> + DEFINE_PER_CPU(unsigned long, write_ind) = 0;

This is subjective but I would:
* stop calling the second variable a "write index" and instead call it
"cursor" - I think this is a leftover from George's early two-cursor
implementation
* wrap these two variables in a struct; that is a bit tricky given that each
instantiation defines its own size (the macros would have to generate that
struct), so bar that I would at least generate the name of the cursor
variable as buffer_name##suffix.

> +
> +#define DECLARE_KVM_DEBUG_BUFFER(type_name, buffer_name, write_ind, size)\
> + DECLARE_PER_CPU(type_name, buffer_name)[size]; \
> + DECLARE_PER_CPU(unsigned long, write_ind);
> +#else
> +#define DECLARE_KVM_DEBUG_BUFFER(type_name, buffer_name, write_ind, size)\
> + DECLARE_KVM_NVHE_PER_CPU(type_name, buffer_name)[size]; \
> + DECLARE_KVM_NVHE_PER_CPU(unsigned long, write_ind);
> +#endif //__KVM_NVHE_HYPERVISOR__
> +
> +#ifdef __ASSEMBLY__
> +#include <asm/assembler.h>
> +
> +.macro clear_buffer tmp1, tmp2, tmp3
> + mov \tmp1, 0
> +#ifdef CONFIG_UBSAN
> + str_this_cpu kvm_ubsan_buff_wr_ind, \tmp1, \tmp2, \tmp3
> +#endif //CONFIG_UBSAN
> +.endm

I find this macro odd. Everything else in this file is about a generic buffer
data structure, yet this macro enumerates all instantiations of it (currently
just one). Please keep this generic and pass the name of the buffer cursor
as a parameter.

> +
> +#endif
> \ No newline at end of file
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 8fcfab0c2567..905ca0d7c52c 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -592,6 +592,12 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);
> void kvm_arm_halt_guest(struct kvm *kvm);
> void kvm_arm_resume_guest(struct kvm *kvm);
>
> +
nit: Two empty lines

> +#ifdef CONFIG_UBSAN
> +extern void __kvm_check_ubsan_buffer(void);
> +#else
> +static inline void __kvm_check_ubsan_buffer(void) {}
> +#endif /* CONFIG_UBSAN */
nit: An empty line here would be nice.

> #define kvm_call_hyp_nvhe(f, ...) \
> ({ \
> struct arm_smccc_res res; \
> @@ -599,7 +605,7 @@ void kvm_arm_resume_guest(struct kvm *kvm);
> arm_smccc_1_1_hvc(KVM_HOST_SMCCC_FUNC(f), \
> ##__VA_ARGS__, &res); \
> WARN_ON(res.a0 != SMCCC_RET_SUCCESS); \
> - \
> + __kvm_check_ubsan_buffer(); \
> res.a1; \
> })
>
> diff --git a/arch/arm64/include/asm/kvm_ubsan.h b/arch/arm64/include/asm/kvm_ubsan.h
> new file mode 100644
> index 000000000000..fb32c7fd65d4
> --- /dev/null
> +++ b/arch/arm64/include/asm/kvm_ubsan.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2020 Google LLC
> + * Author: George Popescu <[email protected]>
> + */
> +
> +#include <ubsan.h>
> +
> +#define UBSAN_MAX_TYPE 6
nit: How about you make this the last entry in the `type` enum in
`kvm_ubsan_info`.

> +#define KVM_UBSAN_BUFFER_SIZE 1000
> +
> +struct kvm_ubsan_info {
> + int type;
> +};
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 13b017284bf9..ab3a9c402550 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -4,6 +4,7 @@
> #
>
> ccflags-y += -I $(srctree)/$(src)
> +CFLAGS_kvm_ubsan_buffer.o += -I $(srctree)/lib/
>
> KVM=../../../virt/kvm
>
> @@ -25,3 +26,4 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o \
> vgic/vgic-its.o vgic/vgic-debug.o
>
> kvm-$(CONFIG_HW_PERF_EVENTS) += pmu-emul.o
> +kvm-$(CONFIG_UBSAN) += kvm_ubsan_buffer.o
nit: Can we just name this kvm_ubsan.c?

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 04c44853b103..63f0495550b8 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1784,6 +1784,15 @@ static int init_hyp_mode(void)
> goto out_err;
> }
> }
> +#ifdef CONFIG_UBSAN
> + /* required by ubsan to access the handlers structures fields */
nit: capital R, grammar "handlers structures fields"

> + err = create_hyp_mappings(kvm_ksym_ref(_data),
> + kvm_ksym_ref(__end_once), PAGE_HYP_RO);
> + if (err) {
> + kvm_err("Cannot map data section\n");
> + goto out_err;
> + }
> +#endif

The hypervisor currently doesn't use the .data section, so this approach works
fine at the moment. But this will stop working as soon as somebody starts using
.data for other purposes. It would be nice to do this properly now rather than
fix it later as part of some unrelated series. What you'd need to do:

* add .data to arch/arm64/kvm/hyp/nvhe/hyp.lds.S; that will rename
hypervisor's .data to .hyp.data
* add .hyp.data to arch/arm64/kernel/vmlinux.lds.S
* add new entries to arch/arm64/include/asm/sections.h
* map .hyp.data here

Related, this currently relies on the fact that all pointers in .data are the
kernel VAs. That means later in this series it is safe to just copy the event
description to the buffer without any conversion. my series "Relocate absolute
hyp VAs" will convert all pointers in .hyp.* sections to the hypervisor VAs.
So once we do introduce .hyp.data, the hypervisor's ubsan.c will also have to
convert all pointers.

>
> /*
> * Map Hyp percpu pages
> diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
> index a820dfdc9c25..0967906265d3 100644
> --- a/arch/arm64/kvm/hyp/nvhe/host.S
> +++ b/arch/arm64/kvm/hyp/nvhe/host.S
> @@ -8,6 +8,7 @@
>
> #include <asm/assembler.h>
> #include <asm/kvm_asm.h>
> +#include <asm/kvm_debug_buffer.h>
> #include <asm/kvm_mmu.h>
>
> .text
> @@ -34,6 +35,9 @@ SYM_FUNC_START(__host_exit)
> /* Store the host regs x18-x29, lr */
> save_callee_saved_regs x0
>
> + /* when entering the host clear the buffers */
nit: capital W

> + clear_buffer x4, x5, x6
> +
> /* Save the host context pointer in x29 across the function call */
> mov x29, x0
> bl handle_trap
> diff --git a/arch/arm64/kvm/hyp/nvhe/ubsan.c b/arch/arm64/kvm/hyp/nvhe/ubsan.c
> index a5db6b61ceb2..8a194fb1f6cf 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ubsan.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ubsan.c
> @@ -3,10 +3,33 @@
> * Copyright 2020 Google LLC
> * Author: George Popescu <[email protected]>
> */
> +#include <linux/bitops.h>
> #include <linux/ctype.h>
> #include <linux/types.h>
> +#include <linux/percpu-defs.h>
> +#include <linux/kvm_host.h>
> +#include <asm/kvm_arm.h>
> +#include <asm/kvm_asm.h>
> +#include <asm/kvm_ubsan.h>
> +#include <asm/kvm_debug_buffer.h>
> +#include <kvm/arm_pmu.h>
> #include <ubsan.h>
>
> +DEFINE_KVM_DEBUG_BUFFER(struct kvm_ubsan_info, kvm_ubsan_buffer,
> + kvm_ubsan_buff_wr_ind, KVM_UBSAN_BUFFER_SIZE);
> +
> +static inline struct kvm_ubsan_info *kvm_ubsan_buffer_next_slot(void)
> +{
> + struct kvm_ubsan_info *res = NULL;
> + unsigned long write_ind = __this_cpu_read(kvm_ubsan_buff_wr_ind);
nit: checkpatch will want an empty line between declarations and code

> + if (write_ind < KVM_UBSAN_BUFFER_SIZE) {
> + res = this_cpu_ptr(&kvm_ubsan_buffer[write_ind]);
> + ++write_ind;
> + __this_cpu_write(kvm_ubsan_buff_wr_ind, write_ind);
nit: Could save a line by just writing write_ind+1

> + }
> + return res;
> +}
> +
> void __ubsan_handle_add_overflow(void *_data, void *lhs, void *rhs) {}
>
> void __ubsan_handle_sub_overflow(void *_data, void *lhs, void *rhs) {}
> diff --git a/arch/arm64/kvm/kvm_ubsan_buffer.c b/arch/arm64/kvm/kvm_ubsan_buffer.c
> new file mode 100644
> index 000000000000..4a1959ba9f68
> --- /dev/null
> +++ b/arch/arm64/kvm/kvm_ubsan_buffer.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 Google LLC
> + * Author: George Popescu <[email protected]>
> + */
> +
> +#include <linux/ctype.h>
> +#include <linux/types.h>
> +#include <asm/kvm_debug_buffer.h>
> +#include <asm/kvm_arm.h>
> +#include <asm/kvm_asm.h>
> +#include <kvm/arm_pmu.h>
> +
> +#include <ubsan.h>
> +#include <asm/kvm_ubsan.h>
> +
> +DECLARE_KVM_DEBUG_BUFFER(struct kvm_ubsan_info, kvm_ubsan_buffer,
> + kvm_ubsan_buff_wr_ind, KVM_UBSAN_BUFFER_SIZE);
> +
> +
> +void iterate_kvm_ubsan_buffer(unsigned long left, unsigned long right)
> +{
> + unsigned long i;
> + struct kvm_ubsan_info *slot;
> +
> + slot = (struct kvm_ubsan_info *) this_cpu_ptr_nvhe_sym(kvm_ubsan_buffer);
nit: no whitespace after bracket?

> + for (i = left; i < right; ++i) {
> + /* check ubsan data */
> + slot[i].type = 0;
> + }
> +}
> +
> +void __kvm_check_ubsan_buffer(void)
> +{
> + unsigned long *write_ind;
> +
> + write_ind = (unsigned long *) this_cpu_ptr_nvhe_sym(kvm_ubsan_buff_wr_ind);
> + iterate_kvm_ubsan_buffer(0, *write_ind);

I think this is another remnant of George's previous implementation. The first
argument will always be zero, so I don't see the point in splitting this into
two functions any more.

> +}
> +
> --
> 2.30.0.296.g2bfb1c46d8-goog
>
> _______________________________________________
> kvmarm mailing list
> [email protected]
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

2021-01-19 03:52:15

by David Brazdil

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] KVM: arm64: Enable UBSAN_BOUNDS for the both the kernel and hyp/nVHE

On Fri, Jan 15, 2021 at 05:18:24PM +0000, Elena Petrova wrote:
> From: George Popescu <[email protected]>
>
> If an out of bounds happens inside the hyp/nVHE code, the ubsan_out_of_bounds
> handler stores the logging data inside the kvm_ubsan_buffer. The one responsible
> for printing is the kernel ubsan_out_of_bounds handler. The process of
> decapsulating the data from the buffer is straightforward.
>
> Signed-off-by: George Popescu <[email protected]>
> Signed-off-by: Elena Petrova <[email protected]>
> ---
> arch/arm64/include/asm/kvm_ubsan.h | 19 ++++++++++++++++++-
> arch/arm64/kvm/hyp/nvhe/ubsan.c | 14 ++++++++++++--
> arch/arm64/kvm/kvm_ubsan_buffer.c | 10 ++++++++++
> 3 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_ubsan.h b/arch/arm64/include/asm/kvm_ubsan.h
> index fb32c7fd65d4..4f471acb88b0 100644
> --- a/arch/arm64/include/asm/kvm_ubsan.h
> +++ b/arch/arm64/include/asm/kvm_ubsan.h
> @@ -9,6 +9,23 @@
> #define UBSAN_MAX_TYPE 6
> #define KVM_UBSAN_BUFFER_SIZE 1000
>
> +
> +struct ubsan_values {
> + void *lval;
> + void *rval;
> + char op;
> +};
> +
> struct kvm_ubsan_info {
> - int type;
> + enum {
> + UBSAN_OUT_OF_BOUNDS,
> + } type;
> + union {
> + struct out_of_bounds_data out_of_bounds_data;
> + };
> + union {
> + struct ubsan_values u_val;
> + };

I don't see this second union having more members later in the series.
Remove it? Even the 'struct ubsan_values' seems redundant and we could just
have those three fields directly here.

> };
> +
> +void __ubsan_handle_out_of_bounds(void *_data, void *index);
> diff --git a/arch/arm64/kvm/hyp/nvhe/ubsan.c b/arch/arm64/kvm/hyp/nvhe/ubsan.c
> index 8a194fb1f6cf..55a8f6db8555 100644
> --- a/arch/arm64/kvm/hyp/nvhe/ubsan.c
> +++ b/arch/arm64/kvm/hyp/nvhe/ubsan.c
> @@ -13,7 +13,6 @@
> #include <asm/kvm_ubsan.h>
> #include <asm/kvm_debug_buffer.h>
> #include <kvm/arm_pmu.h>
> -#include <ubsan.h>
>
> DEFINE_KVM_DEBUG_BUFFER(struct kvm_ubsan_info, kvm_ubsan_buffer,
> kvm_ubsan_buff_wr_ind, KVM_UBSAN_BUFFER_SIZE);
> @@ -44,7 +43,18 @@ void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr) {}
>
> void __ubsan_handle_type_mismatch_v1(void *_data, void *ptr) {}
>
> -void __ubsan_handle_out_of_bounds(void *_data, void *index) {}
> +void __ubsan_handle_out_of_bounds(void *_data, void *index)
> +{
> + struct kvm_ubsan_info *slot;
> + struct out_of_bounds_data *data = _data;
> +
> + slot = kvm_ubsan_buffer_next_slot();
> + if (slot) {
> + slot->type = UBSAN_OUT_OF_BOUNDS;
> + slot->out_of_bounds_data = *data;
> + slot->u_val.lval = index;
> + }
> +}
>
> void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs) {}
>
> diff --git a/arch/arm64/kvm/kvm_ubsan_buffer.c b/arch/arm64/kvm/kvm_ubsan_buffer.c
> index 4a1959ba9f68..a1523f86be3c 100644
> --- a/arch/arm64/kvm/kvm_ubsan_buffer.c
> +++ b/arch/arm64/kvm/kvm_ubsan_buffer.c
> @@ -17,6 +17,15 @@
> DECLARE_KVM_DEBUG_BUFFER(struct kvm_ubsan_info, kvm_ubsan_buffer,
> kvm_ubsan_buff_wr_ind, KVM_UBSAN_BUFFER_SIZE);
>
> +void __kvm_check_ubsan_data(struct kvm_ubsan_info *slot)
> +{
> + switch (slot->type) {
> + case UBSAN_OUT_OF_BOUNDS:
> + __ubsan_handle_out_of_bounds(&slot->out_of_bounds_data,
> + slot->u_val.lval);
> + break;
> + }
> +}
>
> void iterate_kvm_ubsan_buffer(unsigned long left, unsigned long right)
> {
> @@ -26,6 +35,7 @@ void iterate_kvm_ubsan_buffer(unsigned long left, unsigned long right)
> slot = (struct kvm_ubsan_info *) this_cpu_ptr_nvhe_sym(kvm_ubsan_buffer);
> for (i = left; i < right; ++i) {
> /* check ubsan data */
> + __kvm_check_ubsan_data(slot + i);

Not sure why this is breaking out into another function. The code will not
be shared with any other user.

> slot[i].type = 0;

This invalidation is redundant. The buffer's cursor will be reset on next
hypercall, which will implicitly invalidate all entries.

> }
> }
> --
> 2.30.0.296.g2bfb1c46d8-goog
>
> _______________________________________________
> kvmarm mailing list
> [email protected]
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm