Re: [PATCH] EFI: Fix ReadBlocks API reading incorrect sector for, UCLASS_PARTITION devices

Heinrich Schuchardt
2022-06-29T04:59:09Z

On 6/29/22 03:02, Paul Barbieri wrote:
> From 7a7dd7f16352fc916279cca05a3fa617f8bbef64 Mon Sep 17 00:00:00 2001
> From: Paul Barbieri <plb365@gmail.com>
> Date: Tue, 28 Jun 2022 20:24:33 -0400
> Subject: [PATCH] EFI: Fix ReadBlocks API reading incorrect sector for
> UCLASS_PARTITION devices
>
> The requested partition disk sector incorrectly has the partition start
> sector added in twice for UCLASS_PARTITION devices. The
> efi_disk_rw_blocks()
> routine adds the diskobj->offset to the requested lba. When the device
> is a UCLASS_PARTITION, the dev_read() or dev_write() routine is called
> which adds part-gpt_part_info.start. This causes I/O to the wrong sector.
>
> Takahiro Akashi suggested removing the offset field from the efi_disk_obj
> structure since disk-uclass.c handles the partition start biasing. Device
> types other than UCLASS_PARTITION set the diskobj->offset field to zero
> which makes the field unnecessary. This change removes the offset field
> from the structure and removes all references from the code which is
> isolated to the lib/efi_loader/efi_disk.c module.
>
> This change also adds a test for the EFI ReadBlocks() API in the EFI
> selftest code. There is already a test for reading a FAT file. The new
> test uses ReadBlocks() to read the same "disk" block and compare it to
> the data read from the file system API.

This is not a valid patch:

$ git am /tmp/0.patch
warning: Patch sent with format=flowed; space at the end of lines might
be lost.
Applying: EFI: Fix ReadBlocks API reading incorrect sector for,
UCLASS_PARTITION devices
error: corrupt patch at line 11
Patch failed at 0001 EFI: Fix ReadBlocks API reading incorrect sector
for, UCLASS_PARTITION devices

Please, use 'git send-email' for sending patches not Thunderbird.

Best regards

Heinrich

>
> Signed-Off-by: Paul Barbieri <paul.barbieri@verizon.net>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_disk.c                    |  8 +-------
>  lib/efi_selftest/efi_selftest_block_device.c | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 1e82f52dc0..1d700b2a6b 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -35,7 +35,6 @@ const efi_guid_t efi_system_partition_guid =
> PARTITION_SYSTEM_GUID;
>   * @dp:                device path to the block device
>   * @part:      partition
>   * @volume:    simple file system protocol of the partition
> - * @offset:    offset into disk for simple partition
>   * @dev:       associated DM device
>   */
>  struct efi_disk_obj {
> @@ -47,7 +46,6 @@ struct efi_disk_obj {
>         struct efi_device_path *dp;
>         unsigned int part;
>         struct efi_simple_file_system_protocol *volume;
> -       lbaint_t offset;
>         struct udevice *dev; /* TODO: move it to efi_object */
>  };
>
> @@ -117,7 +115,6 @@ static efi_status_t efi_disk_rw_blocks(struct
> efi_block_io *this,
>         diskobj = container_of(this, struct efi_disk_obj, ops);
>         blksz = diskobj->media.block_size;
>         blocks = buffer_size / blksz;
> -       lba += diskobj->offset;
>
>         EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n",
>                   blocks, lba, blksz, direction);
> @@ -440,13 +437,11 @@ static efi_status_t efi_disk_add_dev(
>
>                 diskobj->dp = efi_dp_append_node(dp_parent, node);
>                 efi_free_pool(node);
> -               diskobj->offset = part_info->start;
>                 diskobj->media.last_block = part_info->size - 1;
>                 if (part_info->bootable & PART_EFI_SYSTEM_PARTITION)
>                         guid = &efi_system_partition_guid;
>         } else {
>                 diskobj->dp = efi_dp_from_part(desc, part);
> -               diskobj->offset = 0;
>                 diskobj->media.last_block = desc->lba - 1;
>         }
>         diskobj->part = part;
> @@ -501,12 +496,11 @@ static efi_status_t efi_disk_add_dev(
>                 *disk = diskobj;
>
>         EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d"
> -                 ", offset " LBAF ", last_block %llu\n",
> +                 ", last_block %llu\n",
>                   diskobj->part,
>                   diskobj->media.media_present,
>                   diskobj->media.logical_partition,
>                   diskobj->media.removable_media,
> -                 diskobj->offset,
>                   diskobj->media.last_block);
>
>         /* Store first EFI system partition */
> diff --git a/lib/efi_selftest/efi_selftest_block_device.c
> b/lib/efi_selftest/efi_selftest_block_device.c
> index 60fa655766..ef6bdafe2e 100644
> --- a/lib/efi_selftest/efi_selftest_block_device.c
> +++ b/lib/efi_selftest/efi_selftest_block_device.c
> @@ -11,6 +11,7 @@
>   * ConnectController is used to setup partitions and to install the
> simple
>   * file protocol.
>   * A known file is read from the file system and verified.
> + * Test that the read_blocks API correctly reads a block from the device.
>   */
>
>  #include <efi_selftest.h>
> @@ -312,6 +313,7 @@ static int execute(void)
>         char buf[16] __aligned(ARCH_DMA_MINALIGN);
>         u32 part1_size;
>         u64 pos;
> +       char block[512];
>
>         /* Connect controller to virtual disk */
>         ret = boottime->connect_controller(disk_handle, NULL, NULL, 1);
> @@ -449,6 +451,23 @@ static int execute(void)
>                 return EFI_ST_FAILURE;
>         }
>
> +       /* Test read_blocks() can read same file data. */
> +       boottime->set_mem(block, block_io_protocol->media->block_size, 0);
> +       ret = block_io_protocol->read_blocks(block_io_protocol,
> + block_io_protocol->media->media_id,
> +                                     (0x5000 >> LB_BLOCK_SIZE) - 1,
> + block_io_protocol->media->block_size,
> +                                     block);
> +       if (ret != EFI_SUCCESS) {
> +               efi_st_error("ReadBlocks failed\n");
> +               return EFI_ST_FAILURE;
> +       }
> +
> +       if (memcmp(&block[1], buf, 11)) {