[PATCH] KVM: arm64: Reject attempts to set invalid debug arch version

[PATCH] KVM: arm64: Reject attempts to set invalid debug arch version

[Oliver Upton]

The debug architecture is mandatory in ARMv8, so KVM should not allow
userspace to configure a vCPU with less than that. Of course, this isn't
handled elegantly by the generic ID register plumbing, as the respective
ID register fields have a nonzero starting value.

Add an explicit check for debug versions less than v8 of the
architecture.

Fixes: c118cead07a7 ("KVM: arm64: Use generic sanitisation for ID_(AA64)DFR0_EL1")
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/sys_regs.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 1a13bab1a06c..5b25053a8e04 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1482,6 +1482,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 			       const struct sys_reg_desc *rd,
 			       u64 val)
 {
+	u8 debugver = SYS_FIELD_GET(ID_AA64DFR0_EL1, DebugVer, val);
 	u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, val);
 
 	/*
@@ -1501,6 +1502,13 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 	if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
 		val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
 
+	/*
+	 * ID_AA64DFR0_EL1.DebugVer is one of those awkward fields with a
+	 * nonzero minimum safe value.
+	 */
+	if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP)
+		return -EINVAL;
+
 	return set_id_reg(vcpu, rd, val);
 }
 
@@ -1522,6 +1530,7 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 			   u64 val)
 {
 	u8 perfmon = SYS_FIELD_GET(ID_DFR0_EL1, PerfMon, val);
+	u8 copdbg = SYS_FIELD_GET(ID_DFR0_EL1, CopDbg, val);
 
 	if (perfmon == ID_DFR0_EL1_PerfMon_IMPDEF) {
 		val &= ~ID_DFR0_EL1_PerfMon_MASK;
@@ -1537,6 +1546,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	if (perfmon != 0 && perfmon < ID_DFR0_EL1_PerfMon_PMUv3)
 		return -EINVAL;
 
+	if (copdbg < ID_DFR0_EL1_CopDbg_Armv8)
+		return -EINVAL;
+
 	return set_id_reg(vcpu, rd, val);
 }
 

base-commit: 192df2aa0113ddddee2a93e453ff46610807b425
-- 
2.41.0.178.g377b9f9a00-goog

Re: [PATCH] KVM: arm64: Reject attempts to set invalid debug arch version

[Marc Zyngier]

On Fri, 23 Jun 2023 21:52:32 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> The debug architecture is mandatory in ARMv8, so KVM should not allow
> userspace to configure a vCPU with less than that. Of course, this isn't
> handled elegantly by the generic ID register plumbing, as the respective
> ID register fields have a nonzero starting value.
> 
> Add an explicit check for debug versions less than v8 of the
> architecture.
> 
> Fixes: c118cead07a7 ("KVM: arm64: Use generic sanitisation for ID_(AA64)DFR0_EL1")
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/sys_regs.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 1a13bab1a06c..5b25053a8e04 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1482,6 +1482,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>  			       const struct sys_reg_desc *rd,
>  			       u64 val)
>  {
> +	u8 debugver = SYS_FIELD_GET(ID_AA64DFR0_EL1, DebugVer, val);
>  	u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, val);
>  
>  	/*
> @@ -1501,6 +1502,13 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>  	if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
>  		val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
>  
> +	/*
> +	 * ID_AA64DFR0_EL1.DebugVer is one of those awkward fields with a
> +	 * nonzero minimum safe value.
> +	 */
> +	if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP)
> +		return -EINVAL;
> +

Why isn't that caught by the check at the end of arm64_check_features
which says that for RO fields, the only safe value is the sanitised
version?

Am I missing something obvious (full disclosure, I've had a single
coffee at this time of the day...)?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: arm64: Reject attempts to set invalid debug arch version

[Oliver Upton]

On Mon, Jun 26, 2023 at 10:23:24AM +0100, Marc Zyngier wrote:
> On Fri, 23 Jun 2023 21:52:32 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > The debug architecture is mandatory in ARMv8, so KVM should not allow
> > userspace to configure a vCPU with less than that. Of course, this isn't
> > handled elegantly by the generic ID register plumbing, as the respective
> > ID register fields have a nonzero starting value.
> > 
> > Add an explicit check for debug versions less than v8 of the
> > architecture.
> > 
> > Fixes: c118cead07a7 ("KVM: arm64: Use generic sanitisation for ID_(AA64)DFR0_EL1")
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 1a13bab1a06c..5b25053a8e04 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1482,6 +1482,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> >  			       const struct sys_reg_desc *rd,
> >  			       u64 val)
> >  {
> > +	u8 debugver = SYS_FIELD_GET(ID_AA64DFR0_EL1, DebugVer, val);
> >  	u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, val);
> >  
> >  	/*
> > @@ -1501,6 +1502,13 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> >  	if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
> >  		val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> >  
> > +	/*
> > +	 * ID_AA64DFR0_EL1.DebugVer is one of those awkward fields with a
> > +	 * nonzero minimum safe value.
> > +	 */
> > +	if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP)
> > +		return -EINVAL;
> > +
> 
> Why isn't that caught by the check at the end of arm64_check_features
> which says that for RO fields, the only safe value is the sanitised
> version?

