在 2019年12月17日 12:57, Bhupesh Sharma 写道:
Hi Lianbo,
Thanks for the patch. I have several nitpicks/comments, please see below:
Can you please also capture the change history, i.e. what changed from
v1 -> v2 -> v3 of the patch.
Sure. Thanks for your comment.
On Tue, Dec 17, 2019 at 7:50 AM Lianbo Jiang
<lijiang(a)redhat.com> wrote:
>
> UEFI Secure boot is a signature verification mechanism, is designed to
> prevent malicious code being loaded and executed at the early boot stage.
> This makes sure that code executed is trusted by firmware.
>
> Previously, with kexec_file_load() interface, kernel prevents unsigned
> kernel image from being loaded if secure boot is enabled. So kdump will
> detect whether secure boot is enabled firstly, then decide which interface
> is chosen to execute, kexec_load() or kexec_file_load(). Otherwise unsigned
> kernel loading will fail if secure boot enabled, and kexec_file_load() is
> entered.
>
> Now, the implementation of kexec_file_load() is adjusted in below commit.
> With this change, if CONFIG_KEXEC_SIG_FORCE is not set, unsigned kernel
> still has chance to be allowed to load under some conditions.
s/has chance/has a chance
Tip: If you use vim to draft your git log messages you can use ':set
spell' to make sure vim prompts you for automatic spell check. For the
same add the following to your .gitconfig:
[core]
editor = "vim -c 'setlocal spell'"
Thank you for telling me about that. I enabled the automatic spell check but I
really ignore it, so sorry for this.
> commit 99d5cadfde2b ("kexec_file: split KEXEC_VERIFY_SIG
into KEXEC_SIG
> and KEXEC_SIG_FORCE")
>
> And in the current fedora and RHEL, CONFIG_KEXEC_SIG_FORCE is not set, only
> CONFIG_KEXEC_SIG and CONFIG_BZIMAGE_VERIFY_SIG are set on x86_64 by default.
Since this patch is for Fedora, we can drop RHEL from the commit log.
OK. Looks better.
> It's time to spread kexec_file_load() onto all systems of
x86_64, including
> Secute-boot platforms and legacy platforms.
s/Secute/Secure
Yes. Thank you for pointing out this.
> Please refer to the following
> form.
>
> .----------------------------------------------------------------------.
> | . | signed kernel | unsigned kernel |
> | . types |-----------------------|-----------------------|
> | . |Secure boot| Legacy |Secure boot| Legacy |
> | . |-----------|-----------|-----------|-----------|
> | options . | prev| now | prev| now | | | prev| now |
> | . |(file|(file|(only|(file| prev| now |(only|(file|
> | . |load)|load)|load)|load)| | |load)|load)|
> |----------------------|-----|-----|-----|-----|-----|-----|-----|-----|
> |KEXEC_SIG=y | | | | | | | | |
> |SIG_FORCE is not set |succ |succ |succ |succ | X | X |succ |succ |
> |BZIMAGE_VERIFY_SIG=y | | | | | | | | |
> |----------------------|-----|-----|-----|-----|-----|-----|-----|-----|
> |KEXEC_SIG=y | | | | | | | | |
> |SIG_FORCE is not set | | | | | | | | |
> |BZIMAGE_VERIFY_SIG is |fail |fail |succ |fail | X | X |succ |fail |
> |not set | | | | | | | | |
> |----------------------|-----|-----|-----|-----|-----|-----|-----|-----|
> |KEXEC_SIG=y | | | | | | | | |
> |SIG_FORCE=y |succ |succ |succ |fail | X | X |succ |fail |
> |BZIMAGE_VERIFY_SIG=y | | | | | | | | |
> |----------------------|-----|-----|-----|-----|-----|-----|-----|-----|
> |KEXEC_SIG=y | | | | | | | | |
> |SIG_FORCE=y | | | | | | | | |
> |BZIMAGE_VERIFY_SIG is |fail |fail |succ |fail | X | X |succ |fail |
> |not set | | | | | | | | |
> |----------------------|-----|-----|-----|-----|-----|-----|-----|-----|
> |KEXEC_SIG is not set | | | | | | | | |
> |SIG_FORCE is not set | | | | | | | | |
> |BZIMAGE_VERIFY_SIG is |fail |fail |succ |succ | X | X |succ |succ |
> |not set | | | | | | | | |
> ----------------------------------------------------------------------
I am not sure if its my email client rendering but the above table
appears mangled-up to me.
It should be an email client issue. Or can you try to have a look via the
Zimbra mail?
http://mail.corp.redhat.com/
So, I am not sure everyone reviewinf this can make out all the
use-cases mentioned in the above table.
> Note:
> [1] The 'X' indicates that the 1st kernel(unsigned) can not boot when the
> Secure boot is enabled.
>
> Hence, in this patch, if on x86,
s/x86/x86_64
Exactly.
> lets use the kexec_file_load() only. See if
> anything wrong happened in this case, in Fedora firstly for the time being.
This makes sense. However, do we have a fallback in place, i.e. if the
kexec_file_load() failed (for some reason), should we fall back to
If the kexec_file_load() failed in the default options case, it should be a bug.
Personally think that no need to fall back to kexec_load().
kexec_load() by default and inform the user or display an error
message and ask the user to try kexec_load() instead.
This should be better. I made a draft patch as follow. What's your opinion?
diff --git a/kdumpctl b/kdumpctl
index d1c987773cf9..4e8d8dab49a0 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -675,6 +675,7 @@ check_rebuild()
# as the currently running kernel.
load_kdump()
{
+ local kexec_args=""
KEXEC_ARGS=$(prepare_kexec_args "${KEXEC_ARGS}")
KDUMP_COMMANDLINE=$(prepare_cmdline "${KDUMP_COMMANDLINE}"
"${KDUMP_COMMANDLINE_REMOVE}" "${KDUMP_COMMANDLINE_APPEND}")
@@ -692,6 +693,10 @@ load_kdump()
return 0
else
echo "kexec: failed to load kdump kernel" >&2
+ kexec_args=`echo $KEXEC_ARGS |grep "\-s"`
+ if [ -n "$kexec_args" ]; then
+ echo "kexec_file_load() failed, please try kexec_load()"
>&2
+ fi
return 1
fi
}
Thanks.
Lianbo
Thanks,
Bhupesh
> Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com>
> ---
> dracut-early-kdump.sh | 4 ++--
> kdump-lib.sh | 31 +++++++++++--------------------
> kdumpctl | 10 ++++------
> 3 files changed, 17 insertions(+), 28 deletions(-)
>
> diff --git a/dracut-early-kdump.sh b/dracut-early-kdump.sh
> index 69a34eb996cd..bfb923d6efa1 100755
> --- a/dracut-early-kdump.sh
> +++ b/dracut-early-kdump.sh
> @@ -43,8 +43,8 @@ early_kdump_load()
>
> EARLY_KEXEC_ARGS=$(prepare_kexec_args "${KEXEC_ARGS}")
>
> - if is_secure_boot_enforced; then
> - echo "Secure Boot is enabled. Using kexec file based syscall."
> + if use_kexec_file_load; then
> + echo "Using kexec file based syscall."
> EARLY_KEXEC_ARGS="$EARLY_KEXEC_ARGS -s"
> fi
>
> diff --git a/kdump-lib.sh b/kdump-lib.sh
> index f393c76b9cbb..5b6684992553 100755
> --- a/kdump-lib.sh
> +++ b/kdump-lib.sh
> @@ -589,30 +589,21 @@ need_64bit_headers()
> print (strtonum("0x" r[2]) > strtonum("0xffffffff"));
}'`
> }
>
> -# Check if secure boot is being enforced.
> #
> -# Per Peter Jones, we need check efivar SecureBoot-$(the UUID) and
> -# SetupMode-$(the UUID), they are both 5 bytes binary data. The first four
> -# bytes are the attributes associated with the variable and can safely be
> -# ignored, the last bytes are one-byte true-or-false variables. If SecureBoot
> -# is 1 and SetupMode is 0, then secure boot is being enforced.
> +# Currently, Secure Boot is only used on x86_64 and kernel enabled the
> +# kexec_file_load() on x86_64 by default.
> +# In addition, kernel also enables the option KEXEC_SIG, which makes the
> +# kexec_file_load() syscall checks for a valid signature of the kernel
> +# image if there is a signature(It must be valid). And the images can
> +# still be loaded without a valid signature. So let kexec-tools change
> +# as well.
> #
> -# Assume efivars is mounted at /sys/firmware/efi/efivars.
> -is_secure_boot_enforced()
> +use_kexec_file_load()
> {
> - local secure_boot_file setup_mode_file
> - local secure_boot_byte setup_mode_byte
> + local arch_name=`uname -m`
>
> - secure_boot_file=$(find /sys/firmware/efi/efivars -name SecureBoot-*
2>/dev/null)
> - setup_mode_file=$(find /sys/firmware/efi/efivars -name SetupMode-*
2>/dev/null)
> -
> - if [ -f "$secure_boot_file" ] && [ -f
"$setup_mode_file" ]; then
> - secure_boot_byte=$(hexdump -v -e '/1 "%d\ "'
$secure_boot_file|cut -d' ' -f 5)
> - setup_mode_byte=$(hexdump -v -e '/1 "%d\ "'
$setup_mode_file|cut -d' ' -f 5)
> -
> - if [ "$secure_boot_byte" = "1" ] && [
"$setup_mode_byte" = "0" ]; then
> - return 0
> - fi
> + if [ "$arch_name" == "x86_64" ]; then
> + return 0
> fi
>
> return 1
> diff --git a/kdumpctl b/kdumpctl
> index 2d21a416deb1..d1c987773cf9 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -678,11 +678,9 @@ load_kdump()
> KEXEC_ARGS=$(prepare_kexec_args "${KEXEC_ARGS}")
> KDUMP_COMMANDLINE=$(prepare_cmdline "${KDUMP_COMMANDLINE}"
"${KDUMP_COMMANDLINE_REMOVE}" "${KDUMP_COMMANDLINE_APPEND}")
>
> - # For secureboot enabled machines, use new kexec file based syscall.
> - # Old syscall will always fail as it does not have capability to
> - # to kernel signature verification.
> - if is_secure_boot_enforced; then
> - echo "Secure Boot is enabled. Using kexec file based
syscall."
> + # On x86_64 machine, lets use kexec file based syscall by default.
> + if use_kexec_file_load; then
> + echo "Using kexec file based syscall."
> KEXEC_ARGS="$KEXEC_ARGS -s"
> fi
>
> @@ -1162,7 +1160,7 @@ stop_fadump()
>
> stop_kdump()
> {
> - if is_secure_boot_enforced; then
> + if use_kexec_file_load; then
> $KEXEC -s -p -u
> else
> $KEXEC -p -u
> --
> 2.17.1
> _______________________________________________
> kexec mailing list -- kexec(a)lists.fedoraproject.org
> To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org
> Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives:
https://lists.fedoraproject.org/archives/list/kexec@lists.fedoraproject.org