[PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[Lingutla Chandrasekhar]

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);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
 a Linux Foundation Collaborative Project.

[PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[Lingutla Chandrasekhar]

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@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);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
 a Linux Foundation Collaborative Project.

Re: [PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[Julien Thierry]

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,

-- 
Julien Thierry

[PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[Julien Thierry]

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,

-- 
Julien Thierry

Re: [PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[Chandra Sekhar Lingutla]

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,
>

[PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[Chandra Sekhar Lingutla]

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,
>

Re: [PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[Ard Biesheuvel]

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

[PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[Ard Biesheuvel]

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

Re: [PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[Chandra Sekhar Lingutla]

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.

[PATCH] arm64: setup: Check for overlapping dtb and Image load addresses

[Chandra Sekhar Lingutla]

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.