Re: port-evbarm/54554 (Booting via U-Boot fails due to buffer misalignment)

[Simon South <simon%simonsouth.net@localhost>]

The following reply was made to PR port-evbarm/54554; it has been noted by GNATS.

From: Simon South <simon%simonsouth.net@localhost>
To: Jared McNeill <jmcneill%invisible.ca@localhost>, gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: port-evbarm/54554 (Booting via U-Boot fails due to buffer
 misalignment)
Date: Thu, 19 Sep 2019 09:06:02 -0400

 This is a multi-part message in MIME format.
 --------------BA0A3E4561685F2555A0034B
 Content-Type: text/plain; charset=utf-8; format=flowed
 Content-Transfer-Encoding: 7bit
 
 Here's how I'd refactor this. Tests fine on my ROCK64.
 
 -- 
 Simon South
 simon%simonsouth.net@localhost
 
 --------------BA0A3E4561685F2555A0034B
 Content-Type: text/x-patch; charset=UTF-8;
  name="54554-efiboot-align-buffers-3.patch"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
  filename="54554-efiboot-align-buffers-3.patch"
 
 Index: sys/stand/efiboot/efiblock.c
 ===================================================================
 RCS file: /cvsroot/src/sys/stand/efiboot/efiblock.c,v
 retrieving revision 1.5
 diff -u -r1.5 efiblock.c
 --- sys/stand/efiboot/efiblock.c	9 Mar 2019 13:16:42 -0000	1.5
 +++ sys/stand/efiboot/efiblock.c	19 Sep 2019 12:48:06 -0000
 @@ -98,6 +98,23 @@
  	MD5Final(bpart->hash, &md5ctx);
  }
  
 +static void *
 +efi_block_allocate_device_buffer(struct efi_block_dev *bdev, UINTN size,
 +	void **buf_start)
 +{
 +	void *buf;
 +
 +	if (bdev->bio->Media->IoAlign <= 1)
 +		*buf_start = buf = AllocatePool(size);
 +	else {
 +		buf = AllocatePool(size + bdev->bio->Media->IoAlign - 1);
 +		*buf_start = (buf == NULL) ? NULL : (void *)(((intptr_t)buf +
 +			bdev->bio->Media->IoAlign - 1) & ~(bdev->bio->Media->IoAlign - 1));
 +	}
 +
 +	return buf;
 +}
 +
  static int
  efi_block_find_partitions_disklabel(struct efi_block_dev *bdev, struct mbr_sector *mbr, uint32_t start, uint32_t size)
  {
 @@ -106,19 +123,19 @@
  	struct partition *p;
  	EFI_STATUS status;
  	EFI_LBA lba;
 -	uint8_t *buf;
 +	void *buf, *buf_start;
  	UINT32 sz;
  	int n;
  
  	sz = __MAX(sizeof(d), bdev->bio->Media->BlockSize);
  	sz = roundup(sz, bdev->bio->Media->BlockSize);
 -	buf = AllocatePool(sz);
 -	if (!buf)
 +	if ((buf = efi_block_allocate_device_buffer(bdev, sz, &buf_start)) == NULL)
  		return ENOMEM;
  
  	lba = (((EFI_LBA)start + LABELSECTOR) * DEV_BSIZE) / bdev->bio->Media->BlockSize;
 -	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id, lba, sz, buf);
 -	if (EFI_ERROR(status) || getdisklabel(buf, &d) != NULL) {
 +	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id,
 +		lba, sz, buf_start);
 +	if (EFI_ERROR(status) || getdisklabel(buf_start, &d) != NULL) {
  		FreePool(buf);
  		return EIO;
  	}
 @@ -159,22 +176,22 @@
  	struct mbr_sector mbr;
  	struct mbr_partition *mbr_part;
  	EFI_STATUS status;
 -	uint8_t *buf;
 +	void *buf, *buf_start;
  	UINT32 sz;
  	int n;
  
  	sz = __MAX(sizeof(mbr), bdev->bio->Media->BlockSize);
  	sz = roundup(sz, bdev->bio->Media->BlockSize);
 -	buf = AllocatePool(sz);
 -	if (!buf)
 +	if ((buf = efi_block_allocate_device_buffer(bdev, sz, &buf_start)) == NULL)
  		return ENOMEM;
  
 -	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id, 0, sz, buf);
 +	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id,
 +		0, sz, buf_start);
  	if (EFI_ERROR(status)) {
  		FreePool(buf);
  		return EIO;
  	}
 -	memcpy(&mbr, buf, sizeof(mbr));
 +	memcpy(&mbr, buf_start, sizeof(mbr));
  	FreePool(buf);
  
  	if (le32toh(mbr.mbr_magic) != MBR_MAGIC)
 @@ -240,21 +257,21 @@
  	struct gpt_hdr hdr;
  	struct gpt_ent ent;
  	EFI_STATUS status;
 +	void *buf, *buf_start;
  	UINT32 sz, entry;
 -	uint8_t *buf;
  
  	sz = __MAX(sizeof(hdr), bdev->bio->Media->BlockSize);
  	sz = roundup(sz, bdev->bio->Media->BlockSize);
 -	buf = AllocatePool(sz);
 -	if (!buf)
 +	if ((buf = efi_block_allocate_device_buffer(bdev, sz, &buf_start)) == NULL)
  		return ENOMEM;
  
 -	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id, GPT_HDR_BLKNO, sz, buf);
 +	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id,
 +		GPT_HDR_BLKNO, sz, buf_start);
  	if (EFI_ERROR(status)) {
  		FreePool(buf);
  		return EIO;
  	}
 -	memcpy(&hdr, buf, sizeof(hdr));
 +	memcpy(&hdr, buf_start, sizeof(hdr));
  	FreePool(buf);
  
  	if (memcmp(hdr.hdr_sig, GPT_HDR_SIG, sizeof(hdr.hdr_sig)) != 0)
 @@ -264,18 +281,19 @@
  
  	sz = __MAX(le32toh(hdr.hdr_entsz) * le32toh(hdr.hdr_entries), bdev->bio->Media->BlockSize);
  	sz = roundup(sz, bdev->bio->Media->BlockSize);
 -	buf = AllocatePool(sz);
 -	if (!buf)
 +	if ((buf = efi_block_allocate_device_buffer(bdev, sz, &buf_start)) == NULL)
  		return ENOMEM;
  
 -	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id, le64toh(hdr.hdr_lba_table), sz, buf);
 +	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id,
 +		le64toh(hdr.hdr_lba_table), sz, buf_start);
  	if (EFI_ERROR(status)) {
  		FreePool(buf);
  		return EIO;
  	}
  
  	for (entry = 0; entry < le32toh(hdr.hdr_entries); entry++) {
 -		memcpy(&ent, buf + (entry * le32toh(hdr.hdr_entsz)), sizeof(ent));
 +		memcpy(&ent, buf_start + (entry * le32toh(hdr.hdr_entsz)),
 +			sizeof(ent));
  		efi_block_find_partitions_gpt_entry(bdev, &hdr, &ent, entry);
  	}
  
 @@ -493,6 +511,7 @@
  {
  	struct efi_block_part *bpart = devdata;
  	EFI_STATUS status;
 +	void *allocated_buf, *aligned_buf;
  
  	if (rw != F_READ)
  		return EROFS;
 @@ -518,9 +537,25 @@
  		return EINVAL;
  	}
  
 -	status = uefi_call_wrapper(bpart->bdev->bio->ReadBlocks, 5, bpart->bdev->bio, bpart->bdev->media_id, dblk, size, buf);
 -	if (EFI_ERROR(status))
 +	if ((bpart->bdev->bio->Media->IoAlign <= 1) ||
 +		((intptr_t)buf & (bpart->bdev->bio->Media->IoAlign - 1)) == 0) {
 +		allocated_buf = NULL;
 +		aligned_buf = buf;
 +	} else if ((allocated_buf = efi_block_allocate_device_buffer(bpart->bdev,
 +		size, &aligned_buf)) == NULL)
 +		return ENOMEM;
 +
 +	status = uefi_call_wrapper(bpart->bdev->bio->ReadBlocks, 5,
 +		bpart->bdev->bio, bpart->bdev->media_id, dblk, size, aligned_buf);
 +	if (EFI_ERROR(status)) {
 +		if (allocated_buf != NULL)
 +			FreePool(allocated_buf);
  		return EIO;
 +	}
 +	if (allocated_buf != NULL) {
 +		memcpy(buf, aligned_buf, size);
 +		FreePool(allocated_buf);
 +	}
  
  	*rsize = size;
  
 
 --------------BA0A3E4561685F2555A0034B--