Re: [PATCH v1 1/2] s390x: Add sclp boundary check and fix error priority

[Janosch Frank]

On 9/27/19 10:51 AM, David Hildenbrand wrote:
> On 26.09.19 13:33, Claudio Imbrenda wrote:
>> From: Janosch Frank <address@hidden>
>>
>> * All sclp codes need to be checked for page boundary violations.
>> * Requests over 4k are not a spec exception.
>> * Invalid command checking has to be done before the boundary check.
> 
> Can we split this patch up so we fix one thing at a time?

Sure, but we would end up with very small patches.
Do you want that?

> 
>>
>> Signed-off-by: Janosch Frank <address@hidden>
>> Reviewed-by: Jason J. Herne <address@hidden>
>> ---
>>  hw/s390x/event-facility.c |  3 ---
>>  hw/s390x/sclp.c           | 25 ++++++++++++++++++++++---
>>  2 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
>> index 797ecbb..6620569 100644
>> --- a/hw/s390x/event-facility.c
>> +++ b/hw/s390x/event-facility.c
>> @@ -377,9 +377,6 @@ static void command_handler(SCLPEventFacility *ef, SCCB 
>> *sccb, uint64_t code)
>>      case SCLP_CMD_WRITE_EVENT_MASK:
>>          write_event_mask(ef, sccb);
>>          break;
>> -    default:
>> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> -        break;
>>      }
>>  }
>>  
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index fac7c3b..76feac8 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -213,14 +213,33 @@ int sclp_service_call(CPUS390XState *env, uint64_t 
>> sccb, uint32_t code)
>>      cpu_physical_memory_read(sccb, &work_sccb, sccb_len);
>>  
>>      /* Valid sccb sizes */
>> -    if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader) ||
>> -        be16_to_cpu(work_sccb.h.length) > SCCB_SIZE) {
>> +    if (be16_to_cpu(work_sccb.h.length) < sizeof(SCCBHeader)) {
>>          r = -PGM_SPECIFICATION;
>>          goto out;
>>      }
>>  
>> -    sclp_c->execute(sclp, &work_sccb, code);
>> +    switch (code & SCLP_CMD_CODE_MASK) {
>> +    case SCLP_CMDW_READ_SCP_INFO:
>> +    case SCLP_CMDW_READ_SCP_INFO_FORCED:
>> +    case SCLP_CMDW_READ_CPU_INFO:
>> +    case SCLP_CMDW_CONFIGURE_IOA:
>> +    case SCLP_CMDW_DECONFIGURE_IOA:
>> +    case SCLP_CMD_READ_EVENT_DATA:
>> +    case SCLP_CMD_WRITE_EVENT_DATA:
>> +    case SCLP_CMD_WRITE_EVENT_MASK:
>> +        break;
>> +    default:
>> +        work_sccb.h.response_code = 
>> cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> +        goto out_write;
>> +    }
>>  
>> +    if ((sccb + work_sccb.h.length) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
>> +        work_sccb.h.response_code = 
>> cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>> +        goto out_write;
>> +    }
>> +
>> +    sclp_c->execute(sclp, &work_sccb, code);
>> +out_write:
>>      cpu_physical_memory_write(sccb, &work_sccb,
>>                                be16_to_cpu(work_sccb.h.length));
>>  
>>
> 
>

signature.asc
Description: OpenPGP digital signature