[tip: x86/build] x86/boot: Add back some padding for the CRC-32 checksum

Posted by tip-bot2 for Ard Biesheuvel 9 months, 1 week ago

The following commit has been merged into the x86/build branch of tip:

Commit-ID:     e471a86a8c523eccdfd1c4745ed7ac7cbdcc1f3f
Gitweb:        https://git.kernel.org/tip/e471a86a8c523eccdfd1c4745ed7ac7cbdcc1f3f
Author:        Ard Biesheuvel <ardb@kernel.org>
AuthorDate:    Wed, 12 Mar 2025 09:12:05 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 12 Mar 2025 13:04:52 +01:00

x86/boot: Add back some padding for the CRC-32 checksum

Even though no uses of the bzImage CRC-32 checksum are known, ensure
that the last 4 bytes of the image are unused zero bytes, so that the
checksum can be generated post-build if needed.

[ mingo: Added the 'obsolete' qualifier to the comment. ]

Suggested-by: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Ian Campbell <ijc@hellion.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/r/20250312081204.521411-2-ardb+git@google.com
---
 arch/x86/boot/compressed/vmlinux.lds.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/vmlinux.lds.S b/arch/x86/boot/compressed/vmlinux.lds.S
index 48d0b51..3b2bc61 100644
--- a/arch/x86/boot/compressed/vmlinux.lds.S
+++ b/arch/x86/boot/compressed/vmlinux.lds.S
@@ -48,7 +48,8 @@ SECTIONS
 		*(.data)
 		*(.data.*)
 
-		. = ALIGN(0x200);
+		/* Add 4 bytes of extra space for the obsolete CRC-32 checksum */
+		. = ALIGN(. + 4, 0x200);
 		_edata = . ;
 	}
 	. = ALIGN(L1_CACHE_BYTES);

Re: [tip: x86/build] x86/boot: Add back some padding for the CRC-32 checksum

Posted by Borislav Petkov 8 months, 1 week ago

On Wed, Mar 12, 2025 at 12:09:34PM -0000, tip-bot2 for Ard Biesheuvel wrote:
> The following commit has been merged into the x86/build branch of tip:
> 
> Commit-ID:     e471a86a8c523eccdfd1c4745ed7ac7cbdcc1f3f
> Gitweb:        https://git.kernel.org/tip/e471a86a8c523eccdfd1c4745ed7ac7cbdcc1f3f
> Author:        Ard Biesheuvel <ardb@kernel.org>
> AuthorDate:    Wed, 12 Mar 2025 09:12:05 +01:00
> Committer:     Ingo Molnar <mingo@kernel.org>
> CommitterDate: Wed, 12 Mar 2025 13:04:52 +01:00
> 
> x86/boot: Add back some padding for the CRC-32 checksum
> 
> Even though no uses of the bzImage CRC-32 checksum are known, ensure
> that the last 4 bytes of the image are unused zero bytes, so that the
> checksum can be generated post-build if needed.

Sounds like it is not needed and sounds like we should whack this thing no?

Or are we doing a grace period and then whack it when that grace period
expires?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/build] x86/boot: Add back some padding for the CRC-32 checksum

Posted by Ard Biesheuvel 8 months, 1 week ago

On Mon, 14 Apr 2025 at 15:56, Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Mar 12, 2025 at 12:09:34PM -0000, tip-bot2 for Ard Biesheuvel wrote:
> > The following commit has been merged into the x86/build branch of tip:
> >
> > Commit-ID:     e471a86a8c523eccdfd1c4745ed7ac7cbdcc1f3f
> > Gitweb:        https://git.kernel.org/tip/e471a86a8c523eccdfd1c4745ed7ac7cbdcc1f3f
> > Author:        Ard Biesheuvel <ardb@kernel.org>
> > AuthorDate:    Wed, 12 Mar 2025 09:12:05 +01:00
> > Committer:     Ingo Molnar <mingo@kernel.org>
> > CommitterDate: Wed, 12 Mar 2025 13:04:52 +01:00
> >
> > x86/boot: Add back some padding for the CRC-32 checksum
> >
> > Even though no uses of the bzImage CRC-32 checksum are known, ensure
> > that the last 4 bytes of the image are unused zero bytes, so that the
> > checksum can be generated post-build if needed.
>
> Sounds like it is not needed and sounds like we should whack this thing no?
>
> Or are we doing a grace period and then whack it when that grace period
> expires?
>

This was done on hpa's request - maybe he has a duration in mind for
this grace period?

Re: [tip: x86/build] x86/boot: Add back some padding for the CRC-32 checksum

Posted by H. Peter Anvin 8 months, 1 week ago

