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: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: port-evbarm/54554 (Booting via U-Boot fails due to buffer
 misalignment)
Date: Wed, 18 Sep 2019 17:42:48 -0400

 This is a multi-part message in MIME format.
 --------------6AB8168758FF6A2572F5CA1E
 Content-Type: text/plain; charset=utf-8; format=flowed
 Content-Transfer-Encoding: 7bit
 
 On 2019-09-18 9:48 a.m., Jared McNeill wrote:
 > According to UEFI IoAlign may be 0 or 1 ("no alignment required"). Can 
 > you update the patch to take this into consideration?
 
 Here's an updated patch that handles this case. I've also reformatted 
 the modified code slightly to match the style guide.
 
 I've tested this on my ROCK64 and it seems to work fine.
 
 -- 
 Simon South
 simon%simonsouth.net@localhost
 
 --------------6AB8168758FF6A2572F5CA1E
 Content-Type: text/x-patch; charset=UTF-8;
  name="54554-efiboot-align-buffers-2.patch"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
  filename="54554-efiboot-align-buffers-2.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	18 Sep 2019 21:23:14 -0000
 @@ -106,19 +106,28 @@
  	struct partition *p;
  	EFI_STATUS status;
  	EFI_LBA lba;
 -	uint8_t *buf;
 +	uint8_t *buf, *aligned_buf;
  	UINT32 sz;
  	int n;
  
  	sz = __MAX(sizeof(d), bdev->bio->Media->BlockSize);
  	sz = roundup(sz, bdev->bio->Media->BlockSize);
 -	buf = AllocatePool(sz);
 -	if (!buf)
 -		return ENOMEM;
 +
 +	if (bdev->bio->Media->IoAlign <= 1) {
 +		if ((buf = AllocatePool(sz)) == NULL)
 +			return ENOMEM;
 +		aligned_buf = buf;
 +	} else {
 +		if ((buf = AllocatePool(sz + bdev->bio->Media->IoAlign - 1)) == NULL)
 +			return ENOMEM;
 +		aligned_buf = (uint8_t *)(((intptr_t)buf +
 +			bdev->bio->Media->IoAlign - 1) & ~(bdev->bio->Media->IoAlign - 1));
 +	}
  
  	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, aligned_buf);
 +	if (EFI_ERROR(status) || getdisklabel(aligned_buf, &d) != NULL) {
  		FreePool(buf);
  		return EIO;
  	}
 @@ -159,22 +168,31 @@
  	struct mbr_sector mbr;
  	struct mbr_partition *mbr_part;
  	EFI_STATUS status;
 -	uint8_t *buf;
 +	uint8_t *buf, *aligned_buf;
  	UINT32 sz;
  	int n;
  
  	sz = __MAX(sizeof(mbr), bdev->bio->Media->BlockSize);
  	sz = roundup(sz, bdev->bio->Media->BlockSize);
 -	buf = AllocatePool(sz);
 -	if (!buf)
 -		return ENOMEM;
  
 -	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id, 0, sz, buf);
 +	if (bdev->bio->Media->IoAlign <= 1) {
 +		if ((buf = AllocatePool(sz)) == NULL)
 +			return ENOMEM;
 +		aligned_buf = buf;
 +	} else {
 +		if ((buf = AllocatePool(sz + bdev->bio->Media->IoAlign - 1)) == NULL)
 +			return ENOMEM;
 +		aligned_buf = (uint8_t *)(((intptr_t)buf +
 +			bdev->bio->Media->IoAlign - 1) & ~(bdev->bio->Media->IoAlign - 1));
 +	}
 +
 +	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id,
 +		0, sz, aligned_buf);
  	if (EFI_ERROR(status)) {
  		FreePool(buf);
  		return EIO;
  	}
 -	memcpy(&mbr, buf, sizeof(mbr));
 +	memcpy(&mbr, aligned_buf, sizeof(mbr));
  	FreePool(buf);
  
  	if (le32toh(mbr.mbr_magic) != MBR_MAGIC)
 @@ -241,20 +259,29 @@
  	struct gpt_ent ent;
  	EFI_STATUS status;
  	UINT32 sz, entry;
 -	uint8_t *buf;
 +	uint8_t *buf, *aligned_buf;
  
  	sz = __MAX(sizeof(hdr), bdev->bio->Media->BlockSize);
  	sz = roundup(sz, bdev->bio->Media->BlockSize);
 -	buf = AllocatePool(sz);
 -	if (!buf)
 -		return ENOMEM;
  
 -	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id, GPT_HDR_BLKNO, sz, buf);
 +	if (bdev->bio->Media->IoAlign <= 1) {
 +		if ((buf = AllocatePool(sz)) == NULL)
 +			return ENOMEM;
 +		aligned_buf = buf;
 +	} else {
 +		if ((buf = AllocatePool(sz + bdev->bio->Media->IoAlign - 1)) == NULL)
 +			return ENOMEM;
 +		aligned_buf = (uint8_t *)(((intptr_t)buf +
 +			bdev->bio->Media->IoAlign - 1) & ~(bdev->bio->Media->IoAlign - 1));
 +	}
 +
 +	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id,
 +		GPT_HDR_BLKNO, sz, aligned_buf);
  	if (EFI_ERROR(status)) {
  		FreePool(buf);
  		return EIO;
  	}
 -	memcpy(&hdr, buf, sizeof(hdr));
 +	memcpy(&hdr, aligned_buf, sizeof(hdr));
  	FreePool(buf);
  
  	if (memcmp(hdr.hdr_sig, GPT_HDR_SIG, sizeof(hdr.hdr_sig)) != 0)
 @@ -264,18 +291,28 @@
  
  	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)
 -		return ENOMEM;
  
 -	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id, le64toh(hdr.hdr_lba_table), sz, buf);
 +	if (bdev->bio->Media->IoAlign <= 1) {
 +		if ((buf = AllocatePool(sz)) == NULL)
 +			return ENOMEM;
 +		aligned_buf = buf;
 +	} else {
 +		if ((buf = AllocatePool(sz + bdev->bio->Media->IoAlign - 1)) == NULL)
 +			return ENOMEM;
 +		aligned_buf = (uint8_t *)(((intptr_t)buf +
 +			bdev->bio->Media->IoAlign - 1) & ~(bdev->bio->Media->IoAlign - 1));
 +	}
 +
 +	status = uefi_call_wrapper(bdev->bio->ReadBlocks, 5, bdev->bio, bdev->media_id,
 +		le64toh(hdr.hdr_lba_table), sz, aligned_buf);
  	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, aligned_buf + (entry * le32toh(hdr.hdr_entsz)),
 +			sizeof(ent));
  		efi_block_find_partitions_gpt_entry(bdev, &hdr, &ent, entry);
  	}
  
 @@ -493,6 +530,7 @@
  {
  	struct efi_block_part *bpart = devdata;
  	EFI_STATUS status;
 +	void *allocated_buf, *aligned_buf;
  
  	if (rw != F_READ)
  		return EROFS;
 @@ -518,9 +556,30 @@
  		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 = AllocatePool(size +
 +			bpart->bdev->bio->Media->IoAlign - 1)) == NULL)
 +			return ENOMEM;
 +		aligned_buf = (void *)(((intptr_t)allocated_buf +
 +			bpart->bdev->bio->Media->IoAlign - 1) &
 +			~(bpart->bdev->bio->Media->IoAlign - 1));
 +	}
 +
 +	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;
  
 
 --------------6AB8168758FF6A2572F5CA1E--