[PATCH v2] cmd: abootimg: Prevent use of unintialised variable

[PATCH v2] cmd: abootimg: Prevent use of unintialised variable

[Andrew Goodbody]

vhdr can be used when not initialised so detect this condition
and exit early to prevent the problem.

This issue was found with Smatch.

Fixes: 636da2039aea (android: boot: support boot image header version 3 and 4)
Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
---
Changes in v2:
- Add unmap_sysmem(hdr) in the new exit path
- Link to v1: https://lore.kernel.org/r/20250625-abootimg_fix-v1-1-ce1645ac9879@linaro.org
---
 cmd/abootimg.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/cmd/abootimg.c b/cmd/abootimg.c
index ae7a1a7c83b..31620aa3cfa 100644
--- a/cmd/abootimg.c
+++ b/cmd/abootimg.c
@@ -98,18 +98,20 @@ static int abootimg_get_dtb_load_addr(int argc, char *const argv[])
 	const struct andr_vnd_boot_img_hdr *vhdr;
 
 	hdr = map_sysmem(abootimg_addr(), sizeof(*hdr));
-	if (get_avendor_bootimg_addr() != -1)
+	if (get_avendor_bootimg_addr() != -1) {
 		vhdr = map_sysmem(get_avendor_bootimg_addr(), sizeof(*vhdr));
+	} else {
+		unmap_sysmem(hdr);
+		return CMD_RET_FAILURE;
+	}
 
 	if (!android_image_get_data(hdr, vhdr, &img_data)) {
-		if (get_avendor_bootimg_addr() != -1)
-			unmap_sysmem(vhdr);
+		unmap_sysmem(vhdr);
 		unmap_sysmem(hdr);
 		return CMD_RET_FAILURE;
 	}
 
-	if (get_avendor_bootimg_addr() != -1)
-		unmap_sysmem(vhdr);
+	unmap_sysmem(vhdr);
 	unmap_sysmem(hdr);
 
 	if (img_data.header_version < 2) {

---
base-commit: 903eb123236ccbd8ef05d43507a2a910b785bd56
change-id: 20250625-abootimg_fix-51600dc8356a

Best regards,
-- 
Andrew Goodbody <andrew.goodbody@linaro.org>

Re: [PATCH v2] cmd: abootimg: Prevent use of unintialised variable

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

On Wed, Jun 25, 2025 at 04:52:41PM +0100, Andrew Goodbody wrote:

> vhdr can be used when not initialised so detect this condition
> and exit early to prevent the problem.
> 
> This issue was found with Smatch.
> 
> Fixes: 636da2039aea (android: boot: support boot image header version 3 and 4)
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2] cmd: abootimg: Prevent use of unintialised variable

[Mattijs Korpershoek]

Hi Andrew,

Thank you for the patch.

On Wed, Jun 25, 2025 at 16:52, Andrew Goodbody <andrew.goodbody@linaro.org> wrote:

> vhdr can be used when not initialised so detect this condition
> and exit early to prevent the problem.
>
> This issue was found with Smatch.
>
> Fixes: 636da2039aea (android: boot: support boot image header version 3 and 4)
> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
> ---
> Changes in v2:
> - Add unmap_sysmem(hdr) in the new exit path
> - Link to v1: https://lore.kernel.org/r/20250625-abootimg_fix-v1-1-ce1645ac9879@linaro.org
> ---
>  cmd/abootimg.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/cmd/abootimg.c b/cmd/abootimg.c
> index ae7a1a7c83b..31620aa3cfa 100644
> --- a/cmd/abootimg.c
> +++ b/cmd/abootimg.c
> @@ -98,18 +98,20 @@ static int abootimg_get_dtb_load_addr(int argc, char *const argv[])
>  	const struct andr_vnd_boot_img_hdr *vhdr;
>  
>  	hdr = map_sysmem(abootimg_addr(), sizeof(*hdr));
> -	if (get_avendor_bootimg_addr() != -1)
> +	if (get_avendor_bootimg_addr() != -1) {
>  		vhdr = map_sysmem(get_avendor_bootimg_addr(), sizeof(*vhdr));
> +	} else {
> +		unmap_sysmem(hdr);
> +		return CMD_RET_FAILURE;
> +	}

It's valid for avendor_bootimg_adr() to be -1.
This is the case for boot image v2 and lower, where there is no
boot_vendor partition. (so there is no valid vhdr).

Per my understanding vhdr* is NULL if left unintialised. (I might be
wrong, please let me know).

android_image_get_data() handles the case where vendor_boot_hdr is NULL:
"""
	if (((struct andr_boot_img_hdr_v0 *)boot_hdr)->header_version > 2) {
		if (!vendor_boot_hdr) {
			printf("For boot header v3+ vendor boot image has to be provided\n");
			return false;
		}
"""

So I don't think we should early return here.
If we do, we will probably break boot image v2 support.

