Skip to content

Commit

Permalink
xen/blkfront: don't trust the backend response data blindly
Browse files Browse the repository at this point in the history 
Today blkfront will trust the backend to send only sane response data.
In order to avoid privilege escalations or crashes in case of malicious
backends verify the data to be within expected limits. Especially make
sure that the response always references an outstanding request.

Introduce a new state of the ring BLKIF_STATE_ERROR which will be
switched to in case an inconsistency is being detected. Recovering from
this state is possible only via removing and adding the virtual device
again (e.g. via a suspend/resume cycle).

Make all warning messages issued due to valid error responses rate
limited in order to avoid message floods being triggered by a malicious
backend.

Signed-off-by: Juergen Gross <[email protected]>
Reviewed-by: Jan Beulich <[email protected]>
Acked-by: Roger Pau Monné <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Juergen Gross <[email protected]>
  • Loading branch information
@jgross1
jgross1 committed Aug 30, 2021
1 parent 8f5a695 commit b94e4b1
Showing 1 changed file with 53 additions and 17 deletions.
70 changes: 53 additions & 17 deletions drivers/block/xen-blkfront.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ enum blkif_state {
BLKIF_STATE_DISCONNECTED,
BLKIF_STATE_CONNECTED,
BLKIF_STATE_SUSPENDED,
BLKIF_STATE_ERROR,
};

