| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230206102733.ftq2lu7frwqmcixu@orel>
Date: Mon, 6 Feb 2023 11:27:33 +0100
From: Andrew Jones <ajones@...tanamicro.com>
To: Atish Patra <atishp@...osinc.com>
Cc: linux-kernel@...r.kernel.org, Albert Ou <aou@...s.berkeley.edu>,
Anup Patel <anup@...infault.org>,
Atish Patra <atishp@...shpatra.org>,
Guo Ren <guoren@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
kvm-riscv@...ts.infradead.org, kvm@...r.kernel.org,
linux-riscv@...ts.infradead.org,
Mark Rutland <mark.rutland@....com>,
Palmer Dabbelt <palmer@...belt.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Will Deacon <will@...nel.org>
Subject: Re: [PATCH v5 06/14] RISC-V: KVM: Modify SBI extension handler to
return SBI error code
On Sat, Feb 04, 2023 at 05:15:07PM -0800, Atish Patra wrote:
> Currently, the SBI extension handle is expected to return Linux error code.
> The top SBI layer converts the Linux error code to SBI specific error code
> that can be returned to guest invoking the SBI calls. This model works
> as long as SBI error codes have 1-to-1 mappings between them.
> However, that may not be true always. This patch attempts to disassociate
> both these error codes by allowing the SBI extension implementation to
> return SBI specific error codes as well.
>
> The extension will continue to return the Linux error specific code which
> will indicate any problem *with* the extension emulation while the
> SBI specific error will indicate the problem *of* the emulation.
>
> Suggested-by: Andrew Jones <ajones@...tanamicro.com>
> Signed-off-by: Atish Patra <atishp@...osinc.com>
> ---
> arch/riscv/include/asm/kvm_vcpu_sbi.h | 10 ++++-
> arch/riscv/kvm/vcpu_sbi.c | 61 +++++++++++----------------
> arch/riscv/kvm/vcpu_sbi_base.c | 22 +++++-----
> arch/riscv/kvm/vcpu_sbi_hsm.c | 28 ++++++------
> arch/riscv/kvm/vcpu_sbi_replace.c | 43 +++++++++----------
> arch/riscv/kvm/vcpu_sbi_v01.c | 17 +++-----
> 6 files changed, 83 insertions(+), 98 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> index 45ba341..8425556 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> @@ -18,6 +18,13 @@ struct kvm_vcpu_sbi_context {
> int return_handled;
> };
>
> +struct kvm_vcpu_sbi_return {
> + unsigned long out_val;
> + unsigned long err_val;
> + struct kvm_cpu_trap *utrap;
> + bool uexit;
> +};
> +
> struct kvm_vcpu_sbi_extension {
> unsigned long extid_start;
> unsigned long extid_end;
> @@ -27,8 +34,7 @@ struct kvm_vcpu_sbi_extension {
> * specific error codes.
> */
> int (*handler)(struct kvm_vcpu *vcpu, struct kvm_run *run,
> - unsigned long *out_val, struct kvm_cpu_trap *utrap,
> - bool *exit);
> + struct kvm_vcpu_sbi_return *retdata);
>
> /* Extension specific probe function */
> unsigned long (*probe)(struct kvm_vcpu *vcpu);
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index f96991d..fe2897e 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -12,26 +12,6 @@
> #include <asm/sbi.h>
> #include <asm/kvm_vcpu_sbi.h>
>
> -static int kvm_linux_err_map_sbi(int err)
> -{
> - switch (err) {
> - case 0:
> - return SBI_SUCCESS;
> - case -EPERM:
> - return SBI_ERR_DENIED;
> - case -EINVAL:
> - return SBI_ERR_INVALID_PARAM;
> - case -EFAULT:
> - return SBI_ERR_INVALID_ADDRESS;
> - case -EOPNOTSUPP:
> - return SBI_ERR_NOT_SUPPORTED;
> - case -EALREADY:
> - return SBI_ERR_ALREADY_AVAILABLE;
> - default:
> - return SBI_ERR_FAILURE;
> - };
> -}
> -
> #ifndef CONFIG_RISCV_SBI_V01
> static const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_v01 = {
> .extid_start = -1UL,
> @@ -125,11 +105,14 @@ int kvm_riscv_vcpu_sbi_ecall(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> int ret = 1;
> bool next_sepc = true;
> - bool userspace_exit = false;
> struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
> const struct kvm_vcpu_sbi_extension *sbi_ext;
> - struct kvm_cpu_trap utrap = { 0 };
> - unsigned long out_val = 0;
> + struct kvm_cpu_trap utrap = {0};
> + struct kvm_vcpu_sbi_return sbi_ret = {
> + .out_val = 0,
> + .err_val = 0,
> + .utrap = &utrap,
> + };
> bool ext_is_v01 = false;
>
> sbi_ext = kvm_vcpu_sbi_find_ext(cp->a7);
> @@ -139,42 +122,46 @@ int kvm_riscv_vcpu_sbi_ecall(struct kvm_vcpu *vcpu, struct kvm_run *run)
> cp->a7 <= SBI_EXT_0_1_SHUTDOWN)
> ext_is_v01 = true;
> #endif
> - ret = sbi_ext->handler(vcpu, run, &out_val, &utrap, &userspace_exit);
> + ret = sbi_ext->handler(vcpu, run, &sbi_ret);
> } else {
> /* Return error for unsupported SBI calls */
> cp->a0 = SBI_ERR_NOT_SUPPORTED;
> goto ecall_done;
> }
>
> + /*
> + * When the SBI extension returns a Linux error code, it exits the ioctl
> + * loop and forwards the error to userspace.
> + */
> + if (ret < 0) {
> + next_sepc = false;
> + goto ecall_done;
> + }
> +
> /* Handle special error cases i.e trap, exit or userspace forward */
> - if (utrap.scause) {
> + if (sbi_ret.utrap->scause) {
> /* No need to increment sepc or exit ioctl loop */
> ret = 1;
> - utrap.sepc = cp->sepc;
> - kvm_riscv_vcpu_trap_redirect(vcpu, &utrap);
> + sbi_ret.utrap->sepc = cp->sepc;
> + kvm_riscv_vcpu_trap_redirect(vcpu, sbi_ret.utrap);
> next_sepc = false;
> goto ecall_done;
> }
>
> /* Exit ioctl loop or Propagate the error code the guest */
> - if (userspace_exit) {
> + if (sbi_ret.uexit) {
> next_sepc = false;
> ret = 0;
> } else {
> - /**
> - * SBI extension handler always returns an Linux error code. Convert
> - * it to the SBI specific error code that can be propagated the SBI
> - * caller.
> - */
> - ret = kvm_linux_err_map_sbi(ret);
> - cp->a0 = ret;
> + cp->a0 = sbi_ret.err_val;
> ret = 1;
> }
> ecall_done:
> if (next_sepc)
> cp->sepc += 4;
> - if (!ext_is_v01)
> - cp->a1 = out_val;
> + /* a1 should only be updated when we continue the ioctl loop */
> + if (!ext_is_v01 && ret == 1)
> + cp->a1 = sbi_ret.out_val;
>
> return ret;
> }
> diff --git a/arch/riscv/kvm/vcpu_sbi_base.c b/arch/riscv/kvm/vcpu_sbi_base.c
> index be47828..9945aff 100644
> --- a/arch/riscv/kvm/vcpu_sbi_base.c
> +++ b/arch/riscv/kvm/vcpu_sbi_base.c
> @@ -14,12 +14,11 @@
> #include <asm/kvm_vcpu_sbi.h>
>
> static int kvm_sbi_ext_base_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> - unsigned long *out_val,
> - struct kvm_cpu_trap *trap, bool *exit)
> + struct kvm_vcpu_sbi_return *retdata)
> {
> - int ret = 0;
> struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
> const struct kvm_vcpu_sbi_extension *sbi_ext;
> + unsigned long *out_val = &retdata->out_val;
>
> switch (cp->a6) {
> case SBI_EXT_BASE_GET_SPEC_VERSION:
> @@ -43,10 +42,11 @@ static int kvm_sbi_ext_base_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> * forward it to the userspace
> */
> kvm_riscv_vcpu_sbi_forward(vcpu, run);
> - *exit = true;
> + retdata->uexit = true;
> } else {
> sbi_ext = kvm_vcpu_sbi_find_ext(cp->a0);
> - *out_val = sbi_ext && sbi_ext->probe ? sbi_ext->probe(vcpu) : !!sbi_ext;
> + *out_val = sbi_ext && sbi_ext->probe ?
> + sbi_ext->probe(vcpu) : !!sbi_ext;
This should have been done in the patch that introduced it. Or, not at
all, if we don't mind going up to 100 chars in KVM code (as checkpatch
allows).
> }
> break;
> case SBI_EXT_BASE_GET_MVENDORID:
> @@ -59,11 +59,11 @@ static int kvm_sbi_ext_base_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> *out_val = vcpu->arch.mimpid;
> break;
> default:
> - ret = -EOPNOTSUPP;
> + retdata->err_val = SBI_ERR_NOT_SUPPORTED;
> break;
> }
>
> - return ret;
> + return 0;
> }
>
> const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_base = {
> @@ -73,17 +73,15 @@ const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_base = {
> };
>
> static int kvm_sbi_ext_forward_handler(struct kvm_vcpu *vcpu,
> - struct kvm_run *run,
> - unsigned long *out_val,
> - struct kvm_cpu_trap *utrap,
> - bool *exit)
> + struct kvm_run *run,
> + struct kvm_vcpu_sbi_return *retdata)
> {
> /*
> * Both SBI experimental and vendor extensions are
> * unconditionally forwarded to userspace.
> */
> kvm_riscv_vcpu_sbi_forward(vcpu, run);
> - *exit = true;
> + retdata->uexit = true;
> return 0;
> }
>
> diff --git a/arch/riscv/kvm/vcpu_sbi_hsm.c b/arch/riscv/kvm/vcpu_sbi_hsm.c
> index 619ac0f..7dca0e9 100644
> --- a/arch/riscv/kvm/vcpu_sbi_hsm.c
> +++ b/arch/riscv/kvm/vcpu_sbi_hsm.c
> @@ -21,9 +21,9 @@ static int kvm_sbi_hsm_vcpu_start(struct kvm_vcpu *vcpu)
>
> target_vcpu = kvm_get_vcpu_by_id(vcpu->kvm, target_vcpuid);
> if (!target_vcpu)
> - return -EINVAL;
> + return SBI_ERR_INVALID_PARAM;
> if (!target_vcpu->arch.power_off)
> - return -EALREADY;
> + return SBI_ERR_ALREADY_AVAILABLE;
>
> reset_cntx = &target_vcpu->arch.guest_reset_context;
> /* start address */
> @@ -42,7 +42,7 @@ static int kvm_sbi_hsm_vcpu_start(struct kvm_vcpu *vcpu)
> static int kvm_sbi_hsm_vcpu_stop(struct kvm_vcpu *vcpu)
> {
> if (vcpu->arch.power_off)
> - return -EACCES;
> + return SBI_ERR_FAILURE;
>
> kvm_riscv_vcpu_power_off(vcpu);
>
> @@ -57,7 +57,7 @@ static int kvm_sbi_hsm_vcpu_get_status(struct kvm_vcpu *vcpu)
>
> target_vcpu = kvm_get_vcpu_by_id(vcpu->kvm, target_vcpuid);
> if (!target_vcpu)
> - return -EINVAL;
> + return SBI_ERR_INVALID_PARAM;
> if (!target_vcpu->arch.power_off)
> return SBI_HSM_STATE_STARTED;
> else if (vcpu->stat.generic.blocking)
> @@ -67,9 +67,7 @@ static int kvm_sbi_hsm_vcpu_get_status(struct kvm_vcpu *vcpu)
> }
>
> static int kvm_sbi_ext_hsm_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> - unsigned long *out_val,
> - struct kvm_cpu_trap *utrap,
> - bool *exit)
> + struct kvm_vcpu_sbi_return *retdata)
> {
> int ret = 0;
> struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
> @@ -88,27 +86,29 @@ static int kvm_sbi_ext_hsm_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> case SBI_EXT_HSM_HART_STATUS:
> ret = kvm_sbi_hsm_vcpu_get_status(vcpu);
> if (ret >= 0) {
> - *out_val = ret;
> - ret = 0;
> + retdata->out_val = ret;
> + retdata->err_val = 0;
> }
> - break;
> + return 0;
> case SBI_EXT_HSM_HART_SUSPEND:
> switch (cp->a0) {
> case SBI_HSM_SUSPEND_RET_DEFAULT:
> kvm_riscv_vcpu_wfi(vcpu);
> break;
> case SBI_HSM_SUSPEND_NON_RET_DEFAULT:
> - ret = -EOPNOTSUPP;
> + ret = SBI_ERR_NOT_SUPPORTED;
> break;
> default:
> - ret = -EINVAL;
> + ret = SBI_ERR_INVALID_PARAM;
> }
> break;
> default:
> - ret = -EOPNOTSUPP;
> + ret = SBI_ERR_NOT_SUPPORTED;
> }
>
> - return ret;
> + retdata->err_val = ret;
> +
> + return 0;
> }
>
> const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_hsm = {
> diff --git a/arch/riscv/kvm/vcpu_sbi_replace.c b/arch/riscv/kvm/vcpu_sbi_replace.c
> index 03a0198..38fa4c0 100644
> --- a/arch/riscv/kvm/vcpu_sbi_replace.c
> +++ b/arch/riscv/kvm/vcpu_sbi_replace.c
> @@ -14,15 +14,15 @@
> #include <asm/kvm_vcpu_sbi.h>
>
> static int kvm_sbi_ext_time_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> - unsigned long *out_val,
> - struct kvm_cpu_trap *utrap, bool *exit)
> + struct kvm_vcpu_sbi_return *retdata)
> {
> - int ret = 0;
> struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
> u64 next_cycle;
>
> - if (cp->a6 != SBI_EXT_TIME_SET_TIMER)
> - return -EINVAL;
> + if (cp->a6 != SBI_EXT_TIME_SET_TIMER) {
> + retdata->err_val = SBI_ERR_INVALID_PARAM;
> + return 0;
> + }
>
> #if __riscv_xlen == 32
> next_cycle = ((u64)cp->a1 << 32) | (u64)cp->a0;
> @@ -31,7 +31,7 @@ static int kvm_sbi_ext_time_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> #endif
> kvm_riscv_vcpu_timer_next_event(vcpu, next_cycle);
>
> - return ret;
> + return 0;
> }
>
> const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_time = {
> @@ -41,8 +41,7 @@ const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_time = {
> };
>
> static int kvm_sbi_ext_ipi_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> - unsigned long *out_val,
> - struct kvm_cpu_trap *utrap, bool *exit)
> + struct kvm_vcpu_sbi_return *retdata)
> {
> int ret = 0;
> unsigned long i;
> @@ -51,8 +50,10 @@ static int kvm_sbi_ext_ipi_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> unsigned long hmask = cp->a0;
> unsigned long hbase = cp->a1;
>
> - if (cp->a6 != SBI_EXT_IPI_SEND_IPI)
> - return -EINVAL;
> + if (cp->a6 != SBI_EXT_IPI_SEND_IPI) {
> + retdata->err_val = SBI_ERR_INVALID_PARAM;
> + return 0;
> + }
>
> kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> if (hbase != -1UL) {
> @@ -76,10 +77,8 @@ const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_ipi = {
> };
>
> static int kvm_sbi_ext_rfence_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> - unsigned long *out_val,
> - struct kvm_cpu_trap *utrap, bool *exit)
> + struct kvm_vcpu_sbi_return *retdata)
> {
> - int ret = 0;
> struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
> unsigned long hmask = cp->a0;
> unsigned long hbase = cp->a1;
> @@ -116,10 +115,10 @@ static int kvm_sbi_ext_rfence_handler(struct kvm_vcpu *vcpu, struct kvm_run *run
> */
> break;
> default:
> - ret = -EOPNOTSUPP;
> + retdata->err_val = SBI_ERR_NOT_SUPPORTED;
> }
>
> - return ret;
> + return 0;
> }
>
> const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_rfence = {
> @@ -130,14 +129,12 @@ const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_rfence = {
>
> static int kvm_sbi_ext_srst_handler(struct kvm_vcpu *vcpu,
> struct kvm_run *run,
> - unsigned long *out_val,
> - struct kvm_cpu_trap *utrap, bool *exit)
> + struct kvm_vcpu_sbi_return *retdata)
> {
> struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
> unsigned long funcid = cp->a6;
> u32 reason = cp->a1;
> u32 type = cp->a0;
> - int ret = 0;
>
> switch (funcid) {
> case SBI_EXT_SRST_RESET:
> @@ -146,24 +143,24 @@ static int kvm_sbi_ext_srst_handler(struct kvm_vcpu *vcpu,
> kvm_riscv_vcpu_sbi_system_reset(vcpu, run,
> KVM_SYSTEM_EVENT_SHUTDOWN,
> reason);
> - *exit = true;
> + retdata->uexit = true;
> break;
> case SBI_SRST_RESET_TYPE_COLD_REBOOT:
> case SBI_SRST_RESET_TYPE_WARM_REBOOT:
> kvm_riscv_vcpu_sbi_system_reset(vcpu, run,
> KVM_SYSTEM_EVENT_RESET,
> reason);
> - *exit = true;
> + retdata->uexit = true;
> break;
> default:
> - ret = -EOPNOTSUPP;
> + retdata->err_val = SBI_ERR_NOT_SUPPORTED;
> }
> break;
> default:
> - ret = -EOPNOTSUPP;
> + retdata->err_val = SBI_ERR_NOT_SUPPORTED;
> }
>
> - return ret;
> + return 0;
> }
>
> const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_srst = {
> diff --git a/arch/riscv/kvm/vcpu_sbi_v01.c b/arch/riscv/kvm/vcpu_sbi_v01.c
> index 489f225..8f4c4fa 100644
> --- a/arch/riscv/kvm/vcpu_sbi_v01.c
> +++ b/arch/riscv/kvm/vcpu_sbi_v01.c
> @@ -14,9 +14,7 @@
> #include <asm/kvm_vcpu_sbi.h>
>
> static int kvm_sbi_ext_v01_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> - unsigned long *out_val,
> - struct kvm_cpu_trap *utrap,
> - bool *exit)
> + struct kvm_vcpu_sbi_return *retdata)
> {
> ulong hmask;
> int i, ret = 0;
> @@ -24,6 +22,7 @@ static int kvm_sbi_ext_v01_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> struct kvm_vcpu *rvcpu;
> struct kvm *kvm = vcpu->kvm;
> struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
> + struct kvm_cpu_trap *utrap = retdata->utrap;
>
> switch (cp->a7) {
> case SBI_EXT_0_1_CONSOLE_GETCHAR:
> @@ -33,7 +32,7 @@ static int kvm_sbi_ext_v01_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> * handled in kernel so we forward these to user-space
> */
> kvm_riscv_vcpu_sbi_forward(vcpu, run);
> - *exit = true;
> + retdata->uexit = true;
> break;
> case SBI_EXT_0_1_SET_TIMER:
> #if __riscv_xlen == 32
> @@ -48,8 +47,7 @@ static int kvm_sbi_ext_v01_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> break;
> case SBI_EXT_0_1_SEND_IPI:
> if (cp->a0)
> - hmask = kvm_riscv_vcpu_unpriv_read(vcpu, false, cp->a0,
> - utrap);
> + hmask = kvm_riscv_vcpu_unpriv_read(vcpu, false, cp->a0, utrap);
> else
> hmask = (1UL << atomic_read(&kvm->online_vcpus)) - 1;
> if (utrap->scause)
> @@ -65,14 +63,13 @@ static int kvm_sbi_ext_v01_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> case SBI_EXT_0_1_SHUTDOWN:
> kvm_riscv_vcpu_sbi_system_reset(vcpu, run,
> KVM_SYSTEM_EVENT_SHUTDOWN, 0);
> - *exit = true;
> + retdata->uexit = true;
> break;
> case SBI_EXT_0_1_REMOTE_FENCE_I:
> case SBI_EXT_0_1_REMOTE_SFENCE_VMA:
> case SBI_EXT_0_1_REMOTE_SFENCE_VMA_ASID:
> if (cp->a0)
> - hmask = kvm_riscv_vcpu_unpriv_read(vcpu, false, cp->a0,
> - utrap);
> + hmask = kvm_riscv_vcpu_unpriv_read(vcpu, false, cp->a0, utrap);
> else
> hmask = (1UL << atomic_read(&kvm->online_vcpus)) - 1;
> if (utrap->scause)
> @@ -103,7 +100,7 @@ static int kvm_sbi_ext_v01_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
> }
> break;
> default:
> - ret = -EINVAL;
> + retdata->err_val = SBI_ERR_NOT_SUPPORTED;
> break;
> }
>
> --
> 2.25.1
>
I actually gave this an r-b in v4, since I only pointed out nits, but
anyway, here it is again
Reviewed-by: Andrew Jones <ajones@...tanamicro.com>
Thanks,
drew
Powered by blists - more mailing lists