diff mbox series

[U-Boot,1/1] efi_loader: ListPackageLists() return EFI_NOT_FOUND

Message ID 20190617190246.32188-1-xypron.glpk@gmx.de
State Accepted, archived
Delegated to: Heinrich Schuchardt
Headers show
Series [U-Boot,1/1] efi_loader: ListPackageLists() return EFI_NOT_FOUND | expand

Commit Message

Heinrich Schuchardt June 17, 2019, 7:02 p.m. UTC 
If no matching package list is found in ListPackageLists(), return
EFI_NOT_FOUND.

If we do not support a package type, we can immediately leave the function.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_hii.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

--
2.20.1

Comments

AKASHI Takahiro June 18, 2019, 12:44 a.m. UTC | #1 
On Mon, Jun 17, 2019 at 09:02:46PM +0200, Heinrich Schuchardt wrote:
> If no matching package list is found in ListPackageLists(), return
> EFI_NOT_FOUND.
> 
> If we do not support a package type, we can immediately leave the function.

This won't happen because add_packages() eliminates all unsupported
packages.

> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_hii.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
> index 61b71dec62..8ce526e1c8 100644
> --- a/lib/efi_loader/efi_hii.c
> +++ b/lib/efi_loader/efi_hii.c
> @@ -581,18 +581,22 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>  	struct efi_hii_packagelist *hii =
>  				(struct efi_hii_packagelist *)handle;
>  	int package_cnt, package_max;
> -	efi_status_t ret = EFI_SUCCESS;
> +	efi_status_t ret = EFI_NOT_FOUND;
> 
>  	EFI_ENTRY("%p, %u, %pUl, %p, %p", this, package_type, package_guid,
>  		  handle_buffer_length, handle);
> 
>  	if (!handle_buffer_length ||
> -	    (*handle_buffer_length && !handle))
> -		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +	    (*handle_buffer_length && !handle)) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> 
>  	if ((package_type != EFI_HII_PACKAGE_TYPE_GUID && package_guid) ||
> -	    (package_type == EFI_HII_PACKAGE_TYPE_GUID && !package_guid))
> -		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +	    (package_type == EFI_HII_PACKAGE_TYPE_GUID && !package_guid)) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> 
>  	EFI_PRINT("package type=%x, guid=%pUl, length=%zu\n", (int)package_type,
>  		  package_guid, *handle_buffer_length);
> @@ -610,7 +614,7 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>  		case EFI_HII_PACKAGE_FORMS:
>  			EFI_PRINT("Form package not supported\n");
>  			ret = EFI_INVALID_PARAMETER;
> -			continue;
> +			goto out;
>  		case EFI_HII_PACKAGE_STRINGS:
>  			if (!list_empty(&hii->string_tables))
>  				break;
> @@ -618,19 +622,19 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>  		case EFI_HII_PACKAGE_FONTS:
>  			EFI_PRINT("Font package not supported\n");
>  			ret = EFI_INVALID_PARAMETER;
> -			continue;
> +			goto out;
>  		case EFI_HII_PACKAGE_IMAGES:
>  			EFI_PRINT("Image package not supported\n");
>  			ret = EFI_INVALID_PARAMETER;
> -			continue;
> +			goto out;
>  		case EFI_HII_PACKAGE_SIMPLE_FONTS:
>  			EFI_PRINT("Simple font package not supported\n");
>  			ret = EFI_INVALID_PARAMETER;
> -			continue;
> +			goto out;
>  		case EFI_HII_PACKAGE_DEVICE_PATH:
>  			EFI_PRINT("Device path package not supported\n");
>  			ret = EFI_INVALID_PARAMETER;
> -			continue;
> +			goto out;
>  		case EFI_HII_PACKAGE_KEYBOARD_LAYOUT:
>  			if (!list_empty(&hii->keyboard_packages))
>  				break;
> @@ -638,7 +642,7 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>  		case EFI_HII_PACKAGE_ANIMATIONS:
>  			EFI_PRINT("Animation package not supported\n");
>  			ret = EFI_INVALID_PARAMETER;
> -			continue;
> +			goto out;
>  		case EFI_HII_PACKAGE_END:
>  		case EFI_HII_PACKAGE_TYPE_SYSTEM_BEGIN:
>  		case EFI_HII_PACKAGE_TYPE_SYSTEM_END:
> @@ -647,13 +651,15 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>  		}
> 
>  		package_cnt++;
> -		if (package_cnt <= package_max)
> +		if (package_cnt <= package_max) {
>  			*handle++ = hii;
> -		else
> +			ret = EFI_SUCCESS;
> +		} else {
>  			ret = EFI_BUFFER_TOO_SMALL;
> +		}

