[PATCH] KVM: arm64: Unregister HYP sections from kmemleak in protected mode

[PATCH] KVM: arm64: Unregister HYP sections from kmemleak in protected mode

[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.

Make the unregistration from kmemleak part of marking the sections
as HYP-private. The rest of the HYP-specific data is obtained via
the page allocator, which is not subjected to kmemleak.

Fixes: 90134ac9cabb ("KVM: arm64: Protect the .hyp sections from the host")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: stable@vger.kernel.org # 5.13
---
 arch/arm64/kvm/arm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e9a2b8f27792..23f12e602878 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -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>
@@ -1960,8 +1961,12 @@ static inline int pkvm_mark_hyp(phys_addr_t start, phys_addr_t end)
 }
 
 #define pkvm_mark_hyp_section(__section)		\
+({							\
+	u64 sz = __section##_end - __section##_start;	\
+	kmemleak_free_part(__section##_start, sz);	\
 	pkvm_mark_hyp(__pa_symbol(__section##_start),	\
-			__pa_symbol(__section##_end))
+		      __pa_symbol(__section##_end));	\
+})
 
 static int finalize_hyp_mode(void)
 {
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Unregister HYP sections from kmemleak in protected mode

[Quentin Perret]

On Thursday 29 Jul 2021 at 14:50:16 (+0100), Marc Zyngier wrote:
> 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.
> 
> Make the unregistration from kmemleak part of marking the sections
> as HYP-private. The rest of the HYP-specific data is obtained via
> the page allocator, which is not subjected to kmemleak.
> 
> Fixes: 90134ac9cabb ("KVM: arm64: Protect the .hyp sections from the host")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: stable@vger.kernel.org # 5.13
> ---
>  arch/arm64/kvm/arm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e9a2b8f27792..23f12e602878 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -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>
> @@ -1960,8 +1961,12 @@ static inline int pkvm_mark_hyp(phys_addr_t start, phys_addr_t end)
>  }
>  
>  #define pkvm_mark_hyp_section(__section)		\
> +({							\
> +	u64 sz = __section##_end - __section##_start;	\
> +	kmemleak_free_part(__section##_start, sz);	\
>  	pkvm_mark_hyp(__pa_symbol(__section##_start),	\
> -			__pa_symbol(__section##_end))
> +		      __pa_symbol(__section##_end));	\
> +})

At some point we should also look into unmapping these sections from
EL1 stage-1 as well, as that should lead to better error messages in
case the host accesses hyp-private memory some other way. But this
patch makes sense on its own, so:

Reviewed-by: Quentin Perret <qperret@google.com>

Thanks,
Quentin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Unregister HYP sections from kmemleak in protected mode

[Catalin Marinas]

On Thu, Jul 29, 2021 at 02:50:16PM +0100, Marc Zyngier wrote:
> 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.
> 
> Make the unregistration from kmemleak part of marking the sections
> as HYP-private. The rest of the HYP-specific data is obtained via
> the page allocator, which is not subjected to kmemleak.
> 
> Fixes: 90134ac9cabb ("KVM: arm64: Protect the .hyp sections from the host")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: stable@vger.kernel.org # 5.13
> ---
>  arch/arm64/kvm/arm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e9a2b8f27792..23f12e602878 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -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>
> @@ -1960,8 +1961,12 @@ static inline int pkvm_mark_hyp(phys_addr_t start, phys_addr_t end)
>  }
>  
>  #define pkvm_mark_hyp_section(__section)		\
> +({							\
> +	u64 sz = __section##_end - __section##_start;	\
> +	kmemleak_free_part(__section##_start, sz);	\
>  	pkvm_mark_hyp(__pa_symbol(__section##_start),	\
> -			__pa_symbol(__section##_end))
> +		      __pa_symbol(__section##_end));	\
> +})

Using kmemleak_free_part() is fine in principle as this is not a slab
object. However, the above would call the function even for ranges that
are not tracked at all by kmemleak (text, idmap). Luckily Kmemleak won't
complain, unless you #define DEBUG in the file (initially I tried to
warn all the time but I couldn't fix all the callbacks).

If it was just the BSS, I would move the kmemleak_free_part() call to
finalize_hyp_mode() but there's the __hyp_rodata section as well.

I think we have some inconsistency with .hyp.rodata which falls under
_sdata.._edata while the kernel's own .rodata goes immediately after
_etext. Should we move __hyp_rodata outside _sdata.._edata as well? It
would benefit from the RO NX marking (probably more useful without the
protected mode). If this works, we'd only need to call kmemleak once for
the BSS.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Unregister HYP sections from kmemleak in protected mode

[Marc Zyngier]

On Thu, 29 Jul 2021 17:42:15 +0100,
Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> On Thu, Jul 29, 2021 at 02:50:16PM +0100, Marc Zyngier wrote:
> > 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.
> > 
> > Make the unregistration from kmemleak part of marking the sections
> > as HYP-private. The rest of the HYP-specific data is obtained via
> > the page allocator, which is not subjected to kmemleak.
> > 
> > Fixes: 90134ac9cabb ("KVM: arm64: Protect the .hyp sections from the host")
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Cc: Quentin Perret <qperret@google.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: stable@vger.kernel.org # 5.13
> > ---
> >  arch/arm64/kvm/arm.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index e9a2b8f27792..23f12e602878 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -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>
> > @@ -1960,8 +1961,12 @@ static inline int pkvm_mark_hyp(phys_addr_t start, phys_addr_t end)
> >  }
> >  
> >  #define pkvm_mark_hyp_section(__section)		\
> > +({							\
> > +	u64 sz = __section##_end - __section##_start;	\
> > +	kmemleak_free_part(__section##_start, sz);	\
> >  	pkvm_mark_hyp(__pa_symbol(__section##_start),	\
> > -			__pa_symbol(__section##_end))
> > +		      __pa_symbol(__section##_end));	\
> > +})
> 
> Using kmemleak_free_part() is fine in principle as this is not a slab
> object. However, the above would call the function even for ranges that
> are not tracked at all by kmemleak (text, idmap). Luckily Kmemleak won't
> complain, unless you #define DEBUG in the file (initially I tried to
> warn all the time but I couldn't fix all the callbacks).

Yeah, I had a look last week, and this fires everywhere (KVM only adds
a drop in an ocean of warnings).

> If it was just the BSS, I would move the kmemleak_free_part() call to
> finalize_hyp_mode() but there's the __hyp_rodata section as well.
> 
> I think we have some inconsistency with .hyp.rodata which falls under
> _sdata.._edata while the kernel's own .rodata goes immediately after
> _etext. Should we move __hyp_rodata outside _sdata.._edata as well? It
> would benefit from the RO NX marking (probably more useful without the
> protected mode). If this works, we'd only need to call kmemleak once for
> the BSS.

That's a good idea, and pretty easy to implement. I'll post a respin
shortly.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel