From d33846c8dcc06b83b7acdeac1e8bfbb5c0c26cb2 Mon Sep 17 00:00:00 2001 From: Michael Bommarito Date: Tue, 16 Jun 2026 21:41:49 -0400 Subject: 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 Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Michael Bommarito Reviewed-by: Juergen Gross Signed-off-by: Juergen Gross Message-ID: <20260617014149.2647404-1-michael.bommarito@gmail.com> --- drivers/xen/pvcalls-front.c | 88 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 76 insertions(+), 12 deletions(-) (limited to 'drivers') diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c index 50ce4820f7ee..3e7aa807c317 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 @@ again: } 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) { /* -- cgit v1.2.3 From 45ca1afe2fd14c04e37227e79d3f8455831d8408 Mon Sep 17 00:00:00 2001 From: Wentao Liang Date: Mon, 22 Jun 2026 19:25:41 +0800 Subject: xen/gntdev: fix error handling in ioctl When gntdev_ioctl_map_grant_ref() fails to copy the operation result back to userspace after successfully adding the mapping to the list, the error path returns -EFAULT without releasing the reference acquired by gntdev_alloc_map(). The mapping remains in priv->maps with a refcount of 1, causing a memory leak and a dangling list entry. Additionally, gntdev_add_map() may modify map->index to avoid overlap with existing mappings. Therefore, the index returned to userspace must be obtained after gntdev_add_map() completes. Fix this by holding the mutex across gntdev_add_map(), retrieving the correct index, and copy_to_user(). If copy_to_user() fails, remove the mapping from the list and release the reference while still holding the lock. Cc: stable@vger.kernel.org Fix these issues by properly handling all error cases. Fixes: 1401c00e59ea ("xen/gntdev: convert priv->lock to a mutex") Fixes: 68b025c813c2 ("xen-gntdev: Add reference counting to maps") Signed-off-by: Wentao Liang Reviewed-by: Juergen Gross Signed-off-by: Juergen Gross Message-ID: <20260622112541.38194-1-vulab@iscas.ac.cn> --- drivers/xen/gntdev.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'drivers') diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 61ea855c4508..1dcc4675580e 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -670,11 +670,15 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv, mutex_lock(&priv->lock); gntdev_add_map(priv, map); op.index = map->index << PAGE_SHIFT; - mutex_unlock(&priv->lock); - if (copy_to_user(u, &op, sizeof(op)) != 0) + if (copy_to_user(u, &op, sizeof(op)) != 0) { + list_del(&map->next); + mutex_unlock(&priv->lock); + gntdev_put_map(priv, map); return -EFAULT; + } + mutex_unlock(&priv->lock); return 0; } -- cgit v1.2.3 From 678d59219ce0ae883f04c96936222c6168ef1164 Mon Sep 17 00:00:00 2001 From: Yousef Alhouseen Date: Mon, 29 Jun 2026 18:05:17 +0200 Subject: xen/front-pgdir-shbuf: free grant reference head on errors grant_references() allocates a private grant-reference head before claiming references for the page directory and, for guest-owned buffers, the data pages. The success path frees the remaining head, but claim failures and grant_refs_for_buffer() errors return immediately. Unwind through a common exit path so the private grant-reference head is released even when granting fails part-way through setup. The caller still tears down any references already stored in buf->grefs. Signed-off-by: Yousef Alhouseen Reviewed-by: Stefano Stabellini Signed-off-by: Juergen Gross Message-ID: <20260629160517.29340-1-alhouseenyousef@gmail.com> --- drivers/xen/xen-front-pgdir-shbuf.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'drivers') diff --git a/drivers/xen/xen-front-pgdir-shbuf.c b/drivers/xen/xen-front-pgdir-shbuf.c index 9c7d8af6e6a1..428187edf85d 100644 --- a/drivers/xen/xen-front-pgdir-shbuf.c +++ b/drivers/xen/xen-front-pgdir-shbuf.c @@ -445,8 +445,10 @@ static int grant_references(struct xen_front_pgdir_shbuf *buf) unsigned long frame; cur_ref = gnttab_claim_grant_reference(&priv_gref_head); - if (cur_ref < 0) - return cur_ref; + if (cur_ref < 0) { + ret = cur_ref; + goto out_free_refs; + } frame = xen_page_to_gfn(virt_to_page(buf->directory + PAGE_SIZE * i)); @@ -457,11 +459,13 @@ static int grant_references(struct xen_front_pgdir_shbuf *buf) if (buf->ops->grant_refs_for_buffer) { ret = buf->ops->grant_refs_for_buffer(buf, &priv_gref_head, j); if (ret) - return ret; + goto out_free_refs; } + ret = 0; +out_free_refs: gnttab_free_grant_references(priv_gref_head); - return 0; + return ret; } /* -- cgit v1.2.3 From 51d111301a3ad8e5655687cb6462182e5804fa02 Mon Sep 17 00:00:00 2001 From: Yousef Alhouseen Date: Sat, 27 Jun 2026 00:38:04 +0200 Subject: xen/gntalloc: make grant counters unsigned The module limit and current allocation count cannot validly be negative. Give both variables unsigned types so their representation matches the u32 grant count supplied through the ioctl and negative module parameter values are rejected by parameter parsing. This also prepares the limit check for overflow-safe unsigned arithmetic. Signed-off-by: Yousef Alhouseen Reviewed-by: Juergen Gross Signed-off-by: Juergen Gross Message-ID: <20260626223805.43781-2-alhouseenyousef@gmail.com> --- drivers/xen/gntalloc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c index eadedd1e963e..9279f1521b6f 100644 --- a/drivers/xen/gntalloc.c +++ b/drivers/xen/gntalloc.c @@ -70,14 +70,14 @@ #include #include -static int limit = 1024; -module_param(limit, int, 0644); +static unsigned int limit = 1024; +module_param(limit, uint, 0644); MODULE_PARM_DESC(limit, "Maximum number of grants that may be allocated by " "the gntalloc device"); static LIST_HEAD(gref_list); static DEFINE_MUTEX(gref_mutex); -static int gref_size; +static unsigned int gref_size; struct notify_info { uint16_t pgoff:12; /* Bits 0-11: Offset of the byte to clear */ -- cgit v1.2.3 From 2299822f3f466b5dcad2377bf63986199f881a6b Mon Sep 17 00:00:00 2001 From: Yousef Alhouseen Date: Sat, 27 Jun 2026 00:38:05 +0200 Subject: xen/gntalloc: validate grant count before allocation gntalloc_ioctl_alloc() allocates the grant-id array before checking whether the requested count fits within the global grant limit. Counts above that limit cannot succeed, so reject them before the user-controlled allocation reaches kcalloc(). Use a subtraction-based check while holding gref_mutex so adding the requested count cannot wrap. Also cast the count before advancing the per-file index so the page-size multiplication is performed in 64-bit arithmetic. Signed-off-by: Yousef Alhouseen Reviewed-by: Juergen Gross Signed-off-by: Juergen Gross Message-ID: <20260626223805.43781-3-alhouseenyousef@gmail.com> --- drivers/xen/gntalloc.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) (limited to 'drivers') diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c index 9279f1521b6f..3218686be45b 100644 --- a/drivers/xen/gntalloc.c +++ b/drivers/xen/gntalloc.c @@ -272,6 +272,7 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv, int rc = 0; struct ioctl_gntalloc_alloc_gref op; uint32_t *gref_ids; + unsigned int limit_snapshot; pr_debug("%s: priv %p\n", __func__, priv); @@ -280,6 +281,12 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv, goto out; } + limit_snapshot = READ_ONCE(limit); + if (op.count > limit_snapshot) { + rc = -ENOSPC; + goto out; + } + gref_ids = kcalloc(op.count, sizeof(gref_ids[0]), GFP_KERNEL); if (!gref_ids) { rc = -ENOMEM; @@ -292,14 +299,16 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv, * are about to enforce, removing them here is a good idea. */ do_cleanup(); - if (gref_size + op.count > limit) { + limit_snapshot = READ_ONCE(limit); + if (gref_size > limit_snapshot || + op.count > limit_snapshot - gref_size) { mutex_unlock(&gref_mutex); rc = -ENOSPC; goto out_free; } gref_size += op.count; op.index = priv->index; - priv->index += op.count * PAGE_SIZE; + priv->index += (uint64_t)op.count * PAGE_SIZE; mutex_unlock(&gref_mutex); rc = add_grefs(&op, gref_ids, priv); -- cgit v1.2.3 From cbbef43bdc083892a2d4787245c249502c215bb8 Mon Sep 17 00:00:00 2001 From: Yousef Alhouseen Date: Sat, 27 Jun 2026 00:37:38 +0200 Subject: xenbus: reject unterminated directory replies split_strings() walks each directory entry with strlen(). Although the transport adds a terminator after the reply buffer, a malformed reply without a final NUL inside its advertised length would let that walk cross the protocol payload boundary. Reject such replies before counting the strings. Report the protocol violation once and return -EIO to the caller. Signed-off-by: Yousef Alhouseen Reviewed-by: Juergen Gross Signed-off-by: Juergen Gross Message-ID: <20260626223738.43742-1-alhouseenyousef@gmail.com> --- drivers/xen/xenbus/xenbus_xs.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'drivers') diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c index c202e7c553a6..d1cca4acb6f3 100644 --- a/drivers/xen/xenbus/xenbus_xs.c +++ b/drivers/xen/xenbus/xenbus_xs.c @@ -417,6 +417,12 @@ static char **split_strings(char *strings, unsigned int len, unsigned int *num) { char *p, **ret; + if (len && strings[len - 1]) { + pr_err_once("malformed XS_DIRECTORY reply\n"); + kfree(strings); + return ERR_PTR(-EIO); + } + /* Count the strings. */ *num = count_strings(strings, len); -- cgit v1.2.3