On 9/10/20 1:50 PM, Thomas Huth wrote:
> 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.
Can do.
>
>> - 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.
Good catch, thanks!
>
>> + 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
>
>
Thanks, Thomas!
--
Regards,
Collin
Stay safe and stay healthy
