Bug 1418360 - grub2 incorrectly initialises the boot_params from the kernel image
Summary: grub2 incorrectly initialises the boot_params from the kernel image
Keywords:
Status: CLOSED EOL
Alias: None
Product: Fedora
Classification: Fedora
Component: grub2
Version: 25
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Peter Jones
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-02-01 16:08 UTC by David Howells
Modified: 2017-12-12 10:33 UTC (History)
11 users (show)

Fixed In Version: grub-2.02-0.40.fc26 grub2-2.02-0.40.fc26
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-12-12 10:33:57 UTC


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Bugzilla 1451071 None CLOSED Secure boot flag could not be determined 2019-07-20 01:04:25 UTC

Internal Links: 1451071

Description David Howells 2017-02-01 16:08:49 UTC
Description of problem:

To quote Matt Fleming:

  grub2 memcpy()'s 1024 bytes from the start of kernel image header
  into the allocated (and zeroed) boot_params object. Unfortunately,
  it should only be copying the second 512-byte chunk, not the first
  too.

  The boot loader should only fill out those fields in the first 512
  bytes that it understands. Everything else should be zero, which
  allows us to add fields (and give them default non-zero values in
  the header) in the future without breaking old boot loaders.

In grub-core/loader/i386/efi/linux.c is the following line:

   grub_memcpy (params, &lh, 2 * 512);

But this should be something like:

   grub_memcpy ((grub_uint8_t *)params + 512, (grub_uint8_t *)&lh[512], 512);

The problem with copying 1024 bytes is that it sets the sentinel byte to 0xff rather than leaving it as 0 - and this triggers sanitize_boot_params() to clear various parts of the boot_params struct.

This is what the comment in the definition of struct boot_params says:

 * A bootloader is supposed to only take setup_header and put
 * it into a clean boot_params buffer. If it turns out that
 * it is clumsy or too generous with the buffer, it most
 * probably will pick up the sentinel variable too. The fact
 * that this variable then is still 0xff will let kernel
 * know that some variables in boot_params are invalid and
 * kernel should zero out certain portions of boot_params.

Now, one could argue that this means that only boot_params->hdr should be copied (offset 0x1f1 up to 0x290).

Comment 1 David Howells 2017-02-01 16:14:41 UTC
> Now, one could argue that this means that only boot_params->hdr
> should be copied (offset 0x1f1 up to 0x290).

Actually, 0x1f1 for 119 bytes.  I don't know if the subsequent padding is meant for struct setup_header to grow into.

Comment 2 David Howells 2017-02-01 16:16:49 UTC
The problem with the triggered sanitization is that it clobbers the secure-boot mode parameter calculated in the EFI boot wrapper and passed through boot_params to the core kernel.

