Re: [PATCH v2] ioreq: Check for out of bounds vCPU ID

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2025 17:32, Teddy Astie wrote:
> A 4K page appears to be able to hold 128 ioreq entries, which luckly
> matches the current vCPU limit. However, if we decide to increase the
> vCPU limit, that doesn't hold anymore and this function would now
> silently fetch a out of bounds pointer.
> 
> All architectures have no more than 128 as vCPU limit on HVM guests,
> and have pages that are at most 4 KB, so this case doesn't occurs in
> with the current limits.

DYM "at least 4 KB"? If there was an arch with 2k pages but 128 vCPU limit,
it would be affected, wouldn't it?

> Make sure that out of bounds attempts are reported and adjust the around
> logic to at worst crash the offending domain instead.

Wouldn't we better prevent creation of such guests? And point out the need
to adjust code by a build-time check?

> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -100,7 +100,14 @@ static ioreq_t *get_ioreq(struct ioreq_server *s, struct 
> vcpu *v)
>      ASSERT((v == current) || !vcpu_runnable(v));
>      ASSERT(p != NULL);
>  
> -    return &p->vcpu_ioreq[v->vcpu_id];
> +    if ( likely(v->vcpu_id < (PAGE_SIZE / sizeof(struct ioreq))) )
> +        return &p->vcpu_ioreq[v->vcpu_id];

Imo you then also need to use array_access_nospec() here.

> +    else
> +    {
> +        gprintk(XENLOG_ERR, "Out of bounds vCPU %pv in ioreq server\n", v);
> +        WARN();
> +        return NULL;
> +    }
>  }

While I'm generally arguing against such needless uses of "else", this one
is imo a particularly bad example. The brace-enclosed scope give the strong
(but misleading) impression that the function is lacking a trailing "return".

Jan