linux-kernel - [PATCH 3/3] ARM: dma-mapping: fix array out of bound access in __dma_*

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [day] [month] [year] [list]

Date:	Wed, 22 Feb 2012 09:16:37 +0530
From:	Ajeet Yadav <ajeet.yadav.77@...il.com>
To:	Russell King <linux@....linux.org.uk>,
	Jon Medhurst <tixy@...t.co.uk>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Sumit Bhattacharya <sumitb@...dia.com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc:	Ajeet Yadav <ajeet.yadav.77@...il.com>,
	Naveen Yadav <yad.naveen@...il.com>
Subject: [PATCH 3/3] ARM: dma-mapping: fix array out of bound access in  __dma_*_remap

Let consider
off = (PTRS_PER_PTE - 1);
idx = CONSISTENT_PTE_INDEX(c->vm_start); provide us with last
vaild element in consistent_pte[];

Also __dma_alloc_remap() request 1 page, i.e size == PAGE_SIZE.

Because size, off, idx are vaild values with respect to consistent
mapping region, and its a do{}while loop, so it will exit loop after
first pass. All fine

But their is a catch, we do off++, so
do{
        do_all_important_stuff
        off++;  >>>>> now off = PTRS_PER_PTE
        if (off >= PTRS_PER_PTE) {      >>>>>> condition TRUE
                off = 0;
                pte = consistent_pte[++idx];    >>>>> we did idx++ but
idx was already pointing to last element, so we are trying to access
array out of bound
        }
} while (size -= PAGE_SIZE);>>>>>>> conditon FALSE, but its too late

The proposed solution, move if body from last to begining of do{}while.

In this case we will prevent out of bound acess, without effecting the
flow on first entry and also exit from do{}while loop.
Moving the if body to begining of do{}while ensured that on first entry
to do{}while loop, the if condition fails hence not effecting the first
entry condition.
on subsequent iterations of do{}while loop the while condition will be
checked before we execute the if conditon.

Given functions __dma_alloc_remap()/ __dma_free_remap() already haves
necessary checks to ensure that size is valid, so again if we analyse
from given example

do{
        if (off >= PTRS_PER_PTE) {      >>>>>> condition FALSE
                off = 0;
                pte = consistent_pte[++idx];    >>>>>> we avoid this
        }
        do_all_important_stuff
        off++;  >>>>>> now off = PTRS_PER_PTE
} while (size -= PAGE_SIZE);     >>>>>>> condition FAILS, we exit the
do{}while, effectively prevented the out of bound access of
consistent_pte[]

NOTE: size is valid,  so in proposed solution while conditon is
effectively
preventing the out of bound array access to occur.
If we review the code from normal access point of view, we are not
changing any flow logic, also after the exit from do{}while no access to
off, idx, pte is performed by the function.

So we do not need to worry about post do{}while statments

Signed-off-by: Naveen Yadav <yad.naveen@...il.com>
Signed-off-by: Ajeet Yadav <ajeet.yadav.77@...il.com>
---
 arch/arm/mm/dma-mapping.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 932d288..0e1a2a2 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -251,16 +251,16 @@ __dma_alloc_remap(struct page *page, size_t size, gfp_t gfp, pgprot_t prot)
 		c->vm_pages = page;
 
 		do {
+			if (off >= PTRS_PER_PTE) {
+				off = 0;
+				pte = consistent_pte[++idx];
+			}
 			BUG_ON(!pte_none(*pte));
 
 			set_pte_ext(pte, mk_pte(page, prot), 0);
 			page++;
 			pte++;
 			off++;
-			if (off >= PTRS_PER_PTE) {
-				off = 0;
-				pte = consistent_pte[++idx];
-			}
 		} while (size -= PAGE_SIZE);
 
 		dsb();
@@ -275,6 +275,7 @@ static void __dma_free_remap(void *cpu_addr, size_t size)
 	struct arm_vmregion *c;
 	unsigned long addr;
 	pte_t *ptep;
+	pte_t pte;
 	int idx;
 	u32 off;
 
@@ -298,16 +299,14 @@ static void __dma_free_remap(void *cpu_addr, size_t size)
 	ptep = consistent_pte[idx] + off;
 	addr = c->vm_start;
 	do {
-		pte_t pte = ptep_get_and_clear(&init_mm, addr, ptep);
-
-		ptep++;
-		addr += PAGE_SIZE;
-		off++;
 		if (off >= PTRS_PER_PTE) {
 			off = 0;
 			ptep = consistent_pte[++idx];
 		}
-
+		pte = ptep_get_and_clear(&init_mm, addr, ptep);
+		ptep++;
+		addr += PAGE_SIZE;
+		off++;
 		if (pte_none(pte) || !pte_present(pte))
 			printk(KERN_CRIT "%s: bad page in kernel page table\n",
 			       __func__);
-- 
1.7.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/