Skip to content

Commit b94e4b1

Browse files
jgross1jgross1
committed
xen/blkfront: don't trust the backend response data blindly
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 <jgross@suse.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> Link: https://lore.kernel.org/r/20210730103854.12681-4-jgross@suse.com Signed-off-by: Juergen Gross <jgross@suse.com>
1 parent 8f5a695 commit b94e4b1

1 file changed

Lines changed: 53 additions & 17 deletions

File tree

‎drivers/block/xen-blkfront.c‎

Lines changed: 53 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ enum blkif_state {
8080
BLKIF_STATE_DISCONNECTED,
8181
BLKIF_STATE_CONNECTED,
8282
BLKIF_STATE_SUSPENDED,
83+
BLKIF_STATE_ERROR,
8384
};
8485

8586
struct grant {
@@ -89,6 +90,7 @@ struct grant {
8990
};
9091

9192
enum blk_req_status {
93+
REQ_PROCESSING,
9294
REQ_WAITING,
9395
REQ_DONE,
9496
REQ_ERROR,
@@ -530,7 +532,7 @@ static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo,
530532

531533
id = get_id_from_freelist(rinfo);
532534
rinfo->shadow[id].request = req;
533-
rinfo->shadow[id].status = REQ_WAITING;
535+
rinfo->shadow[id].status = REQ_PROCESSING;
534536
rinfo->shadow[id].associated_id = NO_ASSOCIATED_ID;
535537

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

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

563566
return 0;
564567
}
@@ -834,8 +837,11 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri
834837

835838
/* Copy request(s) to the ring page. */
836839
*final_ring_req = *ring_req;
837-
if (unlikely(require_extra_req))
840+
rinfo->shadow[id].status = REQ_WAITING;
841+
if (unlikely(require_extra_req)) {
838842
*final_extra_ring_req = *extra_ring_req;
843+
rinfo->shadow[extra_id].status = REQ_WAITING;
844+
}
839845

840846
if (new_persistent_gnts)
841847
gnttab_free_grant_references(setup.gref_head);
@@ -1359,8 +1365,8 @@ static enum blk_req_status blkif_rsp_to_req_status(int rsp)
13591365
static int blkif_get_final_status(enum blk_req_status s1,
13601366
enum blk_req_status s2)
13611367
{
1362-
BUG_ON(s1 == REQ_WAITING);
1363-
BUG_ON(s2 == REQ_WAITING);
1368+
BUG_ON(s1 < REQ_DONE);
1369+
BUG_ON(s2 < REQ_DONE);
13641370

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

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

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

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

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

15211533
RING_COPY_RESPONSE(&rinfo->ring, i, &bret);
15221534
id = bret.id;
@@ -1527,14 +1539,28 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
15271539
* look in get_id_from_freelist.
15281540
*/
15291541
if (id >= BLK_RING_SIZE(info)) {
1530-
WARN(1, "%s: response to %s has incorrect id (%ld)\n",
1531-
info->gd->disk_name, op_name(bret.operation), id);
1532-
/* We can't safely get the 'struct request' as
1533-
* the id is busted. */
1534-
continue;
1542+
pr_alert("%s: response has incorrect id (%ld)\n",
1543+
info->gd->disk_name, id);
1544+
goto err;
1545+
}
1546+
if (rinfo->shadow[id].status != REQ_WAITING) {
1547+
pr_alert("%s: response references no pending request\n",
1548+
info->gd->disk_name);
1549+
goto err;
15351550
}
1551+
1552+
rinfo->shadow[id].status = REQ_PROCESSING;
15361553
req = rinfo->shadow[id].request;
15371554

1555+
op = rinfo->shadow[id].req.operation;
1556+
if (op == BLKIF_OP_INDIRECT)
1557+
op = rinfo->shadow[id].req.u.indirect.indirect_op;
1558+
if (bret.operation != op) {
1559+
pr_alert("%s: response has wrong operation (%u instead of %u)\n",
1560+
info->gd->disk_name, bret.operation, op);
1561+
goto err;
1562+
}
1563+
15381564
if (bret.operation != BLKIF_OP_DISCARD) {
15391565
/*
15401566
* We may need to wait for an extra response if the
@@ -1559,7 +1585,8 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
15591585
case BLKIF_OP_DISCARD:
15601586
if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) {
15611587
struct request_queue *rq = info->rq;
1562-
printk(KERN_WARNING "blkfront: %s: %s op failed\n",
1588+
1589+
pr_warn_ratelimited("blkfront: %s: %s op failed\n",
15631590
info->gd->disk_name, op_name(bret.operation));
15641591
blkif_req(req)->error = BLK_STS_NOTSUPP;
15651592
info->feature_discard = 0;
@@ -1571,13 +1598,13 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
15711598
case BLKIF_OP_FLUSH_DISKCACHE:
15721599
case BLKIF_OP_WRITE_BARRIER:
15731600
if (unlikely(bret.status == BLKIF_RSP_EOPNOTSUPP)) {
1574-
printk(KERN_WARNING "blkfront: %s: %s op failed\n",
1601+
pr_warn_ratelimited("blkfront: %s: %s op failed\n",
15751602
info->gd->disk_name, op_name(bret.operation));
15761603
blkif_req(req)->error = BLK_STS_NOTSUPP;
15771604
}
15781605
if (unlikely(bret.status == BLKIF_RSP_ERROR &&
15791606
rinfo->shadow[id].req.u.rw.nr_segments == 0)) {
1580-
printk(KERN_WARNING "blkfront: %s: empty %s op failed\n",
1607+
pr_warn_ratelimited("blkfront: %s: empty %s op failed\n",
15811608
info->gd->disk_name, op_name(bret.operation));
15821609
blkif_req(req)->error = BLK_STS_NOTSUPP;
15831610
}
@@ -1592,8 +1619,9 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
15921619
case BLKIF_OP_READ:
15931620
case BLKIF_OP_WRITE:
15941621
if (unlikely(bret.status != BLKIF_RSP_OKAY))
1595-
dev_dbg(&info->xbdev->dev, "Bad return from blkdev data "
1596-
"request: %x\n", bret.status);
1622+
dev_dbg_ratelimited(&info->xbdev->dev,
1623+
"Bad return from blkdev data request: %#x\n",
1624+
bret.status);
15971625

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

16211649
return IRQ_HANDLED;
1650+
1651+
err:
1652+
info->connected = BLKIF_STATE_ERROR;
1653+
1654+
spin_unlock_irqrestore(&rinfo->ring_lock, flags);
1655+
1656+
pr_alert("%s disabled for further use\n", info->gd->disk_name);
1657+
return IRQ_HANDLED;
16221658
}
16231659

16241660

0 commit comments

Comments
 (0)