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.aka...@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. -- Tom
signature.asc
Description: PGP signature
