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
