linux-kernel - Re: [PATCH] arm64: fix address fault during mapping fdt region

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 1 Aug 2016 11:50:39 +0200
From:	Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:	zijun_hu <zijun_hu@...o.com>
Cc:	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Mark Rutland <mark.rutland@....com>,
	Laura Abbott <labbott@...oraproject.org>,
	"Suzuki K. Poulose" <suzuki.poulose@....com>,
	Jeremy Linton <jeremy.linton@....com>, tj@...nel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>, zijun_hu@....com
Subject: Re: [PATCH] arm64: fix address fault during mapping fdt region

On 1 August 2016 at 11:42, zijun_hu <zijun_hu@...o.com> wrote:
> From 07b9216ec3494515e7a6c41e0333eb8782427db3 Mon Sep 17 00:00:00 2001
> From: zijun_hu <zijun_hu@....com>
> Date: Mon, 1 Aug 2016 17:04:59 +0800
> Subject: [PATCH] arm64: fix address fault during mapping fdt region
>
> fdt_check_header() accesses other fileds of fdt header but
> the first 8 bytes such as version; so accessing unmapped
> address fault happens if fdt region locates below align
> boundary nearly during mapping fdt region, or expressed as
> (offset + sizeof(struct fdt_header)) > SWAPPER_BLOCK_SIZE
>
> fdt header size at least is mapped in order to avoid the issue
>
> Signed-off-by: zijun_hu <zijun_hu@....com>
> ---
>  arch/arm64/mm/mmu.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 0f85a46..0d72b71 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -744,6 +744,7 @@ void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot)
>         const u64 dt_virt_base = __fix_to_virt(FIX_FDT);
>         int offset;
>         void *dt_virt;
> +       int dt_header_map_size;
>
>         /*
>          * Check whether the physical FDT address is set and meets the minimum
> @@ -774,9 +775,18 @@ void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot)
>         offset = dt_phys % SWAPPER_BLOCK_SIZE;
>         dt_virt = (void *)dt_virt_base + offset;
>
> +       /*
> +        * fdt_check_header() maybe access any field of fdt header not
> +        * the first 8 bytes only, so map fdt header size at least for
> +        * checking fdt header without address fault more portably
> +        */
> +       BUILD_BUG_ON(sizeof(struct fdt_header) > SWAPPER_BLOCK_SIZE);
> +       dt_header_map_size = round_up(offset + sizeof(struct fdt_header),
> +                       SWAPPER_BLOCK_SIZE);
> +
>         /* map the first chunk so we can read the size from the header */
>         create_mapping_noalloc(round_down(dt_phys, SWAPPER_BLOCK_SIZE),
> -                       dt_virt_base, SWAPPER_BLOCK_SIZE, prot);
> +                       dt_virt_base, dt_header_map_size, prot);
>
>         if (fdt_check_header(dt_virt) != 0)
>                 return NULL;
> @@ -785,7 +795,7 @@ void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot)
>         if (*size > MAX_FDT_SIZE)
>                 return NULL;
>
> -       if (offset + *size > SWAPPER_BLOCK_SIZE)
> +       if (offset + *size > dt_header_map_size)
>                 create_mapping_noalloc(round_down(dt_phys, SWAPPER_BLOCK_SIZE), dt_virt_base,
>                                round_up(offset + *size, SWAPPER_BLOCK_SIZE), prot);
>

Couldn't we simply do this instead?

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 0f85a46c3e18..e8d3b04a2b57 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -778,7 +778,7 @@ void *__init __fixmap_remap_fdt(phys_addr_t
dt_phys, int *size, pgprot_t prot)
        create_mapping_noalloc(round_down(dt_phys, SWAPPER_BLOCK_SIZE),
                        dt_virt_base, SWAPPER_BLOCK_SIZE, prot);

-       if (fdt_check_header(dt_virt) != 0)
+       if (fdt_magic(dt_virt) != FDT_MAGIC)
                return NULL;

        *size = fdt_totalsize(dt_virt);

We are simply looking for a size field. The OF code will call
fdt_check_header() again, so anything that is checked there in
addition to the magic field will still be checked.

-- 
Ard.