diff mbox series

[1/2] xen/cpupool: Reject attempts to add a domain to CPUPOOLID_NONE

Message ID 20220517194113.2574-2-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series Fix some problems with "arm/dom0less: assign dom0less guests to cpupools" | expand

Commit Message

Andrew Cooper May 17, 2022, 7:41 p.m. UTC
c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for
system domains") removed the path in domain_create() which called
sched_init_domain() with CPUPOOLID_NONE for system domains.

Arguably, that changeset should have cleaned up this path too.

However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
changed domain_create() from using a hardcoded poolid of 0, to using a value
passed by the toolstack.

While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the
cpupool_id parameter and attempt to construct a real domain using default ops,
which at a minimum will fail the assertion in dom_scheduler().

Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Juergen Gross <jgross@suse.com>
CC: Dario Faggioli <dfaggioli@suse.com>
CC: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/common/sched/cpupool.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Juergen Gross May 18, 2022, 10:24 a.m. UTC | #1
On 17.05.22 21:41, Andrew Cooper wrote:
> c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for
> system domains") removed the path in domain_create() which called
> sched_init_domain() with CPUPOOLID_NONE for system domains.
> 
> Arguably, that changeset should have cleaned up this path too.
> 
> However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
> changed domain_create() from using a hardcoded poolid of 0, to using a value
> passed by the toolstack.
> 
> While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the
> cpupool_id parameter and attempt to construct a real domain using default ops,
> which at a minimum will fail the assertion in dom_scheduler().
> 
> Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen
Luca Fancellu May 18, 2022, 10:27 a.m. UTC | #2
Hi Andrew,

> On 17 May 2022, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for
> system domains") removed the path in domain_create() which called
> sched_init_domain() with CPUPOOLID_NONE for system domains.
> 
> Arguably, that changeset should have cleaned up this path too.
> 
> However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
> changed domain_create() from using a hardcoded poolid of 0, to using a value
> passed by the toolstack.
> 
> While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the
> cpupool_id parameter and attempt to construct a real domain using default ops,
> which at a minimum will fail the assertion in dom_scheduler().
> 
> Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks for this fix, with the introduction of 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools”)
we’ve checked all the path passing struct xen_domctl_createdomain, and at that time it seems to be that
the new cpupool_id member would have been always zero when created from the tool stack, am I wrong?

I’m asking so that I will keep in mind for the future.

However with your second patch of this serie, the tool stack is able to write it, so I guess this fix now is mandatory.

I’ve tested your patch, enabling boot time cpupools, on an arm machine and booting Xen+Dom0 and another DomU
by dom0less feature, and all works.

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca
Andrew Cooper May 18, 2022, 11:14 a.m. UTC | #3
On 18/05/2022 11:27, Luca Fancellu wrote:
> Hi Andrew,
>
>> On 17 May 2022, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>> c/s cfc52148444f ("xen/domain: Reduce the quantity of initialisation for
>> system domains") removed the path in domain_create() which called
>> sched_init_domain() with CPUPOOLID_NONE for system domains.
>>
>> Arguably, that changeset should have cleaned up this path too.
>>
>> However, c/s 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
>> changed domain_create() from using a hardcoded poolid of 0, to using a value
>> passed by the toolstack.
>>
>> While CPUPOOLID_NONE is an internal constant, userspace can pass -1 for the
>> cpupool_id parameter and attempt to construct a real domain using default ops,
>> which at a minimum will fail the assertion in dom_scheduler().
>>
>> Fixes: 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Thanks for this fix, with the introduction of 92ea9c54fc81 ("arm/dom0less: assign dom0less guests to cpupools”)
> we’ve checked all the path passing struct xen_domctl_createdomain, and at that time it seems to be that
> the new cpupool_id member would have been always zero when created from the tool stack, am I wrong?

Hypercalls are an entirely public API/ABI.

Looking through xen.git gets you the common users, but it most
definitely doesn't get you all users of the interface.

This hypercall specifically gets fuzzed (there's a KFX PoC somewhere),
but it's a bug for any hypercall to be able to hit an assertion/crash/etc.

> I’m asking so that I will keep in mind for the future.
>
> However with your second patch of this serie, the tool stack is able to write it, so I guess this fix now is mandatory.
>
> I’ve tested your patch, enabling boot time cpupools, on an arm machine and booting Xen+Dom0 and another DomU
> by dom0less feature, and all works.
>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> Tested-by: Luca Fancellu <luca.fancellu@arm.com>

Thanks.

~Andrew
diff mbox series

Patch

diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index f6e3d97e5288..f1aa2db5f463 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -619,8 +619,6 @@  int cpupool_add_domain(struct domain *d, unsigned int poolid)
     int rc;
     int n_dom = 0;
 
-    if ( poolid == CPUPOOLID_NONE )
-        return 0;
     spin_lock(&cpupool_lock);
     c = cpupool_find_by_id(poolid);
     if ( c == NULL )