| Message ID | 20171205083321.102933-5-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05.12.2017 09:33, Christian Borntraeger wrote: > From: Janosch Frank <frankja@linux.vnet.ibm.com> > > All skey functions call skey_check_enable at their start, which checks > if we are in the PSTATE and injects a privileged operation exception > if we are. > > Unfortunately they continue processing afterwards and perform the > operation anyhow as skey_check_enable does not deliver an error if the > exception injection was successful. > > Let's move the PSTATE check into the skey functions and exit them on > such an occasion, also we now do not enable skey handling anymore in > such a case. > > Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Fixes: a7e19ab ("KVM: s390: handle missing storage-key facility") > Cc: <stable@vger.kernel.org> # v4.8+ > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/kvm/priv.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) Reviewed-by: Thomas Huth <thuth@redhat.com>
On Tue, 5 Dec 2017 09:33:21 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > From: Janosch Frank <frankja@linux.vnet.ibm.com> > > All skey functions call skey_check_enable at their start, which checks > if we are in the PSTATE and injects a privileged operation exception > if we are. > > Unfortunately they continue processing afterwards and perform the > operation anyhow as skey_check_enable does not deliver an error if the > exception injection was successful. > > Let's move the PSTATE check into the skey functions and exit them on > such an occasion, also we now do not enable skey handling anymore in > such a case. > > Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Fixes: a7e19ab ("KVM: s390: handle missing storage-key facility") > Cc: <stable@vger.kernel.org> # v4.8+ > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/kvm/priv.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com> This reminds me of something I stumbled upon the other day: handle_ri() and handle_gs() (both implemented in priv.c) don't seem to have a check for PSTATE, yet they enable ri/gs before retrying the instruction. Is that correct?
On 05.12.2017 10:13, Cornelia Huck wrote: > On Tue, 5 Dec 2017 09:33:21 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> From: Janosch Frank <frankja@linux.vnet.ibm.com> >> >> All skey functions call skey_check_enable at their start, which checks >> if we are in the PSTATE and injects a privileged operation exception >> if we are. >> >> Unfortunately they continue processing afterwards and perform the >> operation anyhow as skey_check_enable does not deliver an error if the >> exception injection was successful. >> >> Let's move the PSTATE check into the skey functions and exit them on >> such an occasion, also we now do not enable skey handling anymore in >> such a case. >> >> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> >> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Fixes: a7e19ab ("KVM: s390: handle missing storage-key facility") >> Cc: <stable@vger.kernel.org> # v4.8+ >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/kvm/priv.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> Thanks! > > This reminds me of something I stumbled upon the other day: > > handle_ri() and handle_gs() (both implemented in priv.c) don't seem to > have a check for PSTATE, yet they enable ri/gs before retrying the > instruction. Is that correct? > None of the gs instructions are privileged as far as I know. Same seems to be true for ri as far as I've scanned the spec. The privileged parts are the control register and PSW changes which are handled elsewhere.
On 12/05/2017 10:13 AM, Cornelia Huck wrote: > On Tue, 5 Dec 2017 09:33:21 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> From: Janosch Frank <frankja@linux.vnet.ibm.com> >> >> All skey functions call skey_check_enable at their start, which checks >> if we are in the PSTATE and injects a privileged operation exception >> if we are. >> >> Unfortunately they continue processing afterwards and perform the >> operation anyhow as skey_check_enable does not deliver an error if the >> exception injection was successful. >> >> Let's move the PSTATE check into the skey functions and exit them on >> such an occasion, also we now do not enable skey handling anymore in >> such a case. >> >> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> >> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Fixes: a7e19ab ("KVM: s390: handle missing storage-key facility") >> Cc: <stable@vger.kernel.org> # v4.8+ >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/kvm/priv.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > > This reminds me of something I stumbled upon the other day: > > handle_ri() and handle_gs() (both implemented in priv.c) don't seem to > have a check for PSTATE, yet they enable ri/gs before retrying the > instruction. Is that correct? The guarded storage ops (e3 49 and e3 4d) are problem state. Most of the ri instruction are as well, so problem state can enable RI interpretion. We could do some optimization to only enable RI if the instruction would enable in for the guest (e.g. an inspection of the control block could leave RI disabled). On the other hand that would require to implement these instruction in KVM, which I would like to avoid. Right now we enable RI and re-drive the instruction.
On Tue, 5 Dec 2017 10:32:03 +0100 Janosch Frank <frankja@linux.vnet.ibm.com> wrote: > On 05.12.2017 10:13, Cornelia Huck wrote: > > This reminds me of something I stumbled upon the other day: > > > > handle_ri() and handle_gs() (both implemented in priv.c) don't seem to > > have a check for PSTATE, yet they enable ri/gs before retrying the > > instruction. Is that correct? > > > > None of the gs instructions are privileged as far as I know. Same seems > to be true for ri as far as I've scanned the spec. > > The privileged parts are the control register and PSW changes which are > handled elsewhere. OK, thanks. I found the z14 PoP, but the ri instructions don't seem to be documented in there.
On Tue, 5 Dec 2017 10:39:26 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 12/05/2017 10:13 AM, Cornelia Huck wrote: > > This reminds me of something I stumbled upon the other day: > > > > handle_ri() and handle_gs() (both implemented in priv.c) don't seem to > > have a check for PSTATE, yet they enable ri/gs before retrying the > > instruction. Is that correct? > > The guarded storage ops (e3 49 and e3 4d) are problem state. > Most of the ri instruction are as well, so problem state can enable RI > interpretion. > > We could do some optimization to only enable RI if the > instruction would enable in for the guest (e.g. an inspection of the > control block could leave RI disabled). On the other hand that would > require to implement these instruction in KVM, which I would like > to avoid. Right now we enable RI and re-drive the instruction. It's probably not worth it, I think.
On 05.12.2017 09:33, Christian Borntraeger wrote: > From: Janosch Frank <frankja@linux.vnet.ibm.com> > > All skey functions call skey_check_enable at their start, which checks > if we are in the PSTATE and injects a privileged operation exception > if we are. Correct thing to do, are we sure we even get an intercept? (no documentation at hand :( ) > > Unfortunately they continue processing afterwards and perform the > operation anyhow as skey_check_enable does not deliver an error if the > exception injection was successful. > > Let's move the PSTATE check into the skey functions and exit them on > such an occasion, also we now do not enable skey handling anymore in > such a case. > > Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Fixes: a7e19ab ("KVM: s390: handle missing storage-key facility") > Cc: <stable@vger.kernel.org> # v4.8+ > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/kvm/priv.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 28b69ab..572496c 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -232,8 +232,6 @@ static int try_handle_skey(struct kvm_vcpu *vcpu) > VCPU_EVENT(vcpu, 4, "%s", "retrying storage key operation"); > return -EAGAIN; > } > - if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) > - return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); > return 0; > } > > @@ -244,6 +242,9 @@ static int handle_iske(struct kvm_vcpu *vcpu) > int reg1, reg2; > int rc; > > + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) > + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); > + > rc = try_handle_skey(vcpu); > if (rc) > return rc != -EAGAIN ? rc : 0; > @@ -273,6 +274,9 @@ static int handle_rrbe(struct kvm_vcpu *vcpu) > int reg1, reg2; > int rc; > > + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) > + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); > + > rc = try_handle_skey(vcpu); > if (rc) > return rc != -EAGAIN ? rc : 0; > @@ -308,6 +312,9 @@ static int handle_sske(struct kvm_vcpu *vcpu) > int reg1, reg2; > int rc; > > + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) > + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); > + > rc = try_handle_skey(vcpu); > if (rc) > return rc != -EAGAIN ? rc : 0; >
On 12/05/2017 05:53 PM, David Hildenbrand wrote: > On 05.12.2017 09:33, Christian Borntraeger wrote: >> From: Janosch Frank <frankja@linux.vnet.ibm.com> >> >> All skey functions call skey_check_enable at their start, which checks >> if we are in the PSTATE and injects a privileged operation exception >> if we are. > > Correct thing to do, are we sure we even get an intercept? > (no documentation at hand :( ) If we run keyless we certainly get intercepts. The good thing is that Linux hosts do not use the keys and run with PSW key 0, so this does not open any protection issue for those. If we run with keys enabled, we might also get exits if the host holds the pgste lock for too long.
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c index 28b69ab..572496c 100644 --- a/arch/s390/kvm/priv.c +++ b/arch/s390/kvm/priv.c @@ -232,8 +232,6 @@ static int try_handle_skey(struct kvm_vcpu *vcpu) VCPU_EVENT(vcpu, 4, "%s", "retrying storage key operation"); return -EAGAIN; } - if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) - return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); return 0; } @@ -244,6 +242,9 @@ static int handle_iske(struct kvm_vcpu *vcpu) int reg1, reg2; int rc; + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); + rc = try_handle_skey(vcpu); if (rc) return rc != -EAGAIN ? rc : 0; @@ -273,6 +274,9 @@ static int handle_rrbe(struct kvm_vcpu *vcpu) int reg1, reg2; int rc; + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); + rc = try_handle_skey(vcpu); if (rc) return rc != -EAGAIN ? rc : 0; @@ -308,6 +312,9 @@ static int handle_sske(struct kvm_vcpu *vcpu) int reg1, reg2; int rc; + if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) + return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP); + rc = try_handle_skey(vcpu); if (rc) return rc != -EAGAIN ? rc : 0;