Re: [PATCH 6/8] xen/netfront: don't read data from request on the ring page
From: Jan Beulich <hidden>
Date: 2021-05-17 15:21:50
Also in:
lkml, xen-devel
On 13.05.2021 12:03, Juergen Gross wrote:
In order to avoid a malicious backend being able to influence the local processing of a request build the request locally first and then copy it to the ring page. Any reading from the request needs to be done on the local instance.
"Any reading" isn't really true - you don't change xennet_make_one_txreq(), yet that has a read-modify-write operation. Without that I would have been inclined to ask whether ...
quoted hunk ↗ jump to hunk
--- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c@@ -435,7 +435,8 @@ struct xennet_gnttab_make_txreq { struct netfront_queue *queue; struct sk_buff *skb; struct page *page; - struct xen_netif_tx_request *tx; /* Last request */ + struct xen_netif_tx_request *tx; /* Last request on ring page */ + struct xen_netif_tx_request tx_local; /* Last request local copy*/
... retaining the tx field here is a good idea.
quoted hunk ↗ jump to hunk
@@ -463,30 +464,27 @@ static void xennet_tx_setup_grant(unsigned long gfn, unsigned int offset, queue->grant_tx_page[id] = page; queue->grant_tx_ref[id] = ref; - tx->id = id; - tx->gref = ref; - tx->offset = offset; - tx->size = len; - tx->flags = 0; + info->tx_local.id = id; + info->tx_local.gref = ref; + info->tx_local.offset = offset; + info->tx_local.size = len; + info->tx_local.flags = 0; + + *tx = info->tx_local; info->tx = tx; - info->size += tx->size; + info->size += info->tx_local.size; } static struct xen_netif_tx_request *xennet_make_first_txreq( - struct netfront_queue *queue, struct sk_buff *skb, - struct page *page, unsigned int offset, unsigned int len) + struct xennet_gnttab_make_txreq *info, + unsigned int offset, unsigned int len) { - struct xennet_gnttab_make_txreq info = { - .queue = queue, - .skb = skb, - .page = page, - .size = 0, - }; + info->size = 0; - gnttab_for_one_grant(page, offset, len, xennet_tx_setup_grant, &info); + gnttab_for_one_grant(info->page, offset, len, xennet_tx_setup_grant, info); - return info.tx; + return info->tx; }
Similarly this returning of a pointer into the ring looks at least risky to me. At the very least it looks as if ...
quoted hunk ↗ jump to hunk
@@ -704,14 +699,16 @@ static netdev_tx_t xennet_start_xmit(struct sk_buff *skb, struct net_device *dev } /* First request for the linear area. */ - first_tx = tx = xennet_make_first_txreq(queue, skb, - page, offset, len); + info.queue = queue; + info.skb = skb; + info.page = page; + first_tx = tx = xennet_make_first_txreq(&info, offset, len);
... you could avoid setting tx here; perhaps the local variable could go away altogether, showing it's really just first_rx that is still needed. It's odd that ...
offset += tx->size;
... you don't change this one, when ...
if (offset == PAGE_SIZE) {
page++;
offset = 0;
}
- len -= tx->size;
+ len -= info.tx_local.size;... you do so here. Likely just an oversight. Jan