ARM: gcc5 causes undefined behaviour in mmu_init()

ARM: gcc5 causes undefined behaviour in mmu_init()

[Enrico Scholz]

Hi,

a barebox built with gcc5 hangs in tlb_invalidate() very early.  This
seems to be caused by an integer overflow:

--- arch/arm/cpu/mmu.c:
static int arm_mmu_remap_sdram(struct memory_bank *bank)
{
	unsigned long num_ptes = bank->size >> 10;
	int i;

	for (i = 0; i < num_ptes; i++) {
		ptes[i] = (phys + i * PAGE_SIZE) | PTE_TYPE_SMALL |
			pte_flags_cached;
	}


For 1GiB RAM, 'num_ptes' is 1MiB and due to integer promotion, the 'i *
PAGE_SIZE' overflows and causes undefined behavior.

A trivial fix is to make 'i' an unsigned int or long.


But I wonder whether calculation of 'num_ptes' is really correct.  Does
barebox really use a 1KiB pagesize for PTEs or should the '>> 10' be a
'>> 12'?  When it is really 1KiB, the mapping for memory sizes > 1GiB
seem to be ambiguous then.


For reference; in broken case ('int i'), gcc5 generates:

|   4badc:       f7b4 fe7e       bl      7dc <pr_print>
|   4bae0:       f8d9 3000       ldr.w   r3, [r9]
|   4bae4:       f1aa 0104       sub.w   r1, sl, #4
|   4bae8:       f043 0202       orr.w   r2, r3, #2
|   4baec:       9b03            ldr     r3, [sp, #12]
|   4baee:       eb04 3303       add.w   r3, r4, r3, lsl #12
|
|   4baf2:       42a3            cmp     r3, r4
|   4baf4:       d006            beq.n   4bb04 <mmu_init+0x1c0>
|   4baf6:       ea42 0004       orr.w   r0, r2, r4
|   4bafa:       f504 5480       add.w   r4, r4, #4096   ; 0x1000
|   4bafe:       f841 0f04       str.w   r0, [r1, #4]!
|   4bb02:       e7f6            b.n     4baf2 <mmu_init+0x1ae>

Building with 'unsigned long i' generates:

|   4bad8:       f7b4 fe80       bl      7dc <pr_print>
|   4badc:       683b            ldr     r3, [r7, #0]
|   4bade:       f043 0202       orr.w   r2, r3, #2
|   4bae2:       9b05            ldr     r3, [sp, #20]
|
|   4bae4:       9904            ldr     r1, [sp, #16]
|   4bae6:       4299            cmp     r1, r3
|   4bae8:       d006            beq.n   4baf8 <mmu_init+0x1b4>
|   4baea:       eb0a 3103       add.w   r1, sl, r3, lsl #12
|   4baee:       4311            orrs    r1, r2
|   4baf0:       f849 1023       str.w   r1, [r9, r3, lsl #2]
|   4baf4:       3301            adds    r3, #1
|   4baf6:       e7f5            b.n     4bae4 <mmu_init+0x1a0>


Enrico
-- 
SIGMA Chemnitz GmbH       Registergericht:   Amtsgericht Chemnitz HRB 1750
Am Erlenwald 13           Geschaeftsfuehrer: Grit Freitag, Frank Pyritz
09128 Chemnitz


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] ARM: MMU: fixed dma_flush_range() call

[Enrico Scholz]

dma_flush_range() expects an address as second argument, not a size.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 arch/arm/cpu/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/mmu.c b/arch/arm/cpu/mmu.c
index 37bfa05..8ceb450 100644
--- a/arch/arm/cpu/mmu.c
+++ b/arch/arm/cpu/mmu.c
@@ -246,7 +246,8 @@ static int arm_mmu_remap_sdram(struct memory_bank *bank)
 	}
 
 	dma_flush_range((unsigned long)ttb, (unsigned long)ttb + 0x4000);
-	dma_flush_range((unsigned long)ptes, num_ptes * sizeof(u32));
+	dma_flush_range((unsigned long)ptes,
+			(unsigned long)ptes + num_ptes * sizeof(u32));
 
 	tlb_invalidate();
 
-- 
2.4.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] ARM: MMU: fixed calculation of number of PTEs

[Enrico Scholz]

barebox uses 4KiB pages so that number of PTEs is 'size >> 12', not
'size >> 10'.

Thie 'size >> 10' limit is not an immediate problem because it allocates
too much PTEs only which are not used.  But it can overflow an integer
multiplication ('i * PAGE_SIZE') which causes undefined behaviour with
gcc5.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 arch/arm/cpu/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/cpu/mmu.c b/arch/arm/cpu/mmu.c
index 8ceb450..014bba2 100644
--- a/arch/arm/cpu/mmu.c
+++ b/arch/arm/cpu/mmu.c
@@ -213,7 +213,7 @@ static int arm_mmu_remap_sdram(struct memory_bank *bank)
 	unsigned long phys = (unsigned long)bank->start;
 	unsigned long ttb_start = phys >> 20;
 	unsigned long ttb_end = (phys >> 20) + (bank->size >> 20);
-	unsigned long num_ptes = bank->size >> 10;
+	unsigned long num_ptes = bank->size >> 12;
 	int i, pte;
 	u32 *ptes;
 
-- 
2.4.3


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] ARM: MMU: fixed dma_flush_range() call

[Sascha Hauer]

On Thu, Sep 17, 2015 at 12:45:10PM +0200, Enrico Scholz wrote:
> dma_flush_range() expects an address as second argument, not a size.
> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> ---
>  arch/arm/cpu/mmu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/mmu.c b/arch/arm/cpu/mmu.c
> index 37bfa05..8ceb450 100644
> --- a/arch/arm/cpu/mmu.c
> +++ b/arch/arm/cpu/mmu.c
> @@ -246,7 +246,8 @@ static int arm_mmu_remap_sdram(struct memory_bank *bank)
>  	}
>  
>  	dma_flush_range((unsigned long)ttb, (unsigned long)ttb + 0x4000);
> -	dma_flush_range((unsigned long)ptes, num_ptes * sizeof(u32));
> +	dma_flush_range((unsigned long)ptes,
> +			(unsigned long)ptes + num_ptes * sizeof(u32));

Applied both, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox