arm64: fix address fault during mapping fdt region
Subsystems (2)
PATCH 1/1 arm64: fix address fault during mapping fdt region
Quoted 1 line
From
07b9216ec3494515e7a6c41e0333eb8782427db3Mon Sep 17 00:00:00 2001
From: zijun_hu <[email protected]>
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
--- arch/arm64/mm/mmu.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
arch/arm64/mm/mmu.c +12 -2
@@ -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);
Signature · 1 line
1.9.1
Quoted 51 lines · zijun_hu
From
07b9216ec3494515e7a6c41e0333eb8782427db3Mon Sep 17 00:00:00 2001
From:zijun_hu<[email protected]>
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_SIZEfdt header size at least is mapped in order to avoid the issue
Signed-off-by zijun_hu <[email protected]>--- arch/arm64/mm/mmu.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)arch/arm64/mm/mmu.c +11
-1@@ -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 ;
arch/arm64/mm/mmu.c void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot) · line 801
Quoted 8 lines
arch/arm64/mm/mmu.c +1
-1@@ -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?
arch/arm64/mm/mmu.c +0 -0
@@ -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.
Signature · 1 line
Ard.
Quoted 55 lines · Ard Biesheuvel
Quoted 54 lines · zijun_hu
From
07b9216ec3494515e7a6c41e0333eb8782427db3Mon Sep 17 00:00:00 2001
From:zijun_hu<[email protected]>
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_SIZEfdt header size at least is mapped in order to avoid the issue
Signed-off-by zijun_hu <[email protected]>--- arch/arm64/mm/mmu.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)arch/arm64/mm/mmu.c +11
-1@@ -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 ;
Quoted 7 lines
arch/arm64/mm/mmu.c void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot) · line 801
Quoted 5 lines
arch/arm64/mm/mmu.c +1
-1-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?
this solution maybe better, my reason as follows?
1?it can achieve our original purpose, namely, checking whether fdt header is corrupted before fetching fdt size field; good fdt header can ensure good fdt size field included more rightly than only a magic filed normally
2?it is more portable; we only need to call fdt_check_header() and don't care about fdt header filed layout; moreover,fdt module is another independent module and arm64 only uses it and should not depend on more details of fdt such as size and magic fields locate within the first MIN_FDT_ALIGN bytes; the decision whether a fdt header is corrupted should be left to fdt team
Quoted 16 lines
arch/arm64/mm/mmu.c +1
-1@@ -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);
Quoted 4 lines
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.
here is the first position to involve fdt functions, it maybe more suitable to checking before using
Mark Rutland
reviewerQuoted 9 lines · zijun_hu
Quoted 2 lines · Ard Biesheuvel
On 1 August 2016 at 11:42,
zijun_hu<[email protected]> wrote:
Couldn't we simply do this instead?this solution maybe better, my reason as follows?
1?it can achieve our original purpose, namely, checking whether fdt header is corrupted before fetching fdt size field; good fdt header can ensure good fdt size field included more rightly than only a magic filed normally
The only additional fields fdt_check_header checks are version and last_comp_version. In the absence of corruption, deferring these checks should be ok. We assume that the header is compatible regardless (or those fields would be meaningless).
Even with corruption, it's possible for these to appear valid to fdt_check_header(). For the purpose of best-effort corruption detection, checking the magic alone in this early codepath seems ok to me. It seems unlikely that we'd have a valid magic yet a corrupted totalsize.
That all said, I'm not against mapping the whole header if it's simple enough to do so.
Quoted 5 lines
2?it is more portable; we only need to call
fdt_check_header()and don't care about fdt header filed layout; moreover,fdt module is another independent module and arm64 only uses it and should not depend on more details of fdt such as size and magic fields locate within the firstMIN_FDT_ALIGNbytes; the decision whether a fdt header is corrupted should be left to fdt team
While it's true that we assume knowledge of the FDT format, and ideally we'd leave this to common code, we do so regardless by requiring the header size. So both approaches assume details regarding the FDT format.
Thanks,
Mark.
Quoted 16 lines · Mark Rutland
Quoted 9 lines · zijun_hu
Quoted 2 lines · Ard Biesheuvel
On 1 August 2016 at 11:42,
zijun_hu<[email protected]> wrote:
Couldn't we simply do this instead?this solution maybe better, my reason as follows?
1?it can achieve our original purpose, namely, checking whether fdt header is corrupted before fetching fdt size field; good fdt header can ensure good fdt size field included more rightly than only a magic filed normally
The only additional fields
fdt_check_headerchecks are version andlast_comp_version. In the absence of corruption, deferring these checks should be ok. We assume that the header is compatible regardless (or those fields would be meaningless).
Quoted 8 lines
Even with corruption, it's possible for these to appear valid to
fdt_check_header(). For the purpose of best-effort corruption detection, checking the magic alone in this early codepath seems ok to me. It seems unlikely that we'd have a valid magic yet a corrupted totalsize.That all said, I'm not against mapping the whole header if it's simple enough to do so.
okay, as indicated by another mail sent by ard.biesheuvel, "the only fields we can assume to be mapped are 'magic' and 'totalsize'", accessing last_comp_version or version maybe causes address fault under some extreme conditions
according to current fdt_check_header definition, another case is regard as good fdt header, condition (fdt_magic(dt_virt) != FDT_MAGIC) don't cover as marked within below function body
int fdt_check_header(const void *fdt)
{
if (fdt_magic(fdt) == FDT_MAGIC) {
/* Complete tree */
if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
return -FDT_ERR_BADVERSION;
if (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION)
return -FDT_ERR_BADVERSION;
} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
/* Unfinished sequential-write blob */
if (fdt_size_dt_struct(fdt) == 0)
return -FDT_ERR_BADSTATE;acess field size_dt_struct too, this condition is regarded as good fdt header too
} else {
return -FDT_ERR_BADMAGIC;
}
return 0;
}Quoted 10 lines
Quoted 5 lines
2?it is more portable; we only need to call
fdt_check_header()and don't care about fdt header filed layout; moreover,fdt module is another independent module and arm64 only uses it and should not depend on more details of fdt such as size and magic fields locate within the firstMIN_FDT_ALIGNbytes; the decision whether a fdt header is corrupted should be left to fdt teamWhile it's true that we assume knowledge of the FDT format, and ideally we'd leave this to common code, we do so regardless by requiring the header size. So both approaches assume details regarding the FDT format.
okay, the only thing my solution is depends on is the fdt header struct name which maybe remain unchanged in further fdt source modification regardless of fields layout or position or header size;
by the way, my solution only maps more one SWAPPER_BLOCK_SIZE at extreme condition (offset + sizeof(struct fdt_header)) > SWAPPER_BLOCK_SIZE, it can occurs very rarely, even it happens, it is no matter due to the fast mapping operations
That all said, ard.biesheuvel's can resolves address fault too, you can decide which solution to used, maybe ask fdt team for some advisements
Quoted 3 lines
Thanks,
Mark.
Mark Rutland
reviewerQuoted 16 lines · zijun_hu
Quoted 11 lines · Mark Rutland
Quoted 5 lines · zijun_hu
2?it is more portable; we only need to call
fdt_check_header()and don't care about fdt header filed layout; moreover,fdt module is another independent module and arm64 only uses it and should not depend on more details of fdt such as size and magic fields locate within the firstMIN_FDT_ALIGNbytes; the decision whether a fdt header is corrupted should be left to fdt teamWhile it's true that we assume knowledge of the FDT format, and ideally we'd leave this to common code, we do so regardless by requiring the header size. So both approaches assume details regarding the FDT format.
okay, the only thing my solution is depends on is the fdt header struct name which maybe remain unchanged in further fdt source modification regardless of fields layout or position or header size;
Quoted 6 lines
by the way, my solution only maps more one
SWAPPER_BLOCK_SIZEat extreme condition (offset +sizeof(struct fdt_header)) >SWAPPER_BLOCK_SIZE, it can occurs very rarely, even it happens, it is no matter due to the fast mapping operationsThat all said, ard.biesheuvel's can resolves address fault too, you can decide which solution to used, maybe ask fdt team for some advisements
As a member of the "fdt team" (at least for bindings and the spec), I'm happy with Ard's patch. ;)
Regardless, many thanks for the report, and the proposed fix!
Thanks,
Mark.
Quoted 78 lines · Ard Biesheuvel
Quoted 51 lines · zijun_hu
From
07b9216ec3494515e7a6c41e0333eb8782427db3Mon Sep 17 00:00:00 2001
From:zijun_hu<[email protected]>
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_SIZEfdt header size at least is mapped in order to avoid the issue
Signed-off-by zijun_hu <[email protected]>--- arch/arm64/mm/mmu.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)arch/arm64/mm/mmu.c +11
-1@@ -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 ;arch/arm64/mm/mmu.c void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot) · line 801
Quoted 8 lines
arch/arm64/mm/mmu.c +1
-1@@ -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?
arch/arm64/mm/mmu.c +1
-1@@ -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);
Quoted 3 lines
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.
That looks much nicer to me, and deferring the header check also looks fine.
If you add a comment above the fdt_magic() check, pointing out why we can only safely access the magic and totalsize fields (and thus can't call fdt_check_header() yet):
Thanks,
Mark.
Greg KH
maintainerQuoted 11 lines · zijun_hu
Quoted 1 line
From
07b9216ec3494515e7a6c41e0333eb8782427db3Mon Sep 17 00:00:00 2001From:
zijun_hu<[email protected]>
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
Quoted 6 lines
fdt header size at least is mapped in order to avoid the issue
Signed-off-by zijun_hu <[email protected]>--- arch/arm64/mm/mmu.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly.
</formletter>