diff mbox series

[5/7] efi_loader: PARTITION_UUIDS should be optional

Message ID 20220419010158.47034-6-takahiro.akashi@linaro.org
State New
Headers show
Series disk: don't compile in partition support for spl/tpl if not really necessary | expand

Commit Message

AKASHI Takahiro April 19, 2022, 1:01 a.m. UTC 
In the current implementation, partition table support (either GPT or DOS)
is not mandatory. So CONFIG_PARTITION_UUIDS should not be enabled
(selected) unconditionally.

Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path for SIG_TYPE_GUID")
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/Kconfig           |  2 +-
 lib/efi_loader/efi_device_path.c | 11 ++++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

Comments

Heinrich Schuchardt April 20, 2022, 7:37 a.m. UTC | #1 
On 4/19/22 03:01, AKASHI Takahiro wrote:
> In the current implementation, partition table support (either GPT or DOS)
> is not mandatory. So CONFIG_PARTITION_UUIDS should not be enabled
> (selected) unconditionally.
>
> Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path for SIG_TYPE_GUID")
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   lib/efi_loader/Kconfig           |  2 +-
>   lib/efi_loader/efi_device_path.c | 11 ++++++++++-
>   2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index d50cd2563d3d..bc518d7a413b 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -15,7 +15,7 @@ config EFI_LOADER
>   	depends on !EFI_APP
>   	default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
>   	select LIB_UUID
> -	select PARTITION_UUIDS
> +	imply PARTITION_UUIDS
>   	select HAVE_BLOCK_DEVICE
>   	select REGEX
>   	imply FAT
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 0542aaae16c7..01fb53ae8c40 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -864,11 +864,20 @@ static void *dp_part_node(void *buf, struct blk_desc *desc, int part)
>   			break;
>   		case SIG_TYPE_GUID:

This can only be reached for CONFIG_EFI_PARTITION=y.

>   			hddp->signature_type = 2;
> +#if CONFIG_IS_ENABLED(PARTITION_UUIDS)
> +			/* info.uuid exists only with PARTITION_UUIDS */
>   			if (uuid_str_to_bin(info.uuid,
> -					    hddp->partition_signature, 1))
> +					    hddp->partition_signature, 1)) {

uuid_guid_get_bin() does not write any byte if info.uuid is not valid.

>   				log_warning(
>   					"Partition no. %d: invalid guid: %s\n",
>   					part, info.uuid);

> +				memset(hddp->partition_signature, 0,
> +				       sizeof(hddp->partition_signature));

dp_alloc() already has zeroed out the memory. Please, remove the
superfluous code.

> +			}
> +#else
> +			memset(hddp->partition_signature, 0,
> +			       sizeof(hddp->partition_signature));

ditto.


This does not conform to the UEFI specification. Why should we support it?

Shouldn't we select CONFIG_PARTITION_UUIDS if CONFIG_EFI_LOADER=y and
CONFIG_EFI_PARTITION=y.

Best regards

Heinrich

> +#endif
>   			break;
>   		}
>
AKASHI Takahiro April 21, 2022, 12:57 a.m. UTC | #2 
On Wed, Apr 20, 2022 at 09:37:39AM +0200, Heinrich Schuchardt wrote:
> On 4/19/22 03:01, AKASHI Takahiro wrote:
> > In the current implementation, partition table support (either GPT or DOS)
> > is not mandatory. So CONFIG_PARTITION_UUIDS should not be enabled
> > (selected) unconditionally.
> > 
> > Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path for SIG_TYPE_GUID")
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   lib/efi_loader/Kconfig           |  2 +-
> >   lib/efi_loader/efi_device_path.c | 11 ++++++++++-
> >   2 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index d50cd2563d3d..bc518d7a413b 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -15,7 +15,7 @@ config EFI_LOADER
> >   	depends on !EFI_APP
> >   	default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
> >   	select LIB_UUID
> > -	select PARTITION_UUIDS
> > +	imply PARTITION_UUIDS
> >   	select HAVE_BLOCK_DEVICE
> >   	select REGEX
> >   	imply FAT
> > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> > index 0542aaae16c7..01fb53ae8c40 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -864,11 +864,20 @@ static void *dp_part_node(void *buf, struct blk_desc *desc, int part)
> >   			break;
> >   		case SIG_TYPE_GUID:
> 
> This can only be reached for CONFIG_EFI_PARTITION=y.

I know, but the code won't be exercised anyway if !CONFIG_EFI_PARTITION.

> >   			hddp->signature_type = 2;
> > +#if CONFIG_IS_ENABLED(PARTITION_UUIDS)
> > +			/* info.uuid exists only with PARTITION_UUIDS */
> >   			if (uuid_str_to_bin(info.uuid,
> > -					    hddp->partition_signature, 1))
> > +					    hddp->partition_signature, 1)) {
> 
> uuid_guid_get_bin() does not write any byte if info.uuid is not valid.

What matters here is that "uuid" in struct disk_partition is defined
only if CONFIG_PARTITION_UUIDS.

> >   				log_warning(
> >   					"Partition no. %d: invalid guid: %s\n",
> >   					part, info.uuid);
> 
> > +				memset(hddp->partition_signature, 0,
> > +				       sizeof(hddp->partition_signature));
> 
> dp_alloc() already has zeroed out the memory. Please, remove the
> superfluous code.
> 
> > +			}
> > +#else
> > +			memset(hddp->partition_signature, 0,
> > +			       sizeof(hddp->partition_signature));
> 
> ditto.
> 
> 
> This does not conform to the UEFI specification. Why should we support it?

