ioreq: Make sure ioreq is always in-bounds

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

Posted by Teddy Astie 1 day, 11 hours ago

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@vates.tech>
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
v3:
 - make it a BUILD_BUG_ON instead (compile-time check)
v2:
 - check and report instead of ASSERT and eventually crash offending domain

 xen/common/ioreq.c | 1 +
 1 file changed, 1 insertion(+)

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];
 }
-- 
2.51.2



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

Posted by Jan Beulich 1 day, 11 hours ago

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@vates.tech>
> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>

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.

Jan

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

Posted by Teddy Astie 1 day, 10 hours ago

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@vates.tech>
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> 
> 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.

> Jan



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

Posted by Jan Beulich 1 day, 10 hours ago

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@vates.tech>
>>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>>
>> 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

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

Posted by Alejandro Vallejo 13 hours ago

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@vates.tech>
>>>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
>>>
>>> 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