| Message ID | 20220513095017.16301-2-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: uv-host: Access check extensions and improvements | expand |
On Fri, 2022-05-13 at 09:50 +0000, Janosch Frank wrote: > Let's check if the UV really protected all the memory we donated. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> Reviewed-by: Nico Boehr <nrb@linux.ibm.com> One suggestion below for you to consider. > --- > s390x/uv-host.c | 42 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 40 insertions(+), 2 deletions(-) > > diff --git a/s390x/uv-host.c b/s390x/uv-host.c > index a1a6d120..0f0b18a1 100644 > --- a/s390x/uv-host.c > +++ b/s390x/uv-host.c > @@ -142,7 +142,8 @@ static void test_cpu_destroy(void) > static void test_cpu_create(void) > { > int rc; > - unsigned long tmp; > + unsigned long tmp, i; > + uint8_t *access_ptr; > > report_prefix_push("csc"); > uvcb_csc.header.len = sizeof(uvcb_csc); > @@ -194,6 +195,18 @@ static void test_cpu_create(void) > report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED && > uvcb_csc.cpu_handle, "success"); > > + rc = 1; > + for (i = 0; i < uvcb_qui.cpu_stor_len / PAGE_SIZE; i++) { > + expect_pgm_int(); > + access_ptr = (void *)uvcb_csc.stor_origin + PAGE_SIZE > * i; > + *access_ptr = 42; > + if (clear_pgm_int() != > PGM_INT_CODE_SECURE_STOR_ACCESS) { > + rc = 0; > + break; > + } > + } > + report(rc, "Storage protection"); All of these for loops look pretty similar, would it make sense to move them to their own function like: assert_range_write_protected(void *start, size_t len, int pgm_int_code)?
On 5/16/22 10:21, Nico Boehr wrote: > On Fri, 2022-05-13 at 09:50 +0000, Janosch Frank wrote: >> Let's check if the UV really protected all the memory we donated. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > > Reviewed-by: Nico Boehr <nrb@linux.ibm.com> Thanks > > One suggestion below for you to consider. Sure, I'll rework the loops > >> --- >> s390x/uv-host.c | 42 ++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 40 insertions(+), 2 deletions(-) >> >> diff --git a/s390x/uv-host.c b/s390x/uv-host.c >> index a1a6d120..0f0b18a1 100644 >> --- a/s390x/uv-host.c >> +++ b/s390x/uv-host.c >> @@ -142,7 +142,8 @@ static void test_cpu_destroy(void) >> static void test_cpu_create(void) >> { >> int rc; >> - unsigned long tmp; >> + unsigned long tmp, i; >> + uint8_t *access_ptr; >> >> report_prefix_push("csc"); >> uvcb_csc.header.len = sizeof(uvcb_csc); >> @@ -194,6 +195,18 @@ static void test_cpu_create(void) >> report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED && >> uvcb_csc.cpu_handle, "success"); >> >> + rc = 1; >> + for (i = 0; i < uvcb_qui.cpu_stor_len / PAGE_SIZE; i++) { >> + expect_pgm_int(); >> + access_ptr = (void *)uvcb_csc.stor_origin + PAGE_SIZE >> * i; >> + *access_ptr = 42; >> + if (clear_pgm_int() != >> PGM_INT_CODE_SECURE_STOR_ACCESS) { >> + rc = 0; >> + break; >> + } >> + } >> + report(rc, "Storage protection"); > > All of these for loops look pretty similar, would it make sense to move > them to their own function like: > > assert_range_write_protected(void *start, size_t len, int > pgm_int_code)?
diff --git a/s390x/uv-host.c b/s390x/uv-host.c index a1a6d120..0f0b18a1 100644 --- a/s390x/uv-host.c +++ b/s390x/uv-host.c @@ -142,7 +142,8 @@ static void test_cpu_destroy(void) static void test_cpu_create(void) { int rc; - unsigned long tmp; + unsigned long tmp, i; + uint8_t *access_ptr; report_prefix_push("csc"); uvcb_csc.header.len = sizeof(uvcb_csc); @@ -194,6 +195,18 @@ static void test_cpu_create(void) report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED && uvcb_csc.cpu_handle, "success"); + rc = 1; + for (i = 0; i < uvcb_qui.cpu_stor_len / PAGE_SIZE; i++) { + expect_pgm_int(); + access_ptr = (void *)uvcb_csc.stor_origin + PAGE_SIZE * i; + *access_ptr = 42; + if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS) { + rc = 0; + break; + } + } + report(rc, "Storage protection"); + tmp = uvcb_csc.stor_origin; uvcb_csc.stor_origin = (unsigned long)memalign(PAGE_SIZE, uvcb_qui.cpu_stor_len); rc = uv_call(0, (uint64_t)&uvcb_csc); @@ -205,8 +218,9 @@ static void test_cpu_create(void) static void test_config_create(void) { int rc; - unsigned long vsize, tmp; + unsigned long vsize, tmp, i; static struct uv_cb_cgc uvcb; + uint8_t *access_ptr; uvcb_cgc.header.cmd = UVC_CMD_CREATE_SEC_CONF; uvcb_cgc.header.len = sizeof(uvcb_cgc); @@ -292,6 +306,30 @@ static void test_config_create(void) rc = uv_call(0, (uint64_t)&uvcb_cgc); report(rc == 0 && uvcb_cgc.header.rc == UVC_RC_EXECUTED, "successful"); + rc = 1; + for (i = 0; i < vsize / PAGE_SIZE; i++) { + expect_pgm_int(); + access_ptr = (void *)uvcb_cgc.conf_var_stor_origin + PAGE_SIZE * i; + *access_ptr = 42; + if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS) { + rc = 0; + break; + } + } + report(rc, "Base storage protection"); + + rc = 1; + for (i = 0; i < uvcb_qui.conf_base_phys_stor_len / PAGE_SIZE; i++) { + expect_pgm_int(); + access_ptr = (void *)uvcb_cgc.conf_base_stor_origin + PAGE_SIZE * i; + *access_ptr = 42; + if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS) { + rc = 0; + break; + } + } + report(rc, "Variable storage protection"); + uvcb_cgc.header.rc = 0; uvcb_cgc.header.rrc = 0; tmp = uvcb_cgc.guest_handle;
Let's check if the UV really protected all the memory we donated. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- s390x/uv-host.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)