You're right, I was mistaken in thinking the field was already writable.
Now, having said that, it _is_ an issue with Jing's series to make DFR0
fully writable.

https://lore.kernel.org/kvmarm/20230607194554.87359-2-jingzhangos@google.com/

-- 
Thanks,
Oliver

Re: [PATCH] KVM: arm64: Reject attempts to set invalid debug arch version

[Jitindar Singh, Suraj]

Hi Oliver,

On Fri, 2023-06-23 at 20:52 +0000, Oliver Upton wrote:
> The debug architecture is mandatory in ARMv8, so KVM should not allow
> userspace to configure a vCPU with less than that. Of course, this
> isn't
> handled elegantly by the generic ID register plumbing, as the
> respective
> ID register fields have a nonzero starting value.
> 
> Add an explicit check for debug versions less than v8 of the
> architecture.
> 
> Fixes: c118cead07a7 ("KVM: arm64: Use generic sanitisation for
> ID_(AA64)DFR0_EL1")
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/sys_regs.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 1a13bab1a06c..5b25053a8e04 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1482,6 +1482,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> *vcpu,
>                                const struct sys_reg_desc *rd,
>                                u64 val)
>  {
> +       u8 debugver = SYS_FIELD_GET(ID_AA64DFR0_EL1, DebugVer, val);
>         u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, val);
>  
>         /*
> @@ -1501,6 +1502,13 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> *vcpu,
>         if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
>                 val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
>  
> +       /*
> +        * ID_AA64DFR0_EL1.DebugVer is one of those awkward fields
> with a
> +        * nonzero minimum safe value.
> +        */
> +       if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP)
> +               return -EINVAL;
> +

AA64DFR0_EL1_DebugVer has feature type FTR_EXACT so the guest value
must exactly match the host value. So I don't believe this check is
required.

>         return set_id_reg(vcpu, rd, val);
>  }
>  
> @@ -1522,6 +1530,7 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> *vcpu,
>                            u64 val)
>  {
>         u8 perfmon = SYS_FIELD_GET(ID_DFR0_EL1, PerfMon, val);
> +       u8 copdbg = SYS_FIELD_GET(ID_DFR0_EL1, CopDbg, val);
>  
>         if (perfmon == ID_DFR0_EL1_PerfMon_IMPDEF) {
>                 val &= ~ID_DFR0_EL1_PerfMon_MASK;
> @@ -1537,6 +1546,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> *vcpu,
>         if (perfmon != 0 && perfmon < ID_DFR0_EL1_PerfMon_PMUv3)
>                 return -EINVAL;
>  
> +       if (copdbg < ID_DFR0_EL1_CopDbg_Armv8)
> +               return -EINVAL;
> +

This one has type FTR_LOWER_SAFE and so the check is required.

Thanks,
Suraj

>         return set_id_reg(vcpu, rd, val);
>  }
>  
> 
> base-commit: 192df2aa0113ddddee2a93e453ff46610807b425

Re: [PATCH] KVM: arm64: Reject attempts to set invalid debug arch version

[Oliver Upton]

Hi Suraj

On Thu, Jun 29, 2023 at 01:12:31AM +0000, Jitindar Singh, Suraj wrote:
> > @@ -1501,6 +1502,13 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> > *vcpu,
> >         if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
> >                 val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> >  
> > +       /*
> > +        * ID_AA64DFR0_EL1.DebugVer is one of those awkward fields
> > with a
> > +        * nonzero minimum safe value.
> > +        */
> > +       if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP)
> > +               return -EINVAL;
> > +
> 
> AA64DFR0_EL1_DebugVer has feature type FTR_EXACT so the guest value
> must exactly match the host value. So I don't believe this check is
> required.

I got waaaay ahead of myself and sent this out of context. As I had said
to Marc, the fixes is utter crap on this patch since we don't permit
changes to this field.

