xen/netfront: react properly to failing gnttab_end_foreign_access_ref()

Commit 66e3531b33ee51dad17c463b4d9c9f52e341503d upstream.

When calling gnttab_end_foreign_access_ref() the returned value must
be tested and the reaction to that value should be appropriate.

In case of failure in xennet_get_responses() the reaction should not be
to crash the system, but to disable the network device.

The calls in setup_netfront() can be replaced by calls of
gnttab_end_foreign_access(). While at it avoid double free of ring
pages and grant references via xennet_disconnect_backend() in this case.

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

Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 54645797e810a739b224751b22ddc1c9993135ce..82dcd44b3e5e2ba8ee0063d6a1783ad8b19e9a9c 100644 (file)
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -838,7 +838,6 @@ static int xennet_get_responses(struct netfront_queue *queue,
        int max = XEN_NETIF_NR_SLOTS_MIN + (rx->status <= RX_COPY_THRESHOLD);
        int slots = 1;
        int err = 0;
-       unsigned long ret;
 
        if (rx->flags & XEN_NETRXF_extra_info) {
                err = xennet_get_extras(queue, extras, rp);
@@ -869,8 +868,13 @@ static int xennet_get_responses(struct netfront_queue *queue,
                        goto next;
                }
 
-               ret = gnttab_end_foreign_access_ref(ref, 0);
-               BUG_ON(!ret);
+               if (!gnttab_end_foreign_access_ref(ref, 0)) {
+                       dev_alert(dev,
+                                 "Grant still in use by backend domain\n");
+                       queue->info->broken = true;
+                       dev_alert(dev, "Disabled for further use\n");
+                       return -EINVAL;
+               }
 
                gnttab_release_grant_reference(&queue->gref_rx_head, ref);
 
@@ -1074,6 +1078,10 @@ static int xennet_poll(struct napi_struct *napi, int budget)
                err = xennet_get_responses(queue, &rinfo, rp, &tmpq);
 
                if (unlikely(err)) {
+                       if (queue->info->broken) {
+                               spin_unlock(&queue->rx_lock);
+                               return 0;
+                       }
 err:
                        while ((skb = __skb_dequeue(&tmpq)))
                                __skb_queue_tail(&errq, skb);
@@ -1671,7 +1679,7 @@ static int setup_netfront(struct xenbus_device *dev,
                        struct netfront_queue *queue, unsigned int feature_split_evtchn)
 {
        struct xen_netif_tx_sring *txs;
-       struct xen_netif_rx_sring *rxs;
+       struct xen_netif_rx_sring *rxs = NULL;
        grant_ref_t gref;
        int err;
 
@@ -1691,21 +1699,21 @@ static int setup_netfront(struct xenbus_device *dev,
 
        err = xenbus_grant_ring(dev, txs, 1, &gref);
        if (err < 0)
-               goto grant_tx_ring_fail;
+               goto fail;
        queue->tx_ring_ref = gref;
 
        rxs = (struct xen_netif_rx_sring *)get_zeroed_page(GFP_NOIO | __GFP_HIGH);
        if (!rxs) {
                err = -ENOMEM;
                xenbus_dev_fatal(dev, err, "allocating rx ring page");
-               goto alloc_rx_ring_fail;
+               goto fail;
        }
        SHARED_RING_INIT(rxs);
        FRONT_RING_INIT(&queue->rx, rxs, XEN_PAGE_SIZE);
 
        err = xenbus_grant_ring(dev, rxs, 1, &gref);
        if (err < 0)
-               goto grant_rx_ring_fail;
+               goto fail;
        queue->rx_ring_ref = gref;
 
        if (feature_split_evtchn)
@@ -1718,22 +1726,28 @@ static int setup_netfront(struct xenbus_device *dev,
                err = setup_netfront_single(queue);
 
        if (err)
-               goto alloc_evtchn_fail;
+               goto fail;
 
        return 0;
 
        /* If we fail to setup netfront, it is safe to just revoke access to
         * granted pages because backend is not accessing it at this point.
         */
-alloc_evtchn_fail:
-       gnttab_end_foreign_access_ref(queue->rx_ring_ref, 0);
-grant_rx_ring_fail:
-       free_page((unsigned long)rxs);
-alloc_rx_ring_fail:
-       gnttab_end_foreign_access_ref(queue->tx_ring_ref, 0);
-grant_tx_ring_fail:
-       free_page((unsigned long)txs);
-fail:
+ fail:
+       if (queue->rx_ring_ref != GRANT_INVALID_REF) {
+               gnttab_end_foreign_access(queue->rx_ring_ref, 0,
+                                         (unsigned long)rxs);
+               queue->rx_ring_ref = GRANT_INVALID_REF;
+       } else {
+               free_page((unsigned long)rxs);
+       }
+       if (queue->tx_ring_ref != GRANT_INVALID_REF) {
+               gnttab_end_foreign_access(queue->tx_ring_ref, 0,
+                                         (unsigned long)txs);
+               queue->tx_ring_ref = GRANT_INVALID_REF;
+       } else {
+               free_page((unsigned long)txs);
+       }
        return err;
 }