[PATCH 0/2] efi: small hii conformance fix
Hi,
The following couple of patches fixes a small UEFI HII conformance issue and adds a selftest demonstrating the issue. This is sent in this order to avoid breaking `bootefi selftest' in the middle but feel free to apply in any order if preferred.
Best regards, Vincent.
Vincent Stehlé (2): efi_loader: fix get_package_list_handle() status efi_selftest: add hii database protocol test case
lib/efi_loader/efi_hii.c | 2 +- lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-)
When the HII protocol function get_package_list_handle() is called with an invalid package list handle, it returns EFI_NOT_FOUND but this is not in its list of possible status codes as per the EFI specification. Return EFI_INVALID_PARAMETER instead to fix conformance.
Signed-off-by: Vincent Stehlé vincent.stehle@arm.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/efi_loader/efi_hii.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c index 75ff58aafa5..27db3be6a17 100644 --- a/lib/efi_loader/efi_hii.c +++ b/lib/efi_loader/efi_hii.c @@ -780,7 +780,7 @@ get_package_list_handle(const struct efi_hii_database_protocol *this, } }
- return EFI_EXIT(EFI_NOT_FOUND); + return EFI_EXIT(EFI_INVALID_PARAMETER); }
const struct efi_hii_database_protocol efi_hii_database = {
On Tue, Dec 13, 2022 at 10:39:09PM +0100, Vincent Stehl?? wrote:
When the HII protocol function get_package_list_handle() is called with an invalid package list handle, it returns EFI_NOT_FOUND but this is not in its list of possible status codes as per the EFI specification. Return EFI_INVALID_PARAMETER instead to fix conformance.
Thank you for the heads-up. You're right as far as the EFI specification is concerned.
FYI, EDK-II also returns EFI_NOT_FOUND if the package list handle is *valid* and yet we cannot find any matching handle in the HII database. See MdeModulePkg/Universal/HiiDatabaseDxe/Database.c
-Takahiro Akashi
Signed-off-by: Vincent Stehlé vincent.stehle@arm.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Ilias Apalodimas ilias.apalodimas@linaro.org
lib/efi_loader/efi_hii.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c index 75ff58aafa5..27db3be6a17 100644 --- a/lib/efi_loader/efi_hii.c +++ b/lib/efi_loader/efi_hii.c @@ -780,7 +780,7 @@ get_package_list_handle(const struct efi_hii_database_protocol *this, } }
- return EFI_EXIT(EFI_NOT_FOUND);
- return EFI_EXIT(EFI_INVALID_PARAMETER);
}
const struct efi_hii_database_protocol efi_hii_database = {
2.35.1
On 12/13/22 22:39, Vincent Stehlé wrote:
When the HII protocol function get_package_list_handle() is called with an invalid package list handle, it returns EFI_NOT_FOUND but this is not in its list of possible status codes as per the EFI specification. Return EFI_INVALID_PARAMETER instead to fix conformance.
Signed-off-by: Vincent Stehlé vincent.stehle@arm.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Ilias Apalodimas ilias.apalodimas@linaro.org
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
Add a test for the case when the HII database protocol get_package_list_handle() function is called with an invalid package list handle.
Signed-off-by: Vincent Stehlé vincent.stehle@arm.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/lib/efi_selftest/efi_selftest_hii.c b/lib/efi_selftest/efi_selftest_hii.c index eaf3b0995d4..8a038d9f534 100644 --- a/lib/efi_selftest/efi_selftest_hii.c +++ b/lib/efi_selftest/efi_selftest_hii.c @@ -605,6 +605,16 @@ static int test_hii_database_get_package_list_handle(void) goto out; }
+ /* Invalid package list handle. */ + driver_handle = NULL; + ret = hii_database_protocol->get_package_list_handle( + hii_database_protocol, NULL, &driver_handle); + if (ret != EFI_INVALID_PARAMETER) { + efi_st_error("get_package_list_handle returned %u not invalid\n", + (unsigned int)ret); + goto out; + } + result = EFI_ST_SUCCESS;
out:
On 12/13/22 22:39, Vincent Stehlé wrote:
Add a test for the case when the HII database protocol get_package_list_handle() function is called with an invalid package list handle.
Signed-off-by: Vincent Stehlé vincent.stehle@arm.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Ilias Apalodimas ilias.apalodimas@linaro.org
lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/lib/efi_selftest/efi_selftest_hii.c b/lib/efi_selftest/efi_selftest_hii.c index eaf3b0995d4..8a038d9f534 100644 --- a/lib/efi_selftest/efi_selftest_hii.c +++ b/lib/efi_selftest/efi_selftest_hii.c @@ -605,6 +605,16 @@ static int test_hii_database_get_package_list_handle(void) goto out; }
- /* Invalid package list handle. */
- driver_handle = NULL;
- ret = hii_database_protocol->get_package_list_handle(
hii_database_protocol, NULL, &driver_handle);- if (ret != EFI_INVALID_PARAMETER) {
Here it is unclear, if you get EFI_INVALID_PARAMETER because the PackageListHandle is invalid or DriverHandle is NULL.
We should test both cases separately.
Best regards
Heinrich
efi_st_error("get_package_list_handle returned %u not invalid\n",(unsigned int)ret);goto out;}
result = EFI_ST_SUCCESS;
out:
On Thu, Dec 22, 2022 at 10:45:22AM +0100, Heinrich Schuchardt wrote:
Hi Heinrich,
Happy new year, and thank you for the reviews. More comments below.
On 12/13/22 22:39, Vincent Stehlé wrote:
Add a test for the case when the HII database protocol get_package_list_handle() function is called with an invalid package list handle.
Signed-off-by: Vincent Stehlé vincent.stehle@arm.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Ilias Apalodimas ilias.apalodimas@linaro.org
lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/lib/efi_selftest/efi_selftest_hii.c b/lib/efi_selftest/efi_selftest_hii.c index eaf3b0995d4..8a038d9f534 100644 --- a/lib/efi_selftest/efi_selftest_hii.c +++ b/lib/efi_selftest/efi_selftest_hii.c @@ -605,6 +605,16 @@ static int test_hii_database_get_package_list_handle(void) goto out; }
- /* Invalid package list handle. */
- driver_handle = NULL;
- ret = hii_database_protocol->get_package_list_handle(
hii_database_protocol, NULL, &driver_handle);- if (ret != EFI_INVALID_PARAMETER) {
Here it is unclear, if you get EFI_INVALID_PARAMETER because the PackageListHandle is invalid or DriverHandle is NULL.
In principle, the value used to initialize the (output) parameter driver_handle should not affect the test. Also, we are initializing driver_handle in exactly the same way as was done in the previous test, which we know succeeded.
Anyway, if you think it makes the test more readable I can change the initialization to something like the following:
driver_handle = (efi_handle_t)0x23456789; /* dummy */
We should test both cases separately.
Would you prefer something like the following couple of tests?
/* Invalid package list handle. */ driver_handle = (efi_handle_t)0x23456789; /* dummy */ ret = hii_database_protocol->get_package_list_handle( hii_database_protocol, NULL, &driver_handle);
/* Invalid driver handle. */ ret = hii_database_protocol->get_package_list_handle( hii_database_protocol, handle, NULL);
I could verify that EFI_INVALID_PARAMETER is indeed returned in both cases. Just let me know and I will post a v2.
Best regards, Vincent.
Best regards
Heinrich
efi_st_error("get_package_list_handle returned %u not invalid\n",(unsigned int)ret);goto out;}
result = EFI_ST_SUCCESS;
out:
On 1/5/23 15:48, Vincent Stehlé wrote:
On Thu, Dec 22, 2022 at 10:45:22AM +0100, Heinrich Schuchardt wrote:
Hi Heinrich,
Happy new year, and thank you for the reviews. More comments below.
On 12/13/22 22:39, Vincent Stehlé wrote:
Add a test for the case when the HII database protocol get_package_list_handle() function is called with an invalid package list handle.
Signed-off-by: Vincent Stehlé vincent.stehle@arm.com Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Ilias Apalodimas ilias.apalodimas@linaro.org
lib/efi_selftest/efi_selftest_hii.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/lib/efi_selftest/efi_selftest_hii.c b/lib/efi_selftest/efi_selftest_hii.c index eaf3b0995d4..8a038d9f534 100644 --- a/lib/efi_selftest/efi_selftest_hii.c +++ b/lib/efi_selftest/efi_selftest_hii.c @@ -605,6 +605,16 @@ static int test_hii_database_get_package_list_handle(void) goto out; }
- /* Invalid package list handle. */
- driver_handle = NULL;
- ret = hii_database_protocol->get_package_list_handle(
hii_database_protocol, NULL, &driver_handle);- if (ret != EFI_INVALID_PARAMETER) {
Here it is unclear, if you get EFI_INVALID_PARAMETER because the PackageListHandle is invalid or DriverHandle is NULL.
In principle, the value used to initialize the (output) parameter driver_handle should not affect the test. Also, we are initializing driver_handle in exactly the same way as was done in the previous test, which we know succeeded.
Anyway, if you think it makes the test more readable I can change the initialization to something like the following:
driver_handle = (efi_handle_t)0x23456789; /* dummy */
We should test both cases separately.
Would you prefer something like the following couple of tests?
/* Invalid package list handle. */ driver_handle = (efi_handle_t)0x23456789; /* dummy */
Let's use an address that never will be allocated. driver_handle = (efi_handle_t)~1UL;
ret = hii_database_protocol->get_package_list_handle( hii_database_protocol, NULL, &driver_handle);
/* Invalid driver handle. */ ret = hii_database_protocol->get_package_list_handle( hii_database_protocol, handle, NULL);
I could verify that EFI_INVALID_PARAMETER is indeed returned in both cases. Just let me know and I will post a v2.
Looks like the right path forward.
Best regards
Heinrich
Best regards, Vincent.
Best regards
Heinrich
efi_st_error("get_package_list_handle returned %u not invalid\n",(unsigned int)ret);goto out;}
result = EFI_ST_SUCCESS;
out:
participants (4)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Heinrich Schuchardt -
Vincent Stehlé