[PATCH] kvm: arm: LPAE: Incorrect device to IPA mapping

[PATCH] kvm: arm: LPAE: Incorrect device to IPA mapping

[marek.majtyka]

From: Marek Majtyka <marek.majtyka@tieto.com>

A critical bug has been found in device memory stage1 translation for machines
with more then 4GB of RAM. Once vm_pgoff size is smaller then pa (which is true
for LPAE case, u32 and u64 respectively) some more significant bits of pa may
be lost as a shift operation is performed on u32 and later cast onto u64.

Example: vm_pgoff(u32)=0x00210030, PAGE_SHIFT=12
        expected pa(u64):   0x0000002010030000
        produced pa(u64):   0x0000000010030000

A suggested fix is changing the order of operations (casting first onto
phys_addr_t and then shifting), which works for both alternatives
(with and without LPAE).

A bug has been found, fixed and tested on kernel v3.19. However, it seems to be
valid for most, if not all, newer version starting from v3.18.

Signed-off-by: Marek Majtyka <marek.majtyka@tieto.com>
---
 arch/arm/kvm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 1366625..602bc63 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -1408,7 +1408,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 		if (vma->vm_flags & VM_PFNMAP) {
 			gpa_t gpa = mem->guest_phys_addr +
 				    (vm_start - mem->userspace_addr);
-			phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) +
+			phys_addr_t pa =
+				((phys_addr_t) vma->vm_pgoff << PAGE_SHIFT) +
 					 vm_start - vma->vm_start;
 
 			ret = kvm_phys_addr_ioremap(kvm, gpa, pa,
-- 
1.9.1

[PATCH] kvm: arm: LPAE: Incorrect device to IPA mapping

[Marc Zyngier]

Hi Marek,

On 16/09/15 11:04, marek.majtyka wrote:
> From: Marek Majtyka <marek.majtyka@tieto.com>
> 
> A critical bug has been found in device memory stage1 translation for machines
> with more then 4GB of RAM. Once vm_pgoff size is smaller then pa (which is true
> for LPAE case, u32 and u64 respectively) some more significant bits of pa may
> be lost as a shift operation is performed on u32 and later cast onto u64.
> 
> Example: vm_pgoff(u32)=0x00210030, PAGE_SHIFT=12
>         expected pa(u64):   0x0000002010030000
>         produced pa(u64):   0x0000000010030000
> 
> A suggested fix is changing the order of operations (casting first onto
> phys_addr_t and then shifting), which works for both alternatives
> (with and without LPAE).

Well, you can't use KVM without LPAE, so that's a moot point. Also, the
issue is when you place things above 4GB, not when you have more than
4GB of RAM (I cannot see how you allocate that amount of memory from
userspace on 32bit).

> 
> A bug has been found, fixed and tested on kernel v3.19. However, it seems to be
> valid for most, if not all, newer version starting from v3.18.
> 
> Signed-off-by: Marek Majtyka <marek.majtyka@tieto.com>
> ---
>  arch/arm/kvm/mmu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 1366625..602bc63 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -1408,7 +1408,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  		if (vma->vm_flags & VM_PFNMAP) {
>  			gpa_t gpa = mem->guest_phys_addr +
>  				    (vm_start - mem->userspace_addr);
> -			phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) +
> +			phys_addr_t pa =
> +				((phys_addr_t) vma->vm_pgoff << PAGE_SHIFT) +
>  					 vm_start - vma->vm_start;
>  
>  			ret = kvm_phys_addr_ioremap(kvm, gpa, pa,
> 

Looks perfectly valid to me (the formatting is horrible, but I can fix
that - I can only guess you've followed checkpatch's silly advise).

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

Christoffer: if you're OK with that one, I'll add it to the next batch
of fixes.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] kvm: arm: LPAE: Incorrect device to IPA mapping

[Christoffer Dall]

On Wed, Sep 16, 2015 at 11:33:37AM +0100, Marc Zyngier wrote:
> Hi Marek,
> 
> On 16/09/15 11:04, marek.majtyka wrote:
> > From: Marek Majtyka <marek.majtyka@tieto.com>
> > 
> > A critical bug has been found in device memory stage1 translation for machines
> > with more then 4GB of RAM. Once vm_pgoff size is smaller then pa (which is true
> > for LPAE case, u32 and u64 respectively) some more significant bits of pa may
> > be lost as a shift operation is performed on u32 and later cast onto u64.
> > 
> > Example: vm_pgoff(u32)=0x00210030, PAGE_SHIFT=12
> >         expected pa(u64):   0x0000002010030000
> >         produced pa(u64):   0x0000000010030000
> > 
> > A suggested fix is changing the order of operations (casting first onto
> > phys_addr_t and then shifting), which works for both alternatives
> > (with and without LPAE).
> 
> Well, you can't use KVM without LPAE, so that's a moot point. Also, the
> issue is when you place things above 4GB, not when you have more than
> 4GB of RAM (I cannot see how you allocate that amount of memory from
> userspace on 32bit).
> 
> > 
> > A bug has been found, fixed and tested on kernel v3.19. However, it seems to be
> > valid for most, if not all, newer version starting from v3.18.
> > 
> > Signed-off-by: Marek Majtyka <marek.majtyka@tieto.com>
> > ---
> >  arch/arm/kvm/mmu.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > index 1366625..602bc63 100644
> > --- a/arch/arm/kvm/mmu.c
> > +++ b/arch/arm/kvm/mmu.c
> > @@ -1408,7 +1408,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
> >  		if (vma->vm_flags & VM_PFNMAP) {
> >  			gpa_t gpa = mem->guest_phys_addr +
> >  				    (vm_start - mem->userspace_addr);
> > -			phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) +
> > +			phys_addr_t pa =
> > +				((phys_addr_t) vma->vm_pgoff << PAGE_SHIFT) +
> >  					 vm_start - vma->vm_start;
> >  
> >  			ret = kvm_phys_addr_ioremap(kvm, gpa, pa,
> > 
> 
> Looks perfectly valid to me (the formatting is horrible, but I can fix
> that - I can only guess you've followed checkpatch's silly advise).
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> Christoffer: if you're OK with that one, I'll add it to the next batch
> of fixes.
> 
Yup, looks good to me.

-Christoffer