[PATCH 1/2] kdumpctl: claim that kdump does not support secure boot when service start

[Dave Young]

Kdump does not support secure boot yet, so let's claim it is not supported
at the begginning of service start function.

In this patch for checking secure boot status I'm checking the efivars per
suggestion from pjones. see in code comments for the details.

Tested in Fedora 19 + qemu ovmf with secure boot enabled.

Signed-off-by: Dave Young 
---
 kdumpctl |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

--- kexec-tools.orig/kdumpctl
+++ kexec-tools/kdumpctl
@@ -500,8 +500,46 @@ selinux_relabel()
    done
 }

+# 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.
+#
+# Assume efivars is mounted at /sys/firmware/efi/efivars.
+function is_secure_boot_enforced()
+{
+	local secure_boot_file setup_mode_file
+	local secure_boot_byte setup_mode_byte
+
+	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
+	fi
+
+	return 1
+}
+
+function check_kdump_feasibility()
+{
+	if is_secure_boot_enforced; then
+		return 1;
+	fi
+}
+
 function start()
 {
+	local rc
+
    check_config
    if [ $? -ne 0 ]; then
    	echo "Starting kdump: [FAILED]"
@@ -517,6 +555,13 @@ function start()
    	return 1
    fi

+	check_kdump_feasibility
+	rc=$?
+	if [ $rc == 1 ]; then
+		echo "Secure boot is not supported in kdump yet. Please disable secure boot and retry. [WARNING]"
+		return 1
+	fi
+
    status 
    rc=$?
    if [ $rc == 2 ]; then

[Vivek Goyal]

On Tue, Feb 11, 2014 at 03:05:08PM +0800, Dave Young wrote:

[..]
> function start()
>  {
> local rc
> check_config
>  if [ $? -ne 0 ]; then
>   echo "Starting kdump: [FAILED]"
>
> @@ -517,6 +555,13 @@ function start()
>    	return 1
>    fi
> check_kdump_feasibility
> rc=$?
> if [ $rc == 1 ]; then
> echo "Secure boot is not supported in kdump yet. Please disable secure boot and retry. [WARNING]"

- Let us use the string "Secure Boot" whereever we are referring to it.

- Remove "Please".

- This is FAILED message and not a WARNING message?

May be use following.

"Secure Boot is Enabled. Kdump service can't be started. Disable Secure
Boot and retry. [FAILED]"

Thinking more about it. How about we output two messages. First message
is displayed inside the function where actual check happened. That message
will be very specific and say what's wrong. Second check will be in top
level function and that will display the generic message. Something like
as follows.

check_kdump_feasibility()
{
    if (secure_boot_enabled) {
    	"Secure Boot is Enabled. Kdump service can't be started. Disable Secure Boot and retry"
    	return 1;
    }
}

start() {
  ....
  ....

check_kdump_feasibility
  rc = $?
  if [ $rc -ne 0 ];then
    echo "Kdump is not supported on this kernel: [WARNING]"
  fi

That way we don't have to differentiate between return code 1 and 2.
Top level function says that we can't run kdump on this kernel. And
message inside the function gives more details what's exactly wrong with
this kernel.

Thanks
Vivek

[Dave Young]

On 02/11/14 at 08:32am, Vivek Goyal wrote:
> On Tue, Feb 11, 2014 at 03:05:08PM +0800, Dave Young wrote:
>
> [..]
> > function start()
> >  {
> > local rc
> > check_config
> >  if [ $? -ne 0 ]; then
> >   echo "Starting kdump: [FAILED]"
> >
> > @@ -517,6 +555,13 @@ function start()
> >    	return 1
> >    fi
> > check_kdump_feasibility
> > rc=$?
> > if [ $rc == 1 ]; then
> > echo "Secure boot is not supported in kdump yet. Please disable secure boot and retry. [WARNING]"
> Let us use the string "Secure Boot" whereever we are referring to it.

Ok
> Remove "Please".

Will do
> This is FAILED message and not a WARNING message?

Agree. Actually except the "Kdump is already running, all other cases which return non zero should be FAILED"
> May be use following.
>
> "Secure Boot is Enabled. Kdump service can't be started. Disable Secure
> Boot and retry. [FAILED]"
>
> Thinking more about it. How about we output two messages. First message
> is displayed inside the function where actual check happened. That message
> will be very specific and say what's wrong. Second check will be in top
> level function and that will display the generic message. Something like
> as follows.
>
> check_kdump_feasibility()
> {
>    if (secure_boot_enabled) {
>    	"Secure Boot is Enabled. Kdump service can't be started. Disable Secure Boot and retry"
>    	return 1;
>    }
> }
>
> start() {
>   ....
>   ....
>
> check_kdump_feasibility
>   rc = $?
>   if [ $rc -ne 0 ];then
>    echo "Kdump is not supported on this kernel: [WARNING]"
>   fi
>
> That way we don't have to differentiate between return code 1 and 2.
> Top level function says that we can't run kdump on this kernel. And
> message inside the function gives more details what's exactly wrong with
> this kernel.

I haved tried this but changed to current patch and feel it's cleanner
to put them together.

But I'm also fine with this way.

Thanks
Dave

[Dave Young]

> check_kdump_feasibility
>   rc = $?
>   if [ $rc -ne 0 ];then
>    echo "Kdump is not supported on this kernel: [WARNING]"
>   fi

I prefer to do not say "Kdump is not supported on this kernel" here.
And instead print the detail failure reason in check_kdump_feasibility
function as we agreed for Secure boot checking.

Here in start function if check_kdump_feasibility return non zero I think
it's better to just say "echo "Starting kdump: [FAILED]" as other chunk.

What do you think?

I will send out a new version, if it's not good please just comment there.

Thanks
Dave

[Vivek Goyal]

On Wed, Feb 12, 2014 at 09:54:26AM +0800, Dave Young wrote:
> > check_kdump_feasibility
> >   rc = $?
> >   if [ $rc -ne 0 ];then
> >    echo "Kdump is not supported on this kernel: [WARNING]"
> >   fi
>
> I prefer to do not say "Kdump is not supported on this kernel" here.
> And instead print the detail failure reason in check_kdump_feasibility
> function as we agreed for Secure boot checking.
>
> Here in start function if check_kdump_feasibility return non zero I think
> it's better to just say "echo "Starting kdump: [FAILED]" as other chunk.
>
> What do you think?

Sounds reasonable.

Thanks
Vivek