* Re: [PATCH] xen/pvcalls: bound backend response req_id before indexing rsp[]
2026-06-10 11:41 [PATCH] xen/pvcalls: bound backend response req_id before indexing rsp[] Michael Bommarito
@ 2026-06-10 11:48 ` Juergen Gross
2026-06-10 11:50 ` Michael Bommarito
2026-06-17 1:41 ` [PATCH v2] " Michael Bommarito
1 sibling, 1 reply; 6+ messages in thread
From: Juergen Gross @ 2026-06-10 11:48 UTC (permalink / raw)
To: Michael Bommarito, Stefano Stabellini, Oleksandr Tyshchenko
Cc: xen-devel, linux-kernel
[-- Attachment #1.1.1: Type: text/plain, Size: 2387 bytes --]
On 10.06.26 13:41, Michael Bommarito wrote:
> 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 is insufficient on its own: a backend
> req_id of 0xffffffff becomes -1, passes the >= PVCALLS_NR_RSP_PER_RING
> test, and indexes bedata->rsp[-1], an out-of-bounds write to the left of
> the array. Declare req_id as u32 and add the range check so both ends of
> the index are covered.
>
> 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).
> Reject responses whose req_id is out of range.
>
> Fixes: 235a71c53903 ("xen/pvcalls: implement release command")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
> drivers/xen/pvcalls-front.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
> index 50ce4820f7eeb..78bd4e894b32e 100644
> --- a/drivers/xen/pvcalls-front.c
> +++ b/drivers/xen/pvcalls-front.c
> @@ -168,7 +168,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;
> @@ -185,6 +186,12 @@ static irqreturn_t pvcalls_front_event_handler(int irq, void *dev_id)
> rsp = RING_GET_RESPONSE(&bedata->ring, bedata->ring.rsp_cons);
>
> req_id = rsp->req_id;
> + if (req_id >= PVCALLS_NR_RSP_PER_RING) {
> + /* Malicious or buggy backend: req_id out of range. */
Please add an error message here, using pr_err_once().
Instead of just ignoring such responses, I'd consider stopping communication
with the backend for this device.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] xen/pvcalls: bound backend response req_id before indexing rsp[]
2026-06-10 11:41 [PATCH] xen/pvcalls: bound backend response req_id before indexing rsp[] Michael Bommarito
2026-06-10 11:48 ` Juergen Gross
@ 2026-06-17 1:41 ` Michael Bommarito
2026-06-26 11:52 ` Juergen Gross
1 sibling, 1 reply; 6+ messages in thread
From: Michael Bommarito @ 2026-06-17 1:41 UTC (permalink / raw)
To: Juergen Gross, Stefano Stabellini, Oleksandr Tyshchenko
Cc: xen-devel, linux-kernel
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@suse.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
v2: per Juergen Gross's review
(https://lore.kernel.org/all/ecb43fc6-e821-4532-9f75-06c86a6ac76c@suse.com/):
- 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@gmail.com/
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
^ permalink raw reply [flat|nested] 6+ messages in thread