>  
>  	if (!android_image_get_data(hdr, vhdr, &img_data)) {
> -		if (get_avendor_bootimg_addr() != -1)
> -			unmap_sysmem(vhdr);
> +		unmap_sysmem(vhdr);
>  		unmap_sysmem(hdr);
>  		return CMD_RET_FAILURE;
>  	}
>  
> -	if (get_avendor_bootimg_addr() != -1)
> -		unmap_sysmem(vhdr);
> +	unmap_sysmem(vhdr);
>  	unmap_sysmem(hdr);
>  
>  	if (img_data.header_version < 2) {
>
> ---
> base-commit: 903eb123236ccbd8ef05d43507a2a910b785bd56
> change-id: 20250625-abootimg_fix-51600dc8356a
>
> Best regards,
> -- 
> Andrew Goodbody <andrew.goodbody@linaro.org>

Re: [PATCH v2] cmd: abootimg: Prevent use of unintialised variable

[Andrew Goodbody]

On 25/06/2025 17:10, Mattijs Korpershoek wrote:
> Hi Andrew,
> 
> Thank you for the patch.
> 
> On Wed, Jun 25, 2025 at 16:52, Andrew Goodbody <andrew.goodbody@linaro.org> wrote:
> 
>> vhdr can be used when not initialised so detect this condition
>> and exit early to prevent the problem.
>>
>> This issue was found with Smatch.
>>
>> Fixes: 636da2039aea (android: boot: support boot image header version 3 and 4)
>> Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
>> ---
>> Changes in v2:
>> - Add unmap_sysmem(hdr) in the new exit path
>> - Link to v1: https://lore.kernel.org/r/20250625-abootimg_fix-v1-1-ce1645ac9879@linaro.org
>> ---
>>   cmd/abootimg.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/cmd/abootimg.c b/cmd/abootimg.c
>> index ae7a1a7c83b..31620aa3cfa 100644
>> --- a/cmd/abootimg.c
>> +++ b/cmd/abootimg.c
>> @@ -98,18 +98,20 @@ static int abootimg_get_dtb_load_addr(int argc, char *const argv[])
>>   	const struct andr_vnd_boot_img_hdr *vhdr;
>>   
>>   	hdr = map_sysmem(abootimg_addr(), sizeof(*hdr));
>> -	if (get_avendor_bootimg_addr() != -1)
>> +	if (get_avendor_bootimg_addr() != -1) {
>>   		vhdr = map_sysmem(get_avendor_bootimg_addr(), sizeof(*vhdr));
>> +	} else {
>> +		unmap_sysmem(hdr);
>> +		return CMD_RET_FAILURE;
>> +	}
> 
> It's valid for avendor_bootimg_adr() to be -1.
> This is the case for boot image v2 and lower, where there is no
> boot_vendor partition. (so there is no valid vhdr).
> 
> Per my understanding vhdr* is NULL if left unintialised. (I might be
> wrong, please let me know).

A local uninitialised variable is just allocated space on the stack so 
its value is undefined AFAIK.

> android_image_get_data() handles the case where vendor_boot_hdr is NULL:
> """
> 	if (((struct andr_boot_img_hdr_v0 *)boot_hdr)->header_version > 2) {
> 		if (!vendor_boot_hdr) {
> 			printf("For boot header v3+ vendor boot image has to be provided\n");
> 			return false;
> 		}
> """
> 
> So I don't think we should early return here.
> If we do, we will probably break boot image v2 support.

Well I guess the alternate fix is to just init vhdr to be NULL?

Andrew

>>   
>>   	if (!android_image_get_data(hdr, vhdr, &img_data)) {
>> -		if (get_avendor_bootimg_addr() != -1)
>> -			unmap_sysmem(vhdr);
>> +		unmap_sysmem(vhdr);
>>   		unmap_sysmem(hdr);
>>   		return CMD_RET_FAILURE;
>>   	}
>>   
>> -	if (get_avendor_bootimg_addr() != -1)
>> -		unmap_sysmem(vhdr);
>> +	unmap_sysmem(vhdr);
>>   	unmap_sysmem(hdr);
>>   
>>   	if (img_data.header_version < 2) {
>>
>> ---
>> base-commit: 903eb123236ccbd8ef05d43507a2a910b785bd56
>> change-id: 20250625-abootimg_fix-51600dc8356a
>>
>> Best regards,
>> -- 
>> Andrew Goodbody <andrew.goodbody@linaro.org>

Re: [PATCH v2] cmd: abootimg: Prevent use of unintialised variable

[Mattijs Korpershoek]

On Wed, Jun 25, 2025 at 17:31, Andrew Goodbody <andrew.goodbody@linaro.org> wrote:

> On 25/06/2025 17:10, Mattijs Korpershoek wrote:

[...]

>
>> android_image_get_data() handles the case where vendor_boot_hdr is NULL:
>> """
>> 	if (((struct andr_boot_img_hdr_v0 *)boot_hdr)->header_version > 2) {
>> 		if (!vendor_boot_hdr) {
>> 			printf("For boot header v3+ vendor boot image has to be provided\n");
>> 			return false;
>> 		}
>> """
>> 
>> So I don't think we should early return here.
>> If we do, we will probably break boot image v2 support.
>
> Well I guess the alternate fix is to just init vhdr to be NULL?

Yes, that would be better.

>
> Andrew
>
>>>   

[...]