| Message ID | 20200508230823.22956-5-walling@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390: Extended-Length SCCB & DIAGNOSE 0x318 | expand |
On 09.05.20 01:08, Collin Walling wrote: > The header of the SCCB contains the actual length of the > SCCB. Instead of using a static 4K size, let's allow > for a variable size determined by the value set 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/sclp.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c > index 470d5da7a2..748d04a0e2 100644 > --- a/hw/s390x/sclp.c > +++ b/hw/s390x/sclp.c > @@ -244,15 +244,16 @@ 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); > > - s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); > + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sizeof(SCCBHeader)); > > if (!sclp_command_code_valid(code)) { > work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); > goto out_write; > } > > + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, work_sccb.h.length); be16_to_cpu() necessary. > + > if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) { > goto out_write; > } > @@ -271,8 +272,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) > SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); > SCCB work_sccb; > > - hwaddr sccb_len = sizeof(SCCB); > - > /* first some basic checks on program checks */ > if (env->psw.mask & PSW_MASK_PSTATE) { > return -PGM_PRIVILEGED; > @@ -290,13 +289,16 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) > * 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); > + cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader)); > > /* Valid sccb sizes */ > if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) { > return -PGM_SPECIFICATION; > } > > + /* the header contains the actual length of the sccb */ > + cpu_physical_memory_read(sccb, &work_sccb, work_sccb.h.length); be16_to_cpu() necessary. > + > if (!sclp_command_code_valid(code)) { > work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); > goto out_write; >
On 5/12/20 3:26 AM, David Hildenbrand wrote: > On 09.05.20 01:08, Collin Walling wrote: >> The header of the SCCB contains the actual length of the >> SCCB. Instead of using a static 4K size, let's allow >> for a variable size determined by the value set 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/sclp.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index 470d5da7a2..748d04a0e2 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -244,15 +244,16 @@ 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); >> >> - s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); >> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sizeof(SCCBHeader)); >> >> if (!sclp_command_code_valid(code)) { >> work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); >> goto out_write; >> } >> >> + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, work_sccb.h.length); > > be16_to_cpu() necessary. > >> + >> if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) { >> goto out_write; >> } >> @@ -271,8 +272,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) >> SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); >> SCCB work_sccb; >> >> - hwaddr sccb_len = sizeof(SCCB); >> - >> /* first some basic checks on program checks */ >> if (env->psw.mask & PSW_MASK_PSTATE) { >> return -PGM_PRIVILEGED; >> @@ -290,13 +289,16 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) >> * 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); >> + cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader)); >> >> /* Valid sccb sizes */ >> if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) { >> return -PGM_SPECIFICATION; >> } >> >> + /* the header contains the actual length of the sccb */ >> + cpu_physical_memory_read(sccb, &work_sccb, work_sccb.h.length); > > be16_to_cpu() necessary. > >> + >> if (!sclp_command_code_valid(code)) { >> work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); >> goto out_write; >> > Thanks. Fixed up.
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index 470d5da7a2..748d04a0e2 100644 --- a/hw/s390x/sclp.c +++ b/hw/s390x/sclp.c @@ -244,15 +244,16 @@ 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); - s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len); + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sizeof(SCCBHeader)); if (!sclp_command_code_valid(code)) { work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); goto out_write; } + s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, work_sccb.h.length); + if (!check_sccb_boundary_valid(sccb, code, &work_sccb)) { goto out_write; } @@ -271,8 +272,6 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp); SCCB work_sccb; - hwaddr sccb_len = sizeof(SCCB); - /* first some basic checks on program checks */ if (env->psw.mask & PSW_MASK_PSTATE) { return -PGM_PRIVILEGED; @@ -290,13 +289,16 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code) * 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); + cpu_physical_memory_read(sccb, &work_sccb, sizeof(SCCBHeader)); /* Valid sccb sizes */ if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) { return -PGM_SPECIFICATION; } + /* the header contains the actual length of the sccb */ + cpu_physical_memory_read(sccb, &work_sccb, work_sccb.h.length); + if (!sclp_command_code_valid(code)) { work_sccb.h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND); goto out_write;
The header of the SCCB contains the actual length of the SCCB. Instead of using a static 4K size, let's allow for a variable size determined by the value set 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/sclp.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)