Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return

From: David Brazdil <dbrazdil@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kvmarm@lists.cs.columbia.edu, James Morse <james.morse@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, kernel-team@android.com
Subject: Re: [PATCH] KVM: arm64: Allow PSCI SYSTEM_OFF/RESET to return
Date: Wed, 30 Dec 2020 11:03:00 +0000	[thread overview]
Message-ID: <20201230110300.a2wei7ojhjkqfkgk@google.com> (raw)
In-Reply-To: <229d84b8d7818a170adf489535ea2d43@kernel.org>

On Tue, Dec 29, 2020 at 05:16:41PM +0000, Marc Zyngier wrote:
> Hi David,
> 
> On 2020-12-29 16:00, David Brazdil wrote:
> > The KVM/arm64 PSCI relay assumes that SYSTEM_OFF and SYSTEM_RESET should
> > not return, as dictated by the PSCI spec. However, there is firmware out
> > there which breaks this assumption, leading to a hyp panic. Make KVM
> > more robust to broken firmware by allowing these to return.
> > 
> > Signed-off-by: David Brazdil <dbrazdil@google.com>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/psci-relay.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > index e3947846ffcb..8e7128cb7667 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> > @@ -77,12 +77,6 @@ static unsigned long psci_forward(struct
> > kvm_cpu_context *host_ctxt)
> >  			 cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
> >  }
> > 
> > -static __noreturn unsigned long psci_forward_noreturn(struct
> > kvm_cpu_context *host_ctxt)
> > -{
> > -	psci_forward(host_ctxt);
> > -	hyp_panic(); /* unreachable */
> > -}
> > -
> >  static unsigned int find_cpu_id(u64 mpidr)
> >  {
> >  	unsigned int i;
> > @@ -251,10 +245,13 @@ static unsigned long psci_0_2_handler(u64
> > func_id, struct kvm_cpu_context *host_
> >  	case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> >  	case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
> >  		return psci_forward(host_ctxt);
> > +	/*
> > +	 * SYSTEM_OFF/RESET should not return according to the spec.
> > +	 * Allow it so as to stay robust to broken firmware.
> > +	 */
> >  	case PSCI_0_2_FN_SYSTEM_OFF:
> >  	case PSCI_0_2_FN_SYSTEM_RESET:
> > -		psci_forward_noreturn(host_ctxt);
> > -		unreachable();
> > +		return psci_forward(host_ctxt);
> >  	case PSCI_0_2_FN64_CPU_SUSPEND:
> >  		return psci_cpu_suspend(func_id, host_ctxt);
> >  	case PSCI_0_2_FN64_CPU_ON:
> 
> Thanks for having tracked this.
> 
> I wonder whether we should also taint the kernel in this case,
> because this is completely unexpected, and a major spec violation.
> 
> Ideally, we'd be able to detect this case and prevent pKVM from
> getting initialised at all, but I guess there is no way to detect
> the sucker without ... calling SYSTEM_RESET?

Yeah, there are no bits to check, unfortunately. :(

David