diff mbox

arm64: setup: Check for overlapping dtb and Image load addresses

Message ID 20180119065637.12789-1-clingutla@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Chandrasekhar L Jan. 19, 2018, 6:56 a.m. UTC
Sometime kernel image and dtb load offsets can overlap due to
dynamically increased Image or dtb size if both load addresses
are near to each other, which leads to bootup failures.

So validate dtb load address and kernel image, if they overlap
do not proceed to boot.

Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>

Comments

Julien Thierry Jan. 19, 2018, 9:49 a.m. UTC | #1
Hi Lingutla,

On 19/01/18 06:56, Lingutla Chandrasekhar wrote:
> Sometime kernel image and dtb load offsets can overlap due to
> dynamically increased Image or dtb size if both load addresses
> are near to each other, which leads to bootup failures.
> 
> So validate dtb load address and kernel image, if they overlap
> do not proceed to boot.
> 
> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>
> 
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 30ad2f085d1f..c9dd699f09ab 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -181,11 +181,17 @@ static void __init smp_build_mpidr_hash(void)
>   static void __init setup_machine_fdt(phys_addr_t dt_phys)
>   {
>   	void *dt_virt = fixmap_remap_fdt(dt_phys);
> +	u64 end_phys = __pa_symbol(_end);
> +	u64 start_phys = __pa_symbol(_text);
>   	const char *name;
>   
> -	if (!dt_virt || !early_init_dt_scan(dt_virt)) {
> +	if (!dt_virt || ((dt_phys < start_phys) &&
> +		((dt_phys + fdt_totalsize(dt_virt)) > start_phys)) ||
> +		((dt_phys > start_phys) && (dt_phys < end_phys)) ||
> +		!early_init_dt_scan(dt_virt)) {
>   		pr_crit("\n"
>   			"Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n"
> +			"The dtb load address should not overlap with kernel image\n"
>   			"The dtb must be 8-byte aligned and must not exceed 2 MB in size\n"
>   			"\nPlease check your bootloader.",
>   			&dt_phys, dt_virt);
> 

Could this be checked separately?

When you get that error message you don't know whether the issue is an 
ill formed dtb or overlapping with the kernel.

So it'd be nice to have a separate message (and maybe add info of how 
many bytes are overlapping?).

Cheers,
Chandrasekhar L Jan. 19, 2018, 10:33 a.m. UTC | #2
Hi Julien,
On 1/19/2018 3:19 PM, Julien Thierry wrote:
> Hi Lingutla,
>
> On 19/01/18 06:56, Lingutla Chandrasekhar wrote:
>> Sometime kernel image and dtb load offsets can overlap due to
>> dynamically increased Image or dtb size if both load addresses
>> are near to each other, which leads to bootup failures.
>>
>> So validate dtb load address and kernel image, if they overlap
>> do not proceed to boot.
>>
>> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>
>>
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index 30ad2f085d1f..c9dd699f09ab 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -181,11 +181,17 @@ static void __init smp_build_mpidr_hash(void)
>>   static void __init setup_machine_fdt(phys_addr_t dt_phys)
>>   {
>>       void *dt_virt = fixmap_remap_fdt(dt_phys);
>> +    u64 end_phys = __pa_symbol(_end);
>> +    u64 start_phys = __pa_symbol(_text);
>>       const char *name;
>>   -    if (!dt_virt || !early_init_dt_scan(dt_virt)) {
>> +    if (!dt_virt || ((dt_phys < start_phys) &&
>> +        ((dt_phys + fdt_totalsize(dt_virt)) > start_phys)) ||
>> +        ((dt_phys > start_phys) && (dt_phys < end_phys)) ||
>> +        !early_init_dt_scan(dt_virt)) {
>>           pr_crit("\n"
>>               "Error: invalid device tree blob at physical address 
>> %pa (virtual address 0x%p)\n"
>> +            "The dtb load address should not overlap with kernel 
>> image\n"
>>               "The dtb must be 8-byte aligned and must not exceed 2 
>> MB in size\n"
>>               "\nPlease check your bootloader.",
>>               &dt_phys, dt_virt);
>>
>
> Could this be checked separately?
>
> When you get that error message you don't know whether the issue is an 
> ill formed dtb or overlapping with the kernel.
>
> So it'd be nice to have a separate message (and maybe add info of how 
> many bytes are overlapping?).
>
Sure, will do.
> Cheers,
>
Ard Biesheuvel Jan. 19, 2018, 2:03 p.m. UTC | #3
On 19 January 2018 at 06:56, Lingutla Chandrasekhar
<clingutla@codeaurora.org> wrote:
> Sometime kernel image and dtb load offsets can overlap due to
> dynamically increased Image or dtb size if both load addresses
> are near to each other, which leads to bootup failures.
>
> So validate dtb load address and kernel image, if they overlap
> do not proceed to boot.
>
> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>
>
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 30ad2f085d1f..c9dd699f09ab 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -181,11 +181,17 @@ static void __init smp_build_mpidr_hash(void)
>  static void __init setup_machine_fdt(phys_addr_t dt_phys)
>  {
>         void *dt_virt = fixmap_remap_fdt(dt_phys);
> +       u64 end_phys = __pa_symbol(_end);
> +       u64 start_phys = __pa_symbol(_text);
>         const char *name;
>
> -       if (!dt_virt || !early_init_dt_scan(dt_virt)) {
> +       if (!dt_virt || ((dt_phys < start_phys) &&
> +               ((dt_phys + fdt_totalsize(dt_virt)) > start_phys)) ||
> +               ((dt_phys > start_phys) && (dt_phys < end_phys)) ||
> +               !early_init_dt_scan(dt_virt)) {
>                 pr_crit("\n"
>                         "Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n"
> +                       "The dtb load address should not overlap with kernel image\n"
>                         "The dtb must be 8-byte aligned and must not exceed 2 MB in size\n"
>                         "\nPlease check your bootloader.",
>                         &dt_phys, dt_virt);

Paradoxically, this pr_crit() will only ever be able to produce
readable output if early_init_dt_scan() returns success. If not, no
early DT parsing will have occurred, and we will have no idea where
the UART is nor whether earlycon was passed via /chosen/bootargs.

That means adding the check before the call to early_init_dt_scan() is
not very helpful, since the system will just hang with no output.

If you really want to check for overlap, please do it *after* the
if(), as a separate WARN_ON() that does not force the boot to fail.
This has a much higher likelihood of anyone ever reading your error
message, and being able to do something about it.

Also, checking for overlap can be done using two comparisons:

end1 > start2 && end2 > start1
Chandrasekhar L Jan. 22, 2018, 6:53 a.m. UTC | #4
On 1/19/2018 7:33 PM, Ard Biesheuvel wrote:
> On 19 January 2018 at 06:56, Lingutla Chandrasekhar
> <clingutla@codeaurora.org> wrote:
>> Sometime kernel image and dtb load offsets can overlap due to
>> dynamically increased Image or dtb size if both load addresses
>> are near to each other, which leads to bootup failures.
>>
>> So validate dtb load address and kernel image, if they overlap
>> do not proceed to boot.
>>
>> Signed-off-by: Lingutla Chandrasekhar <clingutla@codeaurora.org>
>>
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index 30ad2f085d1f..c9dd699f09ab 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -181,11 +181,17 @@ static void __init smp_build_mpidr_hash(void)
>>   static void __init setup_machine_fdt(phys_addr_t dt_phys)
>>   {
>>          void *dt_virt = fixmap_remap_fdt(dt_phys);
>> +       u64 end_phys = __pa_symbol(_end);
>> +       u64 start_phys = __pa_symbol(_text);
>>          const char *name;
>>
>> -       if (!dt_virt || !early_init_dt_scan(dt_virt)) {
>> +       if (!dt_virt || ((dt_phys < start_phys) &&
>> +               ((dt_phys + fdt_totalsize(dt_virt)) > start_phys)) ||
>> +               ((dt_phys > start_phys) && (dt_phys < end_phys)) ||
>> +               !early_init_dt_scan(dt_virt)) {
>>                  pr_crit("\n"
>>                          "Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n"
>> +                       "The dtb load address should not overlap with kernel image\n"
>>                          "The dtb must be 8-byte aligned and must not exceed 2 MB in size\n"
>>                          "\nPlease check your bootloader.",
>>                          &dt_phys, dt_virt);
> Paradoxically, this pr_crit() will only ever be able to produce
> readable output if early_init_dt_scan() returns success. If not, no
> early DT parsing will have occurred, and we will have no idea where
> the UART is nor whether earlycon was passed via /chosen/bootargs.
>
> That means adding the check before the call to early_init_dt_scan() is
> not very helpful, since the system will just hang with no output.
>
> If you really want to check for overlap, please do it *after* the
> if(), as a separate WARN_ON() that does not force the boot to fail.
> This has a much higher likelihood of anyone ever reading your error
> message, and being able to do something about it.
If overlap happens, we can't predict the behavior,
ex: in our case, image size increased such that dtb load offset falls 
under bss section,
because of that, initial dtb is been zeroed while clearing the bss 
section, which fails
the early_init_dt_scan() and system hangs there.

In this case, If we put check *after* the if(), it would not be useful 
right?
I think, if we have all check together,  based on PC we could find 
possible failures of system hang.
And if early_init_dt_scan() is success, then we would get overlapped 
bytes (new patchset have this) at pr_crit().
> Also, checking for overlap can be done using two comparisons:
>
> end1 > start2 && end2 > start1
sure, will update.
diff mbox

Patch

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 30ad2f085d1f..c9dd699f09ab 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -181,11 +181,17 @@  static void __init smp_build_mpidr_hash(void)
 static void __init setup_machine_fdt(phys_addr_t dt_phys)
 {
 	void *dt_virt = fixmap_remap_fdt(dt_phys);
+	u64 end_phys = __pa_symbol(_end);
+	u64 start_phys = __pa_symbol(_text);
 	const char *name;
 
-	if (!dt_virt || !early_init_dt_scan(dt_virt)) {
+	if (!dt_virt || ((dt_phys < start_phys) &&
+		((dt_phys + fdt_totalsize(dt_virt)) > start_phys)) ||
+		((dt_phys > start_phys) && (dt_phys < end_phys)) ||
+		!early_init_dt_scan(dt_virt)) {
 		pr_crit("\n"
 			"Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n"
+			"The dtb load address should not overlap with kernel image\n"
 			"The dtb must be 8-byte aligned and must not exceed 2 MB in size\n"
 			"\nPlease check your bootloader.",
 			&dt_phys, dt_virt);