Re: [PATCH v2] spl: fit: Report fdt error for loading u-boot
Dear all,
Any updates or new comments on this? How should I proceed?
BRs/Baocheng Su
On Thu, 2022-10-20 at 15:44 +0200, Mark Kettenis wrote:
> > From: Simon Glass
> > Date: Wed, 19 Oct 2022 08:15:35 -0600
> >
> > Hi Mark,
> >
> > On Wed, 19 Oct 2022 at 08:08, Mark Kettenis
> > wrote:
> > >
> > > > From: Simon Glass
> > > > Date: Wed, 19 Oct 2022 07:18:10 -0600
> > > >
> > > > Hi,
> > > >
> > > > On Mon, 17 Oct 2022 at 05:53, Su, Bao Cheng
> > > > wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > >
> > > > +Tom Rini for guidance
> > > >
> > > > > On Sat, 2022-07-30 at 19:27 -0600, Simon Glass wrote:
> > > > > > Hi Bao Cheng,
> > > > > >
> > > > > > On Sat, 30 Jul 2022 at 03:05, Su, Bao Cheng
> > > > > > wrote:
> > > > > > >
> > > > > > > Commit 71551055cbdb ("spl: fit: Load devicetree when a
> > > > > > > Linux payload is
> > > > > > > found") made a change to not report the spl_fit_append_fdt
> > > > > > > error at all
> > > > > > > if next-stage image is u-boot.
> > > > > > >
> > > > > > > However for u-boot image without CONFIG_OF_EMBED, the
> > > > > > > error should be
> > > > > > > reported to uplevel caller. Otherwise, uplevel caller
> > > > > > > would think the
> > > > > > > fdt is already loaded which is obviously not true.
> > > > > > >
> > > > > > > Signed-off-by: Baocheng Su
> > > > > > > ---
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > > - Fix the wrong wrapping
> > > > > > >
> > > > > > > common/spl/spl_fit.c | 8 ++--
> > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> > > > > > > index a35be52965..00404935cb 100644
> > > > > > > --- a/common/spl/spl_fit.c
> > > > > > > +++ b/common/spl/spl_fit.c
> > > > > > > @@ -770,8 +770,12 @@ int spl_load_simple_fit(struct
> > > > > > > spl_image_info *spl_image,
> > > > > > > */
> > > > > > > if (os_takes_devicetree(spl_image->os)) {
> > > > > > > ret = spl_fit_append_fdt(spl_image, info,
> > > > > > > sector, &ctx);
> > > > > > > - if (ret < 0 && spl_image->os !=
> > > > > > > IH_OS_U_BOOT)
> > > > > > > - return ret;
> > > > > > > + if (ret < 0) {
> > > > > > > + if (spl_image->os != IH_OS_U_BOOT)
> > > > > > > + return ret;
> > > > > > > + else if
> > > > > > > (!IS_ENABLED(CONFIG_OF_EMBED))
> > > > > > > + return ret;
> > > > > >
> > > > > > This is a pretty unpleasant condition. I think we would be
> > > > > > better to
> > > > > > report the error and let the caller figure it out.
> > > > > >
> > > > > > There are no tests associated with this, so it is hard to
> > > > > > know what is
> > > > > > actually going on.
> > > > > >
> > > > > > If we must have this workaround, I suggest adding a Kconfig
> > > > > > so boards
> > > > > > that need it can turn it on, and other boards can use normal
> > > > > > operation, which is to report errors.
> > > > > >
> > > > >
> > > > > Since there is no particular error code stands for such kind
> > > > > of
> > > > > scenario, it would be hard for the caller to determine which
> > > > > step has
> > > > > the problem.
> > > > >
> > > > > Or below code is more clear?
> > > > >
> > > > > if (os_takes_
Re: [PATCH v2] spl: fit: Report fdt error for loading u-boot
Hi Simon,
On Sat, 2022-07-30 at 19:27 -0600, Simon Glass wrote:
> Hi Bao Cheng,
>
> On Sat, 30 Jul 2022 at 03:05, Su, Bao Cheng wrote:
> >
> > Commit 71551055cbdb ("spl: fit: Load devicetree when a Linux payload is
> > found") made a change to not report the spl_fit_append_fdt error at all
> > if next-stage image is u-boot.
> >
> > However for u-boot image without CONFIG_OF_EMBED, the error should be
> > reported to uplevel caller. Otherwise, uplevel caller would think the
> > fdt is already loaded which is obviously not true.
> >
> > Signed-off-by: Baocheng Su
> > ---
> >
> > Changes in v2:
> > - Fix the wrong wrapping
> >
> > common/spl/spl_fit.c | 8 ++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> > index a35be52965..00404935cb 100644
> > --- a/common/spl/spl_fit.c
> > +++ b/common/spl/spl_fit.c
> > @@ -770,8 +770,12 @@ int spl_load_simple_fit(struct spl_image_info
> > *spl_image,
> > */
> > if (os_takes_devicetree(spl_image->os)) {
> > ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
> > - if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
> > - return ret;
> > + if (ret < 0) {
> > + if (spl_image->os != IH_OS_U_BOOT)
> > + return ret;
> > + else if (!IS_ENABLED(CONFIG_OF_EMBED))
> > + return ret;
>
> This is a pretty unpleasant condition. I think we would be better to
> report the error and let the caller figure it out.
>
> There are no tests associated with this, so it is hard to know what is
> actually going on.
>
> If we must have this workaround, I suggest adding a Kconfig so boards
> that need it can turn it on, and other boards can use normal
> operation, which is to report errors.
>
Since there is no particular error code stands for such kind of
scenario, it would be hard for the caller to determine which step has
the problem.
Or below code is more clear?
if (os_takes_devicetree(spl_image->os)) {
ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
- if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
- return ret;
+ if (ret < 0
+&& (spl_image->os != IH_OS_U_BOOT
+ || !IS_ENABLED(CONFIG_OF_EMBED)))
+ return ret;
}
Actually there is already the `CONFIG_OF_EMBED` to tell them apart, see
the previous logic before commit 71551055cbdb:
* Booting a next-stage U-Boot may require us to append the FDT.
* We allow this to fail, as the U-Boot image might embed its
FDT.
*/
- if (spl_image->os == IH_OS_U_BOOT) {
+ if (os_takes_devicetree(spl_image->os)) {
ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
- if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0)
+ if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
return ret;
}
So before the commit 71551055cbdb, the normal case would be to report
the error, but the commit in question changed this to not report the
error for normal spl to boot u-boot, only reports error for SPL to boot
kernel, i.e. falcon mode.
> Regards,
> Simon
[PATCH v2] spl: fit: Report fdt error for loading u-boot
Commit 71551055cbdb ("spl: fit: Load devicetree when a Linux payload is
found") made a change to not report the spl_fit_append_fdt error at all
if next-stage image is u-boot.
However for u-boot image without CONFIG_OF_EMBED, the error should be
reported to uplevel caller. Otherwise, uplevel caller would think the
fdt is already loaded which is obviously not true.
Signed-off-by: Baocheng Su
---
Changes in v2:
- Fix the wrong wrapping
common/spl/spl_fit.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index a35be52965..00404935cb 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -770,8 +770,12 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
*/
if (os_takes_devicetree(spl_image->os)) {
ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
- if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
- return ret;
+ if (ret < 0) {
+ if (spl_image->os != IH_OS_U_BOOT)
+ return ret;
+ else if (!IS_ENABLED(CONFIG_OF_EMBED))
+ return ret;
+ }
}
firmware_node = node;
--
2.30.2
[PATCH] spl: fit: Report fdt error for loading u-boot
Commit 71551055cbdb ("spl: fit: Load devicetree when a Linux payload is
found") made a change to not report the spl_fit_append_fdt error at all
if next-stage image is u-boot.
However for u-boot image without CONFIG_OF_EMBED, the error should be
reported to uplevel caller. Otherwise, uplevel caller would think the
fdt is already loaded which is obviously not true.
Signed-off-by: Baocheng Su
---
common/spl/spl_fit.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index a35be52965..00404935cb 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -770,8 +770,12 @@ int spl_load_simple_fit(struct spl_image_info
*spl_image,
*/
if (os_takes_devicetree(spl_image->os)) {
ret = spl_fit_append_fdt(spl_image, info, sector,
&ctx);
- if (ret < 0 && spl_image->os != IH_OS_U_BOOT)
- return ret;
+ if (ret < 0) {
+ if (spl_image->os != IH_OS_U_BOOT)
+ return ret;
+ else if (!IS_ENABLED(CONFIG_OF_EMBED))
+ return ret;
+ }
}
firmware_node = node;
--
2.30.2
[PATCH v2] efi_loader: PE hashing for image with gap between sections
Some PE image has gap between sections. These gaps could be kernel
symbol table that does not belong to any sections.
For such kind of image, after the last section is added into the
hashing, the bytes_hashed is less than the (PE file size - Size of
AttributeCertificateTable). According to Step 14 of `Calculating the PE
Image Hash` in the `Windows Authenticode Portable Executable Signature
Format` Version 1.0 — March 21, 2008:
"
Create a value called FILE_SIZE, which is not part of the signature.
Set
this value to the image's file size, acquired from the underlying file
system. If FILE_SIZE is greater than SUM_OF_BYTES_HASHED, the file
contains extra data that must be added to the hash. This data begins at
the SUM_OF_BYTES_HASHED file offset, and its length is: (File Size) -
((Size of AttributeCertificateTable) + SUM_OF_BYTES_HASHED)
"
Some overlapped data could be added into the hashing again. Some other
tools such as sbgisn/pesign/EDK2 behave just as the Step 14 when
dealing
with such kind of PE image. However currently u-boot just ignores this
overlapped data and refuses to hash them again.
By toggling the `nocheck` of the efi_image_region_add to 1, the
overlapping checking could be avoided.
Signed-off-by: Baocheng Su
---
Changes in v2:
- Reword the commit message for better understanding.
lib/efi_loader/efi_image_loader.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_image_loader.c
b/lib/efi_loader/efi_image_loader.c
index 9611398885..d85fb6ba08 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -481,7 +481,7 @@ bool efi_image_parse(void *efi, size_t len, struct
efi_image_regions **regp,
EFI_PRINT("extra data for hash: %zu\n",
len - (bytes_hashed + authsz));
efi_image_region_add(regs, efi + bytes_hashed,
-efi + len - authsz, 0);
+efi + len - authsz, 1);
}
/* Return Certificates Table */
--
2.30.2
Re: [PATCH] efi_loader: Allow overlapped extra data for PE hashing
On Mon, 2022-06-27 at 16:32 +0200, Heinrich Schuchardt wrote:
> On 6/24/22 07:32, Su, Bao Cheng wrote:
> > During PE hashing, when holes exists between sections, the extra
> > data
> > calculated could be a dupulicated region of the last section.
> >
> > Such PE image with holes existing between sections may contain the
> > symbol table for the kernel, for example.
> >
> > The Authenticode_PE spec does not rule how to deal with such
> > scenario,
> > however, other tools such as pesign and sbsign both have the
> > overlapped
> > regions hashed. And EDK2 hash the overlapped area as well.
> >
> > Signed-off-by: Baocheng Su
> > ---
> > lib/efi_loader/efi_image_loader.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/efi_loader/efi_image_loader.c
> > b/lib/efi_loader/efi_image_loader.c
> > index 9611398885..d85fb6ba08 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -481,7 +481,7 @@ bool efi_image_parse(void *efi, size_t len,
> > struct
> > efi_image_regions **regp,
> > EFI_PRINT("extra data for hash: %zu\n",
> > len - (bytes_hashed + authsz));
> > efi_image_region_add(regs, efi + bytes_hashed,
> > - efi + len - authsz, 0);
> > + efi + len - authsz, 1);
> > }
> >
> > /* Return Certificates Table */
>
> Let us consider the case that the sum of gaps between sections is
> greater than the size of the last section N.
>
> start[N] > efi + bytes_hashed
> end[N] < efi + len - authsz
>
> Sbsigntool and EDK II sort regions by start address before adding the
> extra data region and will accept this situation.
>
> U-Boot's efi_image_region_add(nocheck = 1) will throw an error "%s:
> new
> region already part of another\n".
>
This is the original code of efi_image_region_add:
```
for (i = 0; i < regs->num; i++) {
reg = ®s->reg[i];
if (nocheck)
continue;
/* new data after registered region */
if (start >= reg->data + reg->size)
continue;
/* new data preceding registered region */
if (end <= reg->data) {
for (j = regs->num - 1; j >= i; j--)
memcpy(®s->reg[j + 1], ®s-
>reg[j],
sizeof(*reg));
break;
}
/* new data overlapping registered region */
EFI_PRINT("%s: new region already part of another\n",
__func__);
return EFI_INVALID_PARAMETER;
}
```
Notice the `if (nocheck) continue;`, I would not say the `new region
already part of another` be executed.
- Baocheng
> It seems that this patch is not a complete solution.
>
> Best regards
>
> Heinrich
Re: [PATCH] efi_loader: Allow overlapped extra data for PE hashing
On Fri, 2022-06-24 at 11:44 +0200, Jan Kiszka wrote: > On 24.06.22 10:53, Heinrich Schuchardt wrote: > > On 6/24/22 07:32, Su, Bao Cheng wrote: > > > During PE hashing, when holes exists between sections, the extra > > > data > > > calculated could be a dupulicated region of the last section. > > > > > > Such PE image with holes existing between sections may contain > > > the > > > symbol table for the kernel, for example. > > > > > > The Authenticode_PE spec does not rule how to deal with such > > > scenario, > > > however, other tools such as pesign and sbsign both have the > > > overlapped > > > > Thanks for analyzing differences in hashing. > > > > Above you mention holes between sections. Here you talk about > > overlapping sections. These two cases are obviously distinct. > > > > Please, provide an accurate description. > > Yeah, I also gave that feedback internally already as it left me a > bit > confused. > > > > > Examples (in text form) would be helpful. > > There is apparently no good PE dump tooling available, so I try to > describe our scenario verbally: > > We are generating a unified kernel image, similar to what systemd > does, > for ARM and ARM64 [1]. The stub has .text and .data sections, and > then > follows the symbol table (some versions of binutils allow to suppress > it, other not, sigh). When appending the actual payload to that > (kernel > image, command line, initrd, dtbs), those sections are added right > after > the symbol table, creating an unhashed gap between the last stub > section > and the first appended one. That unified linux.efi is then signed and > should be verifiable and bootable (as it is with EDK2). > I will try to give a more straightforward description, considering below PE image: ## PE Header: @0x ... ### Section Header 1: ... @0x0108 : 0x8000 - SizeOfRawData @0x010C : 0x1000 - PointerToRawData ... ### Section Header 2: ... @0x0130 : 0x1C00 - SizeOfRawData @0x0134 : 0x9000 - PointerToRawData ... ### Section Header 3: ... @0x0158 : 0x1200 - SizeOfRawData @0x015C : 0xB200 - PointerToRawData ... From the section headers, the end offset of section 2 is 0x1C00 + 0x9000 = 0xAC00, however, the start offset of the section 3 is 0xB200, there is a `hole` here of size 0x600 bytes. In our case Jan has explained this is the symbol table. According to PE hasing spec, when finished the parsing of sections, the bytes_hashed should be calculated and compared to the (total PE size - auth size), and if the bytes_hashed is lesser, it means there are extra data need be hashed as well. According to spec, the offset of the extra data is set to bytes_hashed, this does not cause overlapping for a normal PE image without holes between sections, because the bytes_hashed is equal to the tail of the last section. However, for our case the extra data is the overlapped with the last section or sections, because the bytes_hashed is lesser than the tail of the last section due to the `hole`. U-Boot currently considers this part of data as overlapped and excludes them from the hashing, however other tools or BLs such as pesign/sbsign/EDK2 do not rule out the overlapped data, the hash result stays consistent among these tools, although the last part is hashed twice indeed. Baocheng > HTH, > Jan > > [1] > https://github.com/siemens/efibootguard/blob/master/docs/UNIFIED-KERNEL.md >
Re: [PATCH] efi_loader: Allow overlapped extra data for PE hashing
On Mon, 2022-06-27 at 06:55 +0200, Heinrich Schuchardt wrote: > On 6/27/22 05:43, Su, Bao Cheng wrote: > > On Fri, 2022-06-24 at 11:44 +0200, Jan Kiszka wrote: > > > On 24.06.22 10:53, Heinrich Schuchardt wrote: > > > > On 6/24/22 07:32, Su, Bao Cheng wrote: > > > > > During PE hashing, when holes exists between sections, the > > > > > extra > > > > > data > > > > > calculated could be a dupulicated region of the last section. > > > > > > > > > > Such PE image with holes existing between sections may > > > > > contain > > > > > the > > > > > symbol table for the kernel, for example. > > > > > > > > > > The Authenticode_PE spec does not rule how to deal with such > > > > > scenario, > > > > > however, other tools such as pesign and sbsign both have the > > > > > overlapped > > > > > > > > Thanks for analyzing differences in hashing. > > > > > > > > Above you mention holes between sections. Here you talk about > > > > overlapping sections. These two cases are obviously distinct. > > > > > > > > Please, provide an accurate description. > > > > > > Yeah, I also gave that feedback internally already as it left me > > > a > > > bit > > > confused. > > > > > > > > > > > Examples (in text form) would be helpful. > > > > > > There is apparently no good PE dump tooling available, so I try > > > to > > > describe our scenario verbally: > > You could try > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fxypron%2Fefi_analyzer&data=05%7C01%7Cbaocheng.su%40ad011.siemens.com%7Cc732b1c183ff4cf56ca708da57f943f2%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637919025335317245%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=W1gO6h2%2BiFSUzdNTFi551gyoxlybMOEwQgBnf%2Bswnjw%3D&reserved=0 > . > > > > > > > We are generating a unified kernel image, similar to what systemd > > > does, > > > for ARM and ARM64 [1]. The stub has .text and .data sections, and > > > then > > > follows the symbol table (some versions of binutils allow to > > > suppress > > > it, other not, sigh). When appending the actual payload to that > > > (kernel > > > image, command line, initrd, dtbs), those sections are added > > > right > > > after > > > the symbol table, creating an unhashed gap between the last stub > > > section > > > and the first appended one. That unified linux.efi is then signed > > > and > > > should be verifiable and bootable (as it is with EDK2). > > > > > > > I will try to give a more straightforward description, considering > > below PE image: > > > > ## PE Header: > > @0x > > ... > > ### Section Header 1: > > ... > > @0x0108 : 0x8000 - SizeOfRawData > > @0x010C : 0x1000 - PointerToRawData > > ... > > ### Section Header 2: > > ... > > @0x0130 : 0x1C00 - SizeOfRawData > > @0x0134 : 0x9000 - PointerToRawData > > ... > > ### Section Header 3: > > ... > > @0x0158 : 0x1200 - SizeOfRawData > > @0x015C : 0xB200 - PointerToRawData > > ... > > > > From the section headers, the end offset of section 2 is 0x1C00 + > > 0x9000 = 0xAC00, however, the start offset of the section 3 is > > 0xB200, > > there is a `hole` here of size 0x600 bytes. In our case Jan has > > explained this is the symbol table. > > > > According to PE hasing spec, when finished the parsing of sections, > > the > > bytes_hashed should be calculated and compared to the (total PE > > size - > > auth size), and if the bytes_hashed is lesser, it means there are > > extra > > data need be hashed as well. > > > > According to spec, the offset of the extra data is set to > > bytes_hashed, > > this does not cause overlapping for a normal PE image without holes > > between sections, because the bytes_hashed is equal to the tail of > > the > > last section. However, for our case the extra data is the > > overlapped > > with the last section or sections, because the bytes_hashed is > > lesser > > than the tail of
[PATCH] efi_loader: Allow overlapped extra data for PE hashing
During PE hashing, when holes exists between sections, the extra data
calculated could be a dupulicated region of the last section.
Such PE image with holes existing between sections may contain the
symbol table for the kernel, for example.
The Authenticode_PE spec does not rule how to deal with such scenario,
however, other tools such as pesign and sbsign both have the overlapped
regions hashed. And EDK2 hash the overlapped area as well.
Signed-off-by: Baocheng Su
---
lib/efi_loader/efi_image_loader.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_image_loader.c
b/lib/efi_loader/efi_image_loader.c
index 9611398885..d85fb6ba08 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -481,7 +481,7 @@ bool efi_image_parse(void *efi, size_t len, struct
efi_image_regions **regp,
EFI_PRINT("extra data for hash: %zu\n",
len - (bytes_hashed + authsz));
efi_image_region_add(regs, efi + bytes_hashed,
-efi + len - authsz, 0);
+efi + len - authsz, 1);
}
/* Return Certificates Table */
--
2.30.2
