Re: [PATCH v3 2/4] xen: make VMAP only support in MMU system

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 16/08/2024 17:40, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> On 14/08/2024 13:33, Ayan Kumar Halder wrote:
> > Hi Jan,
> >
> > On 14/08/2024 12:35, Jan Beulich wrote:
> > > On 14.08.2024 12:55, Ayan Kumar Halder wrote:
> > > > Hi Jan,
> > > >
> > > > On 14/08/2024 07:37, Jan Beulich wrote:
> > > > > On 13.08.2024 19:13, Ayan Kumar Halder wrote:
> > > > > > From: Penny Zheng <penny.zheng@xxxxxxx>
> > > > > >
> > > > > > Introduced CONFIG_VMAP which is selected by the architectures 
> > > > > > that use
> > > > > > MMU. vm_init() does not do anything if CONFIG_VMAP is not enabled.
> > > > > >
> > > > > > VMAP is widely used in ALTERNATIVE feature to remap a range of 
> > > > > > memory
> > > > > > with new memory attributes. Since this is highly dependent on 
> > > > > > virtual
> > > > > > address translation, we choose to fold VMAP in MMU system.
> > > > > >
> > > > > > In this patch, we introduce a new Kconfig CONFIG_HAS_VMAP, and 
> > > > > > make it
> > > > > > only support in MMU system on ARM architecture. And ALTERNATIVE now
> > > > > > depends on VMAP.
> > > > > >
> > > > > > HARDEN_BRANCH_PREDICTOR is now gated on HAS_VMAP as speculative
> > > > > > attacks are not possible on non MMU based systems (ie Cortex-R52, 
> > > > > > R82).
> > > > > > See https://developer.arm.com/Arm%20Security%20Center/ 
> > > > > > Speculative%20Processor%20Vulnerability.
> > > > > While I'm not an Arm expert and hence I'm likely missing aspects, 
> > > > > I question
> > > > > the one (Spectre-BHB) vulnerability there to be sufficient to draw a
> > > > > conclusion towards the usefulness of branch hardening. I would advise
> > > > > against encoding such a connection in the Kconfig dependencies.
> > > > AFAIU, to address 'Spectre' like vulnerabilities 'branch hardening' 
> > > > was
> > > > added.
> > > >
> > > > See https://lore.kernel.org/all/E1fNadD-0000fz-9r@rmk- 
> > > > PC.armlinux.org.uk/
> > > > And from
> > > > https://lists.linaro.org/archives/list/linux-stable- 
> > > > mirror@xxxxxxxxxxxxxxxx/message/F4MGL4WT2R7T54NLRDGKFRQNSXF3DZGD/
> > > > Spectre is valid on MMU based systems.
> > > Since then various other issues / flavors were found. I've been 
> > > focusing
> > > on the x86 side of things, but I'd be very surprised if some didn't
> > > affect other architectures as well.
> > We are talking about Arm here as "HARDEN_BRANCH_PREDICTOR" is 
> > specific to Arm.
> > https://developer.arm.com/Arm%20Security%20Center/ 
> > Speculative%20Processor%20Vulnerability covers all the flavours and 
> > it does not include Cortex-R82 or R52.
> > It says the following :-
> >
> > "Cortex-R cores typically use a closed software stack. In those 
> > environments, applications or processes are strictly controlled, and 
> > therefore not exploitable"
> > > Plus branch hardening can be a pre-
> > > cautionary measure, too, I think.
> > The first two Arm non MMU cores that we wish to support in the 
> > forthcoming series is Cortex-R82 and R52.
> > As seen in https://developer.arm.com/documentation/ka005109/latest/, 
> > it explicitly states the following about R82
> > The Cortex-R82 implements the faulting feature (FEAT_FPAC) but is not 
> > vulnerable. The Cortex-R82 behaves differently than vulnerable 
> > A-class CPUs when speculatively executing past an instruction that 
> > authenticates PAC, and that behavior does not allow the attack 
> > software to create the "oracle".
> I am confused. This is describing why R82 is not affected by PACMAN. 
> But the Kconfig HARDEN_BRANCH_PREDICTOR is not for that (Xen doesn't 
> yet use Pointer Authentification Codes). The Kconfig was introduced 
> with XSA-254 which predates PACMAN by nearly 4 years.
Yes, you are correct. I was somehow linking PACMAN to branch prediction 
attacks. So, by mistake I assumed that HARDEN_BRANCH_PREDICTOR is used 
for protection against these attacks.
> > We can re-enable branch hardening if we know there is a valid non MMU 
> > Arm core which is vulnerable.
> Re-quoting what you wrote earlier:
>
> "Cortex-R cores typically use a closed software stack. In those
> environments, applications or processes are strictly controlled, and
> therefore not exploitable"
>
> It is quite subtle. This wording doesn't imply the cores are not 
> vulnerable. It says that if they are, then it would be difficult to 
> exploit because the software should be tightly controlled.
> Now this would be really up to the user to decide whether they want to 
> be extra cautious/futureproof or not.
> As we are at the beginning of the MPU support, then I don't think we 
> need to resolve issue right now. And it would be fine to gate it. But 
> maybe ARCH_VMAP was an incorrect suggestion. It might be better to 
> gate with the !MMU (IIRC this would imply MPU).
I am ok with this. This has the benefit that the change can be contained 
within arch/arm if we do the following :-
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index cb2c0a16b8..26f7406278 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -329,7 +329,9 @@ void asmlinkage __init start_xen(unsigned long 
boot_phys_offset,
     setup_mm();

+#ifdef CONFIG_MMU
     vm_init();
+#endif

     /* Parse the ACPI tables for possible boot-time configuration */
     acpi_boot_table_init();

Are we ok with this ?

The definition of vm_init() is in xen/include/xen/vmap.h. If I enclose 
it using any CONFIG_XXX (like I have done in the current patch), then I 
need to introduce it in common/Kconfig and define it for x86 and PPC. I 
would prefer to contain the change within arch/arm only if possible.
> But before the feature can be marked as security support. We will need 
> to agree on how the hypervisor is intended to be used on ARMv8-R. 
> Maybe it would need a caveat "only trusted software can be run" which 
> means we don't have to worry about speculation on Cortex-R. Although, 
> it would be nice to have some defense in-depth :).
Yes, the support for ARMv8-R will be 'experimental' for some time. I am 
planning to add the minimal support required to just boot one or more DomUs.
- Ayan