Threading of patch series (was: [PATCH v6 00/14] error: Do compile-time format string checking on grub>)
Dear Glenn, Am 06.03.21 um 00:15 schrieb Glenn Washburn: On Fri, 5 Mar 2021 17:27:01 +0100 Daniel Kiper wrote: […] By the way, my I ask you once again to send each patch series as separate thread. Now you are attaching all patch sets to one cover letter which is confusing. Please stop doing that. How do you pull patch series from the mailing list? I'm curious if this is messing with that. Also what email client do you use? You are correct that I'm attaching all new versions of the patch series to one cover letter, but each patch series also has its own cover letter. So I don't consider the cover letter that is the root of the thread to be the cover letter for the new patch series. I can't find our prior correspondence. I recall saying that the patchset series in question had been not done in a less than ideal way because I had start by replying to the cover letter of the prior patchset and then switched to replying to a particular patchset cover letter. This was because with experience I determined that attaching to the thread of the next version of a patchset to the cover letter of the first version of the patchset looked much nicer in my browser. I also recall saying that I'd do this in the future to see if it worked well doing it properly from the start. My goal is to keep all versions of a patchset together in a client with tree view of threads (eg. my mail client claws-mail). This makes it easy to go back to a prior patchset to look at comments. I also wanted to meet this goal in an aesthetically pleasing way. The first attempt which attached a new version of a patchset to its priors cover letter did not meet this because it created a deep tree for patchsets with lots of revisions. However, attaching each new patchset to the cover letter of the first patchset, keeps thread tree to a minimum. It also makes it to collapse only certain patchset versions (aside from the first). Also, since I have threads sorted by thread date, I see patchsets ordered from most recent to least recent, which it how I like to order all my emails. The current case does not strictly adhere to these rules because I'm taking v4 as the initial patchset, which perhaps may be the source of some confusion. Other than that I think its worked out nicely. So I'm curious if this is causing some issue with tooling. And I'm curious what exactly is causing confusion? In my browser its fairly obvious that the root of the thread is the cover letter for v4 of the patch series and that the cover letters of attached patch series are different, noted by a different version number. At least here, Mozilla Thunderbird 78.8.0 is only able to collapse the top thread and not sub threads. The mailing list archive does not seem to care [1], though that might be because the v4 patch set cover letter is in a different month. Anyway, as *displaying* of threading is not defined and different between user agents, maybe it’s better to not rely on that. Kind regards, Paul [1]: https://lists.gnu.org/archive/html/grub-devel/2021-03/threads.html ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] templates: Properly disable the os-prober by default
- disable os-prober by default in grub-mkconfig.in by setting
GRUB_DISABLE_OS_PROBER to true
- fixes logic in 30_os-prober.in
Reason for code shuffle in grub-mkconfig.in:
The default was GRUB_DISABLE_OS_PROBER=false if you don't set
GRUB_DISABLE_OS_PROBER at all. To prevent os-prober from starting
we have to set it by default to true and shuffle GRUB_DISABLE_OS_PROBER
to executed by the script code section, but give the option to the user to
overwrite it with false, if he wants to execute os-prober after all.
Everyone who added GRUB_DISABLE_OS_PROBER=true in grub.cfg can remove
it by now.
Fixes: e3464147 templates: Disable the os-prober by default
---
util/grub-mkconfig.in | 5 -
util/grub.d/30_os-prober.in | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
index d3e879b8e..f8cbb8d7a 100644
--- a/util/grub-mkconfig.in
+++ b/util/grub-mkconfig.in
@@ -140,6 +140,9 @@ GRUB_DEVICE_PARTUUID="`${grub_probe} --device
${GRUB_DEVICE} --target=partuuid 2
GRUB_DEVICE_BOOT="`${grub_probe} --target=device /boot`"
GRUB_DEVICE_BOOT_UUID="`${grub_probe} --device ${GRUB_DEVICE_BOOT}
--target=fs_uuid 2> /dev/null`" || true
+# Disable os-prober by default due to security reasons.
+GRUB_DISABLE_OS_PROBER="true"
+
# Filesystem for the device containing our userland. Used for stuff like
# choosing Hurd filesystem module.
GRUB_FS="`${grub_probe} --device ${GRUB_DEVICE} --target=fs 2> /dev/null ||
echo unknown`"
@@ -201,6 +204,7 @@ export GRUB_DEVICE \
GRUB_DEVICE_PARTUUID \
GRUB_DEVICE_BOOT \
GRUB_DEVICE_BOOT_UUID \
+ GRUB_DISABLE_OS_PROBER \
GRUB_FS \
GRUB_FONT \
GRUB_PRELOAD_MODULES \
@@ -242,7 +246,6 @@ export GRUB_DEFAULT \
GRUB_BACKGROUND \
GRUB_THEME \
GRUB_GFXPAYLOAD_LINUX \
- GRUB_DISABLE_OS_PROBER \
GRUB_INIT_TUNE \
GRUB_SAVEDEFAULT \
GRUB_ENABLE_CRYPTODISK \
diff --git a/util/grub.d/30_os-prober.in b/util/grub.d/30_os-prober.in
index 80685b15f..a258ce71d 100644
--- a/util/grub.d/30_os-prober.in
+++ b/util/grub.d/30_os-prober.in
@@ -26,7 +26,7 @@ export TEXTDOMAINDIR="@localedir@"
. "$pkgdatadir/grub-mkconfig_lib"
-if [ "x${GRUB_DISABLE_OS_PROBER}" = "xfalse" ]; then
+if [ "x${GRUB_DISABLE_OS_PROBER}" != "xfalse" ]; then
gettext_printf "os-prober will not be executed to detect other bootable
partitions.\nSystems on them will not be added to the GRUB boot
configuration.\nCheck GRUB_DISABLE_OS_PROBER documentation entry.\n"
exit 0
fi
--
2.30.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v6 00/14] error: Do compile-time format string checking on grub>
On Fri, 5 Mar 2021 17:27:01 +0100 Daniel Kiper wrote: > On Thu, Mar 04, 2021 at 06:22:31PM -0600, Glenn Washburn wrote: > > Daniel, you mentioned wanting a separate patch series which is the > > real fix for patch #12. I've added it to this patch series, since > > they go together. I can send the single patch as a separate thread > > if that still desirable. > > For all Reviewed-by: Daniel Kiper > > I will take all stuff up to #13. The #14 will be applied after 2.06 > release. Great! > Thank you for fixing these issues. > > By the way, my I ask you once again to send each patch series as > separate thread. Now you are attaching all patch sets to one cover > letter which is confusing. Please stop doing that. How do you pull patch series from the mailing list? I'm curious if this is messing with that. Also what email client do you use? You are correct that I'm attaching all new versions of the patch series to one cover letter, but each patch series also has its own cover letter. So I don't consider the cover letter that is the root of the thread to be the cover letter for the new patch series. I can't find our prior correspondence. I recall saying that the patchset series in question had been not done in a less than ideal way because I had start by replying to the cover letter of the prior patchset and then switched to replying to a particular patchset cover letter. This was because with experience I determined that attaching to the thread of the next version of a patchset to the cover letter of the first version of the patchset looked much nicer in my browser. I also recall saying that I'd do this in the future to see if it worked well doing it properly from the start. My goal is to keep all versions of a patchset together in a client with tree view of threads (eg. my mail client claws-mail). This makes it easy to go back to a prior patchset to look at comments. I also wanted to meet this goal in an aesthetically pleasing way. The first attempt which attached a new version of a patchset to its priors cover letter did not meet this because it created a deep tree for patchsets with lots of revisions. However, attaching each new patchset to the cover letter of the first patchset, keeps thread tree to a minimum. It also makes it to collapse only certain patchset versions (aside from the first). Also, since I have threads sorted by thread date, I see patchsets ordered from most recent to least recent, which it how I like to order all my emails. The current case does not strictly adhere to these rules because I'm taking v4 as the initial patchset, which perhaps may be the source of some confusion. Other than that I think its worked out nicely. So I'm curious if this is causing some issue with tooling. And I'm curious what exactly is causing confusion? In my browser its fairly obvious that the root of the thread is the cover letter for v4 of the patch series and that the cover letters of attached patch series are different, noted by a different version number. Glenn > Daniel > > > Changes since v5 > > * Fix formatting issues with spaces around format string macros > > and casts > > * Add extra patch #14 which is the better fix for #12, but needs > > more testing > > > > Glenn > > > > Glenn Washburn (14): > > misc: Format string for grub_error should be a literal > > error: grub_error missing format string argument > > error: grub_error format string add missing format code > > dmraid_nvidia: Format string error in grub_error > > grub_error: Use format code PRIuGRUB_SIZE for variables of type > > grub_size_t > > pgp: Format code for grub_error is incorrect > > efi: Format string error in grub_error > > error: Use PRI* macros to get correct format string code across > > architectures > > error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument > > in grub_error > > error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in > > grub_error error: Use format code PRIuGRUB_UINT64_T for 64-bit > > typed fileblock in grub_error > > error: Use format code llu for 64-bit uint bp->blk_prop in > > grub_error error: Do compile-time format string checking on > > grub_error zfs: Use grub_uint64_t instead of 1ULL in BF64_*CODE > > macros ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] templates: Properly disable the os-prober by default
Le 05/03/2021 à 22:15, Philip Müller a écrit :
- disable os-prober by default in grub-mkconfig.in by setting
GRUB_DISABLE_OS_PROBER to true
- fixes logic in 30_os-prober.in
Reason for code shuffle in grub-mkconfig.in:
The default was GRUB_DISABLE_OS_PROBER=false if you don't set
GRUB_DISABLE_OS_PROBER at all. To prevent os-prober from starting
we have to set it by default to true and shuffle GRUB_DISABLE_OS_PROBER
to executed by the script code section, but give the option to the user to
overwrite it with false, if he wants to execute os-prober after all.
Everyone who added GRUB_DISABLE_OS_PROBER=true in grub.cfg can remove
it by now.
Fixes: e3464147 templates: Disable the os-prober by default
---
util/grub-mkconfig.in | 5 -
util/grub.d/30_os-prober.in | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
index d3e879b8e..f8cbb8d7a 100644
--- a/util/grub-mkconfig.in
+++ b/util/grub-mkconfig.in
@@ -140,6 +140,9 @@ GRUB_DEVICE_PARTUUID="`${grub_probe} --device
${GRUB_DEVICE} --target=partuuid 2
GRUB_DEVICE_BOOT="`${grub_probe} --target=device /boot`"
GRUB_DEVICE_BOOT_UUID="`${grub_probe} --device ${GRUB_DEVICE_BOOT} --target=fs_uuid
2> /dev/null`" || true
+# Disable os-prober by default due to security reasons.
+GRUB_DISABLE_OS_PROBER="true"
+
# Filesystem for the device containing our userland. Used for stuff like
# choosing Hurd filesystem module.
GRUB_FS="`${grub_probe} --device ${GRUB_DEVICE} --target=fs 2> /dev/null || echo
unknown`"
@@ -201,6 +204,7 @@ export GRUB_DEVICE \
GRUB_DEVICE_PARTUUID \
GRUB_DEVICE_BOOT \
GRUB_DEVICE_BOOT_UUID \
+ GRUB_DISABLE_OS_PROBER \
GRUB_FS \
GRUB_FONT \
GRUB_PRELOAD_MODULES \
@@ -242,7 +246,6 @@ export GRUB_DEFAULT \
GRUB_BACKGROUND \
GRUB_THEME \
GRUB_GFXPAYLOAD_LINUX \
- GRUB_DISABLE_OS_PROBER \
GRUB_INIT_TUNE \
GRUB_SAVEDEFAULT \
GRUB_ENABLE_CRYPTODISK \
diff --git a/util/grub.d/30_os-prober.in b/util/grub.d/30_os-prober.in
index 80685b15f..a258ce71d 100644
--- a/util/grub.d/30_os-prober.in
+++ b/util/grub.d/30_os-prober.in
@@ -26,7 +26,7 @@ export TEXTDOMAINDIR="@localedir@"
. "$pkgdatadir/grub-mkconfig_lib"
-if [ "x${GRUB_DISABLE_OS_PROBER}" = "xfalse" ]; then
+if [ "x${GRUB_DISABLE_OS_PROBER}" != "xfalse" ]; then
gettext_printf "os-prober will not be executed to detect other bootable
partitions.\nSystems on them will not be added to the GRUB boot configuration.\nCheck
GRUB_DISABLE_OS_PROBER documentation entry.\n"
exit 0
fi
Just tested against git master, works as expected.
Thanks!
Best regards,
Didier
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v6 00/14] error: Do compile-time format string checking on grub>
On Thu, Mar 04, 2021 at 06:22:31PM -0600, Glenn Washburn wrote: > Daniel, you mentioned wanting a separate patch series which is the real fix > for patch #12. I've added it to this patch series, since they go together. > I can send the single patch as a separate thread if that still desirable. For all Reviewed-by: Daniel Kiper I will take all stuff up to #13. The #14 will be applied after 2.06 release. Thank you for fixing these issues. By the way, my I ask you once again to send each patch series as separate thread. Now you are attaching all patch sets to one cover letter which is confusing. Please stop doing that. Daniel > Changes since v5 > * Fix formatting issues with spaces around format string macros and casts > * Add extra patch #14 which is the better fix for #12, but needs more testing > > Glenn > > Glenn Washburn (14): > misc: Format string for grub_error should be a literal > error: grub_error missing format string argument > error: grub_error format string add missing format code > dmraid_nvidia: Format string error in grub_error > grub_error: Use format code PRIuGRUB_SIZE for variables of type > grub_size_t > pgp: Format code for grub_error is incorrect > efi: Format string error in grub_error > error: Use PRI* macros to get correct format string code across > architectures > error: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in > grub_error > error: Use format code PRIxGRUB_UINT64_T for 64-bit arg in grub_error > error: Use format code PRIuGRUB_UINT64_T for 64-bit typed fileblock in > grub_error > error: Use format code llu for 64-bit uint bp->blk_prop in grub_error > error: Do compile-time format string checking on grub_error > zfs: Use grub_uint64_t instead of 1ULL in BF64_*CODE macros ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] style: Format string macro should have a space between quotes
On Thu, Mar 04, 2021 at 04:46:50PM -0600, Glenn Washburn wrote: > On Thu, 4 Mar 2021 20:03:34 +0100 > Daniel Kiper wrote: > > > Does this patch fix all such issues in the GRUB source code? > > > > Daniel > > I don't know and its unclear the scope of your question. However, this > is the grep I used to find these instances: > > grep -rnE '\"PRI|PRI[a-zA-Z_0-9]+\"' It looks that it finds all occurrences. So, Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Add chainloaded image as shim's verifiable object
On Fri, Mar 5, 2021 at 1:34 PM Michael Chang wrote: > > On Fri, Mar 05, 2021 at 12:21:49PM +, Dimitri John Ledkov wrote: > > This is not an oversight but intentional. > > > > Currently there is no chainloader support with SBAT as further > > development is required to ensure policy is applied correctly. Once > > SBAT support for chainloading is defined, it will be introduced. > > Hm. What I heard is that shim wouldn't enfore SBAT validation for the > "indirect" image loaded by grub for now. So we should still able to > boot, eg, kernel and other efi image loaded by grub which is still in > lacking of SBAT support. > There is a difference between chainloading a new bootloader, or booting a linux kernel. When chainloading a different grub that is verified by the vendor-db in shim, sbat should be enforced, if the current grub has sbat. When chainloading windows bootmgr, which is signed by things in db, sbat need not be enforced. It is a delicate balance to strike, as otherwise one can do sbat bypass via downgrading to older grub via chainload. I would recommend to add windowsbootmgr efi entry, and then perform `exit 1` such that this current shim+grub exit, and firmware goes to load the next entry for windows. For `exit 1` support you need https://github.com/rhboot/grub2/commit/ccce3d69ae3eacc7bdc70217304586bd7e74fe1e too. I understand that this is very suboptimal. I kind of wish grub2 could generate and display EFI boot entries, and then let people select them which would set it as BootNext and reset. Cause then one would truly be able to jump to a specific boot menu entry from grub menu. I also need chainloader support elsewhere, thus at the moment we are not shipping 2.06rc1 and instead are shipping 2.04 with backports of patches. > > And yes it is intended to continue to allow "boot windows" as the menu > > entry in grub. > > So what is recommended solution in the interim ? We just can't afford > to release new grub version that cannot do the chainload ... > > Thanks, > Michael > > > > > On Fri, Mar 5, 2021 at 12:12 PM Michael Chang via Grub-devel > > wrote: > > > > > > While attempting to dual boot Microsoft Windows with efi chainloader, it > > > failed with below error when secure boot was enabled. > > > > > > error ../../grub-core/kern/verifiers.c:119:verification requested but > > > nobody cares: /EFI/Microsoft/Boot/bootmgfw.efi. > > > > > > It is a regression, as previously it worked without problem. > > > > > > It turns out chainloading image has been locked down introduced by > > > > > > 578c95298 kern: Add lockdown support > > > > > > However we should consider it as verifiable object to shim to allow > > > booting in secure boot enabled mode. The chainloaded image could also > > > have trusted signature signed by vendor with their pubkey cert in db. > > > For that matters it's usage should not be locked down in secure boot, > > > and instead use shim to validate it's signature before running it. > > > > > > Signed-off-by: Michael Chang > > > --- > > > grub-core/kern/efi/sb.c | 1 + > > > grub-core/kern/lockdown.c | 1 - > > > 2 files changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/grub-core/kern/efi/sb.c b/grub-core/kern/efi/sb.c > > > index 41dadcd14..96d237722 100644 > > > --- a/grub-core/kern/efi/sb.c > > > +++ b/grub-core/kern/efi/sb.c > > > @@ -129,6 +129,7 @@ shim_lock_verifier_init (grub_file_t io __attribute__ > > > ((unused)), > > > case GRUB_FILE_TYPE_BSD_KERNEL: > > > case GRUB_FILE_TYPE_XNU_KERNEL: > > > case GRUB_FILE_TYPE_PLAN9_KERNEL: > > > +case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: > > >*flags = GRUB_VERIFY_FLAGS_SINGLE_CHUNK; > > > > > >/* Fall through. */ > > > diff --git a/grub-core/kern/lockdown.c b/grub-core/kern/lockdown.c > > > index 0bc70fd42..e1fd1c1e2 100644 > > > --- a/grub-core/kern/lockdown.c > > > +++ b/grub-core/kern/lockdown.c > > > @@ -48,7 +48,6 @@ lockdown_verifier_init (grub_file_t io __attribute__ > > > ((unused)), > > > case GRUB_FILE_TYPE_PXECHAINLOADER: > > > case GRUB_FILE_TYPE_PCCHAINLOADER: > > > case GRUB_FILE_TYPE_COREBOOT_CHAINLOADER: > > > -case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: > > > case GRUB_FILE_TYPE_ACPI_TABLE: > > > case GRUB_FILE_TYPE_DEVICE_TREE_IMAGE: > > >*flags = GRUB_VERIFY_FLAGS_DEFER_AUTH; > > > -- > > > 2.26.2 > > > > > > > > > ___ > > > Grub-devel mailing list > > > Grub-devel@gnu.org > > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > > > > > > -- > > Regards, > > > > Dimitri. > > > -- Regards, Dimitri. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Add chainloaded image as shim's verifiable object
This is not an oversight but intentional. Currently there is no chainloader support with SBAT as further development is required to ensure policy is applied correctly. Once SBAT support for chainloading is defined, it will be introduced. And yes it is intended to continue to allow "boot windows" as the menu entry in grub. On Fri, Mar 5, 2021 at 12:12 PM Michael Chang via Grub-devel wrote: > > While attempting to dual boot Microsoft Windows with efi chainloader, it > failed with below error when secure boot was enabled. > > error ../../grub-core/kern/verifiers.c:119:verification requested but > nobody cares: /EFI/Microsoft/Boot/bootmgfw.efi. > > It is a regression, as previously it worked without problem. > > It turns out chainloading image has been locked down introduced by > > 578c95298 kern: Add lockdown support > > However we should consider it as verifiable object to shim to allow > booting in secure boot enabled mode. The chainloaded image could also > have trusted signature signed by vendor with their pubkey cert in db. > For that matters it's usage should not be locked down in secure boot, > and instead use shim to validate it's signature before running it. > > Signed-off-by: Michael Chang > --- > grub-core/kern/efi/sb.c | 1 + > grub-core/kern/lockdown.c | 1 - > 2 files changed, 1 insertion(+), 1 deletion(-) > > diff --git a/grub-core/kern/efi/sb.c b/grub-core/kern/efi/sb.c > index 41dadcd14..96d237722 100644 > --- a/grub-core/kern/efi/sb.c > +++ b/grub-core/kern/efi/sb.c > @@ -129,6 +129,7 @@ shim_lock_verifier_init (grub_file_t io __attribute__ > ((unused)), > case GRUB_FILE_TYPE_BSD_KERNEL: > case GRUB_FILE_TYPE_XNU_KERNEL: > case GRUB_FILE_TYPE_PLAN9_KERNEL: > +case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: >*flags = GRUB_VERIFY_FLAGS_SINGLE_CHUNK; > >/* Fall through. */ > diff --git a/grub-core/kern/lockdown.c b/grub-core/kern/lockdown.c > index 0bc70fd42..e1fd1c1e2 100644 > --- a/grub-core/kern/lockdown.c > +++ b/grub-core/kern/lockdown.c > @@ -48,7 +48,6 @@ lockdown_verifier_init (grub_file_t io __attribute__ > ((unused)), > case GRUB_FILE_TYPE_PXECHAINLOADER: > case GRUB_FILE_TYPE_PCCHAINLOADER: > case GRUB_FILE_TYPE_COREBOOT_CHAINLOADER: > -case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: > case GRUB_FILE_TYPE_ACPI_TABLE: > case GRUB_FILE_TYPE_DEVICE_TREE_IMAGE: >*flags = GRUB_VERIFY_FLAGS_DEFER_AUTH; > -- > 2.26.2 > > > ___ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel -- Regards, Dimitri. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2] Add chainloaded image as shim's verifiable object
While attempting to dual boot Microsoft Windows with efi chainloader, it failed with below error when secure boot was enabled. error ../../grub-core/kern/verifiers.c:119:verification requested but nobody cares: /EFI/Microsoft/Boot/bootmgfw.efi. It is a regression, as previously it worked without problem. It turns out chainloading image has been locked down introduced by 578c95298 kern: Add lockdown support However we should consider it as verifiable object to shim to allow booting in secure boot enabled mode. The chainloaded image could also have trusted signature signed by vendor with their pubkey cert in db. For that matters it's usage should not be locked down in secure boot, and instead use shim to validate it's signature before running it. V2: Keep GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE in the lockdown list as it ensures at least one verifer has validated the image. Signed-off-by: Michael Chang --- grub-core/kern/efi/sb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/grub-core/kern/efi/sb.c b/grub-core/kern/efi/sb.c index 41dadcd14..96d237722 100644 --- a/grub-core/kern/efi/sb.c +++ b/grub-core/kern/efi/sb.c @@ -129,6 +129,7 @@ shim_lock_verifier_init (grub_file_t io __attribute__ ((unused)), case GRUB_FILE_TYPE_BSD_KERNEL: case GRUB_FILE_TYPE_XNU_KERNEL: case GRUB_FILE_TYPE_PLAN9_KERNEL: +case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: *flags = GRUB_VERIFY_FLAGS_SINGLE_CHUNK; /* Fall through. */ -- 2.26.2 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Add chainloaded image as shim's verifiable object
On Fri, Mar 05, 2021 at 12:21:49PM +, Dimitri John Ledkov wrote: > This is not an oversight but intentional. > > Currently there is no chainloader support with SBAT as further > development is required to ensure policy is applied correctly. Once > SBAT support for chainloading is defined, it will be introduced. Hm. What I heard is that shim wouldn't enfore SBAT validation for the "indirect" image loaded by grub for now. So we should still able to boot, eg, kernel and other efi image loaded by grub which is still in lacking of SBAT support. > And yes it is intended to continue to allow "boot windows" as the menu > entry in grub. So what is recommended solution in the interim ? We just can't afford to release new grub version that cannot do the chainload ... Thanks, Michael > > On Fri, Mar 5, 2021 at 12:12 PM Michael Chang via Grub-devel > wrote: > > > > While attempting to dual boot Microsoft Windows with efi chainloader, it > > failed with below error when secure boot was enabled. > > > > error ../../grub-core/kern/verifiers.c:119:verification requested but > > nobody cares: /EFI/Microsoft/Boot/bootmgfw.efi. > > > > It is a regression, as previously it worked without problem. > > > > It turns out chainloading image has been locked down introduced by > > > > 578c95298 kern: Add lockdown support > > > > However we should consider it as verifiable object to shim to allow > > booting in secure boot enabled mode. The chainloaded image could also > > have trusted signature signed by vendor with their pubkey cert in db. > > For that matters it's usage should not be locked down in secure boot, > > and instead use shim to validate it's signature before running it. > > > > Signed-off-by: Michael Chang > > --- > > grub-core/kern/efi/sb.c | 1 + > > grub-core/kern/lockdown.c | 1 - > > 2 files changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/grub-core/kern/efi/sb.c b/grub-core/kern/efi/sb.c > > index 41dadcd14..96d237722 100644 > > --- a/grub-core/kern/efi/sb.c > > +++ b/grub-core/kern/efi/sb.c > > @@ -129,6 +129,7 @@ shim_lock_verifier_init (grub_file_t io __attribute__ > > ((unused)), > > case GRUB_FILE_TYPE_BSD_KERNEL: > > case GRUB_FILE_TYPE_XNU_KERNEL: > > case GRUB_FILE_TYPE_PLAN9_KERNEL: > > +case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: > >*flags = GRUB_VERIFY_FLAGS_SINGLE_CHUNK; > > > >/* Fall through. */ > > diff --git a/grub-core/kern/lockdown.c b/grub-core/kern/lockdown.c > > index 0bc70fd42..e1fd1c1e2 100644 > > --- a/grub-core/kern/lockdown.c > > +++ b/grub-core/kern/lockdown.c > > @@ -48,7 +48,6 @@ lockdown_verifier_init (grub_file_t io __attribute__ > > ((unused)), > > case GRUB_FILE_TYPE_PXECHAINLOADER: > > case GRUB_FILE_TYPE_PCCHAINLOADER: > > case GRUB_FILE_TYPE_COREBOOT_CHAINLOADER: > > -case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: > > case GRUB_FILE_TYPE_ACPI_TABLE: > > case GRUB_FILE_TYPE_DEVICE_TREE_IMAGE: > >*flags = GRUB_VERIFY_FLAGS_DEFER_AUTH; > > -- > > 2.26.2 > > > > > > ___ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > > -- > Regards, > > Dimitri. > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Add chainloaded image as shim's verifiable object
On Fri, Mar 05, 2021 at 01:32:57PM +0100, Thomas Frauendorfer wrote: > On Fri, Mar 5, 2021 at 1:12 PM Michael Chang via Grub-devel > wrote: > > > > While attempting to dual boot Microsoft Windows with efi chainloader, it > > failed with below error when secure boot was enabled. > > > > error ../../grub-core/kern/verifiers.c:119:verification requested but > > nobody cares: /EFI/Microsoft/Boot/bootmgfw.efi. > > > > It is a regression, as previously it worked without problem. > > > > It turns out chainloading image has been locked down introduced by > > > > 578c95298 kern: Add lockdown support > > > > However we should consider it as verifiable object to shim to allow > > booting in secure boot enabled mode. The chainloaded image could also > > have trusted signature signed by vendor with their pubkey cert in db. > > For that matters it's usage should not be locked down in secure boot, > > and instead use shim to validate it's signature before running it. > > > > Signed-off-by: Michael Chang > > [cut out] > > >/* Fall through. */ > > diff --git a/grub-core/kern/lockdown.c b/grub-core/kern/lockdown.c > > index 0bc70fd42..e1fd1c1e2 100644 > > --- a/grub-core/kern/lockdown.c > > +++ b/grub-core/kern/lockdown.c > > @@ -48,7 +48,6 @@ lockdown_verifier_init (grub_file_t io __attribute__ > > ((unused)), > > case GRUB_FILE_TYPE_PXECHAINLOADER: > > case GRUB_FILE_TYPE_PCCHAINLOADER: > > case GRUB_FILE_TYPE_COREBOOT_CHAINLOADER: > > -case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: > > case GRUB_FILE_TYPE_ACPI_TABLE: > > case GRUB_FILE_TYPE_DEVICE_TREE_IMAGE: > >*flags = GRUB_VERIFY_FLAGS_DEFER_AUTH; > > -- > > 2.26.2 > > The lockdown verifier makes sure that at least one verifer has > validated the image. > So removing GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE from it is a very bad idea. Indeed. I will send second patch for not removing it from lockdown. Thanks a lot for your review. Michael > > > ___ > > Grub-devel mailing list > > Grub-devel@gnu.orgthe > > https://lists.gnu.org/mailman/listinfo/grub-devel > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Add chainloaded image as shim's verifiable object
On Fri, Mar 5, 2021 at 1:12 PM Michael Chang via Grub-devel wrote: > > While attempting to dual boot Microsoft Windows with efi chainloader, it > failed with below error when secure boot was enabled. > > error ../../grub-core/kern/verifiers.c:119:verification requested but > nobody cares: /EFI/Microsoft/Boot/bootmgfw.efi. > > It is a regression, as previously it worked without problem. > > It turns out chainloading image has been locked down introduced by > > 578c95298 kern: Add lockdown support > > However we should consider it as verifiable object to shim to allow > booting in secure boot enabled mode. The chainloaded image could also > have trusted signature signed by vendor with their pubkey cert in db. > For that matters it's usage should not be locked down in secure boot, > and instead use shim to validate it's signature before running it. > > Signed-off-by: Michael Chang [cut out] >/* Fall through. */ > diff --git a/grub-core/kern/lockdown.c b/grub-core/kern/lockdown.c > index 0bc70fd42..e1fd1c1e2 100644 > --- a/grub-core/kern/lockdown.c > +++ b/grub-core/kern/lockdown.c > @@ -48,7 +48,6 @@ lockdown_verifier_init (grub_file_t io __attribute__ > ((unused)), > case GRUB_FILE_TYPE_PXECHAINLOADER: > case GRUB_FILE_TYPE_PCCHAINLOADER: > case GRUB_FILE_TYPE_COREBOOT_CHAINLOADER: > -case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: > case GRUB_FILE_TYPE_ACPI_TABLE: > case GRUB_FILE_TYPE_DEVICE_TREE_IMAGE: >*flags = GRUB_VERIFY_FLAGS_DEFER_AUTH; > -- > 2.26.2 The lockdown verifier makes sure that at least one verifer has validated the image. So removing GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE from it is a very bad idea. > ___ > Grub-devel mailing list > Grub-devel@gnu.orgthe > https://lists.gnu.org/mailman/listinfo/grub-devel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] Add chainloaded image as shim's verifiable object
While attempting to dual boot Microsoft Windows with efi chainloader, it failed with below error when secure boot was enabled. error ../../grub-core/kern/verifiers.c:119:verification requested but nobody cares: /EFI/Microsoft/Boot/bootmgfw.efi. It is a regression, as previously it worked without problem. It turns out chainloading image has been locked down introduced by 578c95298 kern: Add lockdown support However we should consider it as verifiable object to shim to allow booting in secure boot enabled mode. The chainloaded image could also have trusted signature signed by vendor with their pubkey cert in db. For that matters it's usage should not be locked down in secure boot, and instead use shim to validate it's signature before running it. Signed-off-by: Michael Chang --- grub-core/kern/efi/sb.c | 1 + grub-core/kern/lockdown.c | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/grub-core/kern/efi/sb.c b/grub-core/kern/efi/sb.c index 41dadcd14..96d237722 100644 --- a/grub-core/kern/efi/sb.c +++ b/grub-core/kern/efi/sb.c @@ -129,6 +129,7 @@ shim_lock_verifier_init (grub_file_t io __attribute__ ((unused)), case GRUB_FILE_TYPE_BSD_KERNEL: case GRUB_FILE_TYPE_XNU_KERNEL: case GRUB_FILE_TYPE_PLAN9_KERNEL: +case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: *flags = GRUB_VERIFY_FLAGS_SINGLE_CHUNK; /* Fall through. */ diff --git a/grub-core/kern/lockdown.c b/grub-core/kern/lockdown.c index 0bc70fd42..e1fd1c1e2 100644 --- a/grub-core/kern/lockdown.c +++ b/grub-core/kern/lockdown.c @@ -48,7 +48,6 @@ lockdown_verifier_init (grub_file_t io __attribute__ ((unused)), case GRUB_FILE_TYPE_PXECHAINLOADER: case GRUB_FILE_TYPE_PCCHAINLOADER: case GRUB_FILE_TYPE_COREBOOT_CHAINLOADER: -case GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE: case GRUB_FILE_TYPE_ACPI_TABLE: case GRUB_FILE_TYPE_DEVICE_TREE_IMAGE: *flags = GRUB_VERIFY_FLAGS_DEFER_AUTH; -- 2.26.2 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
