| Message ID | 20210802123830.2195174-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Prevent kmemleak from accessing HYP data | expand |
Hi Marc, On Monday 02 Aug 2021 at 13:38:29 (+0100), Marc Zyngier wrote: > 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. > Fixes: 380e18ade4a5 ("KVM: arm64: Introduce a BSS section for use at Hyp") Not sure this is the patch to blame? > Suggested-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > Cc: stable@vger.kernel.org #5.13 > --- > arch/arm64/kernel/vmlinux.lds.S | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S > index 709d2c433c5e..f6b1a88245db 100644 > --- a/arch/arm64/kernel/vmlinux.lds.S > +++ b/arch/arm64/kernel/vmlinux.lds.S > @@ -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 > -- > 2.30.2 Thanks, Quentin
Hi Quentin, On Mon, 02 Aug 2021 14:11:07 +0100, Quentin Perret <qperret@google.com> wrote: > > Hi Marc, > > On Monday 02 Aug 2021 at 13:38:29 (+0100), Marc Zyngier wrote: > > 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. 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. > > > 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.
On Monday 02 Aug 2021 at 14:20:42 (+0100), Marc Zyngier wrote: > Hi Quentin, > > On Mon, 02 Aug 2021 14:11:07 +0100, > Quentin Perret <qperret@google.com> wrote: > > > > Hi Marc, > > > > On Monday 02 Aug 2021 at 13:38:29 (+0100), Marc Zyngier wrote: > > > 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. > > 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. > 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). > > > > > 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
On Mon, Aug 02, 2021 at 01:38:29PM +0100, Marc Zyngier wrote: > 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 <catalin.marinas@arm.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > Cc: stable@vger.kernel.org #5.13 Acked-by: Catalin Marinas <catalin.marinas@arm.com>
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S index 709d2c433c5e..f6b1a88245db 100644 --- a/arch/arm64/kernel/vmlinux.lds.S +++ b/arch/arm64/kernel/vmlinux.lds.S @@ -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
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 <catalin.marinas@arm.com> Signed-off-by: Marc Zyngier <maz@kernel.org> Cc: stable@vger.kernel.org #5.13 --- arch/arm64/kernel/vmlinux.lds.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)