On 10/09/2020 11.36, 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 <wall...@linux.ibm.com>
> ---
> hw/s390x/event-facility.c | 2 +-
> hw/s390x/sclp.c | 58 ++++++++++++++++++++++-----------------
> include/hw/s390x/sclp.h | 2 +-
> 3 files changed, 35 insertions(+), 27 deletions(-)
>
> 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 69a8724dc7..cb8e2e8ec3 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -231,25 +231,30 @@ 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;
> + SCCB *work_sccb;
I'd maybe use "g_autofree SCCB *work_sccb = NULL" so you don't have to
worry about doing the g_free() later.
> - 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(header.length);
Please use be16_to_cpu(header.length) here as well.
> + 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, work_sccb.h.length)) {
> - work_sccb.h.response_code =
> cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> + if (!sccb_verify_boundary(sccb, 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));
> + g_free(work_sccb);
> sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
> return 0;
> }
> @@ -258,9 +263,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;
> + SCCB *work_sccb;
Maybe g_autofree again?
> /* first some basic checks on program checks */
> if (env->psw.mask & PSW_MASK_PSTATE) {
> @@ -274,33 +278,37 @@ 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(header.length);
Needs be16_to_cpu again.
> + 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, work_sccb.h.length)) {
> - work_sccb.h.response_code =
> cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> + if (!sccb_verify_boundary(sccb, 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));
> + g_free(work_sccb);
> sclp_c->service_interrupt(sclp, sccb);
>
> return 0;
Thomas
