NetBSD Problem Report #54554
From www@netbsd.org Tue Sep 17 14:06:19 2019
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id AF97C7A156
for <gnats-bugs@gnats.NetBSD.org>; Tue, 17 Sep 2019 14:06:19 +0000 (UTC)
Message-Id: <20190917140618.ADE657A1C7@mollari.NetBSD.org>
Date: Tue, 17 Sep 2019 14:06:18 +0000 (UTC)
From: simon@simonsouth.net
Reply-To: simon@simonsouth.net
To: gnats-bugs@NetBSD.org
Subject: Booting via U-Boot fails due to buffer misalignment
X-Send-Pr-Version: www-1.0
>Number: 54554
>Category: port-evbarm
>Synopsis: Booting via U-Boot fails due to buffer misalignment
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: jmcneill
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Tue Sep 17 14:10:00 +0000 2019
>Closed-Date: Sun Sep 22 19:24:14 +0000 2019
>Last-Modified: Sun Sep 22 19:24:14 +0000 2019
>Originator: Simon South
>Release: CURRENT (9.99.11) 2019-09-15 12:10 UTC
>Organization:
>Environment:
NetBSD arm64 9.99.11 NetBSD 9.99.11 (GENERIC64) #8: Mon Sep 16 17:09:14 EDT 2019
>Description:
Booting NetBSD on a PINE64 ROCK64 fails using the current revision of
U-Boot in that project's git repository. NetBSD's EFI bootloader is
unable to open any kernel, with output like the following:
>> NetBSD/evbarm EFI Boot (aarch64), Revision 1.11 (Thu Sep 12 13:07:08 UTC 2019) (from NetBSD 9.99.11)
Press return to boot now, any other key for boot prompt
booting netbsd - starting in 0 seconds.
open netbsd: Invalid argument
boot: netbsd: Invalid argument
booting netbsd.gz - starting in 0 seconds.
open netbsd.gz: Invalid argument
boot: netbsd.gz: Invalid argument
...
The same OS image boots fine when using an earlier version of U-Boot.
This is due to a recent change to U-Boot, commit f59f0825e8 on 5
September 2019, that updates their implementation of the UEFI
ReadBlocks function so it checks the alignment of the buffer passed to
it against the alignment requested by the underlying device, as
required by the UEFI spec. NetBSD's EFI bootloader makes no effort to
ensure the buffer it passes to this function is correctly aligned,
causing (on at least some systems) attempts to open or read from files
to fail with an "Invalid argument" error as shown above.
>How-To-Repeat:
1. Build a NetBSD release image as usual and write it to a microSD
card.
2. Build U-Boot, configured for the ROCK64, from the current version
in the project's git repository (with a second problematic commit,
899c3b3523, reverted), and write this to the microSD card as well.
3. Place the microSD card in the ROCK64 and attempt to boot it.
>Fix:
Functions in sys/stand/efiboot/efiblock.c that call the UEFI
environment's ReadBlocks function need to be modified to ensure they
always pass a buffer aligned as requested by the device in use.
I've made these changes locally and will attach a patch to this
PR. With the changes applied (and the second problematic U-Boot
commit, noted above, reverted) I can boot NetBSD without issue using
the current revision of U-Boot.
>Release-Note:
>Audit-Trail:
From: Simon South <simon@simonsouth.net>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: port-evbarm/54554
Date: Tue, 17 Sep 2019 11:27:50 -0400
This is a multi-part message in MIME format.
--------------42AFB12FB7BE06A1DACFB984
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Here's a patch that fixes this problem by ensuring calls to ReadBlocks
in efiblock.c always pass a buffer aligned as requested by the device.
In the case of efi_block_strategy(), this is done by checking the
alignment of the buffer passed by the caller and, if necessary,
allocating a second buffer, writing into it at the correct alignment and
copying the result back into the caller's buffer.
Note I've taken the liberty of wrapping at 80 columns the lines I modified.
I've tested this successfully on a PINE64 ROCK64 (only).
--
Simon South
simon@simonsouth.net
--------------42AFB12FB7BE06A1DACFB984
Content-Type: text/x-patch; charset=UTF-8;
name="54554-efiboot-align-buffers.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="54554-efiboot-align-buffers.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 17 Sep 2019 15:23:59 -0000
@@ -106,19 +106,22 @@
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);
+ buf = AllocatePool(sz + bdev->bio->Media->IoAlign - 1);
if (!buf)
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 +162,25 @@
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);
+ buf = AllocatePool(sz + bdev->bio->Media->IoAlign - 1);
if (!buf)
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, buf);
+ 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 +247,23 @@
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);
+ buf = AllocatePool(sz + bdev->bio->Media->IoAlign - 1);
if (!buf)
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, buf);
+ 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 +273,22 @@
sz = __MAX(le32toh(hdr.hdr_entsz) * le32toh(hdr.hdr_entries), bdev->bio->Media->BlockSize);
sz = roundup(sz, bdev->bio->Media->BlockSize);
- buf = AllocatePool(sz);
+ buf = AllocatePool(sz + bdev->bio->Media->IoAlign - 1);
if (!buf)
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, buf);
+ 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 +506,7 @@
{
struct efi_block_part *bpart = devdata;
EFI_STATUS status;
+ void *allocated_buf, *aligned_buf;
if (rw != F_READ)
return EROFS;
@@ -518,9 +532,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 (((intptr_t)buf & ~(bpart->bdev->bio->Media->IoAlign - 1)) == 0) {
+ allocated_buf = NULL;
+ aligned_buf = buf;
+ } else {
+ allocated_buf = AllocatePool(size +
+ bpart->bdev->bio->Media->IoAlign - 1);
+ if (!allocated_buf)
+ 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)
+ FreePool(allocated_buf);
return EIO;
+ }
+ if (allocated_buf) {
+ memcpy(buf, aligned_buf, size);
+ FreePool(allocated_buf);
+ }
*rsize = size;
--------------42AFB12FB7BE06A1DACFB984--
From: Simon South <simon@simonsouth.net>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: port-evbarm/54554
Date: Tue, 17 Sep 2019 11:59:37 -0400
With regards to testing I should add that U-Boot works to boot NetBSD on
the ROCK64 only if it (U-Boot) is built using the binary-only TPL
supplied by Rockchip at https://github.com/rockchip-linux/rkbin. NetBSD
currently fails to boot if U-Boot's own TPL is used.
(I'll be submitting separate PRs for this issue and the issue caused by
U-Boot's commit 899c3b3523, which removes code related to the RK3328's
clock.)
--
Simon South
simon@simonsouth.net
From: Simon South <simon@simonsouth.net>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: port-evbarm/54554
Date: Wed, 18 Sep 2019 07:36:47 -0400
It turns out this patch fixes the second problem I encountered with
U-Boot, too, so the current revision of U-Boot can be used (with
Rockchip's TPL) without needing to revert any commits.
The issue with NetBSD failing to boot using U-Boot's TPL I've submitted
as port-evbarm/54557:
https://gnats.netbsd.org/54557
--
Simon South
simon@simonsouth.net
From: Simon South <simon@simonsouth.net>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: port-evbarm/54554
Date: Wed, 18 Sep 2019 07:58:38 -0400
Just spotted a problem with the patch: The tilde on line 535 of the
patched file needs to be removed for the alignment check to work correctly.
In other words, sys/stand/efiboot/efiblock.c:535 should be changed from
if (((intptr_t)buf & ~(bpart->bdev->bio->Media->IoAlign - 1)) == 0) {
to
if (((intptr_t)buf & (bpart->bdev->bio->Media->IoAlign - 1)) == 0) {
--
Simon South
simon@simonsouth.net
Responsible-Changed-From-To: port-evbarm-maintainer->jmcneill
Responsible-Changed-By: jmcneill@NetBSD.org
Responsible-Changed-When: Wed, 18 Sep 2019 13:35:19 +0000
Responsible-Changed-Why:
take
From: Jared McNeill <jmcneill@NetBSD.org>
To: gnats-bugs@netbsd.org
Cc: port-evbarm-maintainer@netbsd.org, netbsd-bugs@netbsd.org,
gnats-admin@netbsd.org, simon@simonsouth.net
Subject: Re: port-evbarm/54554 (Booting via U-Boot fails due to buffer
misalignment)
Date: Wed, 18 Sep 2019 13:48:24 +0000 (UTC)
Thanks for looking at this!
According to UEFI IoAlign may be 0 or 1 ("no alignment required"). Can you
update the patch to take this into consideration?
From: Simon South <simon@simonsouth.net>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: port-evbarm/54554 (Booting via U-Boot fails due to buffer
misalignment)
Date: Wed, 18 Sep 2019 10:00:52 -0400
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?
Certainly, but that doesn't seem to match U-Boot's interpretation of the
IoAlign field. Where are you reading this description? (And how would we
know what to align the buffer to when IoAlign is 1?)
The relevant code[1] in U-Boot is
/* media->io_align is a power of 2 */
if ((uintptr_t)buffer & (this->media->io_align - 1))
return EFI_INVALID_PARAMETER;
which clearly uses IoAlign directly to check for alignment.
[1]
https://gitlab.denx.de/u-boot/u-boot/blob/a9fa70b7b7167487affc5d919e541872c99e814b/lib/efi_loader/efi_disk.c#L124
--
Simon South
simon@simonsouth.net
From: Simon South <simon@simonsouth.net>
To: gnats-bugs@netbsd.org, jmcneill@netbsd.org
Cc:
Subject: Re: port-evbarm/54554 (Booting via U-Boot fails due to buffer
misalignment)
Date: Wed, 18 Sep 2019 10:18:37 -0400
D'oh, of course you mean _in addition_ IoAlign can have either of those
values.
Yes, I'll update the patch to handle that case.
--
Simon South
simon@simonsouth.net
From: Simon South <simon@simonsouth.net>
To: gnats-bugs@netbsd.org
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
--------------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--
From: Jared McNeill <jmcneill@invisible.ca>
To: gnats-bugs@netbsd.org
Cc: jmcneill@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
simon@simonsouth.net
Subject: Re: port-evbarm/54554 (Booting via U-Boot fails due to buffer
misalignment)
Date: Thu, 19 Sep 2019 07:50:08 -0300 (ADT)
There's a bit of code duplication here, maybe we should break this out
into a separate function? Even if you have to memcpy for each transfer, I
think we can clean things up a bit. What do you think?
From: Simon South <simon@simonsouth.net>
To: Jared McNeill <jmcneill@invisible.ca>, gnats-bugs@netbsd.org
Cc:
Subject: Re: port-evbarm/54554 (Booting via U-Boot fails due to buffer
misalignment)
Date: Thu, 19 Sep 2019 07:43:52 -0400
On 2019-09-19 6:50 a.m., Jared McNeill wrote:
> There's a bit of code duplication here, maybe we should break this out
> into a separate function?
I had the same thought but resisted doing this, only because
1. The original code was also duplicated in each spot, and as I'm new to
NetBSD I'm trying to change as little as possible; and
2. The resulting function signature seems a bit ugly, something along
the lines of
static void *
efi_block_allocate_device_buffer(struct efi_block_dev *bdev,
UINTN size, void **buffer_start)
since we need to return a pointer both to the "base" buffer as well
as to its first aligned element.
Does that seem reasonable, though? I'm happy to make this change as well
but you would have a better feel for it than me at this point.
--
Simon South
simon@simonsouth.net
From: Simon South <simon@simonsouth.net>
To: Jared McNeill <jmcneill@invisible.ca>, gnats-bugs@netbsd.org
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
--------------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--
From: "Jared D. McNeill" <jmcneill@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/54554 CVS commit: src/sys/stand/efiboot
Date: Sat, 21 Sep 2019 10:19:37 +0000
Module Name: src
Committed By: jmcneill
Date: Sat Sep 21 10:19:37 UTC 2019
Modified Files:
src/sys/stand/efiboot: efiblock.c
Log Message:
Honour block device's IO alignment requirements.
Patch from Simon South <simon@simonsouth.net> in PR# 54554
To generate a diff of this commit:
cvs rdiff -u -r1.5 -r1.6 src/sys/stand/efiboot/efiblock.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->pending-pullups
State-Changed-By: jmcneill@NetBSD.org
State-Changed-When: Sat, 21 Sep 2019 10:20:31 +0000
State-Changed-Why:
Patch applied, thanks!
From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/54554 CVS commit: [netbsd-9] src/sys/stand/efiboot
Date: Sun, 22 Sep 2019 12:37:39 +0000
Module Name: src
Committed By: martin
Date: Sun Sep 22 12:37:39 UTC 2019
Modified Files:
src/sys/stand/efiboot [netbsd-9]: efiblock.c
Log Message:
Pull up following revision(s) (requested by jmcneill in ticket #225):
sys/stand/efiboot/efiblock.c: revision 1.6
Honour block device's IO alignment requirements.
Patch from Simon South <simon%simonsouth.net@localhost> in PR# 54554
To generate a diff of this commit:
cvs rdiff -u -r1.5 -r1.5.6.1 src/sys/stand/efiboot/efiblock.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: pending-pullups->closed
State-Changed-By: jmcneill@NetBSD.org
State-Changed-When: Sun, 22 Sep 2019 19:24:14 +0000
State-Changed-Why:
Pulled up to netbsd-9.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.43 2018/01/16 07:36:43 maya Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2017
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home