On Thu, 2024-02-01 at 16:14 +0100, Heiko Carstens wrote:
> 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?
When processing a KVM_RUN ioctl, the kernel is going to swap the
host/guest access registers in sync_regs, enter SIE, and then swap them
back in store_regs when it has to exit to userspace. So then on the
QEMU side it might look something like this:
kvm_arch_handle_exit
handle_intercept
handle_instruction
handle_b2
ioinst_handle_stsch
s390_cpu_virt_mem_rw(ar=0xe, is_write=true)
kvm_s390_mem_op
Where the interesting registers at that point are:
acr0 0x3ff 1023
acr1 0x33bff8c0 868219072
...
acr14 0x0 0
Note ACR0/1 are already buggered up from an earlier pass.
The above carries us through the kernel this way:
kvm_arch_vcpu_ioctl(KVM_S390_MEM_OP)
kvm_s390_vcpu_memsida_op
kvm_s390_vcpu_mem_op
write_guest_with_key
access_guest_with_key
get_vcpu_asce
ar_translate
save_access_regs(kvm_run)
>
> > 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.
Well regardless of this patch, I think it's using the contents of the
host registers today, isn't it? save_access_regs() does a STAM to put
the current registers into some bit of memory, so ar_translation() can
do regular logic against it. The above just changes WHERE that bit of
memory lives from something shared with another ioctl to something
local to ar_translation().
My original change just removed the save_access_regs() call entirely
and read the contents of the kvm_run struct since they were last saved
(see below). This "feels" better to me, and works for the scenario I
bumped into too. Maybe this is more appropriate?
---8<---
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 5bfcc50c1a68..c5ed3b0b665a 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -391,7 +391,6 @@ 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];
if (ar == 0 || alet.val == 0) {
