diff mbox series

[v6,3/8] s390/sclp: read sccb from mem based on provided length

Message ID 20200915194416.107460-4-walling@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390: Extended-Length SCCB & DIAGNOSE 0x318 | expand

Commit Message

Collin Walling Sept. 15, 2020, 7:44 p.m. UTC 
The header contained within the SCCB passed to the SCLP service call
contains the actual length of the SCCB. Instead of allocating a static
4K size for the work sccb, let's allow for a variable size determined
by the value in the header. The proper checks are already in place to
ensure the SCCB length is sufficent to store a full response and that
the length does not cross any explicitly-set boundaries.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 hw/s390x/event-facility.c |  2 +-
 hw/s390x/sclp.c           | 55 ++++++++++++++++++++++-----------------
 include/hw/s390x/sclp.h   |  2 +-
 3 files changed, 33 insertions(+), 26 deletions(-)

Comments

Thomas Huth Sept. 16, 2020, 8 a.m. UTC | #1 
On 15/09/2020 21.44, Collin Walling wrote:
> The header contained within the SCCB passed to the SCLP service call
> contains the actual length of the SCCB. Instead of allocating a static
> 4K size for the work sccb, let's allow for a variable size determined
> by the value in the header. The proper checks are already in place to
> ensure the SCCB length is sufficent to store a full response and that
> the length does not cross any explicitly-set boundaries.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/event-facility.c |  2 +-
>  hw/s390x/sclp.c           | 55 ++++++++++++++++++++++-----------------
>  include/hw/s390x/sclp.h   |  2 +-
>  3 files changed, 33 insertions(+), 26 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>
diff mbox series

Patch

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 645b4080c5..ed92ce510d 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -213,7 +213,7 @@  static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
 
     event_buf = &red->ebh;
     event_buf->length = 0;
-    slen = sizeof(sccb->data);
+    slen = sccb_data_len(sccb);
 
     rc = SCLP_RC_NO_EVENT_BUFFERS_STORED;
 
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index a37cfbf534..4ae6fb400b 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -231,25 +231,29 @@  int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
 {
     SCLPDevice *sclp = get_sclp_device();
     SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
-    SCCB work_sccb;
-    hwaddr sccb_len = sizeof(SCCB);
+    SCCBHeader header;
+    g_autofree SCCB *work_sccb = NULL;
 
-    s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
+    s390_cpu_pv_mem_read(env_archcpu(env), 0, &header, sizeof(SCCBHeader));
+
+    work_sccb = g_malloc0(be16_to_cpu(header.length));
+    s390_cpu_pv_mem_read(env_archcpu(env), 0, work_sccb,
+                         be16_to_cpu(header.length));
 
     if (!sclp_command_code_valid(code)) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
         goto out_write;
     }
 
-    if (!sccb_verify_boundary(sccb, be16_to_cpu(work_sccb.h.length))) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+    if (!sccb_verify_boundary(sccb, be16_to_cpu(work_sccb->h.length))) {
+        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
         goto out_write;
     }
 
-    sclp_c->execute(sclp, &work_sccb, code);
+    sclp_c->execute(sclp, work_sccb, code);
 out_write:
-    s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
-                          be16_to_cpu(work_sccb.h.length));
+    s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,
+                          be16_to_cpu(work_sccb->h.length));
     sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
     return 0;
 }
@@ -258,9 +262,8 @@  int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
 {
     SCLPDevice *sclp = get_sclp_device();
     SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
-    SCCB work_sccb;
-
-    hwaddr sccb_len = sizeof(SCCB);
+    SCCBHeader header;
+    g_autofree SCCB *work_sccb = NULL;
 
     /* first some basic checks on program checks */
     if (env->psw.mask & PSW_MASK_PSTATE) {
@@ -274,32 +277,36 @@  int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
         return -PGM_SPECIFICATION;
     }
 
+    /* the header contains the actual length of the sccb */
+    cpu_physical_memory_read(sccb, &header, sizeof(SCCBHeader));
+
+    /* Valid sccb sizes */
+    if (be16_to_cpu(header.length) < sizeof(SCCBHeader)) {
+        return -PGM_SPECIFICATION;
+    }
+
     /*
      * we want to work on a private copy of the sccb, to prevent guests
      * from playing dirty tricks by modifying the memory content after
      * the host has checked the values
      */
-    cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
-
-    /* Valid sccb sizes */
-    if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
-        return -PGM_SPECIFICATION;
-    }
+    work_sccb = g_malloc0(be16_to_cpu(header.length));
+    cpu_physical_memory_read(sccb, work_sccb, be16_to_cpu(header.length));
 
     if (!sclp_command_code_valid(code)) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
         goto out_write;
     }
 
-    if (!sccb_verify_boundary(sccb, be16_to_cpu(work_sccb.h.length))) {
-        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
+    if (!sccb_verify_boundary(sccb, be16_to_cpu(work_sccb->h.length))) {
+        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
         goto out_write;
     }
 
-    sclp_c->execute(sclp, &work_sccb, code);
+    sclp_c->execute(sclp, work_sccb, code);
 out_write:
-    cpu_physical_memory_write(sccb, &work_sccb,
-                              be16_to_cpu(work_sccb.h.length));
+    cpu_physical_memory_write(sccb, work_sccb,
+                              be16_to_cpu(work_sccb->h.length));
 
     sclp_c->service_interrupt(sclp, sccb);
 
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index a87ed2a0ab..98c4727984 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -177,7 +177,7 @@  typedef struct IoaCfgSccb {
 
 typedef struct SCCB {
     SCCBHeader h;
-    char data[SCCB_DATA_LEN];
+    char data[];
  } QEMU_PACKED SCCB;
 
 #define TYPE_SCLP "sclp"