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 <wall...@linux.ibm.com>
>> Acked-by: Janosch Frank <fran...@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <coh...@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;
[...]
--
Regards,
Collin
Stay safe and stay healthy
