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

["Teddy Astie" <teddy.astie@xxxxxxxxxx>]

Le 17/11/2025 à 10:29, Jan Beulich a écrit :
> 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?
>

Yes, made some typo here

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

So overall just

diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index f5fd30ce12..7a0421cc07 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -99,6 +99,7 @@ static ioreq_t *get_ioreq(struct ioreq_server *s,
struct vcpu *v)

      ASSERT((v == current) || !vcpu_runnable(v));
      ASSERT(p != NULL);
+    BUILD_BUG_ON(HVM_MAX_VCPUS > (PAGE_SIZE / sizeof(struct ioreq)));

      return &p->vcpu_ioreq[v->vcpu_id];
  }

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




--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech