From: Reiji Watanabe <reijiw@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
Schspa Shi <schspa@gmail.com>,
kernel-team@android.com, Oliver Upton <oliver.upton@linux.dev>
Subject: Re: [PATCH 05/19] KVM: arm64: Consolidate sysreg userspace accesses
Date: Fri, 8 Jul 2022 23:55:08 -0700 [thread overview]
Message-ID: <CAAeT=FzW78rn-_rXRg2t_RpwBQ1Ap-ugtqE-vb-P6YyGO++VRA@mail.gmail.com> (raw)
In-Reply-To: <20220706164304.1582687-6-maz@kernel.org>
Hi Marc,
On Wed, Jul 6, 2022 at 9:43 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Until now, the .set_user and .get_user callbacks have to implement
> (directly or not) the userspace memory accesses. Although this gives
> us maximem flexibility, this is also a maintenance burden, making it
> hard to audit, and I'd feel much better if it was all located in
> a single place.
>
> So let's do just that, simplifying most of the function signatures
> in the process (the callbacks are now only concerned with the
> data itself, and not with userspace).
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/sys_regs.c | 162 ++++++++++++++------------------------
> arch/arm64/kvm/sys_regs.h | 4 +-
> 2 files changed, 63 insertions(+), 103 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 89e7eddea937..1ce439eed3d8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -321,16 +321,8 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
> }
>
> static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 val)
> {
> - u64 id = sys_reg_to_index(rd);
> - u64 val;
> - int err;
> -
> - err = reg_from_user(&val, uaddr, id);
> - if (err)
> - return err;
> -
> /*
> * The only modifiable bit is the OSLK bit. Refuse the write if
> * userspace attempts to change any other bit in the register.
> @@ -451,22 +443,16 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
> }
>
> static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 val)
> {
> - __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
> -
> - if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> - return -EFAULT;
> + vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = val;
> return 0;
> }
>
> static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 *val)
> {
> - __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
> -
> - if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
> - return -EFAULT;
> + *val = vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
> return 0;
> }
>
> @@ -493,23 +479,16 @@ static bool trap_bcr(struct kvm_vcpu *vcpu,
> }
>
> static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 val)
> {
> - __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
> -
> - if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> - return -EFAULT;
> -
> + vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = val;
> return 0;
> }
>
> static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 *val)
> {
> - __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
> -
> - if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
> - return -EFAULT;
> + *val = vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
> return 0;
> }
>
> @@ -537,22 +516,16 @@ static bool trap_wvr(struct kvm_vcpu *vcpu,
> }
>
> static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 val)
> {
> - __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
> -
> - if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> - return -EFAULT;
> + vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = val;
> return 0;
> }
>
> static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 *val)
> {
> - __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
> -
> - if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
> - return -EFAULT;
> + *val = vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
> return 0;
> }
>
> @@ -579,22 +552,16 @@ static bool trap_wcr(struct kvm_vcpu *vcpu,
> }
>
> static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 val)
> {
> - __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
> -
> - if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
> - return -EFAULT;
> + vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = val;
> return 0;
> }
>
> static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 *val)
> {
> - __u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
> -
> - if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
> - return -EFAULT;
> + *val = vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
> return 0;
> }
>
> @@ -1227,16 +1194,9 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
>
> static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 val)
> {
> - const u64 id = sys_reg_to_index(rd);
> u8 csv2, csv3;
> - int err;
> - u64 val;
> -
> - err = reg_from_user(&val, uaddr, id);
> - if (err)
> - return err;
>
> /*
> * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as
> @@ -1262,7 +1222,7 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> return -EINVAL;
>
> vcpu->kvm->arch.pfr0_csv2 = csv2;
> - vcpu->kvm->arch.pfr0_csv3 = csv3 ;
> + vcpu->kvm->arch.pfr0_csv3 = csv3;
>
> return 0;
> }
> @@ -1275,27 +1235,17 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> * to be changed.
> */
> static int __get_id_reg(const struct kvm_vcpu *vcpu,
> - const struct sys_reg_desc *rd, void __user *uaddr,
> + const struct sys_reg_desc *rd, u64 *val,
> bool raz)
> {
> - const u64 id = sys_reg_to_index(rd);
> - const u64 val = read_id_reg(vcpu, rd, raz);
> -
> - return reg_to_user(uaddr, &val, id);
> + *val = read_id_reg(vcpu, rd, raz);
> + return 0;
> }
>
> static int __set_id_reg(const struct kvm_vcpu *vcpu,
> - const struct sys_reg_desc *rd, void __user *uaddr,
> + const struct sys_reg_desc *rd, u64 val,
> bool raz)
> {
> - const u64 id = sys_reg_to_index(rd);
> - int err;
> - u64 val;
> -
> - err = reg_from_user(&val, uaddr, id);
> - if (err)
> - return err;
> -
> /* This is what we mean by invariant: you can't change it. */
> if (val != read_id_reg(vcpu, rd, raz))
> return -EINVAL;
> @@ -1304,47 +1254,37 @@ static int __set_id_reg(const struct kvm_vcpu *vcpu,
> }
>
> static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 *val)
> {
> bool raz = sysreg_visible_as_raz(vcpu, rd);
>
> - return __get_id_reg(vcpu, rd, uaddr, raz);
> + return __get_id_reg(vcpu, rd, val, raz);
> }
>
> static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 val)
> {
> bool raz = sysreg_visible_as_raz(vcpu, rd);
>
> - return __set_id_reg(vcpu, rd, uaddr, raz);
> + return __set_id_reg(vcpu, rd, val, raz);
> }
>
> static int set_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 val)
> {
> - return __set_id_reg(vcpu, rd, uaddr, true);
> + return __set_id_reg(vcpu, rd, val, true);
> }
>
> static int get_raz_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 *val)
> {
> - const u64 id = sys_reg_to_index(rd);
> - const u64 val = 0;
> -
> - return reg_to_user(uaddr, &val, id);
> + *val = 0;
> + return 0;
> }
>
> static int set_wi_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr)
> + u64 val)
> {
> - int err;
> - u64 val;
> -
> - /* Perform the access even if we are going to ignore the value */
> - err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
> - if (err)
> - return err;
> -
> return 0;
> }
>
> @@ -2854,17 +2794,28 @@ static int demux_c15_set(u64 id, void __user *uaddr)
> int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
> const struct sys_reg_desc table[], unsigned int num)
> {
> - void __user *uaddr = (void __user *)(unsigned long)reg->addr;
> + u64 __user *uaddr = (u64 __user *)(unsigned long)reg->addr;
> const struct sys_reg_desc *r;
> + u64 val;
> + int ret;
>
> r = id_to_sys_reg_desc(vcpu, reg->id, table, num);
> if (!r)
> return -ENOENT;
>
> - if (r->get_user)
> - return (r->get_user)(vcpu, r, reg, uaddr);
> + if (r->get_user) {
> + ret = (r->get_user)(vcpu, r, &val);
> + } else {
> + val = __vcpu_sys_reg(vcpu, r->reg);
> + ret = 0;
> + }
> +
> + if (!ret) {
> + if (put_user(val, uaddr))
> + ret = -EFAULT;
Nit: Since put_user() returns -EFAULT when it fails,
we can simply set 'ret' to the return value from put_user().
(same for get_user())
Reviewed-by: Reiji Watanabe <reijiw@google.com>
I like this fix!
Thank you,
Reiji
> + }
>
> - return reg_to_user(uaddr, &__vcpu_sys_reg(vcpu, r->reg), reg->id);
> + return ret;
> }
>
> int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> @@ -2886,17 +2837,26 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
> int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
> const struct sys_reg_desc table[], unsigned int num)
> {
> - void __user *uaddr = (void __user *)(unsigned long)reg->addr;
> + u64 __user *uaddr = (u64 __user *)(unsigned long)reg->addr;
> const struct sys_reg_desc *r;
> + u64 val;
> + int ret;
> +
> + if (get_user(val, uaddr))
> + return -EFAULT;
>
> r = id_to_sys_reg_desc(vcpu, reg->id, table, num);
> if (!r)
> return -ENOENT;
>
> - if (r->set_user)
> - return (r->set_user)(vcpu, r, reg, uaddr);
> + if (r->set_user) {
> + ret = (r->set_user)(vcpu, r, val);
> + } else {
> + __vcpu_sys_reg(vcpu, r->reg) = val;
> + ret = 0;
> + }
>
> - return reg_from_user(&__vcpu_sys_reg(vcpu, r->reg), uaddr, reg->id);
> + return ret;
> }
>
> int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index 4fb6d59e7874..b8b576a2af2b 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -75,9 +75,9 @@ struct sys_reg_desc {
>
> /* Custom get/set_user functions, fallback to generic if NULL */
> int (*get_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr);
> + u64 *val);
> int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> - const struct kvm_one_reg *reg, void __user *uaddr);
> + u64 val);
>
> /* Return mask of REG_* runtime visibility overrides */
> unsigned int (*visibility)(const struct kvm_vcpu *vcpu,
> --
> 2.34.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-07-09 6:56 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-06 16:42 [PATCH 00/19] KVM: arm64: vgic-v3 userspace access consolidation (and other goodies) Marc Zyngier
2022-07-06 16:42 ` [PATCH 01/19] KVM: arm64: Add get_reg_by_id() as a sys_reg_desc retrieving helper Marc Zyngier
2022-07-07 4:05 ` Reiji Watanabe
2022-07-07 5:16 ` Reiji Watanabe
2022-07-06 16:42 ` [PATCH 02/19] KVM: arm64: Reorder handling of invariant sysregs from userspace Marc Zyngier
2022-07-07 4:24 ` Reiji Watanabe
2022-07-06 16:42 ` [PATCH 03/19] KVM: arm64: Introduce generic get_user/set_user helpers for system registers Marc Zyngier
2022-07-08 19:20 ` Oliver Upton
2022-07-09 6:59 ` Reiji Watanabe
2022-07-06 16:42 ` [PATCH 04/19] KVM: arm64: Push checks for 64bit registers into the low-level accessors Marc Zyngier
2022-07-08 6:13 ` Reiji Watanabe
2022-07-08 8:05 ` Marc Zyngier
2022-07-06 16:42 ` [PATCH 05/19] KVM: arm64: Consolidate sysreg userspace accesses Marc Zyngier
2022-07-08 19:33 ` Oliver Upton
2022-07-09 6:55 ` Reiji Watanabe [this message]
2022-07-12 7:25 ` Marc Zyngier
2022-07-06 16:42 ` [PATCH 06/19] KVM: arm64: Get rid of reg_from/to_user() Marc Zyngier
2022-07-08 19:35 ` Oliver Upton
2022-07-12 4:34 ` Reiji Watanabe
2022-07-06 16:42 ` [PATCH 07/19] KVM: arm64: vgic-v3: Simplify vgic_v3_has_cpu_sysregs_attr() Marc Zyngier
2022-07-08 19:38 ` Oliver Upton
2022-07-12 5:22 ` Reiji Watanabe
2022-07-06 16:42 ` [PATCH 08/19] KVM: arm64: vgic-v3: Push user access into vgic_v3_cpu_sysregs_uaccess() Marc Zyngier
2022-07-12 6:11 ` Reiji Watanabe
2022-07-12 6:52 ` Marc Zyngier
2022-07-13 3:26 ` Reiji Watanabe
2022-07-06 16:42 ` [PATCH 09/19] KVM: arm64: vgic-v3: Make the userspace accessors use sysreg API Marc Zyngier
2022-07-13 5:21 ` Reiji Watanabe
2022-07-06 16:42 ` [PATCH 10/19] KVM: arm64: vgic-v3: Convert userspace accessors over to FIELD_GET/FIELD_PREP Marc Zyngier
2022-07-13 5:51 ` Reiji Watanabe
2022-07-06 16:42 ` [PATCH 11/19] KVM: arm64: vgic-v3: Use u32 to manage the line level from userspace Marc Zyngier
2022-07-13 6:45 ` Reiji Watanabe
2022-07-06 16:42 ` [PATCH 12/19] KVM: arm64: vgic-v3: Consolidate userspace access for MMIO registers Marc Zyngier
2022-07-14 4:11 ` Reiji Watanabe
2022-07-06 16:42 ` [PATCH 13/19] KVM: arm64: vgic-v2: " Marc Zyngier
2022-07-14 4:43 ` Reiji Watanabe
2022-07-14 7:09 ` Marc Zyngier
2022-07-06 16:42 ` [PATCH 14/19] KVM: arm64: vgic: Use {get,put}_user() instead of copy_{from.to}_user Marc Zyngier
2022-07-14 5:09 ` [PATCH 14/19] KVM: arm64: vgic: Use {get, put}_user() " Reiji Watanabe
2022-07-06 16:43 ` [PATCH 15/19] KVM: arm64: vgic-v2: Add helper for legacy dist/cpuif base address setting Marc Zyngier
2022-07-14 6:37 ` Reiji Watanabe
2022-07-14 7:01 ` Marc Zyngier
2022-07-15 6:44 ` Reiji Watanabe
2022-07-06 16:43 ` [PATCH 16/19] KVM: arm64: vgic: Consolidate userspace access for " Marc Zyngier
2022-07-06 16:43 ` [PATCH 17/19] KVM: arm64: Get rid of find_reg_by_id() Marc Zyngier
2022-07-06 16:43 ` [PATCH 18/19] KVM: arm64: Descope kvm_arm_sys_reg_{get,set}_reg() Marc Zyngier
2022-07-06 16:43 ` [PATCH 19/19] KVM: arm64: Get rid or outdated comments Marc Zyngier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAeT=FzW78rn-_rXRg2t_RpwBQ1Ap-ugtqE-vb-P6YyGO++VRA@mail.gmail.com' \
--to=reijiw@google.com \
--cc=kernel-team@android.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=maz@kernel.org \
--cc=oliver.upton@linux.dev \
--cc=schspa@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).