[PATCH v2 0/4] s390x: SCLP error cleanup

[PATCH v2 0/4] s390x: SCLP error cleanup

[Claudio Imbrenda]

SCLP doesn't report a lot of errors like it should do, let's fix that.

Changes v1 to v2:

* added a few missing be16_to_cpu
* split first patch into three smaller patches

Claudio Imbrenda (1):
  s390x: Fix SCLP return code when buffer too small

Janosch Frank (3):
  s390x: sclp: refactor invalid command check
  s390x: sclp: boundary check
  s390x: sclp: fix error handling for oversize control blocks

 hw/s390x/event-facility.c |  3 ---
 hw/s390x/sclp.c           | 37 ++++++++++++++++++++++++++++++++++---
 2 files changed, 34 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] s390x: sclp: refactor invalid command check

[Claudio Imbrenda]

From: Janosch Frank <frankja@linux.ibm.com>

Invalid command checking has to be done before the boundary check,
refactoring it now allows to insert the boundary check at the correct
place later.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
---
 hw/s390x/event-facility.c |  3 ---
 hw/s390x/sclp.c           | 17 ++++++++++++++++-
 2 files changed, 16 insertions(+), 4 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..95ebfe7 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -219,8 +219,23 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
         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;
+    }
 
+    sclp_c->execute(sclp, &work_sccb, code);
+out_write:
     cpu_physical_memory_write(sccb, &work_sccb,
                               be16_to_cpu(work_sccb.h.length));
 
-- 
2.7.4

Re: [PATCH v2 1/4] s390x: sclp: refactor invalid command check

[David Hildenbrand]

On 27.09.19 15:33, Claudio Imbrenda wrote:
> From: Janosch Frank <frankja@linux.ibm.com>
> 
> Invalid command checking has to be done before the boundary check,
> refactoring it now allows to insert the boundary check at the correct
> place later.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>  hw/s390x/event-facility.c |  3 ---
>  hw/s390x/sclp.c           | 17 ++++++++++++++++-
>  2 files changed, 16 insertions(+), 4 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..95ebfe7 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -219,8 +219,23 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>          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;
> +    }
>  
> +    sclp_c->execute(sclp, &work_sccb, code);
> +out_write:
>      cpu_physical_memory_write(sccb, &work_sccb,
>                                be16_to_cpu(work_sccb.h.length));
>  
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 1/4] s390x: sclp: refactor invalid command check

[Christian Borntraeger]

On 27.09.19 15:33, Claudio Imbrenda wrote:
> From: Janosch Frank <frankja@linux.ibm.com>
> 
> Invalid command checking has to be done before the boundary check,
> refactoring it now allows to insert the boundary check at the correct
> place later.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>  hw/s390x/event-facility.c |  3 ---
>  hw/s390x/sclp.c           | 17 ++++++++++++++++-
>  2 files changed, 16 insertions(+), 4 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..95ebfe7 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -219,8 +219,23 @@ int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
>          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;
> +    }
>  
> +    sclp_c->execute(sclp, &work_sccb, code);
> +out_write:
>      cpu_physical_memory_write(sccb, &work_sccb,
>                                be16_to_cpu(work_sccb.h.length));
>  

Thanks applied.

[PATCH v2 2/4] s390x: sclp: boundary check

[Claudio Imbrenda]

From: Janosch Frank <frankja@linux.ibm.com>

All sclp codes need to be checked for page boundary violations.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
---
 hw/s390x/sclp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 95ebfe7..73244c9 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -234,6 +234,11 @@ 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);
+        goto out_write;
+    }
+
     sclp_c->execute(sclp, &work_sccb, code);
 out_write:
     cpu_physical_memory_write(sccb, &work_sccb,
-- 
2.7.4

Re: [PATCH v2 2/4] s390x: sclp: boundary check

[David Hildenbrand]

On 27.09.19 15:33, Claudio Imbrenda wrote:
> From: Janosch Frank <frankja@linux.ibm.com>
> 
> All sclp codes need to be checked for page boundary violations.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 95ebfe7..73244c9 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -234,6 +234,11 @@ 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);
> +        goto out_write;
> +    }
> +
>      sclp_c->execute(sclp, &work_sccb, code);
>  out_write:
>      cpu_physical_memory_write(sccb, &work_sccb,
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 2/4] s390x: sclp: boundary check

[Christian Borntraeger]

