|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [PATCH v2] xen/pvcalls: bound backend response req_id before indexing rsp[]
pvcalls_front_event_handler() takes req_id directly from the
backend-supplied ring response and uses it to index the fixed-size
bedata->rsp[] array for a memcpy() and a store, with no range check. A
malicious or buggy backend can set req_id past PVCALLS_NR_RSP_PER_RING
and drive an out-of-bounds write past the bedata allocation.
req_id was also declared int while the wire field rsp->req_id is u32, so
a range check on the signed value alone is insufficient: a backend
req_id of 0xffffffff becomes -1, passes a >= PVCALLS_NR_RSP_PER_RING
test and indexes bedata->rsp[-1]. Declare req_id as u32 so a single
bound covers both ends.
A backend that sends an out-of-range req_id has violated the wire
protocol, so rather than silently dropping the response, log once and
stop trusting the backend: set bedata->disabled. The event handler then
ignores further responses, and the request paths that wait for a
response return -EIO instead of blocking forever. This mirrors the
fatal-error handling xen-netback uses (xenvif_fatal_tx_err()).
The pvcalls frontend currently trusts its backend, so this is not a
classic-Xen security issue, but it matters for hardening PV frontends
against malicious backends (confidential and disaggregated deployments).
Fixes: 2195046bfd69 ("xen/pvcalls: implement socket command and handle events")
Suggested-by: Juergen Gross <jgross@xxxxxxxx>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@xxxxxxxxx>
---
v2: per Juergen Gross's review
(https://lore.kernel.org/all/ecb43fc6-e821-4532-9f75-06c86a6ac76c@xxxxxxxx/):
- Log the out-of-range req_id once with pr_err_once() instead of
silently dropping the response.
- Stop trusting the backend on a protocol violation: set
bedata->disabled so the event handler ignores further responses and
the request paths waiting for a response return -EIO instead of
blocking forever, following the xen-netback xenvif_fatal_tx_err()
pattern you pointed to.
- Declare req_id as u32 (was int) so a single bound covers both ends.
- pvcalls_front_accept() has a second waiter (a concurrent accept
blocked on PVCALLS_FLAG_ACCEPT_INFLIGHT). On the disabled path,
clear that flag and wake inflight_accept_req so the queued accept
also returns -EIO rather than waiting for a response that the
disabled handler will never deliver.
- Corrected the Fixes: tag to 2195046bfd69, the commit that
introduced the unbounded bedata->rsp[req_id] indexing in the event
handler; the previously cited 235a71c53903 (release command) only
added a waiter and is a descendant.
v1:
https://lore.kernel.org/all/20260610114137.3749027-1-michael.bommarito@xxxxxxxxx/
drivers/xen/pvcalls-front.c | 88 ++++++++++++++++++++++++++++++++-----
1 file changed, 76 insertions(+), 12 deletions(-)
diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
index 50ce4820f7eeb..3e7aa807c3173 100644
--- a/drivers/xen/pvcalls-front.c
+++ b/drivers/xen/pvcalls-front.c
@@ -32,6 +32,7 @@ struct pvcalls_bedata {
struct xen_pvcalls_front_ring ring;
grant_ref_t ref;
int irq;
+ bool disabled;
struct list_head socket_mappings;
spinlock_t socket_lock;
@@ -131,6 +132,20 @@ static inline int get_request(struct pvcalls_bedata
*bedata, int *req_id)
return 0;
}
+/*
+ * Wait for the backend's response to req_id, or for the frontend to be
+ * disabled because the backend violated the wire protocol. Returns 0 once
+ * the response has arrived, or -EIO if the frontend was disabled.
+ */
+static int pvcalls_front_wait_rsp(struct pvcalls_bedata *bedata, u32 req_id)
+{
+ wait_event(bedata->inflight_req,
+ READ_ONCE(bedata->rsp[req_id].req_id) == req_id ||
+ READ_ONCE(bedata->disabled));
+
+ return READ_ONCE(bedata->disabled) ? -EIO : 0;
+}
+
static bool pvcalls_front_write_todo(struct sock_mapping *map)
{
struct pvcalls_data_intf *intf = map->active.ring;
@@ -168,7 +183,8 @@ static irqreturn_t pvcalls_front_event_handler(int irq,
void *dev_id)
struct pvcalls_bedata *bedata;
struct xen_pvcalls_response *rsp;
uint8_t *src, *dst;
- int req_id = 0, more = 0, done = 0;
+ u32 req_id = 0;
+ int more = 0, done = 0;
if (dev == NULL)
return IRQ_HANDLED;
@@ -179,12 +195,31 @@ static irqreturn_t pvcalls_front_event_handler(int irq,
void *dev_id)
pvcalls_exit();
return IRQ_HANDLED;
}
+ if (READ_ONCE(bedata->disabled)) {
+ pvcalls_exit();
+ return IRQ_HANDLED;
+ }
again:
while (RING_HAS_UNCONSUMED_RESPONSES(&bedata->ring)) {
rsp = RING_GET_RESPONSE(&bedata->ring, bedata->ring.rsp_cons);
req_id = rsp->req_id;
+ if (req_id >= PVCALLS_NR_RSP_PER_RING) {
+ /*
+ * The backend supplied a req_id that would index
+ * bedata->rsp[] out of bounds: a protocol violation
+ * from a malicious or buggy backend. Log once, stop
+ * trusting this backend and disable the frontend rather
+ * than silently dropping the response and continuing.
+ */
+ pr_err_once("pvcalls: backend sent out-of-range req_id
%u, disabling frontend\n",
+ req_id);
+ WRITE_ONCE(bedata->disabled, true);
+ bedata->ring.rsp_cons++;
+ done = 1;
+ break;
+ }
if (rsp->cmd == PVCALLS_POLL) {
struct sock_mapping *map = (struct sock_mapping
*)(uintptr_t)
rsp->u.poll.id;
@@ -217,7 +252,7 @@ static irqreturn_t pvcalls_front_event_handler(int irq,
void *dev_id)
}
RING_FINAL_CHECK_FOR_RESPONSES(&bedata->ring, more);
- if (more)
+ if (more && !READ_ONCE(bedata->disabled))
goto again;
if (done)
wake_up(&bedata->inflight_req);
@@ -330,8 +365,11 @@ int pvcalls_front_socket(struct socket *sock)
if (notify)
notify_remote_via_irq(bedata->irq);
- wait_event(bedata->inflight_req,
- READ_ONCE(bedata->rsp[req_id].req_id) == req_id);
+ ret = pvcalls_front_wait_rsp(bedata, req_id);
+ if (ret) {
+ pvcalls_exit();
+ return ret;
+ }
/* read req_id, then the content */
smp_rmb();
@@ -477,8 +515,11 @@ int pvcalls_front_connect(struct socket *sock, struct
sockaddr *addr,
if (notify)
notify_remote_via_irq(bedata->irq);
- wait_event(bedata->inflight_req,
- READ_ONCE(bedata->rsp[req_id].req_id) == req_id);
+ ret = pvcalls_front_wait_rsp(bedata, req_id);
+ if (ret) {
+ pvcalls_exit_sock(sock);
+ return ret;
+ }
/* read req_id, then the content */
smp_rmb();
@@ -711,8 +752,11 @@ int pvcalls_front_bind(struct socket *sock, struct
sockaddr *addr, int addr_len)
if (notify)
notify_remote_via_irq(bedata->irq);
- wait_event(bedata->inflight_req,
- READ_ONCE(bedata->rsp[req_id].req_id) == req_id);
+ ret = pvcalls_front_wait_rsp(bedata, req_id);
+ if (ret) {
+ pvcalls_exit_sock(sock);
+ return ret;
+ }
/* read req_id, then the content */
smp_rmb();
@@ -761,8 +805,11 @@ int pvcalls_front_listen(struct socket *sock, int backlog)
if (notify)
notify_remote_via_irq(bedata->irq);
- wait_event(bedata->inflight_req,
- READ_ONCE(bedata->rsp[req_id].req_id) == req_id);
+ ret = pvcalls_front_wait_rsp(bedata, req_id);
+ if (ret) {
+ pvcalls_exit_sock(sock);
+ return ret;
+ }
/* read req_id, then the content */
smp_rmb();
@@ -820,6 +867,14 @@ int pvcalls_front_accept(struct socket *sock, struct
socket *newsock,
}
}
+ if (READ_ONCE(bedata->disabled)) {
+ clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT,
+ (void *)&map->passive.flags);
+ wake_up(&map->passive.inflight_accept_req);
+ pvcalls_exit_sock(sock);
+ return -EIO;
+ }
+
map2 = kzalloc_obj(*map2);
if (map2 == NULL) {
clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT,
@@ -880,10 +935,18 @@ int pvcalls_front_accept(struct socket *sock, struct
socket *newsock,
}
if (wait_event_interruptible(bedata->inflight_req,
- READ_ONCE(bedata->rsp[req_id].req_id) == req_id)) {
+ READ_ONCE(bedata->rsp[req_id].req_id) == req_id ||
+ READ_ONCE(bedata->disabled))) {
pvcalls_exit_sock(sock);
return -EINTR;
}
+ if (READ_ONCE(bedata->disabled)) {
+ clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT,
+ (void *)&map->passive.flags);
+ wake_up(&map->passive.inflight_accept_req);
+ pvcalls_exit_sock(sock);
+ return -EIO;
+ }
/* read req_id, then the content */
smp_rmb();
@@ -1054,7 +1117,8 @@ int pvcalls_front_release(struct socket *sock)
notify_remote_via_irq(bedata->irq);
wait_event(bedata->inflight_req,
- READ_ONCE(bedata->rsp[req_id].req_id) == req_id);
+ READ_ONCE(bedata->rsp[req_id].req_id) == req_id ||
+ READ_ONCE(bedata->disabled));
if (map->active_socket) {
/*
--
2.53.0
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |