[PATCH] xen-blkfront: fix double completion of split requests on resume

[PATCH] xen-blkfront: fix double completion of split requests on resume

[Doruk Tan Ozturk]

When a block request is too large for a single ring entry and the
backend does not support indirect descriptors, blkfront splits it
across two ring requests.  blkif_ring_get_request() is called twice
and both shadow slots (shadow[id] and shadow[extra_id]) are made to
point at the *same* struct request, linked together through
associated_id.

On the normal completion path blkif_completion() collapses the pair:
it recycles the second slot via add_id_to_freelist() and only completes
the request once.  The suspend/resume path in blkfront_resume() does
not.  It walks every physical shadow slot and, for each slot whose
->request is set, calls blk_mq_end_request() or re-queues
->request.  For an in-flight split request this visits the shared
struct request twice, so on resume/migration the same request is
ended (or re-queued) two times.  The second visit is a double
blk_mq_end_request() (refcount underflow / double free) and a
use-after-free read of req->bio, which was cleared on the first visit.

Skip the secondary slot of a split request in the resume walk, so each
logical request is completed or re-queued exactly once, matching how
blkif_completion() already treats the pair.  The secondary slot is the
one that is linked (associated_id != NO_ASSOCIATED_ID) and carries no
scatter-gather list (num_sg == 0); the first slot always keeps the
scatter-gather list.

This was found by 0sec automated security-research tooling
(https://0sec.ai).  The bug is only reachable on suspend/resume or live
migration of a guest whose backend lacks indirect-descriptor support, so
it has no local reproducer; the fix is by source inspection against the
existing blkif_completion() collapse logic.

Fixes: 6cc568339047 ("xen/blkfront: Handle non-indirect grant with 64KB pages")
Assisted-by: 0sec:claude-opus-4-8
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
 drivers/block/xen-blkfront.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index f765970578f9..b2e83fd0c77b 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -2079,6 +2079,15 @@ static int blkfront_resume(struct xenbus_device *dev)
 			if (!shadow[j].request)
 				continue;
 
+			/*
+			 * Split requests alias one request across two shadow
+			 * slots; skip the sg-less secondary so it completes
+			 * once, like blkif_completion() does.
+			 */
+			if (shadow[j].associated_id != NO_ASSOCIATED_ID &&
+			    shadow[j].num_sg == 0)
+				continue;
+
 			/*
 			 * Get the bios in the request so we can re-queue them.
 			 */
-- 
2.43.0

Re: [PATCH] xen-blkfront: fix double completion of split requests on resume

[Roger Pau Monné]

On Sun, Jul 05, 2026 at 01:56:39PM +0200, Doruk Tan Ozturk wrote:
> When a block request is too large for a single ring entry and the
> backend does not support indirect descriptors, blkfront splits it
> across two ring requests.  blkif_ring_get_request() is called twice
> and both shadow slots (shadow[id] and shadow[extra_id]) are made to
> point at the *same* struct request, linked together through
> associated_id.

This is not exactly accurate.  Under normal operation the blk queue
parameters are already set to ensure the requests match the maximum
size the ring can accommodate.  However on ARM there's a corner case
when the guest is using 64K pages, as in that case even a single page
request cannot possibly fit into a single ring slot, and thus needs to
be split.

This needs mentioning explicitly in the commit message, that such
splitting only happens when the frontend is running on a 64K page
kernel.

> 
> On the normal completion path blkif_completion() collapses the pair:
> it recycles the second slot via add_id_to_freelist() and only completes
> the request once.  The suspend/resume path in blkfront_resume() does
> not.  It walks every physical shadow slot and, for each slot whose
> ->request is set, calls blk_mq_end_request() or re-queues
> ->request.  For an in-flight split request this visits the shared
> struct request twice, so on resume/migration the same request is
> ended (or re-queued) two times.  The second visit is a double
> blk_mq_end_request() (refcount underflow / double free) and a
> use-after-free read of req->bio, which was cleared on the first visit.
> 
> Skip the secondary slot of a split request in the resume walk, so each
> logical request is completed or re-queued exactly once, matching how
> blkif_completion() already treats the pair.  The secondary slot is the
> one that is linked (associated_id != NO_ASSOCIATED_ID) and carries no
> scatter-gather list (num_sg == 0); the first slot always keeps the
> scatter-gather list.

I find the above slightly too verbose, I don't think you need to go
into details about why freeing a requests twice is bad, this is
already well-known.

> This was found by 0sec automated security-research tooling
> (https://0sec.ai).

Isn't this information already conveyed by the `Assisted-by` tag?

> The bug is only reachable on suspend/resume or live
> migration of a guest whose backend lacks indirect-descriptor support, so
> it has no local reproducer; the fix is by source inspection against the
> existing blkif_completion() collapse logic.
> 
> Fixes: 6cc568339047 ("xen/blkfront: Handle non-indirect grant with 64KB pages")
> Assisted-by: 0sec:claude-opus-4-8
> Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
> ---
>  drivers/block/xen-blkfront.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index f765970578f9..b2e83fd0c77b 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -2079,6 +2079,15 @@ static int blkfront_resume(struct xenbus_device *dev)
>  			if (!shadow[j].request)
>  				continue;
>  
> +			/*
> +			 * Split requests alias one request across two shadow
> +			 * slots; skip the sg-less secondary so it completes
> +			 * once, like blkif_completion() does.

I would possibly avoid mentioning blkif_completion(), as those
references tend to get stale as code changes.  What about using:

"For requests split across multiple slots only process the underlying
requests once."

Or something similar?

Thanks, Roger.