| Message ID | 20210412105243.23354-3-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Prevent Dom0 to be loaded when using dom0less | expand |
On 12.04.2021 12:52, Luca Fancellu wrote: > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -1022,6 +1022,9 @@ static always_inline bool is_hardware_domain(const struct domain *d) > if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) ) > return false; > > + if ( !d ) > + return false; > + > return evaluate_nospec(d == hardware_domain); > } On v2 I did say on the respective code that was here (and my suggestion of this alternative adjustment): "Can you point out code paths where d may actually be NULL, and where [...] would not behave as intended (i.e. where bad speculation would result)?" Since you've taken the suggestion as-is, and since the commit message says nothing in either direction here, did you actually verify that there's no abuse of speculation possible with this extra return path? And did you find any caller at all which may pass NULL into here? Jan
> On 12 Apr 2021, at 12:03, Jan Beulich <jbeulich@suse.com> wrote: > > On 12.04.2021 12:52, Luca Fancellu wrote: >> --- a/xen/include/xen/sched.h >> +++ b/xen/include/xen/sched.h >> @@ -1022,6 +1022,9 @@ static always_inline bool is_hardware_domain(const struct domain *d) >> if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) ) >> return false; >> >> + if ( !d ) >> + return false; >> + >> return evaluate_nospec(d == hardware_domain); >> } > > On v2 I did say on the respective code that was here (and my > suggestion of this alternative adjustment): "Can you point out > code paths where d may actually be NULL, and where [...] would > not behave as intended (i.e. where bad speculation would > result)?" > > Since you've taken the suggestion as-is, and since the commit > message says nothing in either direction here, did you actually > verify that there's no abuse of speculation possible with this > extra return path? And did you find any caller at all which may > pass NULL into here? Hi Jan, I’ve analysed the code and seems that there are no path that calls Is_hardware_domain() with a NULL domain, however I found this function in xen/arch/arm/irq.c: bool irq_type_set_by_domain(const struct domain *d) { return is_hardware_domain(d); } That is calling directly is_hardware_domain and it is global. It can be the case that a future code can call irq_type_set_by_domain potentially with a null domain... I introduced a check for the domain for that reason, do you think that maybe it’s better to put that check on the irq_type_set_by_domain instead? Thank you for your feedback. Cheers, Luca > > Jan
On 12.04.2021 15:58, Luca Fancellu wrote: > > >> On 12 Apr 2021, at 12:03, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 12.04.2021 12:52, Luca Fancellu wrote: >>> --- a/xen/include/xen/sched.h >>> +++ b/xen/include/xen/sched.h >>> @@ -1022,6 +1022,9 @@ static always_inline bool is_hardware_domain(const struct domain *d) >>> if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) ) >>> return false; >>> >>> + if ( !d ) >>> + return false; >>> + >>> return evaluate_nospec(d == hardware_domain); >>> } >> >> On v2 I did say on the respective code that was here (and my >> suggestion of this alternative adjustment): "Can you point out >> code paths where d may actually be NULL, and where [...] would >> not behave as intended (i.e. where bad speculation would >> result)?" >> >> Since you've taken the suggestion as-is, and since the commit >> message says nothing in either direction here, did you actually >> verify that there's no abuse of speculation possible with this >> extra return path? And did you find any caller at all which may >> pass NULL into here? > > Hi Jan, > > I’ve analysed the code and seems that there are no path that calls > Is_hardware_domain() with a NULL domain, however I found this > function in xen/arch/arm/irq.c: > > bool irq_type_set_by_domain(const struct domain *d) > { > return is_hardware_domain(d); > } > > That is calling directly is_hardware_domain and it is global. > It can be the case that a future code can call irq_type_set_by_domain > potentially with a null domain... I don't think that a function being global (or not) matters here. This might be different in an environment like Linux, where modules may also call functions, and where guarding against NULL might be desirable in certain cases. > I introduced a check for the domain for that reason, do you think that > maybe it’s better to put that check on the irq_type_set_by_domain instead? If there's a specific reason to have a guard here, then it should be here, yes. As per above I would think though that if there's no present reason to check for NULL, such a check would best be omitted. We don't typically check internal function arguments like this, after all. Jan
> On 12 Apr 2021, at 15:22, Jan Beulich <jbeulich@suse.com> wrote: > > On 12.04.2021 15:58, Luca Fancellu wrote: >> >> >>> On 12 Apr 2021, at 12:03, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 12.04.2021 12:52, Luca Fancellu wrote: >>>> --- a/xen/include/xen/sched.h >>>> +++ b/xen/include/xen/sched.h >>>> @@ -1022,6 +1022,9 @@ static always_inline bool is_hardware_domain(const struct domain *d) >>>> if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) ) >>>> return false; >>>> >>>> + if ( !d ) >>>> + return false; >>>> + >>>> return evaluate_nospec(d == hardware_domain); >>>> } >>> >>> On v2 I did say on the respective code that was here (and my >>> suggestion of this alternative adjustment): "Can you point out >>> code paths where d may actually be NULL, and where [...] would >>> not behave as intended (i.e. where bad speculation would >>> result)?" >>> >>> Since you've taken the suggestion as-is, and since the commit >>> message says nothing in either direction here, did you actually >>> verify that there's no abuse of speculation possible with this >>> extra return path? And did you find any caller at all which may >>> pass NULL into here? >> >> Hi Jan, >> >> I’ve analysed the code and seems that there are no path that calls >> Is_hardware_domain() with a NULL domain, however I found this >> function in xen/arch/arm/irq.c: >> >> bool irq_type_set_by_domain(const struct domain *d) >> { >> return is_hardware_domain(d); >> } >> >> That is calling directly is_hardware_domain and it is global. >> It can be the case that a future code can call irq_type_set_by_domain >> potentially with a null domain... > > I don't think that a function being global (or not) matters here. This > might be different in an environment like Linux, where modules may > also call functions, and where guarding against NULL might be desirable > in certain cases. > >> I introduced a check for the domain for that reason, do you think that >> maybe it’s better to put that check on the irq_type_set_by_domain instead? > > If there's a specific reason to have a guard here, then it should be > here, yes. As per above I would think though that if there's no > present reason to check for NULL, such a check would best be omitted. > We don't typically check internal function arguments like this, after > all. Thank you Jan, so as you pointed out, since there is no actual path to call Is_hardware_domain() with a NULL pointer, then I will remove the check from is_hardware_domain() in a v4 patch. Cheers, Luca > > Jan
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index b71b099e6f..b761d90c40 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -412,7 +412,7 @@ bool is_assignable_irq(unsigned int irq) */ bool irq_type_set_by_domain(const struct domain *d) { - return (d == hardware_domain); + return is_hardware_domain(d); } /* diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c b/xen/drivers/passthrough/arm/ipmmu-vmsa.c index aef358d880..8b8e3a00ba 100644 --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c @@ -1168,7 +1168,7 @@ static int ipmmu_reassign_device(struct domain *s, struct domain *t, int ret = 0; /* Don't allow remapping on other domain than hwdom */ - if ( t && t != hardware_domain ) + if ( t && !is_hardware_domain(t) ) return -EPERM; if ( t == s ) diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c index 53d150cdb6..d115df7320 100644 --- a/xen/drivers/passthrough/arm/smmu-v3.c +++ b/xen/drivers/passthrough/arm/smmu-v3.c @@ -3366,7 +3366,7 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t, int ret = 0; /* Don't allow remapping on other domain than hwdom */ - if (t && t != hardware_domain) + if ( t && !is_hardware_domain(t) ) return -EPERM; if (t == s) diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 3e8aa37866..932fdfd6dd 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -2670,7 +2670,7 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t, int ret = 0; /* Don't allow remapping on other domain than hwdom */ - if (t && t != hardware_domain) + if ( t && !is_hardware_domain(t) ) return -EPERM; if (t == s) diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 1da90f207d..0a74df9931 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -30,7 +30,7 @@ enum domain_type { #endif /* The hardware domain has always its memory direct mapped. */ -#define is_domain_direct_mapped(d) ((d) == hardware_domain) +#define is_domain_direct_mapped(d) is_hardware_domain(d) struct vtimer { struct vcpu *v; diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 5485d08afb..5ba90f1214 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -1022,6 +1022,9 @@ static always_inline bool is_hardware_domain(const struct domain *d) if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) ) return false; + if ( !d ) + return false; + return evaluate_nospec(d == hardware_domain); }
The function is_hardware_domain() returns true if the hardware_domain and the passed domain is NULL, here we add a check to return false if there is no hardware_domain. Among the common and arm codebase there are few cases where the hardware_domain variable is checked to see if the current domain is equal to the hardware_domain, change this cases to use is_hardware_domain() function instead. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- v3 changes: - removed unneeded parenthesis for macro is_domain_direct_mapped - is_hardware_domain() checks for the passed domain and if it is NULL, it returns false. - reverted back checks in the function late_hwdom_init --- xen/arch/arm/irq.c | 2 +- xen/drivers/passthrough/arm/ipmmu-vmsa.c | 2 +- xen/drivers/passthrough/arm/smmu-v3.c | 2 +- xen/drivers/passthrough/arm/smmu.c | 2 +- xen/include/asm-arm/domain.h | 2 +- xen/include/xen/sched.h | 3 +++ 6 files changed, 8 insertions(+), 5 deletions(-)