Re: [PATCH v8 00/10] Add basic Boot Loader Interface support

[Daniel Kiper]

On Wed, May 24, 2023 at 04:55:48PM +0200, Oliver Steffen wrote:
> This is a step towards supporting unified kernel images (UKI) in Grub.
>
> Add a new module named bli.  It implements a small but quite useful part
> of the Boot Loader Interface [0].  This interface uses EFI variables for
> communication between the boot loader and the operating system.
>
> This module sets two EFI variables under the vendor GUID
> 4a67b082-0a4c-41cf-b6c7-440b29bb8c4f:
>
> - LoaderInfo: contains GRUB + <version number>.
>   This allows the running operating system to identify the boot loader
>   used during boot.
>
> - LoaderDevicePartUUID: contains the partition UUID of the
>   EFI System Partition (ESP). This is used by
>   systemd-gpt-auto-generator [1] to find the root partitions (and
>   others too), via partition type IDs [2]. This is especially useful for
>   UKIs, where the kernel command line is fixed and usually does not
>   contain any information about the root partition.
>
> This module is only available on EFI platforms.
>
> This series also unifies the GUID implementations and introduces a
> printf format specifier for them: %pG.
>
> [0] https://systemd.io/BOOT_LOADER_INTERFACE/
> [1] 
> https://www.freedesktop.org/software/systemd/man/systemd-gpt-auto-generator.html
> [2] 
> https://uapi-group.org/specifications/specs/discoverable_partitions_specification/
>
> v8:
> - Rebase to latest master branch
> - Fix the documentation build (I hope)
> - Fix build on arm
> - Make more grub_guid instances 'static'
> - Move endian conversion back out of the grub_guid printf code
>
> v7:
> - https://mail.gnu.org/archive/html/grub-devel/2023-05/msg00005.html
> - Add a "Modules" chapter to the documentation and describe the bli
>   module there
> - Reword some section headings in the documentation
>
> v6:
> - https://mail.gnu.org/archive/html/grub-devel/2023-04/msg00068.html
> - Initialize the Boot Loader Interface on module load and remove the bli
>   command.
> - Extract code for grub_efi_set_variable_to_string()
> - Improve error messages in the bli module
> - Improve GUID printing code
>
> v5:
> - https://mail.gnu.org/archive/html/grub-devel/2023-03/msg00120.html
> - Replace more grub_*_guid with the new grub_guid struct
>
> v4:
> - https://mail.gnu.org/archive/html/grub-devel/2023-03/msg00058.html
> - Place grub_utf8_to_utf16_alloc() in kern/misc.c and use it in kern/efi.c
> - bli: rework partition GUID code, improve error reporting
> - Add documentation for the bli module
> - Address other comments
>
> v3:
> - https://mail.gnu.org/archive/html/grub-devel/2023-03/msg00008.html
> - Unify GUID implementations for GPT and EFI
> - Add printf format specifier for GUIDs, drop guid print function from
>   v2.
> - WIP/Please comment: Started extracting utf8->utf16 code.  Where should
>   this go?
> - Address other comments
>
> v2:
> - https://mail.gnu.org/archive/html/grub-devel/2023-02/msg00099.html
> - Addressed comments from Daniel
> - Added a print function for gpt guids`
> - Added integer overflow check in UTF16 conversion
> - Added config drop-in file that loads the module on EFI
>
> v1:
> - https://mail.gnu.org/archive/html/grub-devel/2023-01/msg00104.html
>
> Oliver Steffen (10):
>   efi: Add grub_efi_set_variable_with_attributes()
>   Unify GUID types
>   kern/misc: Add a format specifier GUIDs
>   grub-core: Make use of guid printf format specifier
>   types.h: Add GRUB_SSIZE_MAX
>   kern/misc, kern/efi: Extract UTF-8 to UTF-16 code
>   efi: Add grub_efi_set_variable_to_string()
>   docs: Reword section headings
>   Add a module for the Boot Loader Interface
>   util/grub.d: Activate bli module on EFI

Good news is that you can add Reviewed-by: Daniel Kiper 
<daniel.kiper@oracle.com>
to all the patches... :-)

Sadly there are two bad news... :-(

I have to ask you to do rebase once again. You were racing making
patches with Ard and he won this time... ;-) Sorry about that...
Now your patches are first in the queue...

Additionaly, your patches tickled Coverity and it spit following CID... :-(

** CID 407779:  Null pointer dereferences  (FORWARD_NULL)

________________________________________________________________________________________________________
*** CID 407779:  Null pointer dereferences  (FORWARD_NULL)
/grub-core/commands/bli.c: 80 in get_part_uuid()
74
75       *part_uuid = grub_xasprintf ("%pG", &entry.guid);
76       if (*part_uuid == NULL)
77         status = grub_errno;
78
79      fail:
>>>     CID 407779:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "disk" to "grub_disk_close", which dereferences it.
80       grub_disk_close (disk);
81       grub_device_close (device);
82
83       return status;
84     }

The problem is with this code

  void
  grub_disk_close (grub_disk_t disk)
  {
    grub_partition_t part;
    grub_dprintf ("disk", "Closing `%s'.\n", disk->name);
    ...

I think it should be enough to do

  void
  grub_disk_close (grub_disk_t disk)
  {
    grub_partition_t part;

    if (disk == NULL)
      return;

    grub_dprintf ("disk", "Closing `%s'.\n", disk->name);
    ...

Could you add a such patch?

Daniel