| Message ID | 20230607194554.87359-2-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable writable for idregs DFR0,PFR0, MMFR{0,1,2} | expand |
On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote: > Since number of context-aware breakpoints must be no more than number > of supported breakpoints according to Arm ARM, return an error if > userspace tries to set CTX_CMPS field to such value. > > Signed-off-by: Jing Zhang <jingzhangos@google.com> > --- > arch/arm64/kvm/sys_regs.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 50d4e25f42d3..a6299c796d03 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1539,9 +1539,14 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, > const struct sys_reg_desc *rd, > u64 val) > { > - u8 pmuver, host_pmuver; > + u8 pmuver, host_pmuver, brps, ctx_cmps; > bool valid_pmu; > > + brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val); > + ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val); > + if (ctx_cmps > brps) > + return -EINVAL; > + I'm not fully convinced on the need to do this sort of cross-field validation... I think it is probably more trouble than it is worth. If userspace writes something illogical to the register, oh well. All we should care about is that the advertised feature set is a subset of what's supported by the host. The series doesn't even do complete sanity checking, and instead works on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require special handling depending on how pedantic you're feeling. AArch32 support at a higher exception level implies AArch32 support at all lower exception levels. But that isn't a suggestion to implement it, more of a suggestion to just avoid the problem as a whole. > host_pmuver = kvm_arm_pmu_get_pmuver_limit(); > > /* > @@ -2061,7 +2066,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { > .get_user = get_id_reg, > .set_user = set_id_aa64dfr0_el1, > .reset = read_sanitised_id_aa64dfr0_el1, > - .val = ID_AA64DFR0_EL1_PMUVer_MASK, }, > + .val = GENMASK(63, 0), }, DebugVer requires special handling, as the minimum safe value is 0x6 for the field. IIUC, as written we would permit userspace to write any value less than the current register value. I posted a patch to 'fix' this, but it isn't actually a bug in what's upstream. Could you pick that patch up and discard the 'Fixes' tag on it? https://lore.kernel.org/kvmarm/20230623205232.2837077-1-oliver.upton@linux.dev/ > ID_SANITISED(ID_AA64DFR1_EL1), > ID_UNALLOCATED(5,2), > ID_UNALLOCATED(5,3), > -- > 2.41.0.rc0.172.g3f132b7071-goog > >
On Mon, Jun 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote: > On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote: >> Since number of context-aware breakpoints must be no more than number >> of supported breakpoints according to Arm ARM, return an error if >> userspace tries to set CTX_CMPS field to such value. >> >> Signed-off-by: Jing Zhang <jingzhangos@google.com> >> --- >> arch/arm64/kvm/sys_regs.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index 50d4e25f42d3..a6299c796d03 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -1539,9 +1539,14 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, >> const struct sys_reg_desc *rd, >> u64 val) >> { >> - u8 pmuver, host_pmuver; >> + u8 pmuver, host_pmuver, brps, ctx_cmps; >> bool valid_pmu; >> >> + brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val); >> + ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val); >> + if (ctx_cmps > brps) >> + return -EINVAL; >> + > > I'm not fully convinced on the need to do this sort of cross-field > validation... I think it is probably more trouble than it is worth. If > userspace writes something illogical to the register, oh well. All we > should care about is that the advertised feature set is a subset of > what's supported by the host. > > The series doesn't even do complete sanity checking, and instead works > on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require > special handling depending on how pedantic you're feeling. AArch32 > support at a higher exception level implies AArch32 support at all lower > exception levels. > > But that isn't a suggestion to implement it, more of a suggestion to > just avoid the problem as a whole. Generally speaking, how much effort do we want to invest to prevent userspace from doing dumb things? "Make sure we advertise a subset of features of what the host supports" and "disallow writing values that are not allowed by the architecture in the first place" seem reasonable, but if userspace wants to create weird frankencpus[1], should it be allowed to break the guest and get to keep the pieces? I'd be more in favour to rely on userspace to configure something that is actually usable; it needs to sanitize any user-provided configuration anyway. [1] I think userspace will end up creating frankencpus in any case, but at least it should be the kind that doesn't look out of place in the subway if you dress it in proper clothing.
Hi Cornelia, On Tue, Jul 04, 2023 at 05:06:30PM +0200, Cornelia Huck wrote: > On Mon, Jun 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote: > > > On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote: > >> + brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val); > >> + ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val); > >> + if (ctx_cmps > brps) > >> + return -EINVAL; > >> + > > > > I'm not fully convinced on the need to do this sort of cross-field > > validation... I think it is probably more trouble than it is worth. If > > userspace writes something illogical to the register, oh well. All we > > should care about is that the advertised feature set is a subset of > > what's supported by the host. > > > > The series doesn't even do complete sanity checking, and instead works > > on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require > > special handling depending on how pedantic you're feeling. AArch32 > > support at a higher exception level implies AArch32 support at all lower > > exception levels. > > > > But that isn't a suggestion to implement it, more of a suggestion to > > just avoid the problem as a whole. > > Generally speaking, how much effort do we want to invest to prevent > userspace from doing dumb things? "Make sure we advertise a subset of > features of what the host supports" and "disallow writing values that > are not allowed by the architecture in the first place" seem reasonable, > but if userspace wants to create weird frankencpus[1], should it be > allowed to break the guest and get to keep the pieces? What I'm specifically objecting to is having KVM do sanity checks across multiple fields. That requires explicit, per-field plumbing that will eventually become a tangled mess that Marc and I will have to maintain. The context-aware breakpoints is one example, as is ensuring SVE is exposed iff FP is too. In all likelihood we'll either get some part of this wrong, or miss a required check altogether. Modulo a few exceptions to this case, I think per-field validation is going to cover almost everything we're worried about, and we get that largely for free from arm64_check_features(). > I'd be more in favour to rely on userspace to configure something that > is actually usable; it needs to sanitize any user-provided configuration > anyway. Just want to make sure I understand your sentiment here, you'd be in favor of the more robust sanitization? > [1] I think userspace will end up creating frankencpus in any case, but > at least it should be the kind that doesn't look out of place in the > subway if you dress it in proper clothing. I mean, KVM already advertises a frankencpu in the first place, so we're off to a good start :) -- Thanks, Oliver
On Tue, Jul 04 2023, Oliver Upton <oliver.upton@linux.dev> wrote: > Hi Cornelia, > > On Tue, Jul 04, 2023 at 05:06:30PM +0200, Cornelia Huck wrote: >> On Mon, Jun 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote: >> >> > On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote: >> >> + brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val); >> >> + ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val); >> >> + if (ctx_cmps > brps) >> >> + return -EINVAL; >> >> + >> > >> > I'm not fully convinced on the need to do this sort of cross-field >> > validation... I think it is probably more trouble than it is worth. If >> > userspace writes something illogical to the register, oh well. All we >> > should care about is that the advertised feature set is a subset of >> > what's supported by the host. >> > >> > The series doesn't even do complete sanity checking, and instead works >> > on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require >> > special handling depending on how pedantic you're feeling. AArch32 >> > support at a higher exception level implies AArch32 support at all lower >> > exception levels. >> > >> > But that isn't a suggestion to implement it, more of a suggestion to >> > just avoid the problem as a whole. >> >> Generally speaking, how much effort do we want to invest to prevent >> userspace from doing dumb things? "Make sure we advertise a subset of >> features of what the host supports" and "disallow writing values that >> are not allowed by the architecture in the first place" seem reasonable, >> but if userspace wants to create weird frankencpus[1], should it be >> allowed to break the guest and get to keep the pieces? > > What I'm specifically objecting to is having KVM do sanity checks across > multiple fields. That requires explicit, per-field plumbing that will > eventually become a tangled mess that Marc and I will have to maintain. > The context-aware breakpoints is one example, as is ensuring SVE is > exposed iff FP is too. In all likelihood we'll either get some part of > this wrong, or miss a required check altogether. Nod, this sounds like more trouble than it's worth in the end. > > Modulo a few exceptions to this case, I think per-field validation is > going to cover almost everything we're worried about, and we get that > largely for free from arm64_check_features(). > >> I'd be more in favour to rely on userspace to configure something that >> is actually usable; it needs to sanitize any user-provided configuration >> anyway. > > Just want to make sure I understand your sentiment here, you'd be in > favor of the more robust sanitization? In userspace. E.g. QEMU can go ahead and try to implement the user-exposed knobs in a way that the really broken configurations are not even possible. I'd also expect userspace to have a more complete view of what it is trying to instantiate (especially if code is shared between instantiating a vcpu for use with KVM and a fully emulated vcpu -- we probably don't want to go all crazy in the latter case, either.) > >> [1] I think userspace will end up creating frankencpus in any case, but >> at least it should be the kind that doesn't look out of place in the >> subway if you dress it in proper clothing. > > I mean, KVM already advertises a frankencpu in the first place, so we're > off to a good start :) Indeed :)
Hi Oliver, Cornelia, Thanks for the discussion about the cross-field validation. I'm happy to know that we all agree to avoid that. I'll remove those validations for later posts. Thanks, Jing On Wed, Jul 5, 2023 at 1:49 AM Cornelia Huck <cohuck@redhat.com> wrote: > > On Tue, Jul 04 2023, Oliver Upton <oliver.upton@linux.dev> wrote: > > > Hi Cornelia, > > > > On Tue, Jul 04, 2023 at 05:06:30PM +0200, Cornelia Huck wrote: > >> On Mon, Jun 26 2023, Oliver Upton <oliver.upton@linux.dev> wrote: > >> > >> > On Wed, Jun 07, 2023 at 07:45:51PM +0000, Jing Zhang wrote: > >> >> + brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val); > >> >> + ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val); > >> >> + if (ctx_cmps > brps) > >> >> + return -EINVAL; > >> >> + > >> > > >> > I'm not fully convinced on the need to do this sort of cross-field > >> > validation... I think it is probably more trouble than it is worth. If > >> > userspace writes something illogical to the register, oh well. All we > >> > should care about is that the advertised feature set is a subset of > >> > what's supported by the host. > >> > > >> > The series doesn't even do complete sanity checking, and instead works > >> > on a few cherry-picked examples. AA64PFR0.EL{0-3} would also require > >> > special handling depending on how pedantic you're feeling. AArch32 > >> > support at a higher exception level implies AArch32 support at all lower > >> > exception levels. > >> > > >> > But that isn't a suggestion to implement it, more of a suggestion to > >> > just avoid the problem as a whole. > >> > >> Generally speaking, how much effort do we want to invest to prevent > >> userspace from doing dumb things? "Make sure we advertise a subset of > >> features of what the host supports" and "disallow writing values that > >> are not allowed by the architecture in the first place" seem reasonable, > >> but if userspace wants to create weird frankencpus[1], should it be > >> allowed to break the guest and get to keep the pieces? > > > > What I'm specifically objecting to is having KVM do sanity checks across > > multiple fields. That requires explicit, per-field plumbing that will > > eventually become a tangled mess that Marc and I will have to maintain. > > The context-aware breakpoints is one example, as is ensuring SVE is > > exposed iff FP is too. In all likelihood we'll either get some part of > > this wrong, or miss a required check altogether. > > Nod, this sounds like more trouble than it's worth in the end. > > > > > Modulo a few exceptions to this case, I think per-field validation is > > going to cover almost everything we're worried about, and we get that > > largely for free from arm64_check_features(). > > > >> I'd be more in favour to rely on userspace to configure something that > >> is actually usable; it needs to sanitize any user-provided configuration > >> anyway. > > > > Just want to make sure I understand your sentiment here, you'd be in > > favor of the more robust sanitization? > > In userspace. E.g. QEMU can go ahead and try to implement the > user-exposed knobs in a way that the really broken configurations are > not even possible. I'd also expect userspace to have a more complete > view of what it is trying to instantiate (especially if code is shared > between instantiating a vcpu for use with KVM and a fully emulated > vcpu -- we probably don't want to go all crazy in the latter case, > either.) > > > > >> [1] I think userspace will end up creating frankencpus in any case, but > >> at least it should be the kind that doesn't look out of place in the > >> subway if you dress it in proper clothing. > > > > I mean, KVM already advertises a frankencpu in the first place, so we're > > off to a good start :) > > Indeed :) >
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 50d4e25f42d3..a6299c796d03 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1539,9 +1539,14 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, u64 val) { - u8 pmuver, host_pmuver; + u8 pmuver, host_pmuver, brps, ctx_cmps; bool valid_pmu; + brps = FIELD_GET(ID_AA64DFR0_EL1_BRPs_MASK, val); + ctx_cmps = FIELD_GET(ID_AA64DFR0_EL1_CTX_CMPs_MASK, val); + if (ctx_cmps > brps) + return -EINVAL; + host_pmuver = kvm_arm_pmu_get_pmuver_limit(); /* @@ -2061,7 +2066,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { .get_user = get_id_reg, .set_user = set_id_aa64dfr0_el1, .reset = read_sanitised_id_aa64dfr0_el1, - .val = ID_AA64DFR0_EL1_PMUVer_MASK, }, + .val = GENMASK(63, 0), }, ID_SANITISED(ID_AA64DFR1_EL1), ID_UNALLOCATED(5,2), ID_UNALLOCATED(5,3),
Since number of context-aware breakpoints must be no more than number of supported breakpoints according to Arm ARM, return an error if userspace tries to set CTX_CMPS field to such value. Signed-off-by: Jing Zhang <jingzhangos@google.com> --- arch/arm64/kvm/sys_regs.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)