xen/blkfront: don't use gnttab_query_foreign_access() for mapped status

It isn't enough to check whether a grant is still being in use by
calling gnttab_query_foreign_access(), as a mapping could be realized
by the other side just after having called that function.

In case the call was done in preparation of revoking a grant it is
better to do so via gnttab_end_foreign_access_ref() and check the
success of that operation instead.

For the ring allocation use alloc_pages_exact() in order to avoid
high order pages in case of a multi-page ring.

If a grant wasn't unmapped by the backend without persistent grants
being used, set the device state to "error".

This is CVE-2022-23036 / part of XSA-396.

Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
V2:
- use gnttab_try_end_foreign_access()
V4:
- use alloc_pages_exact() and free_pages_exact()
- set state to error if backend didn't unmap (Roger Pau Monné)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index ca71a0585333fbbe1e553ddb9c37a0d8b6ec3072..03b5fb341e5890a83bfdb125d093569d528e1237 100644 (file)
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1288,7 +1288,8 @@ free_shadow:
                        rinfo->ring_ref[i] = GRANT_INVALID_REF;
                }
        }
-       free_pages((unsigned long)rinfo->ring.sring, get_order(info->nr_ring_pages * XEN_PAGE_SIZE));
+       free_pages_exact(rinfo->ring.sring,
+                        info->nr_ring_pages * XEN_PAGE_SIZE);
        rinfo->ring.sring = NULL;
 
        if (rinfo->irq)
@@ -1372,9 +1373,15 @@ static int blkif_get_final_status(enum blk_req_status s1,
        return BLKIF_RSP_OKAY;
 }
 
-static bool blkif_completion(unsigned long *id,
-                            struct blkfront_ring_info *rinfo,
-                            struct blkif_response *bret)
+/*
+ * Return values:
+ *  1 response processed.
+ *  0 missing further responses.
+ * -1 error while processing.
+ */
+static int blkif_completion(unsigned long *id,
+                           struct blkfront_ring_info *rinfo,
+                           struct blkif_response *bret)
 {
        int i = 0;
        struct scatterlist *sg;
@@ -1397,7 +1404,7 @@ static bool blkif_completion(unsigned long *id,
 
                /* Wait the second response if not yet here. */
                if (s2->status < REQ_DONE)
-                       return false;
+                       return 0;
 
                bret->status = blkif_get_final_status(s->status,
                                                      s2->status);
@@ -1448,42 +1455,43 @@ static bool blkif_completion(unsigned long *id,
        }
        /* Add the persistent grant into the list of free grants */
        for (i = 0; i < num_grant; i++) {
-               if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
+               if (!gnttab_try_end_foreign_access(s->grants_used[i]->gref)) {
                        /*
                         * If the grant is still mapped by the backend (the
                         * backend has chosen to make this grant persistent)
                         * we add it at the head of the list, so it will be
                         * reused first.
                         */
-                       if (!info->feature_persistent)
-                               pr_alert_ratelimited("backed has not unmapped grant: %u\n",
-                                                    s->grants_used[i]->gref);
+                       if (!info->feature_persistent) {
+                               pr_alert("backed has not unmapped grant: %u\n",
+                                        s->grants_used[i]->gref);
+                               return -1;
+                       }
                        list_add(&s->grants_used[i]->node, &rinfo->grants);
                        rinfo->persistent_gnts_c++;
                } else {
                        /*
-                        * If the grant is not mapped by the backend we end the
-                        * foreign access and add it to the tail of the list,
-                        * so it will not be picked again unless we run out of
-                        * persistent grants.
+                        * If the grant is not mapped by the backend we add it
+                        * to the tail of the list, so it will not be picked
+                        * again unless we run out of persistent grants.
                         */
-                       gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
                        s->grants_used[i]->gref = GRANT_INVALID_REF;
                        list_add_tail(&s->grants_used[i]->node, &rinfo->grants);
                }
        }
        if (s->req.operation == BLKIF_OP_INDIRECT) {
                for (i = 0; i < INDIRECT_GREFS(num_grant); i++) {
-                       if (gnttab_query_foreign_access(s->indirect_grants[i]->gref)) {
-                               if (!info->feature_persistent)
-                                       pr_alert_ratelimited("backed has not unmapped grant: %u\n",
-                                                            s->indirect_grants[i]->gref);
+                       if (!gnttab_try_end_foreign_access(s->indirect_grants[i]->gref)) {
+                               if (!info->feature_persistent) {
+                                       pr_alert("backed has not unmapped grant: %u\n",
+                                                s->indirect_grants[i]->gref);
+                                       return -1;
+                               }
                                list_add(&s->indirect_grants[i]->node, &rinfo->grants);
                                rinfo->persistent_gnts_c++;
                        } else {
                                struct page *indirect_page;
 
-                               gnttab_end_foreign_access(s->indirect_grants[i]->gref, 0, 0UL);
                                /*
                                 * Add the used indirect page back to the list of
                                 * available pages for indirect grefs.
@@ -1498,7 +1506,7 @@ static bool blkif_completion(unsigned long *id,
                }
        }
 
-       return true;
+       return 1;
 }
 
 static irqreturn_t blkif_interrupt(int irq, void *dev_id)
@@ -1564,12 +1572,17 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
                }
 
                if (bret.operation != BLKIF_OP_DISCARD) {
+                       int ret;
+
                        /*
                         * We may need to wait for an extra response if the
                         * I/O request is split in 2
                         */
-                       if (!blkif_completion(&id, rinfo, &bret))
+                       ret = blkif_completion(&id, rinfo, &bret);
+                       if (!ret)
                                continue;
+                       if (unlikely(ret < 0))
+                               goto err;
                }
 
                if (add_id_to_freelist(rinfo, id)) {
@@ -1676,8 +1689,7 @@ static int setup_blkring(struct xenbus_device *dev,
        for (i = 0; i < info->nr_ring_pages; i++)
                rinfo->ring_ref[i] = GRANT_INVALID_REF;
 
-       sring = (struct blkif_sring *)__get_free_pages(GFP_NOIO | __GFP_HIGH,
-                                                      get_order(ring_size));
+       sring = alloc_pages_exact(ring_size, GFP_NOIO);
        if (!sring) {
                xenbus_dev_fatal(dev, -ENOMEM, "allocating shared ring");
                return -ENOMEM;
@@ -1687,7 +1699,7 @@ static int setup_blkring(struct xenbus_device *dev,
 
        err = xenbus_grant_ring(dev, rinfo->ring.sring, info->nr_ring_pages, gref);
        if (err < 0) {
-               free_pages((unsigned long)sring, get_order(ring_size));
+               free_pages_exact(sring, ring_size);
                rinfo->ring.sring = NULL;
                goto fail;
        }
@@ -2532,11 +2544,10 @@ static void purge_persistent_grants(struct blkfront_info *info)
                list_for_each_entry_safe(gnt_list_entry, tmp, &rinfo->grants,
                                         node) {
                        if (gnt_list_entry->gref == GRANT_INVALID_REF ||
-                           gnttab_query_foreign_access(gnt_list_entry->gref))
+                           !gnttab_try_end_foreign_access(gnt_list_entry->gref))
                                continue;
 
                        list_del(&gnt_list_entry->node);
-                       gnttab_end_foreign_access(gnt_list_entry->gref, 0, 0UL);
                        rinfo->persistent_gnts_c--;
                        gnt_list_entry->gref = GRANT_INVALID_REF;
                        list_add_tail(&gnt_list_entry->node, &rinfo->grants);