Comment 3 poma 2017-04-04 14:35:53 UTC
(In reply to David Howells from comment #0)
[...]
> In grub-core/loader/i386/efi/linux.c is the following line:
> 
>    grub_memcpy (params, &lh, 2 * 512);
> 
> But this should be something like:
> 
>    grub_memcpy ((grub_uint8_t *)params + 512, (grub_uint8_t *)&lh[512], 512);
[...]

diff --git a/grub-core/loader/i386/efi/linux.c b/grub-core/loader/i386/efi/linux.c
index 010bf98..02b1327 100644
--- a/grub-core/loader/i386/efi/linux.c
+++ b/grub-core/loader/i386/efi/linux.c
@@ -269,7 +269,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   loaded=1;
 
   lh.code32_start = (grub_uint32_t)(grub_uint64_t) kernel_mem;
-  grub_memcpy (params, &lh, 2 * 512);
+  grub_memcpy ((grub_uint8_t *)params + 512, (grub_uint8_t *)&lh[512], 512);
 
   params->type_of_loader = 0x21;
 

make
...
loader/i386/efi/linux.c: In function ‘grub_cmd_linux’:
loader/i386/efi/linux.c:272:65: error: subscripted value is neither array nor pointer nor vector
   grub_memcpy ((grub_uint8_t *)params + 512, (grub_uint8_t *)&lh[512], 512);
                                                                 ^
...

Comment 4 poma 2017-04-04 14:38:44 UTC
*** Bug 1438397 has been marked as a duplicate of this bug. ***

Comment 5 hanishkvc 2017-06-05 13:15:31 UTC
Any specific reason why this bug is still not resolved? Also isn't there a bug in kernel also.

Looking at the note given in struct boot_param, as well as how the efi boot wrapper within in the kernel is using some part of the boot_param to set its own parameters, after it has got control from the original bootloader, why is the sanitize_boot_params WRONGLY reseting parameters which are (or can be as is the case here) set outside/after the bootloader.

Other than fixing the blind and potentially wrong copying of parameters area into boot_param by grub. The kernel should also be fixed to have some additional parameter to check if bootloader has set the secure boot related params or the kernel's efi boot wrapper has set it, and then sanitize only if required.

OR am I missing something here?

Comment 6 Fedora Update System 2017-06-26 21:27:47 UTC
grub2-2.02-0.40.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-01b07bc086

Comment 7 Fedora Blocker Bugs Application 2017-06-26 21:33:51 UTC
Proposed as a Blocker and Freeze Exception for 26-final by Fedora user pjones using the blocker tracking app because:

 Without this we can't verify from inside the OS that Secure Boot is enabled.

Comment 8 Dominik 'Rathann' Mierzejewski 2017-06-27 10:16:39 UTC
(In reply to Fedora Update System from comment #6)
> grub2-2.02-0.40.fc26 has been submitted as an update to Fedora 26.
> https://bodhi.fedoraproject.org/updates/FEDORA-2017-01b07bc086

Please fix on F25 as well and possibly on F24, as originally reported.

Comment 9 Fedora Update System 2017-06-27 20:24:54 UTC
grub2-2.02-0.40.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-01b07bc086

Comment 10 David Howells 2017-06-27 21:26:15 UTC
The new grub works for me.

Comment 11 Adam Williamson 2017-06-28 21:50:17 UTC
Would the fix for this take effect after a normal update of grub2, or is there some reason the fix for this needs to appear in the frozen release package set?

Comment 12 Adam Williamson 2017-06-29 19:10:36 UTC
For the record, pjones added further information by email:

"basically Secure Boot doesn't trigger kmod signature checking, read-only /dev/mem, etc., in the current trees. This update fixes a grub bug that's triggering that behavior in the newer kernels, but was not triggering it in the older ones."

AIUI, "kmod signature checking, read-only /dev/mem, etc." refers to various measures that are taken by the kernel when SB is known to be enabled, intended to defeat attempts to circumvent SB by e.g. loading a nefarious kernel module. So if I understand correctly, this is effectively a security issue (as it reduces the effectiveness of SB protections on systems which are using SB).

With that in mind: Discussed at 2017-06-29 blocker review meeting: https://meetbot-raw.fedoraproject.org/fedora-blocker-review/2017-06-29/f26-blocker-review.2017-06-29-16.00.html . We agreed to at least accept this (and the companion bug #1451071) as freeze exception issues, which should be sufficient to ensure they're resolved in Final as the fixes are already available. We considered also accepting them as blockers under the "The release must contain no known security bugs of 'important' or higher impact according to the Red Hat severity classification scale which cannot be satisfactorily resolved by a package update (e.g. issues during installation)." criterion, but as this issue isn't *formally* a security issue at least yet, and doesn't have a formal security evaluation, and FE status should be sufficient to get the fixes in anyhow, we decided to leave blocker status undetermined for now. FESCo may choose to make these bugs blockers by fiat.

Comment 13 Fedora Update System 2017-06-29 23:28:25 UTC
grub2-2.02-0.40.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

Comment 14 Dominik 'Rathann' Mierzejewski 2017-07-21 22:46:35 UTC
This is not fixed in F25 or F24, so it shouldn't be closed.

Comment 15 Fedora End Of Life 2017-07-26 00:13:49 UTC
This message is a reminder that Fedora 24 is nearing its end of life.
Approximately 2 (two) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 24. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '24'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version'
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not
able to fix it before Fedora 24 is end of life. If you would still like
to see this bug fixed and are able to reproduce it against a later version
of Fedora, you are encouraged  change the 'version' to a later Fedora
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.

Comment 16 Lutz Wolf 2017-08-03 15:44:29 UTC
The issue persists in F25. It is a security issue IMO, as the failure detect secure boot status causes the kernel to disable a number of security checks, as outlined by Adam Williamson above. It's a pretty nasty as it can go undetected quite easily. I only noticed it because I accidentally loaded an unsigned kernel module -- which normally shouldn't have been possible.

Building and installing the F26 package on F25 remediates the issue. Could someone please change the "Version" field of this bug to 25 to prevent the EOL process from reaping this report? Or should I file a new report?

Comment 18 Fedora End Of Life 2017-11-16 19:03:12 UTC
This message is a reminder that Fedora 25 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 25. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '25'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version'
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not
able to fix it before Fedora 25 is end of life. If you would still like
to see this bug fixed and are able to reproduce it against a later version
of Fedora, you are encouraged  change the 'version' to a later Fedora
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.

Comment 19 Fedora End Of Life 2017-12-12 10:33:57 UTC
Fedora 25 changed to end-of-life (EOL) status on 2017-12-12. Fedora 25 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.


Note You need to log in before you can comment on or make changes to this bug.