Re: [PATCH 4/4] xen/scsifront: harden driver against malicious backend
On 21.04.22 12:13, Juergen Gross wrote:
On 20.04.22 18:13, Boris Ostrovsky wrote:
Just a couple of nits.
On 4/20/22 5:25 AM, Juergen Gross wrote:
-static int scsifront_ring_drain(struct vscsifrnt_info *info)
+static int scsifront_ring_drain(struct vscsifrnt_info *info,
+ unsigned int *eoiflag)
{
- struct vscsiif_response *ring_rsp;
+ struct vscsiif_response ring_rsp;
RING_IDX i, rp;
int more_to_do = 0;
- rp = info->ring.sring->rsp_prod;
- rmb(); /* ordering required respective to dom0 */
+ rp = READ_ONCE(info->ring.sring->rsp_prod);
+ virt_rmb(); /* ordering required respective to backend */
+ if (RING_RESPONSE_PROD_OVERFLOW(>ring, rp)) {
+ scsifront_set_error(info, "illegal number of responses");
In net and block drivers we report number of such responses. (But not in usb)
I'm not sure the specific value is of any interest.
+ return 0;
+ }
for (i = info->ring.rsp_cons; i != rp; i++) {
- ring_rsp = RING_GET_RESPONSE(>ring, i);
- scsifront_do_response(info, ring_rsp);
+ RING_COPY_RESPONSE(>ring, i, _rsp);
+ scsifront_do_response(info, _rsp);
+ if (info->host_active == STATE_ERROR)
+ return 0;
+ *eoiflag = 0;
*eoiflags &= ~XEN_EOI_FLAG_SPURIOUS; ?
Yes, probably better.
We also use eoi_flags name in other instances in this file.
I'll unify the name.
Oh, eoi_flags is used in the backend driver. So nothing to unify.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 4/4] xen/scsifront: harden driver against malicious backend
On 20.04.22 18:13, Boris Ostrovsky wrote:
Just a couple of nits.
On 4/20/22 5:25 AM, Juergen Gross wrote:
-static int scsifront_ring_drain(struct vscsifrnt_info *info)
+static int scsifront_ring_drain(struct vscsifrnt_info *info,
+ unsigned int *eoiflag)
{
- struct vscsiif_response *ring_rsp;
+ struct vscsiif_response ring_rsp;
RING_IDX i, rp;
int more_to_do = 0;
- rp = info->ring.sring->rsp_prod;
- rmb(); /* ordering required respective to dom0 */
+ rp = READ_ONCE(info->ring.sring->rsp_prod);
+ virt_rmb(); /* ordering required respective to backend */
+ if (RING_RESPONSE_PROD_OVERFLOW(>ring, rp)) {
+ scsifront_set_error(info, "illegal number of responses");
In net and block drivers we report number of such responses. (But not in usb)
I'm not sure the specific value is of any interest.
+ return 0;
+ }
for (i = info->ring.rsp_cons; i != rp; i++) {
- ring_rsp = RING_GET_RESPONSE(>ring, i);
- scsifront_do_response(info, ring_rsp);
+ RING_COPY_RESPONSE(>ring, i, _rsp);
+ scsifront_do_response(info, _rsp);
+ if (info->host_active == STATE_ERROR)
+ return 0;
+ *eoiflag = 0;
*eoiflags &= ~XEN_EOI_FLAG_SPURIOUS; ?
Yes, probably better.
We also use eoi_flags name in other instances in this file.
I'll unify the name.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 4/4] xen/scsifront: harden driver against malicious backend
Just a couple of nits.
On 4/20/22 5:25 AM, Juergen Gross wrote:
-static int scsifront_ring_drain(struct vscsifrnt_info *info)
+static int scsifront_ring_drain(struct vscsifrnt_info *info,
+ unsigned int *eoiflag)
{
- struct vscsiif_response *ring_rsp;
+ struct vscsiif_response ring_rsp;
RING_IDX i, rp;
int more_to_do = 0;
- rp = info->ring.sring->rsp_prod;
- rmb(); /* ordering required respective to dom0 */
+ rp = READ_ONCE(info->ring.sring->rsp_prod);
+ virt_rmb(); /* ordering required respective to backend */
+ if (RING_RESPONSE_PROD_OVERFLOW(>ring, rp)) {
+ scsifront_set_error(info, "illegal number of responses");
In net and block drivers we report number of such responses. (But not in usb)
+ return 0;
+ }
for (i = info->ring.rsp_cons; i != rp; i++) {
- ring_rsp = RING_GET_RESPONSE(>ring, i);
- scsifront_do_response(info, ring_rsp);
+ RING_COPY_RESPONSE(>ring, i, _rsp);
+ scsifront_do_response(info, _rsp);
+ if (info->host_active == STATE_ERROR)
+ return 0;
+ *eoiflag = 0;
*eoiflags &= ~XEN_EOI_FLAG_SPURIOUS; ?
We also use eoi_flags name in other instances in this file.
-boris
[PATCH 4/4] xen/scsifront: harden driver against malicious backend
Instead of relying on a well behaved PV scsi backend verify all meta
data received from the backend and avoid multiple reads of the same
data from the shared ring page.
In case any illegal data from the backend is detected switch the
PV device to a new "error" state and deactivate it for further use.
Use the "lateeoi" variant for the event channel in order to avoid
event storms blocking the guest.
Signed-off-by: Juergen Gross
---
drivers/scsi/xen-scsifront.c | 104 +--
1 file changed, 76 insertions(+), 28 deletions(-)
diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c
index 8511bfc62963..761c9c463ecd 100644
--- a/drivers/scsi/xen-scsifront.c
+++ b/drivers/scsi/xen-scsifront.c
@@ -83,6 +83,8 @@ struct vscsifrnt_shadow {
uint16_t rqid;
uint16_t ref_rqid;
+ bool inflight;
+
unsigned int nr_grants; /* number of grants in gref[] */
struct scsiif_request_segment *sg; /* scatter/gather elements */
struct scsiif_request_segment seg[VSCSIIF_SG_TABLESIZE];
@@ -104,7 +106,11 @@ struct vscsifrnt_info {
struct xenbus_device *dev;
struct Scsi_Host *host;
- int host_active;
+ enum {
+ STATE_INACTIVE,
+ STATE_ACTIVE,
+ STATE_ERROR
+ } host_active;
unsigned int evtchn;
unsigned int irq;
@@ -217,6 +223,8 @@ static int scsifront_do_request(struct vscsifrnt_info *info,
for (i = 0; i < (shadow->nr_segments & ~VSCSIIF_SG_GRANT); i++)
ring_req->seg[i] = shadow->seg[i];
+ shadow->inflight = true;
+
RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify);
if (notify)
notify_remote_via_irq(info->irq);
@@ -224,6 +232,13 @@ static int scsifront_do_request(struct vscsifrnt_info
*info,
return 0;
}
+static void scsifront_set_error(struct vscsifrnt_info *info, const char *msg)
+{
+ shost_printk(KERN_ERR, info->host, KBUILD_MODNAME "%s\n"
+"Disabling device for further use\n", msg);
+ info->host_active = STATE_ERROR;
+}
+
static void scsifront_gnttab_done(struct vscsifrnt_info *info,
struct vscsifrnt_shadow *shadow)
{
@@ -234,9 +249,8 @@ static void scsifront_gnttab_done(struct vscsifrnt_info
*info,
for (i = 0; i < shadow->nr_grants; i++) {
if (unlikely(!gnttab_try_end_foreign_access(shadow->gref[i]))) {
- shost_printk(KERN_ALERT, info->host, KBUILD_MODNAME
-"grant still in use by backend\n");
- BUG();
+ scsifront_set_error(info, "grant still in use by
backend");
+ return;
}
}
@@ -308,6 +322,8 @@ static void scsifront_cdb_cmd_done(struct vscsifrnt_info
*info,
BUG_ON(sc == NULL);
scsifront_gnttab_done(info, shadow);
+ if (info->host_active == STATE_ERROR)
+ return;
scsifront_put_rqid(info, id);
set_host_byte(sc, scsifront_host_byte(ring_rsp->rslt));
@@ -348,9 +364,7 @@ static void scsifront_sync_cmd_done(struct vscsifrnt_info
*info,
scsifront_wake_up(info);
return;
default:
- shost_printk(KERN_ERR, info->host, KBUILD_MODNAME
-"bad reset state %d, possibly leaking %u\n",
-shadow->rslt_reset, id);
+ scsifront_set_error(info, "bad reset state");
break;
}
spin_unlock_irqrestore(>shadow_lock, flags);
@@ -361,28 +375,41 @@ static void scsifront_sync_cmd_done(struct vscsifrnt_info
*info,
static void scsifront_do_response(struct vscsifrnt_info *info,
struct vscsiif_response *ring_rsp)
{
- if (WARN(ring_rsp->rqid >= VSCSIIF_MAX_REQS ||
-test_bit(ring_rsp->rqid, info->shadow_free_bitmap),
-"illegal rqid %u returned by backend!\n", ring_rsp->rqid))
+ struct vscsifrnt_shadow *shadow;
+
+ if (ring_rsp->rqid >= VSCSIIF_MAX_REQS ||
+ !info->shadow[ring_rsp->rqid]->inflight) {
+ scsifront_set_error(info, "illegal rqid returned by backend!");
return;
+ }
+ shadow = info->shadow[ring_rsp->rqid];
+ shadow->inflight = false;
- if (info->shadow[ring_rsp->rqid]->act == VSCSIIF_ACT_SCSI_CDB)
+ if (shadow->act == VSCSIIF_ACT_SCSI_CDB)
scsifront_cdb_cmd_done(info, ring_rsp);
else
scsifront_sync_cmd_done(info, ring_rsp);
}
-static int scsifront_ring_drain(struct vscsifrnt_info *info)
+static int scsifront_ring_drain(struct vscsifrnt_info *info,
+ unsigned int *eoiflag)
{
- struct vscsiif_response *ring_rsp;
+ struct vscsiif_response ring_rsp;
RING_IDX i, rp;
int
