[v4,3/8] s390/sclp: rework sclp boundary and length checks

Rework the SCLP boundary check to account for different SCLP commands
(eventually) allowing different boundary sizes.

Move the length check code into a separate function, and introduce a
new function to determine the length of the read SCP data (i.e. the size
from the start of the struct to where the CPU entries should begin).

The format of read CPU info is unlikely to change in the future,
so we do not require a separate function to calculate its length.

Signed-off-by: Collin Walling <walling@linux.ibm.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/sclp.c | 54 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 10 deletions(-)

On 24/06/2020 22.23, Collin Walling wrote:
> Rework the SCLP boundary check to account for different SCLP commands
> (eventually) allowing different boundary sizes.
> 
> Move the length check code into a separate function, and introduce a
> new function to determine the length of the read SCP data (i.e. the size
> from the start of the struct to where the CPU entries should begin).
> 
> The format of read CPU info is unlikely to change in the future,
> so we do not require a separate function to calculate its length.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   hw/s390x/sclp.c | 54 ++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 44 insertions(+), 10 deletions(-)

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

On 24.06.20 22:23, Collin Walling wrote:
> Rework the SCLP boundary check to account for different SCLP commands
> (eventually) allowing different boundary sizes.
> 
> Move the length check code into a separate function, and introduce a
> new function to determine the length of the read SCP data (i.e. the size
> from the start of the struct to where the CPU entries should begin).
> 
> The format of read CPU info is unlikely to change in the future,
> so we do not require a separate function to calculate its length.
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/sclp.c | 54 ++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 181ce04007..5899c1e3b8 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code)
>      return false;
>  }
>  
> +static bool sccb_verify_boundary(uint64_t sccb_addr, uint32_t code,
> +                                 SCCBHeader *header)
> +{
> +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(header->length) - 1;
> +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> +
> +    switch (code & SCLP_CMD_CODE_MASK) {
> +    default:
> +        if (sccb_max_addr < sccb_boundary) {
> +            return true;
> +        }
> +    }

^ what is that?

    if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
        return true;
    }

> +    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +    return false;

So we return "false" on success? At least I consider that weird when
returning the bool type. Maybe make it clearer what the function indicates

"sccb_boundary_is_invalid"

or leave it named as is and switch from return value "bool" to "int",
using "0" on success and "-EINVAL" on error.

> +}
> +
> +/* Calculates sufficient SCCB length to store a full Read SCP/CPU response */
> +static bool sccb_verify_length(SCCB *sccb, int num_cpus, int offset_cpu)
> +{
> +    int required_len = offset_cpu + num_cpus * sizeof(CPUEntry);
> +
> +    if (be16_to_cpu(sccb->h.length) < required_len) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return false;
> +    }
> +    return true;
> +}
> +
>  static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>  {
>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> @@ -66,6 +94,11 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry *entry, int *count)
>      }
>  }
>  
> +static inline int get_read_scp_info_offset_cpu(void)
> +{
> +    return offsetof(ReadInfo, entries);
> +}
> +
>  /* Provide information about the configuration, CPUs and storage */
>  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  {
> @@ -74,17 +107,16 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int cpu_count;
>      int rnsize, rnmax;
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
> +    int offset_cpu = get_read_scp_info_offset_cpu();
>  
> -    if (be16_to_cpu(sccb->h.length) <
> -          (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +    if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
>          return;
>      }
>  
>      /* CPU information */
>      prepare_cpu_entries(machine, read_info->entries, &cpu_count);
>      read_info->entries_cpu = cpu_to_be16(cpu_count);
> -    read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
> +    read_info->offset_cpu = cpu_to_be16(offset_cpu);
>      read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1);
>  
>      read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
> @@ -133,17 +165,16 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
>      ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
> +    int offset_cpu = offsetof(ReadCpuInfo, entries);
>      int cpu_count;
>  
> -    if (be16_to_cpu(sccb->h.length) <
> -          (sizeof(ReadInfo) + machine->possible_cpus->len * sizeof(CPUEntry))) {
> -        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +    if (!sccb_verify_length(sccb, machine->possible_cpus->len, offset_cpu)) {
>          return;
>      }
>  
>      prepare_cpu_entries(machine, cpu_info->entries, &cpu_count);
>      cpu_info->nr_configured = cpu_to_be16(cpu_count);
> -    cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
> +    cpu_info->offset_configured = cpu_to_be16(offset_cpu);
>      cpu_info->nr_standby = cpu_to_be16(0);
>  
>      /* The standby offset is 16-byte for each CPU */
> @@ -229,6 +260,10 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>          goto out_write;
>      }
>  
> +    if (!sccb_verify_boundary(sccb, code, &work_sccb.h)) {
> +        goto out_write;
> +    }
> +
>      sclp_c->execute(sclp, &work_sccb, code);
>  out_write:
>      s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
> @@ -274,8 +309,7 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>          goto out_write;
>      }
>  
> -    if ((sccb + be16_to_cpu(work_sccb.h.length)) > ((sccb & PAGE_MASK) + PAGE_SIZE)) {
> -        work_sccb.h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> +    if (!sccb_verify_boundary(sccb, code, &work_sccb.h)) {
>          goto out_write;
>      }
>  
>

On 7/20/20 4:17 AM, David Hildenbrand wrote:
> On 24.06.20 22:23, Collin Walling wrote:
>> Rework the SCLP boundary check to account for different SCLP commands
>> (eventually) allowing different boundary sizes.
>>
>> Move the length check code into a separate function, and introduce a
>> new function to determine the length of the read SCP data (i.e. the size
>> from the start of the struct to where the CPU entries should begin).
>>
>> The format of read CPU info is unlikely to change in the future,
>> so we do not require a separate function to calculate its length.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  hw/s390x/sclp.c | 54 ++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 44 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 181ce04007..5899c1e3b8 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -49,6 +49,34 @@ static inline bool sclp_command_code_valid(uint32_t code)
>>      return false;
>>  }
>>  
>> +static bool sccb_verify_boundary(uint64_t sccb_addr, uint32_t code,
>> +                                 SCCBHeader *header)
>> +{
>> +    uint64_t sccb_max_addr = sccb_addr + be16_to_cpu(header->length) - 1;
>> +    uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
>> +
>> +    switch (code & SCLP_CMD_CODE_MASK) {
>> +    default:
>> +        if (sccb_max_addr < sccb_boundary) {
>> +            return true;
>> +        }
>> +    }
> 
> ^ what is that?
> 
>     if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
>         return true;
>     }
> 

