If a memop fails due to key checked protection, after already having written to the guest, don't indicate suppression to the guest, as that would imply that memory wasn't modified.
This could be considered a fix to the code introducing storage key support, however this is a bug in KVM only if we emulate an instructions writing to an operand spanning multiple pages, which I don't believe we do.
Janis Schoetterl-Glausch (2): KVM: s390: Don't indicate suppression on dirtying, failing memop KVM: s390: selftest: Test suppression indication on key prot exception
arch/s390/kvm/gaccess.c | 47 ++++++++++++++--------- tools/testing/selftests/kvm/s390x/memop.c | 43 ++++++++++++++++++++- 2 files changed, 70 insertions(+), 20 deletions(-)
base-commit: 1ebdbeb03efe89f01f15df038a589077df3d21f5
If user space uses a memop to emulate an instruction and that memop fails, the execution of the instruction ends. Instruction execution can end in different ways, one of which is suppression, which requires that the instruction execute like a no-op. A writing memop that spans multiple pages and fails due to key protection can modified guest memory. Therefore do not indicate a suppressing instruction ending in this case.
Signed-off-by: Janis Schoetterl-Glausch scgl@linux.ibm.com --- arch/s390/kvm/gaccess.c | 47 ++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c index d53a183c2005..3b1fbef82288 100644 --- a/arch/s390/kvm/gaccess.c +++ b/arch/s390/kvm/gaccess.c @@ -491,8 +491,8 @@ enum prot_type { PROT_TYPE_IEP = 4, };
-static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, - u8 ar, enum gacc_mode mode, enum prot_type prot) +static int trans_exc_ending(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar, + enum gacc_mode mode, enum prot_type prot, bool suppress) { struct kvm_s390_pgm_info *pgm = &vcpu->arch.pgm; struct trans_exc_code_bits *tec; @@ -503,22 +503,24 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
switch (code) { case PGM_PROTECTION: - switch (prot) { - case PROT_TYPE_IEP: - tec->b61 = 1; - fallthrough; - case PROT_TYPE_LA: - tec->b56 = 1; - break; - case PROT_TYPE_KEYC: - tec->b60 = 1; - break; - case PROT_TYPE_ALC: - tec->b60 = 1; - fallthrough; - case PROT_TYPE_DAT: - tec->b61 = 1; - break; + if (suppress) { + switch (prot) { + case PROT_TYPE_IEP: + tec->b61 = 1; + fallthrough; + case PROT_TYPE_LA: + tec->b56 = 1; + break; + case PROT_TYPE_KEYC: + tec->b60 = 1; + break; + case PROT_TYPE_ALC: + tec->b60 = 1; + fallthrough; + case PROT_TYPE_DAT: + tec->b61 = 1; + break; + } } fallthrough; case PGM_ASCE_TYPE: @@ -552,6 +554,12 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, return code; }
+static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar, + enum gacc_mode mode, enum prot_type prot) +{ + return trans_exc_ending(vcpu, code, gva, ar, mode, prot, true); +} + static int get_vcpu_asce(struct kvm_vcpu *vcpu, union asce *asce, unsigned long ga, u8 ar, enum gacc_mode mode) { @@ -1110,7 +1118,8 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, ga = kvm_s390_logical_to_effective(vcpu, ga + fragment_len); } if (rc > 0) - rc = trans_exc(vcpu, rc, ga, ar, mode, prot); + rc = trans_exc_ending(vcpu, rc, ga, ar, mode, prot, + (mode != GACC_STORE) || (idx == 0)); out_unlock: if (need_ipte_lock) ipte_unlock(vcpu);
Am 01.04.22 um 19:02 schrieb Janis Schoetterl-Glausch:
If user space uses a memop to emulate an instruction and that memop fails, the execution of the instruction ends. Instruction execution can end in different ways, one of which is suppression, which requires that the instruction execute like a no-op. A writing memop that spans multiple pages and fails due to key protection can modified guest memory. Therefore do not indicate a suppressing instruction ending in this case.
Make it explicit in the changelog that this is "terminating" instead of "suppressing". z/VM has the same logic and the architecture allows for terminating in those cases (even for ESOP2).
Signed-off-by: Janis Schoetterl-Glausch scgl@linux.ibm.com
arch/s390/kvm/gaccess.c | 47 ++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c index d53a183c2005..3b1fbef82288 100644 --- a/arch/s390/kvm/gaccess.c +++ b/arch/s390/kvm/gaccess.c @@ -491,8 +491,8 @@ enum prot_type { PROT_TYPE_IEP = 4, }; -static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva,
u8 ar, enum gacc_mode mode, enum prot_type prot)+static int trans_exc_ending(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar,
enum gacc_mode mode, enum prot_type prot, bool suppress)@@ -503,22 +503,24 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, switch (code) { case PGM_PROTECTION:
switch (prot) {case PROT_TYPE_IEP:tec->b61 = 1;fallthrough;case PROT_TYPE_LA:tec->b56 = 1;break;case PROT_TYPE_KEYC:tec->b60 = 1;break;case PROT_TYPE_ALC:tec->b60 = 1;fallthrough;case PROT_TYPE_DAT:tec->b61 = 1;break;
if (suppress) {switch (prot) {case PROT_TYPE_IEP:tec->b61 = 1;fallthrough;case PROT_TYPE_LA:tec->b56 = 1;break;case PROT_TYPE_KEYC:tec->b60 = 1;break;case PROT_TYPE_ALC:tec->b60 = 1;fallthrough;case PROT_TYPE_DAT:tec->b61 = 1;break;}@@ -552,6 +554,12 @@ static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, return code; } +static int trans_exc(struct kvm_vcpu *vcpu, int code, unsigned long gva, u8 ar,
enum gacc_mode mode, enum prot_type prot)+{
- return trans_exc_ending(vcpu, code, gva, ar, mode, prot, true);
+}
- static int get_vcpu_asce(struct kvm_vcpu *vcpu, union asce *asce, unsigned long ga, u8 ar, enum gacc_mode mode) {
@@ -1110,7 +1118,8 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, ga = kvm_s390_logical_to_effective(vcpu, ga + fragment_len); } if (rc > 0)
rc = trans_exc(vcpu, rc, ga, ar, mode, prot);
rc = trans_exc_ending(vcpu, rc, ga, ar, mode, prot,(mode != GACC_STORE) || (idx == 0));
On 4/1/22 19:13, Christian Borntraeger wrote:
Am 01.04.22 um 19:02 schrieb Janis Schoetterl-Glausch:
If user space uses a memop to emulate an instruction and that memop fails, the execution of the instruction ends. Instruction execution can end in different ways, one of which is suppression, which requires that the instruction execute like a no-op. A writing memop that spans multiple pages and fails due to key protection can modified guest memory. Therefore do not indicate a suppressing instruction ending in this case.
A writing memop that spans multiple pages and fails due to key protection can modified guest memory, as a result, the likely correct ending is termination. Therefore do not indicate a suppressing instruction ending in this case.
?
It's phrased a bit vaguely, because we don't really know what user space wants when emulating an instruction, I guess it could try to revert the changes? And the TEID does not indicate termination, it only indicates that the guest cannot assume that the instruction was suppressed.
Make it explicit in the changelog that this is "terminating" instead of "suppressing". z/VM has the same logic and the architecture allows for terminating in those cases (even for ESOP2). >
Signed-off-by: Janis Schoetterl-Glausch scgl@linux.ibm.com
Check that suppression is not indicated on injection of a key checked protection exception caused by a memop after it already modified guest memory, as that violates the definition of suppression.
Signed-off-by: Janis Schoetterl-Glausch scgl@linux.ibm.com --- tools/testing/selftests/kvm/s390x/memop.c | 43 ++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/s390x/memop.c b/tools/testing/selftests/kvm/s390x/memop.c index b04c2c1b3c30..93664fbf0fef 100644 --- a/tools/testing/selftests/kvm/s390x/memop.c +++ b/tools/testing/selftests/kvm/s390x/memop.c @@ -194,6 +194,7 @@ static int err_memop_ioctl(struct test_vcpu vcpu, struct kvm_s390_mem_op *ksmo) #define SIDA_OFFSET(o) ._sida_offset = 1, .sida_offset = (o) #define AR(a) ._ar = 1, .ar = (a) #define KEY(a) .f_key = 1, .key = (a) +#define INJECT .f_inject = 1
#define CHECK_N_DO(f, ...) ({ f(__VA_ARGS__, CHECK_ONLY); f(__VA_ARGS__); })
@@ -430,9 +431,18 @@ static void test_copy_key_fetch_prot(void) TEST_ASSERT(rv == 4, "Should result in protection exception"); \ })
+static void guest_error_key(void) +{ + GUEST_SYNC(STAGE_INITED); + set_storage_key_range(mem1, PAGE_SIZE, 0x18); + set_storage_key_range(mem1 + PAGE_SIZE, sizeof(mem1) - PAGE_SIZE, 0x98); + GUEST_SYNC(STAGE_SKEYS_SET); + GUEST_SYNC(STAGE_IDLED); +} + static void test_errors_key(void) { - struct test_default t = test_default_init(guest_copy_key_fetch_prot); + struct test_default t = test_default_init(guest_error_key);
HOST_SYNC(t.vcpu, STAGE_INITED); HOST_SYNC(t.vcpu, STAGE_SKEYS_SET); @@ -446,6 +456,36 @@ static void test_errors_key(void) kvm_vm_free(t.kvm_vm); }
+static void test_termination(void) +{ + struct test_default t = test_default_init(guest_error_key); + uint64_t prefix; + uint64_t teid; + uint64_t psw[2]; + + HOST_SYNC(t.vcpu, STAGE_INITED); + HOST_SYNC(t.vcpu, STAGE_SKEYS_SET); + + /* vcpu, mismatching keys after first page */ + ERR_PROT_MOP(t.vcpu, LOGICAL, WRITE, mem1, t.size, GADDR_V(mem1), KEY(1), INJECT); + /* + * The memop injected a program exception and the test needs to check the + * Translation-Exception Identification (TEID). It is necessary to run + * the guest in order to be able to read the TEID from guest memory. + * Set the guest program new PSW, so the guest state is not clobbered. + */ + prefix = t.run->s.regs.prefix; + psw[0] = t.run->psw_mask; + psw[1] = t.run->psw_addr; + MOP(t.vm, ABSOLUTE, WRITE, psw, sizeof(psw), GADDR(prefix + 464)); + HOST_SYNC(t.vcpu, STAGE_IDLED); + MOP(t.vm, ABSOLUTE, READ, &teid, sizeof(teid), GADDR(prefix + 168)); + /* Bits 56, 60, 61 form a code, 0 being the only one allowing for termination */ + ASSERT_EQ(teid & 0x4c, 0); + + kvm_vm_free(t.kvm_vm); +} + static void test_errors_key_storage_prot_override(void) { struct test_default t = test_default_init(guest_copy_key_fetch_prot); @@ -668,6 +708,7 @@ int main(int argc, char *argv[]) test_copy_key_fetch_prot(); test_copy_key_fetch_prot_override(); test_errors_key(); + test_termination(); test_errors_key_storage_prot_override(); test_errors_key_fetch_prot_override_not_enabled(); test_errors_key_fetch_prot_override_enabled();
linux-kselftest-mirror@lists.linaro.org
-
Christian Borntraeger -
Janis Schoetterl-Glausch