[PATCH 0/2] efi: small hii conformance fix

[PATCH 0/2] efi: small hii conformance fix

[Vincent Stehlé]

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(-)

-- 
2.35.1

[PATCH 1/2] efi_loader: fix get_package_list_handle() status

[Vincent Stehlé]

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 = {
-- 
2.35.1

Re: [PATCH 1/2] efi_loader: fix get_package_list_handle() status

[AKASHI Takahiro]

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
> 

Re: [PATCH 1/2] efi_loader: fix get_package_list_handle() status

[Heinrich Schuchardt]

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>

[PATCH 2/2] efi_selftest: add hii database protocol test case

[Vincent Stehlé]

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:
-- 
2.35.1

Re: [PATCH 2/2] efi_selftest: add hii database protocol test case

[Heinrich Schuchardt]

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:

Re: [PATCH 2/2] efi_selftest: add hii database protocol test case

[Vincent Stehlé]

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

Re: [PATCH 2/2] efi_selftest: add hii database protocol test case

[Heinrich Schuchardt]

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