Re: [PATCH v1 1/1] s390x: css: report errors from ccw_dstream_read/write

[Halil Pasic <pasic@linux.ibm.com>]

On Tue,  6 Apr 2021 09:44:13 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> ccw_dstream_read/write functions returned values are sometime
> not taking into account and reported back to the upper level
> of interpretation of CCW instructions.


The return values of ccw_dstream_write/read were intentionally ignored
in commit f57ba05823 ("virtio-ccw: use ccw data stream") to avoid
changes in behavior that wound not contribute to the objective of the
patch-set, like silently doing more error checking and handling.

If we consider the first hunk:


--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -289,49 +289,19 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
         return -EFAULT;
     }
     if (is_legacy) {
-        linfo.queue = address_space_ldq_be(&address_space_memory, ccw.cda,
-                                           MEMTXATTRS_UNSPECIFIED, NULL);
-        linfo.align = address_space_ldl_be(&address_space_memory,
-                                           ccw.cda + sizeof(linfo.queue),
-                                           MEMTXATTRS_UNSPECIFIED,
-                                           NULL);
-        linfo.index = address_space_lduw_be(&address_space_memory,
-                                            ccw.cda + sizeof(linfo.queue)
-                                            + sizeof(linfo.align),
-                                            MEMTXATTRS_UNSPECIFIED,
-                                            NULL);
-        linfo.num = address_space_lduw_be(&address_space_memory,
-                                          ccw.cda + sizeof(linfo.queue)
-                                          + sizeof(linfo.align)
-                                          + sizeof(linfo.index),
-                                          MEMTXATTRS_UNSPECIFIED,
-                                          NULL);
+        ccw_dstream_read(&sch->cds, linfo);
+        be64_to_cpus(&linfo.queue);
+        be32_to_cpus(&linfo.align);
+        be16_to_cpus(&linfo.index);
+        be16_to_cpus(&linfo.num);

we can see, that the original code did not contain any error checking regarding
the invalidity of the guest physical address.

What was the behavior there? The last argument of address_space_* where we pass
the NULL is actually an optional pointer to the MemTxResult, which would tell
us if the operation succeeded or failed. Passing NULL there means, we don't care.

My guess is that when those loads fail (or the read fails) we will just carry on
with the garbage we found on the stack.

So this begs the question, do we need this fixed for old releases as well?

My answer is yes we do. Conny what do you think?

Regards,
Halil