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

[Heinrich Schuchardt]

On 6/30/22 13:02, Paul Barbieri wrote:
> The requsted partition disk sector incorrectly has the parition 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.
>
> Signed-Off-by: Paul Barbieri <plb365 at gmail.com>
> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
> Cc: AKASHI Takahiro <takahiro.akashi at linaro.org>
> ---
> Changes for v3:
>      - Added requested comment in test code
>
>   lib/efi_loader/efi_disk.c                    |  8 +-----
>   lib/efi_selftest/efi_selftest_block_device.c | 28 ++++++++++++++++++++
>   2 files changed, 29 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..d11f673148 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,9 @@ static int execute(void)
>   	char buf[16] __aligned(ARCH_DMA_MINALIGN);
>   	u32 part1_size;
>   	u64 pos;
> +	char block[2 * (1 << LB_BLOCK_SIZE)];
> +	char *block_io_aligned;
> +	u32 align;
>
>   	/* Connect controller to virtual disk */
>   	ret = boottime->connect_controller(disk_handle, NULL, NULL, 1);
> @@ -449,6 +453,30 @@ static int execute(void)
>   		return EFI_ST_FAILURE;
>   	}
>
> +	/* Test read_blocks() can read same file data. */
> +	boottime->set_mem(block, sizeof(block), 0);
> +	align = block_io_protocol->media->io_align;
> +	block_io_aligned = (char *)(((uintptr_t)block + align-1) & ~(align-1));

Align is a 32 bit value but the pointer may be a 64 bit address:

Running the test on the sandbox results in a crash:

align: 512
block:            0x00007ffe78df28b8
block_io_aligned: 0x0000000078df2a00
Segmentation fault (core dumped)

IoAlign = 0 is an allowable value. Your code would set the last 32 bits
to zero in this case.

Keep it simple. Just define block_io_align as an LB_BLOCK_SIZE aligned
array of size LB_BLOCK_SIZE. Anyway the block driver in this unit test
does not care about alignment. read_blocks() only executes memcpy().

Best regards

Heinrich

> +	/* In the test data, the partition starts at block 1 and the file
> +	   hello.txt with the content 'Hello world!' is located at 0x5000
> +	   of the disk. Here we read block 0x27 (offset 0x4e00 of the
> +	   partition) and expect the string 'Hello world!' to be at the
> +	   start of block. */
> +	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_io_aligned);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("ReadBlocks failed\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	if (memcmp(&block_io_aligned[1], buf, 11)) {
> +		efi_st_error("Unexpected block content\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
>   #ifdef CONFIG_FAT_WRITE
>   	/* Write file */
>   	ret = root->open(root, &file, u"u-boot.txt", EFI_FILE_MODE_READ |