As I said in my previous comment at patch#6,
CONFIG_EFI_PARTITION can be seen optional under the current implementation.

> 
> Shouldn't we select CONFIG_PARTITION_UUIDS if CONFIG_EFI_LOADER=y and
> CONFIG_EFI_PARTITION=y.

Unfortunately some platforms enable (in other words, "select") PARTITION_UUIDS
unconditionally even without explicitly enabling EFI_LOADER or EFI_PARTITION.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +#endif
> >   			break;
> >   		}
> > 
>
Tom Rini April 21, 2022, 1:13 p.m. UTC | #3 
On Thu, Apr 21, 2022 at 09:57:50AM +0900, AKASHI Takahiro wrote:
> On Wed, Apr 20, 2022 at 09:37:39AM +0200, Heinrich Schuchardt wrote:
> > On 4/19/22 03:01, AKASHI Takahiro wrote:
> > > In the current implementation, partition table support (either GPT or DOS)
> > > is not mandatory. So CONFIG_PARTITION_UUIDS should not be enabled
> > > (selected) unconditionally.
> > > 
> > > Fixes: commit 17f8cda505e3 ("efi_loader: set partition GUID in device path for SIG_TYPE_GUID")
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >   lib/efi_loader/Kconfig           |  2 +-
> > >   lib/efi_loader/efi_device_path.c | 11 ++++++++++-
> > >   2 files changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index d50cd2563d3d..bc518d7a413b 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -15,7 +15,7 @@ config EFI_LOADER
> > >   	depends on !EFI_APP
> > >   	default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
> > >   	select LIB_UUID
> > > -	select PARTITION_UUIDS
> > > +	imply PARTITION_UUIDS
> > >   	select HAVE_BLOCK_DEVICE
> > >   	select REGEX
> > >   	imply FAT
> > > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> > > index 0542aaae16c7..01fb53ae8c40 100644
> > > --- a/lib/efi_loader/efi_device_path.c
> > > +++ b/lib/efi_loader/efi_device_path.c
> > > @@ -864,11 +864,20 @@ static void *dp_part_node(void *buf, struct blk_desc *desc, int part)
> > >   			break;
> > >   		case SIG_TYPE_GUID:
> > 
> > This can only be reached for CONFIG_EFI_PARTITION=y.
> 
> I know, but the code won't be exercised anyway if !CONFIG_EFI_PARTITION.
> 
> > >   			hddp->signature_type = 2;
> > > +#if CONFIG_IS_ENABLED(PARTITION_UUIDS)
> > > +			/* info.uuid exists only with PARTITION_UUIDS */
> > >   			if (uuid_str_to_bin(info.uuid,
> > > -					    hddp->partition_signature, 1))
> > > +					    hddp->partition_signature, 1)) {
> > 
> > uuid_guid_get_bin() does not write any byte if info.uuid is not valid.
> 
> What matters here is that "uuid" in struct disk_partition is defined
> only if CONFIG_PARTITION_UUIDS.
> 
> > >   				log_warning(
> > >   					"Partition no. %d: invalid guid: %s\n",
> > >   					part, info.uuid);
> > 
> > > +				memset(hddp->partition_signature, 0,
> > > +				       sizeof(hddp->partition_signature));
> > 
> > dp_alloc() already has zeroed out the memory. Please, remove the
> > superfluous code.
> > 
> > > +			}
> > > +#else
> > > +			memset(hddp->partition_signature, 0,
> > > +			       sizeof(hddp->partition_signature));
> > 
> > ditto.
> > 
> > 
> > This does not conform to the UEFI specification. Why should we support it?
> 
> As I said in my previous comment at patch#6,
> CONFIG_EFI_PARTITION can be seen optional under the current implementation.
> 
> > 
> > Shouldn't we select CONFIG_PARTITION_UUIDS if CONFIG_EFI_LOADER=y and
> > CONFIG_EFI_PARTITION=y.
> 
> Unfortunately some platforms enable (in other words, "select") PARTITION_UUIDS
> unconditionally even without explicitly enabling EFI_LOADER or EFI_PARTITION.

I wouldn't assume that they're correct in doing so and it might have
been how some other problem was addressed at the time to fix incorrect /
not yet implemented in Kconfig at the time dependencies.
diff mbox series

Patch

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index d50cd2563d3d..bc518d7a413b 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -15,7 +15,7 @@  config EFI_LOADER
 	depends on !EFI_APP
 	default y if !ARM || SYS_CPU = armv7 || SYS_CPU = armv8
 	select LIB_UUID
-	select PARTITION_UUIDS
+	imply PARTITION_UUIDS
 	select HAVE_BLOCK_DEVICE
 	select REGEX
 	imply FAT
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 0542aaae16c7..01fb53ae8c40 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -864,11 +864,20 @@  static void *dp_part_node(void *buf, struct blk_desc *desc, int part)
 			break;
 		case SIG_TYPE_GUID:
 			hddp->signature_type = 2;
+#if CONFIG_IS_ENABLED(PARTITION_UUIDS)
+			/* info.uuid exists only with PARTITION_UUIDS */
 			if (uuid_str_to_bin(info.uuid,
-					    hddp->partition_signature, 1))
+					    hddp->partition_signature, 1)) {
 				log_warning(
 					"Partition no. %d: invalid guid: %s\n",
 					part, info.uuid);
+				memset(hddp->partition_signature, 0,
+				       sizeof(hddp->partition_signature));
+			}
+#else
+			memset(hddp->partition_signature, 0,
+			       sizeof(hddp->partition_signature));
+#endif
 			break;
 		}