On 27.09.19 15:33, Claudio Imbrenda wrote:
> From: Janosch Frank <frankja@linux.ibm.com>
> 
> All sclp codes need to be checked for page boundary violations.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 95ebfe7..73244c9 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -234,6 +234,11 @@ 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);
> +        goto out_write;
> +    }
> +
>      sclp_c->execute(sclp, &work_sccb, code);
>  out_write:
>      cpu_physical_memory_write(sccb, &work_sccb,
>

checkpatch complains about the line length, but splitting makes it really
less readable.

Thanks applied. 

[PATCH v2 3/4] s390x: sclp: fix error handling for oversize control blocks

[Claudio Imbrenda]

From: Janosch Frank <frankja@linux.ibm.com>

Requests over 4k are not a spec exception.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
---
 hw/s390x/sclp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 73244c9..abb6e50 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -213,8 +213,7 @@ 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;
     }
-- 
2.7.4

Re: [PATCH v2 3/4] s390x: sclp: fix error handling for oversize control blocks

[David Hildenbrand]

On 27.09.19 15:33, Claudio Imbrenda wrote:
> From: Janosch Frank <frankja@linux.ibm.com>
> 
> Requests over 4k are not a spec exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 73244c9..abb6e50 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -213,8 +213,7 @@ 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;
>      }
> 

Acked-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 3/4] s390x: sclp: fix error handling for oversize control blocks

[Christian Borntraeger]

On 27.09.19 15:33, Claudio Imbrenda wrote:
> From: Janosch Frank <frankja@linux.ibm.com>
> 
> Requests over 4k are not a spec exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 73244c9..abb6e50 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -213,8 +213,7 @@ 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;
>      }
> 

Thanks applied.

[PATCH v2 4/4] s390x: Fix SCLP return code when buffer too small

[Claudio Imbrenda]

Return the correct error code when the SCCB buffer is too small to
contain all of the output, for the Read SCP Information and
Read CPU Information commands.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
---
 hw/s390x/sclp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index abb6e50..f57ce7b 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -68,6 +68,12 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 
     read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
 
+    if (be16_to_cpu(sccb->h.length) <
+            (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        return;
+    }
+
     /* Configuration Characteristic (Extension) */
     s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
                          read_info->conf_char);
@@ -118,6 +124,12 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
     cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
     cpu_info->nr_standby = cpu_to_be16(0);
 
+    if (be16_to_cpu(sccb->h.length) <
+            (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
+        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+        return;
+    }
+
     /* The standby offset is 16-byte for each CPU */
     cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
         + cpu_info->nr_configured*sizeof(CPUEntry));
-- 
2.7.4

Re: [PATCH v2 4/4] s390x: Fix SCLP return code when buffer too small

[David Hildenbrand]

On 27.09.19 15:33, Claudio Imbrenda wrote:
> Return the correct error code when the SCCB buffer is too small to
> contain all of the output, for the Read SCP Information and
> Read CPU Information commands.

"s390x: sclp: Report insufficient SCCB length"

Reviewed-by: David Hildenbrand <david@redhat.com>

> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index abb6e50..f57ce7b 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -68,6 +68,12 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  
>      read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
>  
> +    if (be16_to_cpu(sccb->h.length) <
> +            (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return;
> +    }
> +
>      /* Configuration Characteristic (Extension) */
>      s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
>                           read_info->conf_char);
> @@ -118,6 +124,12 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>      cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
>      cpu_info->nr_standby = cpu_to_be16(0);
>  
> +    if (be16_to_cpu(sccb->h.length) <
> +            (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return;
> +    }
> +
>      /* The standby offset is 16-byte for each CPU */
>      cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
>          + cpu_info->nr_configured*sizeof(CPUEntry));
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v2 4/4] s390x: Fix SCLP return code when buffer too small

[Christian Borntraeger]

On 27.09.19 15:33, Claudio Imbrenda wrote:
> Return the correct error code when the SCCB buffer is too small to
> contain all of the output, for the Read SCP Information and
> Read CPU Information commands.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index abb6e50..f57ce7b 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -68,6 +68,12 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>  
>      read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
>  
> +    if (be16_to_cpu(sccb->h.length) <
> +            (sizeof(ReadInfo) + cpu_count * sizeof(CPUEntry))) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return;
> +    }
> +
>      /* Configuration Characteristic (Extension) */
>      s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
>                           read_info->conf_char);
> @@ -118,6 +124,12 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
>      cpu_info->offset_configured = cpu_to_be16(offsetof(ReadCpuInfo, entries));
>      cpu_info->nr_standby = cpu_to_be16(0);
>  
> +    if (be16_to_cpu(sccb->h.length) <
> +            (sizeof(ReadCpuInfo) + cpu_count * sizeof(CPUEntry))) {
> +        sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +        return;
> +    }
> +
>      /* The standby offset is 16-byte for each CPU */
>      cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
>          + cpu_info->nr_configured*sizeof(CPUEntry));
> 

Thanks applied.