I agree it looks pointless in this patch, but it makes more sense in
patch #6 where we introduce cases for the SCLP commands that bypass
these checks if the extended-length sccb feature is enabled.

>> +    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>> +    return false;
> 
> So we return "false" on success? At least I consider that weird when
> returning the bool type. Maybe make it clearer what the function indicates
> 

Hmmm... I figured since there were more paths that can lead to success
(i.e. when I introduce the feat check in a later patch), then it made
more sense to to return false at the end. sclp_command_code_valid has
similar logic.

But if boolean functions traditionally return true as the last return
value, I can rework it to align to coding preferences / standards.

> "sccb_boundary_is_invalid"
> 

Unless it's simply the name that is confusing?

> or leave it named as is and switch from return value "bool" to "int",
> using "0" on success and "-EINVAL" on error.
> 

Is the switch statement an overkill? I thought of it as a cleaner way to
later show which commands have a special conditions (introduced in patch
6 for the ELS stuff) instead of a nasty long if statement.

The alternative...

/* Comment explaining this check */
if ((code & SCLP_CMD_CODE_MASK) & (SCLP_CMDW_READ_SCP_INFO |
	SCLP_CMDW_READ_SCP_INFO_FORCED | SCLP_CMDW_READ_CPU_INFO) &&
	s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
	return true;
}

if (sccb_max_addr < sccb_boundary) {
        return true;
}

header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
return false;

[...]

[...]

>>> +    switch (code & SCLP_CMD_CODE_MASK) {
>>> +    default:
>>> +        if (sccb_max_addr < sccb_boundary) {
>>> +            return true;
>>> +        }
>>> +    }
>>
>> ^ what is that?
>>
>>     if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
>>         return true;
>>     }

Oh, my tired eyes missed that it's actually only

if (sccb_max_addr < sccb_boundary) :)

>>
> 
> I agree it looks pointless in this patch, but it makes more sense in
> patch #6 where we introduce cases for the SCLP commands that bypass
> these checks if the extended-length sccb feature is enabled.

I am really a friend of introducing stuff where needed. Just use a
simple "if" here and convert to the switch in patch #6.

> 
>>> +    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>> +    return false;
>>
>> So we return "false" on success? At least I consider that weird when
>> returning the bool type. Maybe make it clearer what the function indicates
>>
> 
> Hmmm... I figured since there were more paths that can lead to success
> (i.e. when I introduce the feat check in a later patch), then it made
> more sense to to return false at the end. sclp_command_code_valid has
> similar logic.
> 
> But if boolean functions traditionally return true as the last return
> value, I can rework it to align to coding preferences / standards.
> 
>> "sccb_boundary_is_invalid"
>>
> 
> Unless it's simply the name that is confusing?

The options I would support are

1. "sccb_boundary_is_valid" which returns "true" if valid
2. "sccb_boundary_is_invalid" which returns "true" if invalid
3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.

Which makes reading this code a bit easier.

> 
>> or leave it named as is and switch from return value "bool" to "int",
>> using "0" on success and "-EINVAL" on error.
>>
> 
> Is the switch statement an overkill? I thought of it as a cleaner way to
> later show which commands have a special conditions (introduced in patch
> 6 for the ELS stuff) instead of a nasty long if statement.

