Re: [RFC PATCH] KVM: s390: remove extra copy of access registers into KVM_RUN

[Heiko Carstens <hca@linux.ibm.com>]

On Wed, Jan 31, 2024 at 09:58:32PM +0100, Eric Farman wrote:
> The routine ar_translation() is called by get_vcpu_asce(), which is
> called from a handful of places, such as an interception that is
> being handled during KVM_RUN processing. In that case, the access
> registers of the vcpu had been saved to a host_acrs struct and then
> the guest access registers loaded from the KVM_RUN struct prior to
> entering SIE. Saving them back to KVM_RUN at this point doesn't do
> any harm, since it will be done again at the end of the KVM_RUN
> loop when the host access registers are restored.
> 
> But that's not the only path into this code. The MEM_OP ioctl can
> be used while specifying an access register, and will arrive here.
> 
> Linux itself doesn't use the access registers for much, but it does
> squirrel the thread local storage variable into ACRs 0 and 1 in
> copy_thread() [1]. This means that the MEM_OP ioctl may copy
> non-zero access registers (the upper- and lower-halves of the TLS
> pointer) to the KVM_RUN struct, which will end up getting propogated
> to the guest once KVM_RUN ioctls occur. Since these are almost
> certainly invalid as far as an ALET goes, an ALET Specification
> Exception would be triggered if it were attempted to be used.

What's the code path that can lead to this scenario?

>  arch/s390/kvm/gaccess.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 5bfcc50c1a68..9205496195a4 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -380,6 +380,7 @@ void ipte_unlock(struct kvm *kvm)
>  static int ar_translation(struct kvm_vcpu *vcpu, union asce *asce, u8 ar,
>  			  enum gacc_mode mode)
>  {
> +	int acrs[NUM_ACRS];
>  	union alet alet;
>  	struct ale ale;
>  	struct aste aste;
> @@ -391,8 +392,8 @@ static int ar_translation(struct kvm_vcpu *vcpu, union asce *asce, u8 ar,
>  	if (ar >= NUM_ACRS)
>  		return -EINVAL;
>  
> -	save_access_regs(vcpu->run->s.regs.acrs);
> -	alet.val = vcpu->run->s.regs.acrs[ar];
> +	save_access_regs(acrs);
> +	alet.val = acrs[ar];

If the above is like you said, then this code would use the host
access register contents for ar translation of the guest?

Or maybe I'm simply misunderstanding what you write.