Re: [PATCH v3] ioreq: Make sure ioreq is always in-bounds

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Mon Nov 17, 2025 at 3:09 PM CET, Jan Beulich wrote:
> On 17.11.2025 14:43, Teddy Astie wrote:
>> Le 17/11/2025 à 13:46, Jan Beulich a écrit :
>>> On 17.11.2025 13:35, 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
>>>> domain vCPU limit, that doesn't hold anymore and this function would now
>>>> silently create a out of bounds pointer leading to confusing problems.
>>>>
>>>> All architectures with ioreq support don't support 128 vCPU limit for
>>>> HVM guests, and  have pages that are at least 4 KB large, so this case
>>>> doesn't occurs in with the current limits.
>>>>
>>>> For the time being, make sure we can't make a Xen build that can
>>>> accidentally make a out of bounds pointers here.
>>>>
>>>> No functional change.
>>>>
>>>> Reported-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
>>>> Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
>>>
>>> I was meaning to ack this, but ...
>>>
>>>> --- 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)));
>>>
>>> ... does this even build on e.g. Arm? IOREQ_SERVER is a setting which can be
>>> enabled (with EXPERT=y) also for non-x86. Yet HVM_MAX_VCPUS looks to be an
>>> x86-only thing. (I then also wonder about some of what the description 
>>> says).
>>>
>>> Just to mention (no further change requested at this point, in this regard):
>>> HVM_MAX_VCPUS being part of the public interface, we'll need to see whether 
>>> we
>>> can sensibly retain that identifier to carry changed meaning once we up the
>>> limit. The check here may therefore not trigger at that point; the hope then
>>> is that while making respective changes, people would at least stumble 
>>> across
>>> it by e.g. seeing it in grep output.
>>>
>> 
>> Apparently it doesn't build (debian-bookworm-gcc-arm32-randconfig 
>> catched it).
>> ARM does provide MAX_VIRT_CPUS and GUEST_MAX_VCPUS which is 128 or 
>> lower, but that doesn't map (or not properly) with what we have in x86 
>> (MAX_VIRT_CPUS=8192 is PV-specific, and GUEST_MAX_VCPUS doesn't exist).
>> 
>> I am not sure what to do, looks like many things are redundant here.
>
> Maybe non-x86 could surface HVM_MAX_VCPUS as an alias of whatever they already
> got, much like CONFIG_HVM exists also for Arm, and will likely need 
> introducing
> for PPC and RISC-V (despite not being overly meaningful for non-x86)?
>
> Jan

I'd say this is the better choice, pending some non-x86 people acking the plan.

Cheers,
Alejandro