struct grant {
Expand All @@ -89,6 +90,7 @@ struct grant {
};

enum blk_req_status {
REQ_PROCESSING,
REQ_WAITING,
REQ_DONE,
REQ_ERROR,
Expand Down Expand Up @@ -530,7 +532,7 @@ static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo,

id = get_id_from_freelist(rinfo);
rinfo->shadow[id].request = req;
rinfo->shadow[id].status = REQ_WAITING;
rinfo->shadow[id].status = REQ_PROCESSING;
rinfo->shadow[id].associated_id = NO_ASSOCIATED_ID;

rinfo->shadow[id].req.u.rw.id = id;
Expand Down Expand Up @@ -559,6 +561,7 @@ static int blkif_queue_discard_req(struct request *req, struct blkfront_ring_inf

/* Copy the request to the ring page. */
*final_ring_req = *ring_req;
rinfo->shadow[id].status = REQ_WAITING;

return 0;
}
Expand Down Expand Up @@ -834,8 +837,11 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri

/* Copy request(s) to the ring page. */
*final_ring_req = *ring_req;
if (unlikely(require_extra_req))
rinfo->shadow[id].status = REQ_WAITING;
if (unlikely(require_extra_req)) {
*final_extra_ring_req = *extra_ring_req;
rinfo->shadow[extra_id].status = REQ_WAITING;
}

if (new_persistent_gnts)
gnttab_free_grant_references(setup.gref_head);
Expand Down Expand Up @@ -1359,8 +1365,8 @@ static enum blk_req_status blkif_rsp_to_req_status(int rsp)
static int blkif_get_final_status(enum blk_req_status s1,
enum blk_req_status s2)
{
BUG_ON(s1 == REQ_WAITING);
BUG_ON(s2 == REQ_WAITING);
BUG_ON(s1 < REQ_DONE);
BUG_ON(s2 < REQ_DONE);

if (s1 == REQ_ERROR || s2 == REQ_ERROR)
return BLKIF_RSP_ERROR;
Expand Down Expand Up @@ -1393,7 +1399,7 @@ static bool blkif_completion(unsigned long *id,
s->status = blkif_rsp_to_req_status(bret->status);

/* Wait the second response if not yet here. */
if (s2->status == REQ_WAITING)
if (s2->status < REQ_DONE)
return false;

bret->status = blkif_get_final_status(s->status,
Expand Down Expand Up @@ -1512,11 +1518,17 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)

spin_lock_irqsave(&rinfo->ring_lock, flags);
again:
rp = rinfo->ring.sring->rsp_prod;
rmb(); /* Ensure we see queued responses up to 'rp'. */
rp = READ_ONCE(rinfo->ring.sring->rsp_prod);
virt_rmb(); /* Ensure we see queued responses up to 'rp'. */
if (RING_RESPONSE_PROD_OVERFLOW(&rinfo->ring, rp)) {
pr_alert("%s: illegal number of responses %u\n",
info->gd->disk_name, rp - rinfo->ring.rsp_cons);
goto err;
}

for (i = rinfo->ring.rsp_cons; i != rp; i++) {
unsigned long id;
unsigned int op;

RING_COPY_RESPONSE(&rinfo->ring, i, &bret);
id = bret.id;
Expand All @@ -1527,14 +1539,28 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
* look in get_id_from_freelist.
*/
if (id >= BLK_RING_SIZE(info)) {
WARN(1, "%s: response to %s has incorrect id (%ld)\n",
info->gd->disk_name, op_name(bret.operation), id);
/* We can't safely get the 'struct request' as
* the id is busted. */
continue;
pr_alert("%s: response has incorrect id (%ld)\n",
info->gd->disk_name, id);
goto err;
}
if (rinfo->shadow[id].status != REQ_WAITING) {
pr_alert("%s: response references no pending request\n",
info->gd->disk_name);
goto err;
}

rinfo->shadow[id].status = REQ_PROCESSING;
req = rinfo->shadow[id].request;

op = rinfo->shadow[id].req.operation;
if (op == BLKIF_OP_INDIRECT)
op = rinfo->shadow[id].req.u.indirect.indirect_op;
if (bret.operation != op) {
pr_alert("%s: response has wrong operation (%u instead of %u)\n",
info->gd->disk_name, bret.operation, op);
goto err;
}

if (bret.operation != BLKIF_OP_DISCARD) {
/*
* We may need to wait for an extra response if the
Expand All @@ -1559,7 +1585,8 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
case BLKIF_OP_DISCARD:
if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) {
struct request_queue *rq = info->rq;
printk(KERN_WARNING "blkfront: %s: %s op failed\n",

pr_warn_ratelimited("blkfront: %s: %s op failed\n",
info->gd->disk_name, op_name(bret.operation));
blkif_req(req)->error = BLK_STS_NOTSUPP;
info->feature_discard = 0;
Expand All @@ -1571,13 +1598,13 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
case BLKIF_OP_FLUSH_DISKCACHE:
case BLKIF_OP_WRITE_BARRIER:
if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) {
printk(KERN_WARNING "blkfront: %s: %s op failed\n",
pr_warn_ratelimited("blkfront: %s: %s op failed\n",
info->gd->disk_name, op_name(bret.operation));
blkif_req(req)->error = BLK_STS_NOTSUPP;
}
if (unlikely(bret.status == BLKIF_RSP_ERROR &&
rinfo->shadow[id].req.u.rw.nr_segments == 0)) {
printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
pr_warn_ratelimited("blkfront: %s: empty %s op failed\n",
info->gd->disk_name, op_name(bret.operation));
blkif_req(req)->error = BLK_STS_NOTSUPP;
}
Expand All @@ -1592,8 +1619,9 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
case BLKIF_OP_READ:
case BLKIF_OP_WRITE:
if (unlikely(bret.status != BLKIF_RSP_OKAY))
dev_dbg(&info->xbdev->dev, "Bad return from blkdev data "
"request: %x\n", bret.status);
dev_dbg_ratelimited(&info->xbdev->dev,
"Bad return from blkdev data request: %#x\n",
bret.status);

break;
default:
Expand All @@ -1619,6 +1647,14 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
spin_unlock_irqrestore(&rinfo->ring_lock, flags);

return IRQ_HANDLED;

err:
info->connected = BLKIF_STATE_ERROR;

spin_unlock_irqrestore(&rinfo->ring_lock, flags);

pr_alert("%s disabled for further use\n", info->gd->disk_name);
return IRQ_HANDLED;
}


Expand Down

0 comments on commit b94e4b1

Please sign in to comment.