I think the switch make sense in patch #6.

On 7/21/20 4:41 AM, David Hildenbrand wrote:
> [...]
> 
>>>> +    switch (code & SCLP_CMD_CODE_MASK) {
>>>> +    default:
>>>> +        if (sccb_max_addr < sccb_boundary) {
>>>> +            return true;
>>>> +        }
>>>> +    }
>>>
>>> ^ what is that?
>>>
>>>     if ((code & SCLP_CMD_CODE_MASK) && sccb_max_addr < sccb_boundary) {
>>>         return true;
>>>     }
> 
> Oh, my tired eyes missed that it's actually only
> 
> if (sccb_max_addr < sccb_boundary) :)
> 
>>>
>>
>> I agree it looks pointless in this patch, but it makes more sense in
>> patch #6 where we introduce cases for the SCLP commands that bypass
>> these checks if the extended-length sccb feature is enabled.
> 
> I am really a friend of introducing stuff where needed. Just use a
> simple "if" here and convert to the switch in patch #6.
> 

I can understand that. This follows the whole "each patch should be
sufficient on its own" way of thinking. A switch with no cases and a
default _does_ look silly.

>>
>>>> +    header->response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
>>>> +    return false;
>>>
>>> So we return "false" on success? At least I consider that weird when
>>> returning the bool type. Maybe make it clearer what the function indicates
>>>
>>
>> Hmmm... I figured since there were more paths that can lead to success
>> (i.e. when I introduce the feat check in a later patch), then it made
>> more sense to to return false at the end. sclp_command_code_valid has
>> similar logic.
>>
>> But if boolean functions traditionally return true as the last return
>> value, I can rework it to align to coding preferences / standards.
>>
>>> "sccb_boundary_is_invalid"
>>>
>>
>> Unless it's simply the name that is confusing?
> 
> The options I would support are
> 
> 1. "sccb_boundary_is_valid" which returns "true" if valid
> 2. "sccb_boundary_is_invalid" which returns "true" if invalid
> 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.
> 
> Which makes reading this code a bit easier.
> 

Sounds good. I'll takes this into consideration for the next round. (I
may wait just a little longer for that to allow more reviews to come in
from whoever has the time, if that's okay.)

>>
>>> or leave it named as is and switch from return value "bool" to "int",
>>> using "0" on success and "-EINVAL" on error.
>>>
>>
>> Is the switch statement an overkill? I thought of it as a cleaner way to
>> later show which commands have a special conditions (introduced in patch
>> 6 for the ELS stuff) instead of a nasty long if statement.
> 
> I think the switch make sense in patch #6.
>

On Tue, 21 Jul 2020 14:40:14 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> On 7/21/20 4:41 AM, David Hildenbrand wrote:

> > The options I would support are
> > 
> > 1. "sccb_boundary_is_valid" which returns "true" if valid
> > 2. "sccb_boundary_is_invalid" which returns "true" if invalid
> > 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.
> > 
> > Which makes reading this code a bit easier.
> >   

Of these, I like option 1 best.

> 
> Sounds good. I'll takes this into consideration for the next round. (I
> may wait just a little longer for that to allow more reviews to come in
> from whoever has the time, if that's okay.)

We have to wait for (a) QEMU to do a release and (b) the Linux changes
to merge upstream anyway, so we're not in a hurry :)

As said before, it already looked good from my side, but the suggested
changes are fine with me as well.

On 7/23/20 2:26 AM, Cornelia Huck wrote:
> On Tue, 21 Jul 2020 14:40:14 -0400
> Collin Walling <walling@linux.ibm.com> wrote:
> 
>> On 7/21/20 4:41 AM, David Hildenbrand wrote:
> 
>>> The options I would support are
>>>
>>> 1. "sccb_boundary_is_valid" which returns "true" if valid
>>> 2. "sccb_boundary_is_invalid" which returns "true" if invalid
>>> 3. "sccb_boundary_validate" which returns "0" if valid and -EINVAL if not.
>>>
>>> Which makes reading this code a bit easier.
>>>   
> 
> Of these, I like option 1 best.
> 
>>
>> Sounds good. I'll takes this into consideration for the next round. (I
>> may wait just a little longer for that to allow more reviews to come in
>> from whoever has the time, if that's okay.)
> 
> We have to wait for (a) QEMU to do a release and (b) the Linux changes
> to merge upstream anyway, so we're not in a hurry :)
> 
> As said before, it already looked good from my side, but the suggested
> changes are fine with me as well.
> 
> 

Okay, thanks for the info.

I do want to send out a v5 of these patches. While working with someone
who is implementing the kernel support for the extended-length SCCB, we
found some snags. I'll highlight these changes/fixes in the respective
patches on the next version.

Thanks!