Re: [Qemu-devel] [PATCH v3 4/5] 390x/css: introduce maximum data address checking

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

* Halil Pasic <pasic@linux.vnet.ibm.com> [2017-09-19 20:27:44 +0200]:

> The architecture mandates the addresses to be  accessed on the first
> indirection level (that is, the data addresses without IDA, and the
> (M)IDAW addresses with (M)IDA) to be checked against an CCW format
> dependent limit maximum address.  If a violation is detected, the storage
> access is not to be performed and a channel program check needs to be
> generated. As of today, we fail to do this check.
> 
> Let us stick even closer to the architecture specification.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/s390x/css.c         | 10 ++++++++++
>  include/hw/s390x/css.h |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 6b0cd8861b..2d37a9ddde 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -795,6 +795,11 @@ static inline int cds_check_len(CcwDataStream *cds, int len)
>      return cds->flags & CDS_F_STREAM_BROKEN ? -EINVAL : len;
>  }
> 
> +static inline bool cds_ccw_addrs_ok(hwaddr addr, int len, bool ccw_fmt1)
> +{
> +    return (addr + len) < (ccw_fmt1 ? (1UL << 31) : (1UL << 24));
> +}
> +
>  static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>                                    CcwDataStreamOp op)
>  {
> @@ -804,6 +809,9 @@ static int ccw_dstream_rw_noflags(CcwDataStream *cds, void *buff, int len,
>      if (ret <= 0) {
>          return ret;
>      }
> +    if (!cds_ccw_addrs_ok(cds->cda, len, cds->flags & CDS_F_FMT)) {
> +        return -EINVAL; /* channel program check */
> +    }
>      if (op == CDS_OP_A) {
>          goto incr;
>      }
> @@ -828,7 +836,9 @@ void ccw_dstream_init(CcwDataStream *cds, CCW1 const *ccw, ORB const *orb)
>      g_assert(!(orb->ctrl1 & ORB_CTRL1_MASK_MIDAW));
>      cds->flags = (orb->ctrl0 & ORB_CTRL0_MASK_I2K ? CDS_F_I2K : 0) |
>                   (orb->ctrl0 & ORB_CTRL0_MASK_C64 ? CDS_F_C64 : 0) |
> +                 (orb->ctrl0 & ORB_CTRL0_MASK_FMT ? CDS_F_FMT : 0) |
This reminds me one more question:
Calling ccw_dsteram_init() after copy_ccw_from_guest() may lead to a
fmt-1 @ccw with an @orb that designates fmt-0 ccw. This sounds insane.

>                   (ccw->flags & CCW_FLAG_IDA ? CDS_F_IDA : 0);
> +
>      cds->count = ccw->count;
>      cds->cda_orig = ccw->cda;
>      ccw_dstream_rewind(cds);
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 078356e94c..69b374730e 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -87,6 +87,7 @@ typedef struct CcwDataStream {
>  #define CDS_F_MIDA  0x02
>  #define CDS_F_I2K   0x04
>  #define CDS_F_C64   0x08
> +#define CDS_F_FMT   0x10 /* CCW format-1 */
>  #define CDS_F_STREAM_BROKEN  0x80
>      uint8_t flags;
>      uint8_t at_idaw;
> -- 
> 2.13.5
> 

-- 
Dong Jia Shi