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

[Jan Beulich <jbeulich@xxxxxxxx>]

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