It will be enough that this check be done only once at the end of
this function.

>  	}
>  	*handle_buffer_length = package_cnt * sizeof(*handle);

here,
        if (package_cnt > package_max)
                ret = EFI_BUFFER_TOO_SMALL;

- Takahiro Akashi

> -
> +out:
>  	return EFI_EXIT(ret);
>  }
> 
> --
> 2.20.1
>
Heinrich Schuchardt June 18, 2019, 5:41 a.m. UTC | #2 
On 6/18/19 2:44 AM, AKASHI Takahiro wrote:
> On Mon, Jun 17, 2019 at 09:02:46PM +0200, Heinrich Schuchardt wrote:
>> If no matching package list is found in ListPackageLists(), return
>> EFI_NOT_FOUND.
>>
>> If we do not support a package type, we can immediately leave the function.
>
> This won't happen because add_packages() eliminates all unsupported
> packages.

The caller still can pass an unsupported package type.

Regards

Heinrich

>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   lib/efi_loader/efi_hii.c | 34 ++++++++++++++++++++--------------
>>   1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
>> index 61b71dec62..8ce526e1c8 100644
>> --- a/lib/efi_loader/efi_hii.c
>> +++ b/lib/efi_loader/efi_hii.c
>> @@ -581,18 +581,22 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>>   	struct efi_hii_packagelist *hii =
>>   				(struct efi_hii_packagelist *)handle;
>>   	int package_cnt, package_max;
>> -	efi_status_t ret = EFI_SUCCESS;
>> +	efi_status_t ret = EFI_NOT_FOUND;
>>
>>   	EFI_ENTRY("%p, %u, %pUl, %p, %p", this, package_type, package_guid,
>>   		  handle_buffer_length, handle);
>>
>>   	if (!handle_buffer_length ||
>> -	    (*handle_buffer_length && !handle))
>> -		return EFI_EXIT(EFI_INVALID_PARAMETER);
>> +	    (*handle_buffer_length && !handle)) {
>> +		ret = EFI_INVALID_PARAMETER;
>> +		goto out;
>> +	}
>>
>>   	if ((package_type != EFI_HII_PACKAGE_TYPE_GUID && package_guid) ||
>> -	    (package_type == EFI_HII_PACKAGE_TYPE_GUID && !package_guid))
>> -		return EFI_EXIT(EFI_INVALID_PARAMETER);
>> +	    (package_type == EFI_HII_PACKAGE_TYPE_GUID && !package_guid)) {
>> +		ret = EFI_INVALID_PARAMETER;
>> +		goto out;
>> +	}
>>
>>   	EFI_PRINT("package type=%x, guid=%pUl, length=%zu\n", (int)package_type,
>>   		  package_guid, *handle_buffer_length);
>> @@ -610,7 +614,7 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>>   		case EFI_HII_PACKAGE_FORMS:
>>   			EFI_PRINT("Form package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_STRINGS:
>>   			if (!list_empty(&hii->string_tables))
>>   				break;
>> @@ -618,19 +622,19 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>>   		case EFI_HII_PACKAGE_FONTS:
>>   			EFI_PRINT("Font package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_IMAGES:
>>   			EFI_PRINT("Image package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_SIMPLE_FONTS:
>>   			EFI_PRINT("Simple font package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_DEVICE_PATH:
>>   			EFI_PRINT("Device path package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_KEYBOARD_LAYOUT:
>>   			if (!list_empty(&hii->keyboard_packages))
>>   				break;
>> @@ -638,7 +642,7 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>>   		case EFI_HII_PACKAGE_ANIMATIONS:
>>   			EFI_PRINT("Animation package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_END:
>>   		case EFI_HII_PACKAGE_TYPE_SYSTEM_BEGIN:
>>   		case EFI_HII_PACKAGE_TYPE_SYSTEM_END:
>> @@ -647,13 +651,15 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>>   		}
>>
>>   		package_cnt++;
>> -		if (package_cnt <= package_max)
>> +		if (package_cnt <= package_max) {
>>   			*handle++ = hii;
>> -		else
>> +			ret = EFI_SUCCESS;
>> +		} else {
>>   			ret = EFI_BUFFER_TOO_SMALL;
>> +		}
>
> It will be enough that this check be done only once at the end of
> this function.
>
>>   	}
>>   	*handle_buffer_length = package_cnt * sizeof(*handle);
>
> here,
>          if (package_cnt > package_max)
>                  ret = EFI_BUFFER_TOO_SMALL;
>
> - Takahiro Akashi
>
>> -
>> +out:
>>   	return EFI_EXIT(ret);
>>   }
>>
>> --
>> 2.20.1
>>
>
Heinrich Schuchardt June 18, 2019, 5:58 a.m. UTC | #3 
On 6/18/19 2:44 AM, AKASHI Takahiro wrote:
> On Mon, Jun 17, 2019 at 09:02:46PM +0200, Heinrich Schuchardt wrote:
>> If no matching package list is found in ListPackageLists(), return
>> EFI_NOT_FOUND.
>>
>> If we do not support a package type, we can immediately leave the function.
>
> This won't happen because add_packages() eliminates all unsupported
> packages.

So we should remove all those EFI_PRINT() statements.

>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   lib/efi_loader/efi_hii.c | 34 ++++++++++++++++++++--------------
>>   1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
>> index 61b71dec62..8ce526e1c8 100644
>> --- a/lib/efi_loader/efi_hii.c
>> +++ b/lib/efi_loader/efi_hii.c
>> @@ -581,18 +581,22 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>>   	struct efi_hii_packagelist *hii =
>>   				(struct efi_hii_packagelist *)handle;
>>   	int package_cnt, package_max;
>> -	efi_status_t ret = EFI_SUCCESS;
>> +	efi_status_t ret = EFI_NOT_FOUND;
>>
>>   	EFI_ENTRY("%p, %u, %pUl, %p, %p", this, package_type, package_guid,
>>   		  handle_buffer_length, handle);
>>
>>   	if (!handle_buffer_length ||
>> -	    (*handle_buffer_length && !handle))
>> -		return EFI_EXIT(EFI_INVALID_PARAMETER);
>> +	    (*handle_buffer_length && !handle)) {
>> +		ret = EFI_INVALID_PARAMETER;
>> +		goto out;
>> +	}
>>
>>   	if ((package_type != EFI_HII_PACKAGE_TYPE_GUID && package_guid) ||
>> -	    (package_type == EFI_HII_PACKAGE_TYPE_GUID && !package_guid))
>> -		return EFI_EXIT(EFI_INVALID_PARAMETER);
>> +	    (package_type == EFI_HII_PACKAGE_TYPE_GUID && !package_guid)) {
>> +		ret = EFI_INVALID_PARAMETER;
>> +		goto out;
>> +	}
>>
>>   	EFI_PRINT("package type=%x, guid=%pUl, length=%zu\n", (int)package_type,
>>   		  package_guid, *handle_buffer_length);
>> @@ -610,7 +614,7 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>>   		case EFI_HII_PACKAGE_FORMS:
>>   			EFI_PRINT("Form package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_STRINGS:
>>   			if (!list_empty(&hii->string_tables))
>>   				break;
>> @@ -618,19 +622,19 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>>   		case EFI_HII_PACKAGE_FONTS:
>>   			EFI_PRINT("Font package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_IMAGES:
>>   			EFI_PRINT("Image package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_SIMPLE_FONTS:
>>   			EFI_PRINT("Simple font package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_DEVICE_PATH:
>>   			EFI_PRINT("Device path package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_KEYBOARD_LAYOUT:
>>   			if (!list_empty(&hii->keyboard_packages))
>>   				break;
>> @@ -638,7 +642,7 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>>   		case EFI_HII_PACKAGE_ANIMATIONS:
>>   			EFI_PRINT("Animation package not supported\n");
>>   			ret = EFI_INVALID_PARAMETER;
>> -			continue;
>> +			goto out;
>>   		case EFI_HII_PACKAGE_END:
>>   		case EFI_HII_PACKAGE_TYPE_SYSTEM_BEGIN:
>>   		case EFI_HII_PACKAGE_TYPE_SYSTEM_END:
>> @@ -647,13 +651,15 @@ list_package_lists(const struct efi_hii_database_protocol *this,
>>   		}
>>
>>   		package_cnt++;
>> -		if (package_cnt <= package_max)
>> +		if (package_cnt <= package_max) {
>>   			*handle++ = hii;
>> -		else
>> +			ret = EFI_SUCCESS;
>> +		} else {
>>   			ret = EFI_BUFFER_TOO_SMALL;
>> +		}
>
> It will be enough that this check be done only once at the end of
> this function.

For copying to *handle we need the `if'. The code will remain shorter if
we do not introduce a second `if' after the loop.

Regards

Heinrich

>
>>   	}
>>   	*handle_buffer_length = package_cnt * sizeof(*handle);
>
> here,
>          if (package_cnt > package_max)
>                  ret = EFI_BUFFER_TOO_SMALL;
>
> - Takahiro Akashi
>
>> -
>> +out:
>>   	return EFI_EXIT(ret);
>>   }
>>
>> --
>> 2.20.1
>>
>
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
index 61b71dec62..8ce526e1c8 100644
--- a/lib/efi_loader/efi_hii.c
+++ b/lib/efi_loader/efi_hii.c
@@ -581,18 +581,22 @@  list_package_lists(const struct efi_hii_database_protocol *this,
 	struct efi_hii_packagelist *hii =
 				(struct efi_hii_packagelist *)handle;
 	int package_cnt, package_max;
-	efi_status_t ret = EFI_SUCCESS;
+	efi_status_t ret = EFI_NOT_FOUND;

 	EFI_ENTRY("%p, %u, %pUl, %p, %p", this, package_type, package_guid,
 		  handle_buffer_length, handle);

 	if (!handle_buffer_length ||
-	    (*handle_buffer_length && !handle))
-		return EFI_EXIT(EFI_INVALID_PARAMETER);
+	    (*handle_buffer_length && !handle)) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}

 	if ((package_type != EFI_HII_PACKAGE_TYPE_GUID && package_guid) ||
-	    (package_type == EFI_HII_PACKAGE_TYPE_GUID && !package_guid))
-		return EFI_EXIT(EFI_INVALID_PARAMETER);
+	    (package_type == EFI_HII_PACKAGE_TYPE_GUID && !package_guid)) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}

 	EFI_PRINT("package type=%x, guid=%pUl, length=%zu\n", (int)package_type,
 		  package_guid, *handle_buffer_length);
@@ -610,7 +614,7 @@  list_package_lists(const struct efi_hii_database_protocol *this,
 		case EFI_HII_PACKAGE_FORMS:
 			EFI_PRINT("Form package not supported\n");
 			ret = EFI_INVALID_PARAMETER;
-			continue;
+			goto out;
 		case EFI_HII_PACKAGE_STRINGS:
 			if (!list_empty(&hii->string_tables))
 				break;
@@ -618,19 +622,19 @@  list_package_lists(const struct efi_hii_database_protocol *this,
 		case EFI_HII_PACKAGE_FONTS:
 			EFI_PRINT("Font package not supported\n");
 			ret = EFI_INVALID_PARAMETER;
-			continue;
+			goto out;
 		case EFI_HII_PACKAGE_IMAGES:
 			EFI_PRINT("Image package not supported\n");
 			ret = EFI_INVALID_PARAMETER;
-			continue;
+			goto out;
 		case EFI_HII_PACKAGE_SIMPLE_FONTS:
 			EFI_PRINT("Simple font package not supported\n");
 			ret = EFI_INVALID_PARAMETER;
-			continue;
+			goto out;
 		case EFI_HII_PACKAGE_DEVICE_PATH:
 			EFI_PRINT("Device path package not supported\n");
 			ret = EFI_INVALID_PARAMETER;
-			continue;
+			goto out;
 		case EFI_HII_PACKAGE_KEYBOARD_LAYOUT:
 			if (!list_empty(&hii->keyboard_packages))
 				break;
@@ -638,7 +642,7 @@  list_package_lists(const struct efi_hii_database_protocol *this,
 		case EFI_HII_PACKAGE_ANIMATIONS:
 			EFI_PRINT("Animation package not supported\n");
 			ret = EFI_INVALID_PARAMETER;
-			continue;
+			goto out;
 		case EFI_HII_PACKAGE_END:
 		case EFI_HII_PACKAGE_TYPE_SYSTEM_BEGIN:
 		case EFI_HII_PACKAGE_TYPE_SYSTEM_END:
@@ -647,13 +651,15 @@  list_package_lists(const struct efi_hii_database_protocol *this,
 		}

 		package_cnt++;
-		if (package_cnt <= package_max)
+		if (package_cnt <= package_max) {
 			*handle++ = hii;
-		else
+			ret = EFI_SUCCESS;
+		} else {
 			ret = EFI_BUFFER_TOO_SMALL;
+		}
 	}
 	*handle_buffer_length = package_cnt * sizeof(*handle);
-
+out:
 	return EFI_EXIT(ret);
 }