What I was actually looking at is raising the limit for the debug
architecture on KVM and permitting safe values less than that of the
host. So in isolation this is pointless, but will be useful for a
functional change.

> >         return set_id_reg(vcpu, rd, val);
> >  }
> >  
> > @@ -1522,6 +1530,7 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> > *vcpu,
> >                            u64 val)
> >  {
> >         u8 perfmon = SYS_FIELD_GET(ID_DFR0_EL1, PerfMon, val);
> > +       u8 copdbg = SYS_FIELD_GET(ID_DFR0_EL1, CopDbg, val);
> >  
> >         if (perfmon == ID_DFR0_EL1_PerfMon_IMPDEF) {
> >                 val &= ~ID_DFR0_EL1_PerfMon_MASK;
> > @@ -1537,6 +1546,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> > *vcpu,
> >         if (perfmon != 0 && perfmon < ID_DFR0_EL1_PerfMon_PMUv3)
> >                 return -EINVAL;
> >  
> > +       if (copdbg < ID_DFR0_EL1_CopDbg_Armv8)
> > +               return -EINVAL;
> > +
> 
> This one has type FTR_LOWER_SAFE and so the check is required.

Same goes for this field -- any changes are forbidden since it is not in
the writable mask.

Thanks for having a look, apologies for the confusion.

--
Thanks,
Oliver

Re: [PATCH] KVM: arm64: Reject attempts to set invalid debug arch version

[Jitindar Singh, Suraj]

Hi Oliver,

On Wed, 2023-06-28 at 19:58 -0700, Oliver Upton wrote:
> Hi Suraj
> 
> On Thu, Jun 29, 2023 at 01:12:31AM +0000, Jitindar Singh, Suraj
> wrote:
> > > @@ -1501,6 +1502,13 @@ static int set_id_aa64dfr0_el1(struct
> > > kvm_vcpu
> > > *vcpu,
> > >         if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
> > >                 val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> > >  
> > > +       /*
> > > +        * ID_AA64DFR0_EL1.DebugVer is one of those awkward
> > > fields
> > > with a
> > > +        * nonzero minimum safe value.
> > > +        */
> > > +       if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP)
> > > +               return -EINVAL;
> > > +
> > 
> > AA64DFR0_EL1_DebugVer has feature type FTR_EXACT so the guest value
> > must exactly match the host value. So I don't believe this check is
> > required.
> 
> I got waaaay ahead of myself and sent this out of context. As I had
> said
> to Marc, the fixes is utter crap on this patch since we don't permit
> changes to this field.

For now this is correct. However with the series from Jing enabling
writes to all fields of these registers [1] it will be necessary in
future to have such checks so potentially worth folding this patch into
that series.

[1]
https://lore.kernel.org/linux-arm-kernel/20230607194554.87359-1-jingzhangos@google.com/

> 
> What I was actually looking at is raising the limit for the debug
> architecture on KVM and permitting safe values less than that of the
> host. So in isolation this is pointless, but will be useful for a
> functional change.

Understood. I guess doing something like setting the type to
FTR_LOWER_SAFE in kvm_arm64_ftr_safe_value(). Then this check will be
required :)

> 
> > >         return set_id_reg(vcpu, rd, val);
> > >  }
> > >  
> > > @@ -1522,6 +1530,7 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> > > *vcpu,
> > >                            u64 val)
> > >  {
> > >         u8 perfmon = SYS_FIELD_GET(ID_DFR0_EL1, PerfMon, val);
> > > +       u8 copdbg = SYS_FIELD_GET(ID_DFR0_EL1, CopDbg, val);
> > >  
> > >         if (perfmon == ID_DFR0_EL1_PerfMon_IMPDEF) {
> > >                 val &= ~ID_DFR0_EL1_PerfMon_MASK;
> > > @@ -1537,6 +1546,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> > > *vcpu,
> > >         if (perfmon != 0 && perfmon < ID_DFR0_EL1_PerfMon_PMUv3)
> > >                 return -EINVAL;
> > >  
> > > +       if (copdbg < ID_DFR0_EL1_CopDbg_Armv8)
> > > +               return -EINVAL;
> > > +
> > 
> > This one has type FTR_LOWER_SAFE and so the check is required.
> 
> Same goes for this field -- any changes are forbidden since it is not
> in
> the writable mask.

Comment as above. This is required when enabling writes to all fields
in the register.

Thanks,
Suraj

> 
> Thanks for having a look, apologies for the confusion.
> 
> --
> Thanks,
> Oliver