[PATCH] xen/pvcalls: bound backend response req_id before indexing rsp[]

[PATCH] xen/pvcalls: bound backend response req_id before indexing rsp[]

[Michael Bommarito]

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. */
+			bedata->ring.rsp_cons++;
+			done = 1;
+			continue;
+		}
 		if (rsp->cmd == PVCALLS_POLL) {
 			struct sock_mapping *map = (struct sock_mapping *)(uintptr_t)
 						   rsp->u.poll.id;
-- 
2.53.0

Re: [PATCH] xen/pvcalls: bound backend response req_id before indexing rsp[]

[Juergen Gross]

[-- 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 --]

Re: [PATCH] xen/pvcalls: bound backend response req_id before indexing rsp[]

[Michael Bommarito]

On Wed, Jun 10, 2026 at 7:48 AM Juergen Gross <jgross@suse.com> wrote:
> 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.

Good points.  Do you know if there is a canonical way to track
bad/buggy backends I should reference or reuse?

Re: [PATCH] xen/pvcalls: bound backend response req_id before indexing rsp[]

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 564 bytes --]

On 10.06.26 13:50, Michael Bommarito wrote:
> On Wed, Jun 10, 2026 at 7:48 AM Juergen Gross <jgross@suse.com> wrote:
>> 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.
> 
> Good points.  Do you know if there is a canonical way to track
> bad/buggy backends I should reference or reuse?

You could follow the xen-netback example (see xenvif_fatal_tx_err() and its
callers in drivers/net/xen-netback/netback.c).


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 --]

[PATCH v2] xen/pvcalls: bound backend response req_id before indexing rsp[]

[Michael Bommarito]

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

Re: [PATCH v2] xen/pvcalls: bound backend response req_id before indexing rsp[]

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1673 bytes --]

On 17.06.26 03: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 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>

Reviewed-by: Juergen Gross <jgross@suse.com>

Thanks,


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 --]