Marek Marczykowski-Górecki
9 years ago
parent
f40e68d1cb
commit
283f837b72
@ -0,0 +1,57 @@
|
||||
From 4e2bc423e0cef0a42f93d989c0980301df1bd462 Mon Sep 17 00:00:00 2001
|
||||
From: David Vrabel <david.vrabel@citrix.com>
|
||||
Date: Fri, 30 Oct 2015 14:58:08 +0000
|
||||
Subject: [PATCH 1/7] xen: Add RING_COPY_REQUEST()
|
||||
|
||||
Using RING_GET_REQUEST() on a shared ring is easy to use incorrectly
|
||||
(i.e., by not considering that the other end may alter the data in the
|
||||
shared ring while it is being inspected). Safe usage of a request
|
||||
generally requires taking a local copy.
|
||||
|
||||
Provide a RING_COPY_REQUEST() macro to use instead of
|
||||
RING_GET_REQUEST() and an open-coded memcpy(). This takes care of
|
||||
ensuring that the copy is done correctly regardless of any possible
|
||||
compiler optimizations.
|
||||
|
||||
Use a volatile source to prevent the compiler from reordering or
|
||||
omitting the copy.
|
||||
|
||||
This is part of XSA155.
|
||||
|
||||
CC: stable@vger.kernel.org
|
||||
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
|
||||
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
|
||||
---
|
||||
v2: Update about GCC and bitfields.
|
||||
---
|
||||
include/xen/interface/io/ring.h | 14 ++++++++++++++
|
||||
1 file changed, 14 insertions(+)
|
||||
|
||||
diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
|
||||
index 7d28aff..7dc685b 100644
|
||||
--- a/include/xen/interface/io/ring.h
|
||||
+++ b/include/xen/interface/io/ring.h
|
||||
@@ -181,6 +181,20 @@ struct __name##_back_ring { \
|
||||
#define RING_GET_REQUEST(_r, _idx) \
|
||||
(&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
|
||||
|
||||
+/*
|
||||
+ * Get a local copy of a request.
|
||||
+ *
|
||||
+ * Use this in preference to RING_GET_REQUEST() so all processing is
|
||||
+ * done on a local copy that cannot be modified by the other end.
|
||||
+ *
|
||||
+ * Note that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 may cause this
|
||||
+ * to be ineffective where _req is a struct which consists of only bitfields.
|
||||
+ */
|
||||
+#define RING_COPY_REQUEST(_r, _idx, _req) do { \
|
||||
+ /* Use volatile to force the copy into _req. */ \
|
||||
+ *(_req) = *(volatile typeof(_req))RING_GET_REQUEST(_r, _idx); \
|
||||
+} while (0)
|
||||
+
|
||||
#define RING_GET_RESPONSE(_r, _idx) \
|
||||
(&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
|
||||
|
||||
--
|
||||
2.1.0
|
||||
|
||||
@ -0,0 +1,54 @@
|
||||
From 084b8c2e77f1ac07e4a3a121ff957c49a9379385 Mon Sep 17 00:00:00 2001
|
||||
From: =?UTF-8?q?Roger=20Pau=20Monn=C3=A9?= <roger.pau@citrix.com>
|
||||
Date: Tue, 3 Nov 2015 16:34:09 +0000
|
||||
Subject: [PATCH 4/7] xen-blkback: only read request operation from shared ring
|
||||
once
|
||||
MIME-Version: 1.0
|
||||
Content-Type: text/plain; charset=UTF-8
|
||||
Content-Transfer-Encoding: 8bit
|
||||
|
||||
A compiler may load a switch statement value multiple times, which could
|
||||
be bad when the value is in memory shared with the frontend.
|
||||
|
||||
When converting a non-native request to a native one, ensure that
|
||||
src->operation is only loaded once by using READ_ONCE().
|
||||
|
||||
This is part of XSA155.
|
||||
|
||||
CC: stable@vger.kernel.org
|
||||
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
|
||||
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
|
||||
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
|
||||
---
|
||||
drivers/block/xen-blkback/common.h | 8 ++++----
|
||||
1 file changed, 4 insertions(+), 4 deletions(-)
|
||||
|
||||
diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
|
||||
index 68e87a0..c929ae2 100644
|
||||
--- a/drivers/block/xen-blkback/common.h
|
||||
+++ b/drivers/block/xen-blkback/common.h
|
||||
@@ -408,8 +408,8 @@ static inline void blkif_get_x86_32_req(struct blkif_request *dst,
|
||||
struct blkif_x86_32_request *src)
|
||||
{
|
||||
int i, n = BLKIF_MAX_SEGMENTS_PER_REQUEST, j;
|
||||
- dst->operation = src->operation;
|
||||
- switch (src->operation) {
|
||||
+ dst->operation = ACCESS_ONCE(src->operation);
|
||||
+ switch (dst->operation) {
|
||||
case BLKIF_OP_READ:
|
||||
case BLKIF_OP_WRITE:
|
||||
case BLKIF_OP_WRITE_BARRIER:
|
||||
@@ -456,8 +456,8 @@ static inline void blkif_get_x86_64_req(struct blkif_request *dst,
|
||||
struct blkif_x86_64_request *src)
|
||||
{
|
||||
int i, n = BLKIF_MAX_SEGMENTS_PER_REQUEST, j;
|
||||
- dst->operation = src->operation;
|
||||
- switch (src->operation) {
|
||||
+ dst->operation = ACCESS_ONCE(src->operation);
|
||||
+ switch (dst->operation) {
|
||||
case BLKIF_OP_READ:
|
||||
case BLKIF_OP_WRITE:
|
||||
case BLKIF_OP_WRITE_BARRIER:
|
||||
--
|
||||
2.1.0
|
||||
|
||||
@ -0,0 +1,81 @@
|
||||
From f6f4388c917ce96b075a239a4535b8efc6064d14 Mon Sep 17 00:00:00 2001
|
||||
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
|
||||
Date: Mon, 16 Nov 2015 12:40:48 -0500
|
||||
Subject: [PATCH 7/7] xen/pciback: Save xen_pci_op commands before processing
|
||||
it
|
||||
|
||||
Double fetch vulnerabilities that happen when a variable is
|
||||
fetched twice from shared memory but a security check is only
|
||||
performed the first time.
|
||||
|
||||
The xen_pcibk_do_op function performs a switch statements on the op->cmd
|
||||
value which is stored in shared memory. Interestingly this can result
|
||||
in a double fetch vulnerability depending on the performed compiler
|
||||
optimization.
|
||||
|
||||
This patch fixes it by saving the xen_pci_op command before
|
||||
processing it. We also use 'barrier' to make sure that the
|
||||
compiler does not perform any optimization.
|
||||
|
||||
This is part of XSA155.
|
||||
|
||||
CC: stable@vger.kernel.org
|
||||
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
|
||||
Signed-off-by: Jan Beulich <JBeulich@suse.com>
|
||||
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
|
||||
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
|
||||
---
|
||||
drivers/xen/xen-pciback/pciback.h | 1 +
|
||||
drivers/xen/xen-pciback/pciback_ops.c | 15 ++++++++++++++-
|
||||
2 files changed, 15 insertions(+), 1 deletion(-)
|
||||
|
||||
diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
|
||||
index 58e38d5..4d529f3 100644
|
||||
--- a/drivers/xen/xen-pciback/pciback.h
|
||||
+++ b/drivers/xen/xen-pciback/pciback.h
|
||||
@@ -37,6 +37,7 @@ struct xen_pcibk_device {
|
||||
struct xen_pci_sharedinfo *sh_info;
|
||||
unsigned long flags;
|
||||
struct work_struct op_work;
|
||||
+ struct xen_pci_op op;
|
||||
};
|
||||
|
||||
struct xen_pcibk_dev_data {
|
||||
diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c
|
||||
index c4a0666..a0e0e3e 100644
|
||||
--- a/drivers/xen/xen-pciback/pciback_ops.c
|
||||
+++ b/drivers/xen/xen-pciback/pciback_ops.c
|
||||
@@ -298,9 +298,11 @@ void xen_pcibk_do_op(struct work_struct *data)
|
||||
container_of(data, struct xen_pcibk_device, op_work);
|
||||
struct pci_dev *dev;
|
||||
struct xen_pcibk_dev_data *dev_data = NULL;
|
||||
- struct xen_pci_op *op = &pdev->sh_info->op;
|
||||
+ struct xen_pci_op *op = &pdev->op;
|
||||
int test_intx = 0;
|
||||
|
||||
+ *op = pdev->sh_info->op;
|
||||
+ barrier();
|
||||
dev = xen_pcibk_get_pci_dev(pdev, op->domain, op->bus, op->devfn);
|
||||
|
||||
if (dev == NULL)
|
||||
@@ -342,6 +344,17 @@ void xen_pcibk_do_op(struct work_struct *data)
|
||||
if ((dev_data->enable_intx != test_intx))
|
||||
xen_pcibk_control_isr(dev, 0 /* no reset */);
|
||||
}
|
||||
+ pdev->sh_info->op.err = op->err;
|
||||
+ pdev->sh_info->op.value = op->value;
|
||||
+#ifdef CONFIG_PCI_MSI
|
||||
+ if (op->cmd == XEN_PCI_OP_enable_msix && op->err == 0) {
|
||||
+ unsigned int i;
|
||||
+
|
||||
+ for (i = 0; i < op->value; i++)
|
||||
+ pdev->sh_info->op.msix_entries[i].vector =
|
||||
+ op->msix_entries[i].vector;
|
||||
+ }
|
||||
+#endif
|
||||
/* Tell the driver domain that we're done. */
|
||||
wmb();
|
||||
clear_bit(_XEN_PCIF_active, (unsigned long *)&pdev->sh_info->flags);
|
||||
--
|
||||
2.1.0
|
||||
|
||||
@ -0,0 +1,40 @@
|
||||
From 100ac372a0e07ccc8c508c3884fa9020cfe08094 Mon Sep 17 00:00:00 2001
|
||||
From: David Vrabel <david.vrabel@citrix.com>
|
||||
Date: Fri, 30 Oct 2015 15:16:01 +0000
|
||||
Subject: [PATCH 2/7] xen-netback: don't use last request to determine minimum
|
||||
Tx credit
|
||||
|
||||
The last from guest transmitted request gives no indication about the
|
||||
minimum amount of credit that the guest might need to send a packet
|
||||
since the last packet might have been a small one.
|
||||
|
||||
Instead allow for the worst case 128 KiB packet.
|
||||
|
||||
This is part of XSA155.
|
||||
|
||||
CC: stable@vger.kernel.org
|
||||
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
|
||||
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
|
||||
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
|
||||
---
|
||||
drivers/net/xen-netback/netback.c | 4 +---
|
||||
1 file changed, 1 insertion(+), 3 deletions(-)
|
||||
|
||||
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
|
||||
index e481f37..b683581 100644
|
||||
--- a/drivers/net/xen-netback/netback.c
|
||||
+++ b/drivers/net/xen-netback/netback.c
|
||||
@@ -679,9 +679,7 @@ static void tx_add_credit(struct xenvif_queue *queue)
|
||||
* Allow a burst big enough to transmit a jumbo packet of up to 128kB.
|
||||
* Otherwise the interface can seize up due to insufficient credit.
|
||||
*/
|
||||
- max_burst = RING_GET_REQUEST(&vif->tx, vif->tx.req_cons)->size;
|
||||
- max_burst = min(max_burst, 131072UL);
|
||||
- max_burst = max(max_burst, vif->credit_bytes);
|
||||
+ max_burst = max(131072UL, vif->credit_bytes);
|
||||
|
||||
/* Take care that adding a new chunk of credit doesn't wrap to zero. */
|
||||
max_credit = vif->remaining_credit + vif->credit_bytes;
|
||||
--
|
||||
2.1.0
|
||||
|
||||
@ -0,0 +1,129 @@
|
||||
From 4127e9ccae0eda622421d21132846abdf74f66ed Mon Sep 17 00:00:00 2001
|
||||
From: David Vrabel <david.vrabel@citrix.com>
|
||||
Date: Fri, 30 Oct 2015 15:17:06 +0000
|
||||
Subject: [PATCH 3/7] xen-netback: use RING_COPY_REQUEST() throughout
|
||||
|
||||
Instead of open-coding memcpy()s and directly accessing Tx and Rx
|
||||
requests, use the new RING_COPY_REQUEST() that ensures the local copy
|
||||
is correct.
|
||||
|
||||
This is more than is strictly necessary for guest Rx requests since
|
||||
only the id and gref fields are used and it is harmless if the
|
||||
frontend modifies these.
|
||||
|
||||
This is part of XSA155.
|
||||
|
||||
CC: stable@vger.kernel.org
|
||||
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
|
||||
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
|
||||
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
|
||||
---
|
||||
drivers/net/xen-netback/netback.c | 30 ++++++++++++++----------------
|
||||
1 file changed, 14 insertions(+), 16 deletions(-)
|
||||
|
||||
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
|
||||
index b683581..1049c34 100644
|
||||
--- a/drivers/net/xen-netback/netback.c
|
||||
+++ b/drivers/net/xen-netback/netback.c
|
||||
@@ -258,17 +258,17 @@ static struct xenvif_rx_meta *get_next_rx_buffer(struct xenvif *vif,
|
||||
struct netrx_pending_operations *npo)
|
||||
{
|
||||
struct xenvif_rx_meta *meta;
|
||||
- struct xen_netif_rx_request *req;
|
||||
+ struct xen_netif_rx_request req;
|
||||
|
||||
- req = RING_GET_REQUEST(&vif->rx, vif->rx.req_cons++);
|
||||
+ RING_COPY_REQUEST(&vif->rx, vif->rx.req_cons++, &req);
|
||||
|
||||
meta = npo->meta + npo->meta_prod++;
|
||||
meta->gso_size = 0;
|
||||
meta->size = 0;
|
||||
- meta->id = req->id;
|
||||
+ meta->id = req.id;
|
||||
|
||||
npo->copy_off = 0;
|
||||
- npo->copy_gref = req->gref;
|
||||
+ npo->copy_gref = req.gref;
|
||||
|
||||
return meta;
|
||||
}
|
||||
@@ -424,7 +424,7 @@ static int xenvif_gop_skb(struct sk_buff *skb,
|
||||
struct xenvif *vif = netdev_priv(skb->dev);
|
||||
int nr_frags = skb_shinfo(skb)->nr_frags;
|
||||
int i;
|
||||
- struct xen_netif_rx_request *req;
|
||||
+ struct xen_netif_rx_request req;
|
||||
struct xenvif_rx_meta *meta;
|
||||
unsigned char *data;
|
||||
int head = 1;
|
||||
@@ -443,14 +443,14 @@ static int xenvif_gop_skb(struct sk_buff *skb,
|
||||
|
||||
/* Set up a GSO prefix descriptor, if necessary */
|
||||
if (skb_shinfo(skb)->gso_size && vif->gso_prefix) {
|
||||
- req = RING_GET_REQUEST(&vif->rx, vif->rx.req_cons++);
|
||||
+ RING_COPY_REQUEST(&vif->rx, vif->rx.req_cons++, &req);
|
||||
meta = npo->meta + npo->meta_prod++;
|
||||
meta->gso_size = skb_shinfo(skb)->gso_size;
|
||||
meta->size = 0;
|
||||
- meta->id = req->id;
|
||||
+ meta->id = req.id;
|
||||
}
|
||||
|
||||
- req = RING_GET_REQUEST(&vif->rx, vif->rx.req_cons++);
|
||||
+ RING_COPY_REQUEST(&vif->rx, vif->rx.req_cons++, &req);
|
||||
meta = npo->meta + npo->meta_prod++;
|
||||
|
||||
if (!vif->gso_prefix)
|
||||
@@ -463,9 +463,9 @@ static int xenvif_gop_skb(struct sk_buff *skb,
|
||||
meta->gso_size = 0;
|
||||
|
||||
meta->size = 0;
|
||||
- meta->id = req->id;
|
||||
+ meta->id = req.id;
|
||||
npo->copy_off = 0;
|
||||
- npo->copy_gref = req->gref;
|
||||
+ npo->copy_gref = req.gref;
|
||||
|
||||
data = skb->data;
|
||||
while (data < skb_tail_pointer(skb)) {
|
||||
@@ -709,7 +709,7 @@ static void xenvif_tx_err(struct xenvif *vif,
|
||||
make_tx_response(vif, txp, XEN_NETIF_RSP_ERROR);
|
||||
if (cons == end)
|
||||
break;
|
||||
- txp = RING_GET_REQUEST(&vif->tx, cons++);
|
||||
+ RING_COPY_REQUEST(&vif->tx, cons++, txp);
|
||||
} while (1);
|
||||
vif->tx.req_cons = cons;
|
||||
}
|
||||
@@ -776,8 +776,7 @@ static int xenvif_count_requests(struct xenvif *vif,
|
||||
if (drop_err)
|
||||
txp = &dropped_tx;
|
||||
|
||||
- memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
|
||||
- sizeof(*txp));
|
||||
+ RING_COPY_REQUEST(&vif->tx, cons + slots, txp);
|
||||
|
||||
/* If the guest submitted a frame >= 64 KiB then
|
||||
* first->size overflowed and following slots will
|
||||
@@ -1110,8 +1109,7 @@ static int xenvif_get_extras(struct xenvif *vif,
|
||||
return -EBADR;
|
||||
}
|
||||
|
||||
- memcpy(&extra, RING_GET_REQUEST(&vif->tx, cons),
|
||||
- sizeof(extra));
|
||||
+ RING_COPY_REQUEST(&vif->tx, cons, &extra);
|
||||
if (unlikely(!extra.type ||
|
||||
extra.type >= XEN_NETIF_EXTRA_TYPE_MAX)) {
|
||||
vif->tx.req_cons = ++cons;
|
||||
@@ -1320,7 +1318,7 @@ static void xenvif_tx_build_gops(struct xenvif *vif,
|
||||
|
||||
idx = vif->tx.req_cons;
|
||||
rmb(); /* Ensure that we see the request before we copy it. */
|
||||
- memcpy(&txreq, RING_GET_REQUEST(&vif->tx, idx), sizeof(txreq));
|
||||
+ RING_COPY_REQUEST(&vif->tx, idx, &txreq);
|
||||
|
||||
/* Credit-based scheduling. */
|
||||
if (txreq.size > vif->remaining_credit &&
|
||||
--
|
||||
2.1.0
|
||||
|
||||
@ -0,0 +1,65 @@
|
||||
From d52f00960c1070c683809faddd35a2223e2b8a6e Mon Sep 17 00:00:00 2001
|
||||
From: =?UTF-8?q?Roger=20Pau=20Monn=C3=A9?= <roger.pau@citrix.com>
|
||||
Date: Tue, 3 Nov 2015 16:40:43 +0000
|
||||
Subject: [PATCH 6/7] xen-blkback: read from indirect descriptors only once
|
||||
MIME-Version: 1.0
|
||||
Content-Type: text/plain; charset=UTF-8
|
||||
Content-Transfer-Encoding: 8bit
|
||||
|
||||
Since indirect descriptors are in memory shared with the frontend, the
|
||||
frontend could alter the first_sect and last_sect values after they have
|
||||
been validated but before they are recorded in the request. This may
|
||||
result in I/O requests that overflow the foreign page, possibly
|
||||
overwriting local pages when the I/O request is executed.
|
||||
|
||||
When parsing indirect descriptors, only read first_sect and last_sect
|
||||
once.
|
||||
|
||||
This is part of XSA155.
|
||||
|
||||
CC: stable@vger.kernel.org
|
||||
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
|
||||
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
|
||||
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
|
||||
----
|
||||
v2: This is against v4.3
|
||||
---
|
||||
drivers/block/xen-blkback/blkback.c | 12 +++++++-----
|
||||
1 file changed, 7 insertions(+), 5 deletions(-)
|
||||
|
||||
diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
|
||||
index 6a685ae..f2e7a38 100644
|
||||
--- a/drivers/block/xen-blkback/blkback.c
|
||||
+++ b/drivers/block/xen-blkback/blkback.c
|
||||
@@ -950,6 +950,8 @@ static int xen_blkbk_parse_indirect(struct blkif_request *req,
|
||||
goto unmap;
|
||||
|
||||
for (n = 0, i = 0; n < nseg; n++) {
|
||||
+ uint8_t first_sect, last_sect;
|
||||
+
|
||||
if ((n % SEGS_PER_INDIRECT_FRAME) == 0) {
|
||||
/* Map indirect segments */
|
||||
if (segments)
|
||||
@@ -958,14 +960,14 @@ static int xen_blkbk_parse_indirect(struct blkif_request *req,
|
||||
}
|
||||
i = n % SEGS_PER_INDIRECT_FRAME;
|
||||
pending_req->segments[n]->gref = segments[i].gref;
|
||||
- seg[n].nsec = segments[i].last_sect -
|
||||
- segments[i].first_sect + 1;
|
||||
- seg[n].offset = (segments[i].first_sect << 9);
|
||||
- if ((segments[i].last_sect >= (PAGE_SIZE >> 9)) ||
|
||||
- (segments[i].last_sect < segments[i].first_sect)) {
|
||||
+ first_sect = ACCESS_ONCE(segments[i].first_sect);
|
||||
+ last_sect = ACCESS_ONCE(segments[i].last_sect);
|
||||
+ if (last_sect >= (PAGE_SIZE >> 9) || last_sect < first_sect) {
|
||||
rc = -EINVAL;
|
||||
goto unmap;
|
||||
}
|
||||
+ seg[n].nsec = last_sect - first_sect + 1;
|
||||
+ seg[n].offset = first_sect << 9;
|
||||
preq->nr_sects += seg[n].nsec;
|
||||
}
|
||||
|
||||
--
|
||||
2.1.0
|
||||
|
||||
Loading…
Reference in new issue