KVM: arm64: Prevent kmemleak from accessing HYP data
Subsystems (4)
Marc Zyngier
maintainerv2 cover KVM: arm64: Prevent kmemleak from accessing HYP data
This is a rework of the patch previously posted at [1].
The gist of the problem is that kmemleak can legitimately access data that has been removed from the kernel view, for two reasons:
(1) .hyp.rodata is lumped together with the BSS
(2) there is no separation of the HYP BSS from the kernel BSS
(1) can easily be addressed by moving the .hyp.rodata section into the kernel's RO zone, which avoids any kmemleak into that section.
(2) must be addressed by telling kmemleak about the address range.
Tested on a SC2A11 system, in protected and non-protected modes with kmemleak active. Both patches are stable candidates.
[1] https://lore.kernel.org/r/[email protected]
Marc Zyngier (2):
arm64: Move .hyp.rodata outside of the _sdata.._edata range
KVM: arm64: Unregister HYP sections from kmemleak in protected mode
arch/arm64/kernel/vmlinux.lds.S | 4 ++-- arch/arm64/kvm/arm.c | 7 +++++++ 2 files changed, 9 insertions(+), 2 deletions(-)
Signature · 1 line
2.30.2
Marc Zyngier
maintainerPATCH v2 1/2 arm64: Move .hyp.rodata outside of the _sdata.._edata range
The HYP rodata section is currently lumped together with the BSS, which isn't exactly what is expected (it gets registered with kmemleak, for example).
Move it away so that it is actually marked RO. As an added benefit, it isn't registered with kmemleak anymore.
--- arch/arm64/kernel/vmlinux.lds.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm64/kernel/vmlinux.lds.S +2 -2
@@ -181,6 +181,8 @@ SECTIONS /* everything from this point to __init_begin will be marked RO NX */ RO_DATA(PAGE_SIZE) + HYPERVISOR_DATA_SECTIONS + idmap_pg_dir = .; . += IDMAP_DIR_SIZE; idmap_pg_end = .;
@@ -260,8 +262,6 @@ SECTIONS _sdata = .; RW_DATA(L1_CACHE_BYTES, PAGE_SIZE, THREAD_ALIGN) - HYPERVISOR_DATA_SECTIONS - /* * Data written with the MMU off but read with the MMU on requires * cache lines to be invalidated, discarding up to a Cache Writeback
Signature · 1 line
2.30.2
Re: [PATCH v2 1/2] arm64: Move .hyp.rodata outside of the _sdata.._edata range
Hi Marc,
Quoted 6 lines · Marc Zyngier
The HYP rodata section is currently lumped together with the BSS, which isn't exactly what is expected (it gets registered with kmemleak, for example).
Move it away so that it is actually marked RO. As an added benefit, it isn't registered with kmemleak anymore.
2d7bf218ca73 ("KVM: arm64: Add .hyp.data..ro_after_init ELF section") states explicitly that the hyp ro_after_init section should remain RW in the host as it is expected to modify it before initializing EL2. But I can't seem to trigger anything with this patch applied, so I'll look into this a bit more.
Quoted 1 line
Fixes 380e18ade4a5 ("KVM: arm64: Introduce a BSS section for use at Hyp")
Not sure this is the patch to blame?
Quoted 31 lines
Suggested-by Catalin Marinas <[email protected]>Signed-off-by Marc Zyngier <[email protected]>Cc [email protected] #5.13--- arch/arm64/kernel/vmlinux.lds.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm64/kernel/vmlinux.lds.S +2
-2@@ -181,6 +181,8 @@ SECTIONS /* everything from this point to __init_begin will be marked RO NX */ RO_DATA(PAGE_SIZE) + HYPERVISOR_DATA_SECTIONS + idmap_pg_dir = .; . += IDMAP_DIR_SIZE; idmap_pg_end = .;@@ -260,8 +262,6 @@ SECTIONS _sdata = .; RW_DATA(L1_CACHE_BYTES, PAGE_SIZE, THREAD_ALIGN) - HYPERVISOR_DATA_SECTIONS - /* * Data written with the MMU off but read with the MMU on requires * cache lines to be invalidated, discarding up to a Cache WritebackSignature · 1 line
2.30.2
Thanks,
Quentin
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Marc Zyngier
maintainerRe: [PATCH v2 1/2] arm64: Move .hyp.rodata outside of the _sdata.._edata range
Hi Quentin,
Quoted 8 lines · Quentin Perret
Hi Marc,
Quoted 4 lines · Marc Zyngier
The HYP rodata section is currently lumped together with the BSS, which isn't exactly what is expected (it gets registered with kmemleak, for example).
Quoted 8 lines
Quoted 2 lines
Move it away so that it is actually marked RO. As an added benefit, it isn't registered with kmemleak anymore.
2d7bf218ca73("KVM: arm64: Add .hyp.data..ro_after_init ELF section") states explicitly that the hypro_after_initsection should remain RW in the host as it is expected to modify it before initializing EL2. But I can't seem to trigger anything with this patch applied, so I'll look into this a bit more.
The switch to RO happens quite late. And if the host was to actually try and change things there, it would be screwed anyway (we will have already removed the pages from its S2).
I wouldn't be surprised if this was a consequence of the way we now build the HYP object, and the comment in the original commit may not be valid anymore.
Quoted 4 lines
Quoted 2 lines
Fixes 380e18ade4a5 ("KVM: arm64: Introduce a BSS section for use at Hyp")Not sure this is the patch to blame?
My bad, this is plain wrong. I'm not sure it can be applied earlier though if my rambling above is correct.
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 1/2] arm64: Move .hyp.rodata outside of the _sdata.._edata range
Quoted 21 lines · Marc Zyngier
Hi Quentin,
Quoted 8 lines · Quentin Perret
Hi Marc,
Quoted 4 lines · Marc Zyngier
The HYP rodata section is currently lumped together with the BSS, which isn't exactly what is expected (it gets registered with kmemleak, for example).
Quoted 8 lines
Quoted 2 lines
Move it away so that it is actually marked RO. As an added benefit, it isn't registered with kmemleak anymore.
2d7bf218ca73("KVM: arm64: Add .hyp.data..ro_after_init ELF section") states explicitly that the hypro_after_initsection should remain RW in the host as it is expected to modify it before initializing EL2. But I can't seem to trigger anything with this patch applied, so I'll look into this a bit more.
Quoted 3 lines
The switch to RO happens quite late. And if the host was to actually try and change things there, it would be screwed anyway (we will have already removed the pages from its S2).
Yes, clearly mapping rodata RO in host happens much later than I thought, so this should indeed be fine.
Quoted 3 lines
I wouldn't be surprised if this was a consequence of the way we now build the HYP object, and the comment in the original commit may not be valid anymore.
Just had a quick look and that still seems valid, at least for some things (e.g. see how we set hyp_cpu_logical_map[] early from EL1 while it is clearly annotated as __ro_after_init in the EL2 code).
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Fixes 380e18ade4a5 ("KVM: arm64: Introduce a BSS section for use at Hyp")Not sure this is the patch to blame?
My bad, this is plain wrong. I'm not sure it can be applied earlier though if my rambling above is correct.
By the look of it going all the way back to 2d7bf218ca73 (in David's PSCI proxy series) should actually be correct. But not sure if that's really going to make a difference before the patch you've mentioned above as the kmemleak issue will only be visible once we have a host stage-2, so no big deal.
Thanks,
Quentin
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/2] arm64: Move .hyp.rodata outside of the _sdata.._edata range
In reply to Marc ZyngierQuoted 11 lines · Marc Zyngier
The HYP rodata section is currently lumped together with the BSS, which isn't exactly what is expected (it gets registered with kmemleak, for example).
Move it away so that it is actually marked RO. As an added benefit, it isn't registered with kmemleak anymore.
Fixes 380e18ade4a5 ("KVM: arm64: Introduce a BSS section for use at Hyp")Suggested-by Catalin Marinas <[email protected]>Signed-off-by Marc Zyngier <[email protected]>Cc [email protected] #5.13
Marc Zyngier
maintainerPATCH v2 2/2 KVM: arm64: Unregister HYP sections from kmemleak in protected mode
In reply to Marc ZyngierBooting a KVM host in protected mode with kmemleak quickly results in a pretty bad crash, as kmemleak doesn't know that the HYP sections have been taken away. This is specially true for the BSS section, which is part of the kernel BSS section and registered at boot time by kmemleak itself.
Unregister the HYP part of the BSS before making that section HYP-private. The rest of the HYP-specific data is obtained via the page allocator or lives in other sections, none of which is subjected to kmemleak.
--- arch/arm64/kvm/arm.c | 7 +++++++ 1 file changed, 7 insertions(+)
arch/arm64/kvm/arm.c +7 -0
@@ -15,6 +15,7 @@#include <linux/fs.h> #include <linux/mman.h> #include <linux/sched.h> +#include <linux/kmemleak.h> #include <linux/kvm.h> #include <linux/kvm_irqfd.h> #include <linux/irqbypass.h>
@@ -1982,6 +1983,12 @@ static int finalize_hyp_mode(void)if (ret)return ret; +/* +* Exclude HYP BSS from kmemleak so that it doesn't get peeked +* at, which would end badly once the section is inaccessible. +* None of other sections should ever be introspected. +*/ +kmemleak_free_part (__hyp_bss_start, __hyp_bss_end - __hyp_bss_start); ret =pkvm_mark_hyp_section (__hyp_bss);if (ret)return ret;
Signature · 1 line
2.30.2
Re: [PATCH v2 2/2] KVM: arm64: Unregister HYP sections from kmemleak in protected mode
Quoted 11 lines · Marc Zyngier
Booting a KVM host in protected mode with kmemleak quickly results in a pretty bad crash, as kmemleak doesn't know that the HYP sections have been taken away. This is specially true for the BSS section, which is part of the kernel BSS section and registered at boot time by kmemleak itself.
Unregister the HYP part of the BSS before making that section HYP-private. The rest of the HYP-specific data is obtained via the page allocator or lives in other sections, none of which is subjected to kmemleak.
Quoted 5 lines
Fixes 90134ac9cabb ("KVM: arm64: Protect the .hyp sections from the host")Reviewed-by Quentin Perret <[email protected]>Signed-off-by Marc Zyngier <[email protected]>Cc Catalin Marinas <[email protected]>Cc [email protected] # 5.13
Quoted 9 lines · Marc Zyngier
This is a rework of the patch previously posted at [1].
The gist of the problem is that kmemleak can legitimately access data that has been removed from the kernel view, for two reasons:
(1) .hyp.rodata is lumped together with the BSS
(2) there is no separation of the HYP BSS from the kernel BSS[...]
Applied to next, thanks!
[1/2] arm64: Move .hyp.rodata outside of the _sdata.._edata range
commit: eb48d154cd0dade56a0e244f0cfa198ea2925ed3
[2/2] KVM: arm64: Unregister HYP sections from kmemleak in protected mode
commit: 47e6223c841e029bfc23c3ce594dac5525cebaf8Cheers,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.