diff mbox series

[12/16] efi_loader: optional pointer for ConvertPointer

Message ID 20200327052800.11022-13-xypron.glpk@gmx.de
State Superseded
Headers show
Series efi_loader: non-volatile and runtime variables | expand

Commit Message

Heinrich Schuchardt March 27, 2020, 5:27 a.m. UTC 
If the EFI_OPTIONAL_PTR is set in DebugDisposition, a NULL pointer does not
constitute an invalid parameter.

Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
---
 include/efi_api.h            | 2 ++
 lib/efi_loader/efi_runtime.c | 6 ++++++
 2 files changed, 8 insertions(+)

--
2.25.1

Comments

AKASHI Takahiro March 31, 2020, 5:23 a.m. UTC | #1 
On Fri, Mar 27, 2020 at 06:27:56AM +0100, Heinrich Schuchardt wrote:
> If the EFI_OPTIONAL_PTR is set in DebugDisposition, a NULL pointer does not
> constitute an invalid parameter.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
>  include/efi_api.h            | 2 ++
>  lib/efi_loader/efi_runtime.c | 6 ++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 1c40ffc4f5..c56703fc5e 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -250,6 +250,8 @@ struct efi_rt_properties_table {
>  	u32 runtime_services_supported;
>  };
> 
> +#define EFI_OPTIONAL_PTR	0x00000001
> +
>  struct efi_runtime_services {
>  	struct efi_table_hdr hdr;
>  	efi_status_t (EFIAPI *get_time)(struct efi_time *time,
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 67fa693e41..664a0422e2 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -511,6 +511,12 @@ efi_convert_pointer(efi_uintn_t debug_disposition, void **address)
>  		ret = EFI_INVALID_PARAMETER;
>  		goto out;
>  	}
> +	if (!*address) {
> +		if (debug_disposition & EFI_OPTIONAL_PTR)
> +			return EFI_SUCCESS;
> +		else
> +			return EFI_INVALID_PARAMETER;
> +	}

In either case, NULL won't be converted.
Is this a behavior UEFI specification expects?
(I haven't found any description there.)

-Takahiro Akashi


>  	for (i = 0; i < efi_descriptor_count; i++) {
>  		struct efi_mem_desc *map = (void *)efi_virtmap +
> --
> 2.25.1
>
Heinrich Schuchardt March 31, 2020, 6:52 a.m. UTC | #2 
On March 31, 2020, 5:23 a.m. UTC Takahiro Akashi wrote
 >On Fri, Mar 27, 2020 at 06:27:56AM +0100, Heinrich Schuchardt wrote:
 >> If the EFI_OPTIONAL_PTR is set in DebugDisposition, a NULL pointer
does not
 >> constitute an invalid parameter.
 >>
 >> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
 >> ---
 >>  include/efi_api.h            | 2 ++
 >>  lib/efi_loader/efi_runtime.c | 6 ++++++
 >>  2 files changed, 8 insertions(+)
 >>
 >> diff --git a/include/efi_api.h b/include/efi_api.h
 >> index 1c40ffc4f5..c56703fc5e 100644
 >> --- a/include/efi_api.h
 >> +++ b/include/efi_api.h
 >> @@ -250,6 +250,8 @@ struct efi_rt_properties_table {
 >>  	u32 runtime_services_supported;
 >>  };
 >>
 >> +#define EFI_OPTIONAL_PTR	0x00000001
 >> +
 >>  struct efi_runtime_services {
 >>  	struct efi_table_hdr hdr;
 >>  	efi_status_t (EFIAPI *get_time)(struct efi_time *time,
 >> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
 >> index 67fa693e41..664a0422e2 100644
 >> --- a/lib/efi_loader/efi_runtime.c
 >> +++ b/lib/efi_loader/efi_runtime.c
 >> @@ -511,6 +511,12 @@ efi_convert_pointer(efi_uintn_t
debug_disposition, void **address)
 >>  		ret = EFI_INVALID_PARAMETER;
 >>  		goto out;
 >>  	}
 >> +	if (!*address) {
 >> +		if (debug_disposition & EFI_OPTIONAL_PTR)
 >> +			return EFI_SUCCESS;
 >> +		else
 >> +			return EFI_INVALID_PARAMETER;
 >> +	}
 >
 >In either case, NULL won't be converted.
 >Is this a behavior UEFI specification expects?
 >(I haven't found any description there.)
 >
 >-Takahiro Akashi


The spec has:

EFI_INVALID_PARAMETER - *Address is NULL and DebugDisposition does not
have the EFI_OPTIONAL_PTR bit set.

EFI_OPTIONAL_PTR is handled in the same way in EDK2. An optional pointer
is one that should not be converted if is NULL. It should stay NULL, so
that the firmware can detect that it is not set.

Best regards

Heinrich

 >
 >
 >>  	for (i = 0; i < efi_descriptor_count; i++) {
 >>  		struct efi_mem_desc *map = (void *)efi_virtmap +
 >> --
 >> 2.25.1
 >>
AKASHI Takahiro March 31, 2020, 7:51 a.m. UTC | #3 
On Tue, Mar 31, 2020 at 08:52:23AM +0200, Heinrich Schuchardt wrote:
> On March 31, 2020, 5:23 a.m. UTC Takahiro Akashi wrote
> >On Fri, Mar 27, 2020 at 06:27:56AM +0100, Heinrich Schuchardt wrote:
> >> If the EFI_OPTIONAL_PTR is set in DebugDisposition, a NULL pointer
> does not
> >> constitute an invalid parameter.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> >> ---
> >>  include/efi_api.h            | 2 ++
> >>  lib/efi_loader/efi_runtime.c | 6 ++++++
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/include/efi_api.h b/include/efi_api.h
> >> index 1c40ffc4f5..c56703fc5e 100644
> >> --- a/include/efi_api.h
> >> +++ b/include/efi_api.h
> >> @@ -250,6 +250,8 @@ struct efi_rt_properties_table {
> >>  	u32 runtime_services_supported;
> >>  };
> >>
> >> +#define EFI_OPTIONAL_PTR	0x00000001
> >> +
> >>  struct efi_runtime_services {
> >>  	struct efi_table_hdr hdr;
> >>  	efi_status_t (EFIAPI *get_time)(struct efi_time *time,
> >> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> >> index 67fa693e41..664a0422e2 100644
> >> --- a/lib/efi_loader/efi_runtime.c
> >> +++ b/lib/efi_loader/efi_runtime.c
> >> @@ -511,6 +511,12 @@ efi_convert_pointer(efi_uintn_t
> debug_disposition, void **address)
> >>  		ret = EFI_INVALID_PARAMETER;
> >>  		goto out;
> >>  	}
> >> +	if (!*address) {
> >> +		if (debug_disposition & EFI_OPTIONAL_PTR)
> >> +			return EFI_SUCCESS;
> >> +		else
> >> +			return EFI_INVALID_PARAMETER;
> >> +	}
> >
> >In either case, NULL won't be converted.
> >Is this a behavior UEFI specification expects?
> >(I haven't found any description there.)
> >
> >-Takahiro Akashi
> 
> 
> The spec has:
> 
> EFI_INVALID_PARAMETER - *Address is NULL and DebugDisposition does not
> have the EFI_OPTIONAL_PTR bit set.

That's it.
There is no description about how NULL be handled if OPTIONAL_PTR.

> EFI_OPTIONAL_PTR is handled in the same way in EDK2. An optional pointer
> is one that should not be converted if is NULL. It should stay NULL, so
> that the firmware can detect that it is not set.

Even so, when you implement it based on your own *interpretation,*
you should describe it explicitly.

-Takahiro Akashi

> 
> Best regards
> 
> Heinrich
> 
> >
> >
> >>  	for (i = 0; i < efi_descriptor_count; i++) {
> >>  		struct efi_mem_desc *map = (void *)efi_virtmap +
> >> --
> >> 2.25.1
> >>
diff mbox series

Patch

diff --git a/include/efi_api.h b/include/efi_api.h
index 1c40ffc4f5..c56703fc5e 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -250,6 +250,8 @@  struct efi_rt_properties_table {
 	u32 runtime_services_supported;
 };

+#define EFI_OPTIONAL_PTR	0x00000001
+
 struct efi_runtime_services {
 	struct efi_table_hdr hdr;
 	efi_status_t (EFIAPI *get_time)(struct efi_time *time,
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 67fa693e41..664a0422e2 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -511,6 +511,12 @@  efi_convert_pointer(efi_uintn_t debug_disposition, void **address)
 		ret = EFI_INVALID_PARAMETER;
 		goto out;
 	}
+	if (!*address) {
+		if (debug_disposition & EFI_OPTIONAL_PTR)
+			return EFI_SUCCESS;
+		else
+			return EFI_INVALID_PARAMETER;
+	}

 	for (i = 0; i < efi_descriptor_count; i++) {
 		struct efi_mem_desc *map = (void *)efi_virtmap +