On April 14, 2025 7:07:53 AM PDT, Ard Biesheuvel <ardb@kernel.org> wrote:
>On Mon, 14 Apr 2025 at 15:56, Borislav Petkov <bp@alien8.de> wrote:
>>
>> On Wed, Mar 12, 2025 at 12:09:34PM -0000, tip-bot2 for Ard Biesheuvel wrote:
>> > The following commit has been merged into the x86/build branch of tip:
>> >
>> > Commit-ID:     e471a86a8c523eccdfd1c4745ed7ac7cbdcc1f3f
>> > Gitweb:        https://git.kernel.org/tip/e471a86a8c523eccdfd1c4745ed7ac7cbdcc1f3f
>> > Author:        Ard Biesheuvel <ardb@kernel.org>
>> > AuthorDate:    Wed, 12 Mar 2025 09:12:05 +01:00
>> > Committer:     Ingo Molnar <mingo@kernel.org>
>> > CommitterDate: Wed, 12 Mar 2025 13:04:52 +01:00
>> >
>> > x86/boot: Add back some padding for the CRC-32 checksum
>> >
>> > Even though no uses of the bzImage CRC-32 checksum are known, ensure
>> > that the last 4 bytes of the image are unused zero bytes, so that the
>> > checksum can be generated post-build if needed.
>>
>> Sounds like it is not needed and sounds like we should whack this thing no?
>>
>> Or are we doing a grace period and then whack it when that grace period
>> expires?
>>
>
>This was done on hpa's request - maybe he has a duration in mind for
>this grace period?

I would prefer to leave it indefinitely, because an all zero pattern is far easier to detect than what would look like a false checksum.

So I remembered eventually who wanted it: it was a direct from flash boot loader who wanted to detect a partial flash failure before invoking the kernel, so that it can redirect to a secondary kernel.

That would obviously not be an UEFI environment, so the signing issue is not applicable.

An all zero end field actually works for that purpose (although requires a boot loader patch), because an unprogrammed flash sector contains FFFFFFFF not 00000000.

We have kept the bzImage format backwards compatible – sometimes at considerable effort – and the cost of reasonably continuing to do so is absolutely minimal. This is an incompatible change, so at least it is appropriate to give unambiguous indication thereof.

In other words: it ain't broken, don't try to fix it. It is all downside, no upside.

Re: [tip: x86/build] x86/boot: Add back some padding for the CRC-32 checksum

Posted by Borislav Petkov 8 months ago

On Tue, Apr 15, 2025 at 10:00:11AM -0700, H. Peter Anvin wrote:
> >This was done on hpa's request - maybe he has a duration in mind for
> >this grace period?
> 
> I would prefer to leave it indefinitely, because an all zero pattern is far easier to detect than what would look like a false checksum.
> 
> So I remembered eventually who wanted it: it was a direct from flash boot loader who wanted to detect a partial flash failure before invoking the kernel, so that it can redirect to a secondary kernel.

What is a "direct from flash boot loader"?

> That would obviously not be an UEFI environment, so the signing issue is not applicable.
> 
> An all zero end field actually works for that purpose (although requires a boot loader patch), because an unprogrammed flash sector contains FFFFFFFF not 00000000.
> 
> We have kept the bzImage format backwards compatible – sometimes at considerable effort – and the cost of reasonably continuing to do so is absolutely minimal. This is an incompatible change, so at least it is appropriate to give unambiguous indication thereof.
> 
> In other words: it ain't broken, don't try to fix it. It is all downside, no upside.

Sure but look at what is there now:

                /* Add 4 bytes of extra space for the obsolete CRC-32 checksum */
                . = ALIGN(. + 4, 0x200);
                _edata = . ;

This basically screams at me: "delete me, delete me!" :-P

So I would probably put the gist of what you say above as a comment there so
that we have the rationale stated there, for future, trigger-happy
generations.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/build] x86/boot: Add back some padding for the CRC-32 checksum

Posted by H. Peter Anvin 8 months ago

On April 15, 2025 2:54:27 PM PDT, Borislav Petkov <bp@alien8.de> wrote:
>On Tue, Apr 15, 2025 at 10:00:11AM -0700, H. Peter Anvin wrote:
>> >This was done on hpa's request - maybe he has a duration in mind for
>> >this grace period?
>> 
>> I would prefer to leave it indefinitely, because an all zero pattern is far easier to detect than what would look like a false checksum.
>> 
>> So I remembered eventually who wanted it: it was a direct from flash boot loader who wanted to detect a partial flash failure before invoking the kernel, so that it can redirect to a secondary kernel.
>
>What is a "direct from flash boot loader"?
>
>> That would obviously not be an UEFI environment, so the signing issue is not applicable.
>> 
>> An all zero end field actually works for that purpose (although requires a boot loader patch), because an unprogrammed flash sector contains FFFFFFFF not 00000000.
>> 
>> We have kept the bzImage format backwards compatible – sometimes at considerable effort – and the cost of reasonably continuing to do so is absolutely minimal. This is an incompatible change, so at least it is appropriate to give unambiguous indication thereof.
>> 
>> In other words: it ain't broken, don't try to fix it. It is all downside, no upside.
>
>Sure but look at what is there now:
>
>                /* Add 4 bytes of extra space for the obsolete CRC-32 checksum */
>                . = ALIGN(. + 4, 0x200);
>                _edata = . ;
>
>This basically screams at me: "delete me, delete me!" :-P
>
>So I would probably put the gist of what you say above as a comment there so
>that we have the rationale stated there, for future, trigger-happy
>generations.
>
>Thx.
>

It is an embedded boot loader that loads the kernel image from flash (either NAND or NOR), without a file system.