Update the Xen PV-scsi interface from the Xen tree and adapt the
related drivers to use the new definitions.
Harden the frontend driver to be no longer vulnerable to a malicious
backend.
Juergen Gross (4):
xen: update vscsiif.h
xen/scsiback: use new command result macros
xen/scsifront: use new command result macros
xen/scsifront: harden driver against malicious backend
drivers/scsi/xen-scsifront.c | 168 ++++++++++++++++++++++-------
drivers/xen/xen-scsiback.c | 82 +++++++++++++-
include/xen/interface/io/vscsiif.h | 133 ++++++++++++++++++++++-
3 files changed, 340 insertions(+), 43 deletions(-)
--
2.34.1
Add a translation layer for the command result values.
Signed-off-by: Juergen Gross <[email protected]>
---
drivers/scsi/xen-scsifront.c | 64 +++++++++++++++++++++++++++++++-----
1 file changed, 56 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index 12109e4c73d4..8511bfc62963 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -243,6 +243,56 @@ static void scsifront_gnttab_done(struct vscsifrnt_info *info,
kfree(shadow->sg);
}
+static unsigned int scsifront_host_byte(int32_t rslt)
+{
+ switch (XEN_VSCSIIF_RSLT_HOST(rslt)) {
+ case XEN_VSCSIIF_RSLT_HOST_OK:
+ return DID_OK;
+ case XEN_VSCSIIF_RSLT_HOST_NO_CONNECT:
+ return DID_NO_CONNECT;
+ case XEN_VSCSIIF_RSLT_HOST_BUS_BUSY:
+ return DID_BUS_BUSY;
+ case XEN_VSCSIIF_RSLT_HOST_TIME_OUT:
+ return DID_TIME_OUT;
+ case XEN_VSCSIIF_RSLT_HOST_BAD_TARGET:
+ return DID_BAD_TARGET;
+ case XEN_VSCSIIF_RSLT_HOST_ABORT:
+ return DID_ABORT;
+ case XEN_VSCSIIF_RSLT_HOST_PARITY:
+ return DID_PARITY;
+ case XEN_VSCSIIF_RSLT_HOST_ERROR:
+ return DID_ERROR;
+ case XEN_VSCSIIF_RSLT_HOST_RESET:
+ return DID_RESET;
+ case XEN_VSCSIIF_RSLT_HOST_BAD_INTR:
+ return DID_BAD_INTR;
+ case XEN_VSCSIIF_RSLT_HOST_PASSTHROUGH:
+ return DID_PASSTHROUGH;
+ case XEN_VSCSIIF_RSLT_HOST_SOFT_ERROR:
+ return DID_SOFT_ERROR;
+ case XEN_VSCSIIF_RSLT_HOST_IMM_RETRY:
+ return DID_IMM_RETRY;
+ case XEN_VSCSIIF_RSLT_HOST_REQUEUE:
+ return DID_REQUEUE;
+ case XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED:
+ return DID_TRANSPORT_DISRUPTED;
+ case XEN_VSCSIIF_RSLT_HOST_TRANSPORT_FAILFAST:
+ return DID_TRANSPORT_FAILFAST;
+ case XEN_VSCSIIF_RSLT_HOST_TARGET_FAILURE:
+ return DID_TARGET_FAILURE;
+ case XEN_VSCSIIF_RSLT_HOST_NEXUS_FAILURE:
+ return DID_NEXUS_FAILURE;
+ case XEN_VSCSIIF_RSLT_HOST_ALLOC_FAILURE:
+ return DID_ALLOC_FAILURE;
+ case XEN_VSCSIIF_RSLT_HOST_MEDIUM_ERROR:
+ return DID_MEDIUM_ERROR;
+ case XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL:
+ return DID_TRANSPORT_MARGINAL;
+ default:
+ return DID_ERROR;
+ }
+}
+
static void scsifront_cdb_cmd_done(struct vscsifrnt_info *info,
struct vscsiif_response *ring_rsp)
{
@@ -250,7 +300,6 @@ static void scsifront_cdb_cmd_done(struct vscsifrnt_info *info,
struct scsi_cmnd *sc;
uint32_t id;
uint8_t sense_len;
- int result;
id = ring_rsp->rqid;
shadow = info->shadow[id];
@@ -261,12 +310,8 @@ static void scsifront_cdb_cmd_done(struct vscsifrnt_info *info,
scsifront_gnttab_done(info, shadow);
scsifront_put_rqid(info, id);
- result = ring_rsp->rslt;
- if (result >> 24)
- set_host_byte(sc, DID_ERROR);
- else
- set_host_byte(sc, host_byte(result));
- set_status_byte(sc, result & 0xff);
+ set_host_byte(sc, scsifront_host_byte(ring_rsp->rslt));
+ set_status_byte(sc, XEN_VSCSIIF_RSLT_STATUS(ring_rsp->rslt));
scsi_set_resid(sc, ring_rsp->residual_len);
sense_len = min_t(uint8_t, VSCSIIF_SENSE_BUFFERSIZE,
@@ -290,7 +335,10 @@ static void scsifront_sync_cmd_done(struct vscsifrnt_info *info,
shadow->wait_reset = 1;
switch (shadow->rslt_reset) {
case RSLT_RESET_WAITING:
- shadow->rslt_reset = ring_rsp->rslt;
+ if (ring_rsp->rslt == XEN_VSCSIIF_RSLT_RESET_SUCCESS)
+ shadow->rslt_reset = SUCCESS;
+ else
+ shadow->rslt_reset = FAILED;
break;
case RSLT_RESET_ERR:
kick = _scsifront_put_rqid(info, id);
--
2.34.1
Update include/xen/interface/io/vscsiif.h to its newest version.
Signed-off-by: Juergen Gross <[email protected]>
---
include/xen/interface/io/vscsiif.h | 133 ++++++++++++++++++++++++++++-
1 file changed, 129 insertions(+), 4 deletions(-)
diff --git a/include/xen/interface/io/vscsiif.h b/include/xen/interface/io/vscsiif.h
index 1f6047d3de44..7ea4dc9611c4 100644
--- a/include/xen/interface/io/vscsiif.h
+++ b/include/xen/interface/io/vscsiif.h
@@ -43,7 +43,7 @@
*
* A string specifying the backend device: either a 4-tuple "h:c:t:l"
* (host, controller, target, lun, all integers), or a WWN (e.g.
- * "naa.60014054ac780582").
+ * "naa.60014054ac780582:0").
*
* v-dev
* Values: string
@@ -87,6 +87,75 @@
* response structures.
*/
+/*
+ * Xenstore format in practice
+ * ===========================
+ *
+ * The backend driver uses a single_host:many_devices notation to manage domU
+ * devices. Everything is stored in /local/domain/<backend_domid>/backend/vscsi/.
+ * The xenstore layout looks like this (dom0 is assumed to be the backend_domid):
+ *
+ * <domid>/<vhost>/feature-host = "0"
+ * <domid>/<vhost>/frontend = "/local/domain/<domid>/device/vscsi/0"
+ * <domid>/<vhost>/frontend-id = "<domid>"
+ * <domid>/<vhost>/online = "1"
+ * <domid>/<vhost>/state = "4"
+ * <domid>/<vhost>/vscsi-devs/dev-0/p-dev = "8:0:2:1" or "naa.wwn:lun"
+ * <domid>/<vhost>/vscsi-devs/dev-0/state = "4"
+ * <domid>/<vhost>/vscsi-devs/dev-0/v-dev = "0:0:0:0"
+ * <domid>/<vhost>/vscsi-devs/dev-1/p-dev = "8:0:2:2"
+ * <domid>/<vhost>/vscsi-devs/dev-1/state = "4"
+ * <domid>/<vhost>/vscsi-devs/dev-1/v-dev = "0:0:1:0"
+ *
+ * The frontend driver maintains its state in
+ * /local/domain/<domid>/device/vscsi/.
+ *
+ * <vhost>/backend = "/local/domain/0/backend/vscsi/<domid>/<vhost>"
+ * <vhost>/backend-id = "0"
+ * <vhost>/event-channel = "20"
+ * <vhost>/ring-ref = "43"
+ * <vhost>/state = "4"
+ * <vhost>/vscsi-devs/dev-0/state = "4"
+ * <vhost>/vscsi-devs/dev-1/state = "4"
+ *
+ * In addition to the entries for backend and frontend these flags are stored
+ * for the toolstack:
+ *
+ * <domid>/<vhost>/vscsi-devs/dev-1/p-devname = "/dev/$device"
+ * <domid>/<vhost>/libxl_ctrl_index = "0"
+ *
+ *
+ * Backend/frontend protocol
+ * =========================
+ *
+ * To create a vhost along with a device:
+ * <domid>/<vhost>/feature-host = "0"
+ * <domid>/<vhost>/frontend = "/local/domain/<domid>/device/vscsi/0"
+ * <domid>/<vhost>/frontend-id = "<domid>"
+ * <domid>/<vhost>/online = "1"
+ * <domid>/<vhost>/state = "1"
+ * <domid>/<vhost>/vscsi-devs/dev-0/p-dev = "8:0:2:1"
+ * <domid>/<vhost>/vscsi-devs/dev-0/state = "1"
+ * <domid>/<vhost>/vscsi-devs/dev-0/v-dev = "0:0:0:0"
+ * Wait for <domid>/<vhost>/state + <domid>/<vhost>/vscsi-devs/dev-0/state become 4
+ *
+ * To add another device to a vhost:
+ * <domid>/<vhost>/state = "7"
+ * <domid>/<vhost>/vscsi-devs/dev-1/p-dev = "8:0:2:2"
+ * <domid>/<vhost>/vscsi-devs/dev-1/state = "1"
+ * <domid>/<vhost>/vscsi-devs/dev-1/v-dev = "0:0:1:0"
+ * Wait for <domid>/<vhost>/state + <domid>/<vhost>/vscsi-devs/dev-1/state become 4
+ *
+ * To remove a device from a vhost:
+ * <domid>/<vhost>/state = "7"
+ * <domid>/<vhost>/vscsi-devs/dev-1/state = "5"
+ * Wait for <domid>/<vhost>/state to become 4
+ * Wait for <domid>/<vhost>/vscsi-devs/dev-1/state become 6
+ * Remove <domid>/<vhost>/vscsi-devs/dev-1/{state,p-dev,v-dev,p-devname}
+ * Remove <domid>/<vhost>/vscsi-devs/dev-1/
+ *
+ */
+
/* Requests from the frontend to the backend */
/*
@@ -117,7 +186,8 @@
* (plus the set VSCSIIF_SG_GRANT bit), the number of scsiif_request_segment
* elements referencing the target data buffers is calculated from the lengths
* of the seg[] elements (the sum of all valid seg[].length divided by the
- * size of one scsiif_request_segment structure).
+ * size of one scsiif_request_segment structure). The frontend may use a mix of
+ * direct and indirect requests.
*/
#define VSCSIIF_ACT_SCSI_CDB 1
@@ -154,12 +224,14 @@
/*
* based on Linux kernel 2.6.18, still valid
+ *
* Changing these values requires support of multiple protocols via the rings
* as "old clients" will blindly use these values and the resulting structure
* sizes.
*/
#define VSCSIIF_MAX_COMMAND_SIZE 16
#define VSCSIIF_SENSE_BUFFERSIZE 96
+#define VSCSIIF_PAGE_SIZE 4096
struct scsiif_request_segment {
grant_ref_t gref;
@@ -167,7 +239,8 @@ struct scsiif_request_segment {
uint16_t length;
};
-#define VSCSIIF_SG_PER_PAGE (PAGE_SIZE / sizeof(struct scsiif_request_segment))
+#define VSCSIIF_SG_PER_PAGE (VSCSIIF_PAGE_SIZE / \
+ sizeof(struct scsiif_request_segment))
/* Size of one request is 252 bytes */
struct vscsiif_request {
@@ -207,6 +280,58 @@ struct vscsiif_response {
uint32_t reserved[36];
};
+/* SCSI I/O status from vscsiif_response->rslt */
+#define XEN_VSCSIIF_RSLT_STATUS(x) ((x) & 0x00ff)
+
+/* Host I/O status from vscsiif_response->rslt */
+#define XEN_VSCSIIF_RSLT_HOST(x) (((x) & 0x00ff0000) >> 16)
+#define XEN_VSCSIIF_RSLT_HOST_OK 0
+/* Couldn't connect before timeout */
+#define XEN_VSCSIIF_RSLT_HOST_NO_CONNECT 1
+/* Bus busy through timeout */
+#define XEN_VSCSIIF_RSLT_HOST_BUS_BUSY 2
+/* Timed out for other reason */
+#define XEN_VSCSIIF_RSLT_HOST_TIME_OUT 3
+/* Bad target */
+#define XEN_VSCSIIF_RSLT_HOST_BAD_TARGET 4
+/* Abort for some other reason */
+#define XEN_VSCSIIF_RSLT_HOST_ABORT 5
+/* Parity error */
+#define XEN_VSCSIIF_RSLT_HOST_PARITY 6
+/* Internal error */
+#define XEN_VSCSIIF_RSLT_HOST_ERROR 7
+/* Reset by somebody */
+#define XEN_VSCSIIF_RSLT_HOST_RESET 8
+/* Unexpected interrupt */
+#define XEN_VSCSIIF_RSLT_HOST_BAD_INTR 9
+/* Force command past mid-layer */
+#define XEN_VSCSIIF_RSLT_HOST_PASSTHROUGH 10
+/* Retry requested */
+#define XEN_VSCSIIF_RSLT_HOST_SOFT_ERROR 11
+/* Hidden retry requested */
+#define XEN_VSCSIIF_RSLT_HOST_IMM_RETRY 12
+/* Requeue command requested */
+#define XEN_VSCSIIF_RSLT_HOST_REQUEUE 13
+/* Transport error disrupted I/O */
+#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED 14
+/* Transport class fastfailed */
+#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_FAILFAST 15
+/* Permanent target failure */
+#define XEN_VSCSIIF_RSLT_HOST_TARGET_FAILURE 16
+/* Permanent nexus failure on path */
+#define XEN_VSCSIIF_RSLT_HOST_NEXUS_FAILURE 17
+/* Space allocation on device failed */
+#define XEN_VSCSIIF_RSLT_HOST_ALLOC_FAILURE 18
+/* Medium error */
+#define XEN_VSCSIIF_RSLT_HOST_MEDIUM_ERROR 19
+/* Transport marginal errors */
+#define XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL 20
+
+/* Result values of reset operations */
+#define XEN_VSCSIIF_RSLT_RESET_SUCCESS 0x2002
+#define XEN_VSCSIIF_RSLT_RESET_FAILED 0x2003
+
DEFINE_RING_TYPES(vscsiif, struct vscsiif_request, struct vscsiif_response);
-#endif /*__XEN__PUBLIC_IO_SCSI_H__*/
+
+#endif /*__XEN__PUBLIC_IO_SCSI_H__*/
--
2.34.1
Instead of using the kernel's values for the result of PV scsi
operations use the values of the interface definition.
Signed-off-by: Juergen Gross <[email protected]>
---
drivers/xen/xen-scsiback.c | 82 ++++++++++++++++++++++++++++++++++++--
1 file changed, 79 insertions(+), 3 deletions(-)
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 0c5e565aa8cf..673dd73844ff 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -280,6 +280,82 @@ static void scsiback_free_translation_entry(struct kref *kref)
kfree(entry);
}
+static int32_t scsiback_result(int32_t result)
+{
+ int32_t host_status;
+
+ switch (result >> 16) {
+ case DID_OK:
+ host_status = XEN_VSCSIIF_RSLT_HOST_OK;
+ break;
+ case DID_NO_CONNECT:
+ host_status = XEN_VSCSIIF_RSLT_HOST_NO_CONNECT;
+ break;
+ case DID_BUS_BUSY:
+ host_status = XEN_VSCSIIF_RSLT_HOST_BUS_BUSY;
+ break;
+ case DID_TIME_OUT:
+ host_status = XEN_VSCSIIF_RSLT_HOST_TIME_OUT;
+ break;
+ case DID_BAD_TARGET:
+ host_status = XEN_VSCSIIF_RSLT_HOST_BAD_TARGET;
+ break;
+ case DID_ABORT:
+ host_status = XEN_VSCSIIF_RSLT_HOST_ABORT;
+ break;
+ case DID_PARITY:
+ host_status = XEN_VSCSIIF_RSLT_HOST_PARITY;
+ break;
+ case DID_ERROR:
+ host_status = XEN_VSCSIIF_RSLT_HOST_ERROR;
+ break;
+ case DID_RESET:
+ host_status = XEN_VSCSIIF_RSLT_HOST_RESET;
+ break;
+ case DID_BAD_INTR:
+ host_status = XEN_VSCSIIF_RSLT_HOST_BAD_INTR;
+ break;
+ case DID_PASSTHROUGH:
+ host_status = XEN_VSCSIIF_RSLT_HOST_PASSTHROUGH;
+ break;
+ case DID_SOFT_ERROR:
+ host_status = XEN_VSCSIIF_RSLT_HOST_SOFT_ERROR;
+ break;
+ case DID_IMM_RETRY:
+ host_status = XEN_VSCSIIF_RSLT_HOST_IMM_RETRY;
+ break;
+ case DID_REQUEUE:
+ host_status = XEN_VSCSIIF_RSLT_HOST_REQUEUE;
+ break;
+ case DID_TRANSPORT_DISRUPTED:
+ host_status = XEN_VSCSIIF_RSLT_HOST_TRANSPORT_DISRUPTED;
+ break;
+ case DID_TRANSPORT_FAILFAST:
+ host_status = XEN_VSCSIIF_RSLT_HOST_TRANSPORT_FAILFAST;
+ break;
+ case DID_TARGET_FAILURE:
+ host_status = XEN_VSCSIIF_RSLT_HOST_TARGET_FAILURE;
+ break;
+ case DID_NEXUS_FAILURE:
+ host_status = XEN_VSCSIIF_RSLT_HOST_NEXUS_FAILURE;
+ break;
+ case DID_ALLOC_FAILURE:
+ host_status = XEN_VSCSIIF_RSLT_HOST_ALLOC_FAILURE;
+ break;
+ case DID_MEDIUM_ERROR:
+ host_status = XEN_VSCSIIF_RSLT_HOST_MEDIUM_ERROR;
+ break;
+ case DID_TRANSPORT_MARGINAL:
+ host_status = XEN_VSCSIIF_RSLT_HOST_TRANSPORT_MARGINAL;
+ break;
+ default:
+ host_status = XEN_VSCSIIF_RSLT_HOST_ERROR;
+ break;
+ }
+
+ return (host_status << 16) | (result & 0x00ff);
+}
+
static void scsiback_send_response(struct vscsibk_info *info,
char *sense_buffer, int32_t result, uint32_t resid,
uint16_t rqid)
@@ -295,7 +371,7 @@ static void scsiback_send_response(struct vscsibk_info *info,
ring_res = RING_GET_RESPONSE(&info->ring, info->ring.rsp_prod_pvt);
info->ring.rsp_prod_pvt++;
- ring_res->rslt = result;
+ ring_res->rslt = scsiback_result(result);
ring_res->rqid = rqid;
if (sense_buffer != NULL &&
@@ -555,7 +631,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
struct scsiback_nexus *nexus = tpg->tpg_nexus;
struct se_cmd *se_cmd = &pending_req->se_cmd;
u64 unpacked_lun = pending_req->v2p->lun;
- int rc, err = FAILED;
+ int rc, err = XEN_VSCSIIF_RSLT_RESET_FAILED;
init_completion(&pending_req->tmr_done);
@@ -569,7 +645,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
wait_for_completion(&pending_req->tmr_done);
err = (se_cmd->se_tmr_req->response == TMR_FUNCTION_COMPLETE) ?
- SUCCESS : FAILED;
+ XEN_VSCSIIF_RSLT_RESET_SUCCESS : XEN_VSCSIIF_RSLT_RESET_FAILED;
scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
transport_generic_free_cmd(&pending_req->se_cmd, 0);
--
2.34.1
On 4/21/22 4:40 AM, Juergen Gross wrote:
> On 20.04.22 18:12, Boris Ostrovsky wrote:
>>
>> On 4/20/22 5:25 AM, Juergen Gross wrote:
>>> @@ -569,7 +645,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
>>> wait_for_completion(&pending_req->tmr_done);
>>> err = (se_cmd->se_tmr_req->response == TMR_FUNCTION_COMPLETE) ?
>>> - SUCCESS : FAILED;
>>> + XEN_VSCSIIF_RSLT_RESET_SUCCESS : XEN_VSCSIIF_RSLT_RESET_FAILED;
>>> scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
>>> transport_generic_free_cmd(&pending_req->se_cmd, 0);
>>
>>
>> You also want to initialize err to XEN_VSCSIIF_RSLT_RESET_FAILED.
>
> I did that.
Yes you did. I don't know what I was was looking at.
>
>> And also looking at invocations of scsiback_do_resp_with_sense() I think those may need to be adjusted as well.
>
> No, the invocations are fine, but scsiback_result() needs to pass through
> the lowest 16 bits instead of only the lowest 8 bits of the result value.
>
What I was thinking was that this could use the reverse of XEN_VSCSIIF_RSLT_HOST(), i.e. something like
#define RSLT_HOST_TO_XEN_VSCSIIF(x) ((x)<<16)
to be explicit about namespaces.
BTW, should scsiback_result() use XEN_VSCSIIF_RSLT_HOST() at the top?
-boris
On 21.04.22 22:56, Boris Ostrovsky wrote:
>
> On 4/21/22 4:40 AM, Juergen Gross wrote:
>> On 20.04.22 18:12, Boris Ostrovsky wrote:
>>> And also looking at invocations of scsiback_do_resp_with_sense() I think
>>> those may need to be adjusted as well.
>>
>> No, the invocations are fine, but scsiback_result() needs to pass through
>> the lowest 16 bits instead of only the lowest 8 bits of the result value.
>>
>
> What I was thinking was that this could use the reverse of
> XEN_VSCSIIF_RSLT_HOST(), i.e. something like
>
> #define RSLT_HOST_TO_XEN_VSCSIIF(x) ((x)<<16)
>
> to be explicit about namespaces.
I don't think this is needed.
> BTW, should scsiback_result() use XEN_VSCSIIF_RSLT_HOST() at the top?
Yes, I'll do that.
Juergen