Don't allow use of runtime args unless built-in includes marker

As discussed, in both secure and insecure modes, we want to enforce
that use of runtime args is only enabled *if* kernel.efi includes
a builtin string including the stubby marker

- Update truth-table in kcmdline.c
- Print out message when runtime args are rejected due to missing
  marker (test harness needs this)
- extend test/harness program to print out pretty table of test
  results, optionally pass -C flag to print out the test configuration.
- verbosified the tests.yaml to be explicit about what values are
  being used.
- Updated workflow to use threads=NR_CPUS

Signed-off-by: Ryan Harper <[email protected]>

.github/workflows/build.yml

@@ -22,7 +22,8 @@ jobs:
           sudo apt-get update --quiet
           sudo apt-get install --quiet --assume-yes --no-install-recommends gnu-efi build-essential
           sudo apt-get install --quiet --assume-yes --no-install-recommends \
-              dosfstools mtools ovmf python3-minimal qemu-system-x86 qemu-utils shim-signed swtpm
+              dosfstools mtools ovmf python3-minimal python3-prettytable \
+              qemu-system-x86 qemu-utils shim-signed swtpm
       - name: Collect Inputs
         run: |
           mkdir ./test-inputs/
@@ -51,7 +52,8 @@ jobs:
             KVM=false ./test/harness run "--boot-mode=$m" \
               --inputs=./test-inputs "--results=./test-results/$m" \
               --sbat=sbat.csv --stubby=stubby.efi \
-              --num-threads=4 --timeout=120 \
+              --num-threads=$(grep -c processor /proc/cpuinfo) \
+              --timeout=120 \
               test/tests.yaml || fails="$fails $m"
           done
 

kcmdline.c

@@ -130,14 +130,14 @@ EFI_STATUS check_cmdline(CONST CHAR8 *cmdline, UINTN cmdline_len, CHAR16 *errmsg
 // (as returned by strlen).
 // return values:
 //   EFI_SUCCESS:
-//     - builtin command line is valid AND
-//       ( insecureboot || (secureboot and allowed runtime) )
+//    - builtin command line is valid AND
+//    ( insecureboot || (secureboot and allowed runtime) )
 //   EFI_OUT_OF_RESOURCES: AllocatePool failed.
 //   EFI_INVALID_PARAMETER:
 //     - builtin cmdline is invalid
-//     - runtime parameters given to non-empty builtin without marker.
+//	 - runtime parameters given to non-empty builtin without marker.
 //   EFI_SECURITY_VIOLATION:
-//     - secureboot and runtime is not allowed
+//	 - secureboot and runtime is not allowed
 //
 EFI_STATUS get_cmdline(
 		BOOLEAN secure,
@@ -189,13 +189,16 @@ EFI_STATUS get_cmdline(
 	 * |------|---------|---------|--------------------|----------|--------|
 	 * |  a   |  True   | False   | False              | insecure | ok     |
 	 * |  b   |  True   | False   | True               | insecure | ok     |
-	 * |  c   |  True   | True    | False              | insecure | ok     |
+	 * |  c   |  True   | True    | False              | insecure | fail   |
 	 * |  d   |  True   | True    | True               | insecure | ok     |
 	 * |--------------- ---------------------------------------------------|
 	 * |  e   |  True   | False   | False              |   secure | ok     |
 	 * |  f   |  True   | False   | True               |   secure | ok     |
 	 * |  g   |  True   | True    | False              |   secure | fail   |
 	 * |  h   |  True   | True    | True               |   secure | ok     |
+	 * |--------------- ---------------------------------------------------|
+	 * |  j   |  False  | True    | False              | insecure | fail   |
+	 * |  k   |  False  | True    | False              |   secure | fail   |
 	 * `-------------------------------------------------------------------'
 	 */
 
@@ -205,25 +208,15 @@ EFI_STATUS get_cmdline(
 		if (p == NULL) {
 			// cases: a, c, e, g
 			if (runtime_len != 0) {
-				// cases: c, g
-				if (secure) {
-					// cases: g
-					status = EFI_INVALID_PARAMETER;
-					UnicodeSPrint(errbuf, errbuf_buflen,
-							L"runtime arguments cannot be given to non-empty builtin without marker");
-					goto out;
-				}
-				// cases: c handled in common path below
+				// cases: c, g -- cannot use runtime without a built-in marker
+				status = EFI_INVALID_PARAMETER;
+				UnicodeSPrint(errbuf, errbuf_buflen,
+					L"runtime arguments cannot be given to non-empty builtin without marker");
+				goto out;
 			}
-			// cases: a, c, e
+			// cases: a, e
 			CopyMem(part1, builtin, builtin_len);
 			part1_len = builtin_len;
-			// make space for joining builtin and runtime if we have both
-			if (runtime_len != 0) {
-				part1_len = builtin_len + 1;
-				*(part1 + part1_len - 1) = ' ';
-				*(part1 + part1_len) = '\0';
-			}
 		} else {
 			// cases: b, d, f, h
 			// builtin has a marker, check that there is only one.
@@ -248,6 +241,14 @@ EFI_STATUS get_cmdline(
 			CopyMem(part2, p + marker_len, part2_len);
 			*(part2 + part2_len) = '\0';
 		}
+	} else {
+		// cases: j, k
+		if (runtime_len != 0) {
+			status = EFI_INVALID_PARAMETER;
+			UnicodeSPrint(errbuf, errbuf_buflen,
+				L"runtime arguments cannot be given to non-empty builtin without marker");
+			goto out;
+		}
 	}
 
 	// namespace appeared in the builtin (other than marker)
@@ -384,6 +385,9 @@ EFI_STATUS get_cmdline_with_print(
 			err = EFI_SUCCESS;
 		}
 	}
+    if (err == EFI_INVALID_PARAMETER) {
+        Print(L"Custom kernel command line rejected\n");
+    }
 
 out:
 	if (errmsg != NULL) {

test-get-cmdline.c

@@ -28,18 +28,18 @@ TestData tests[] = {
 		"",
 		"root=atomix console=ttyS0 verbose"},
 	// no builtin, use runtime.
-	{EFI_SUCCESS, true,
+	{EFI_INVALID_PARAMETER, true,
 		"",
 		"root=atomix verbose",
-		"root=atomix verbose"},
+		ResultNotChecked},
 	// no builtin no runtime means empty, do not boot
 	{EFI_SECURITY_VIOLATION, true,
 		"",
 		"",
-		""},
-	// insecure, no builtin, bad runtime token allows runtime
+		ResultNotChecked},
+	// insecure, built-in marker, bad runtime arg, inscure boots
 	{EFI_SECURITY_VIOLATION, false,
-		"",
+		"STUBBY_RT_CLI1",
 		"root=atomix verbose rootkit=yes",
 		"root=atomix verbose rootkit=yes"},
 	// all good secure marker at beginning
@@ -77,11 +77,11 @@ TestData tests[] = {
 		"root=atomix",
 		"console=ttyS0",
 		ResultNotChecked},
-	// no marker in insecure - just append.
-	{EFI_SUCCESS, false,
+	// no marker in insecure - runtime args requires marker even insecure
+	{EFI_INVALID_PARAMETER, false,
 		"root=atomix",
 		"console=ttyS0",
-		"root=atomix console=ttyS0"},
+		""},
 	// namespace for marker found twice in builtin secure
 	{EFI_INVALID_PARAMETER, true,
 		"root=atomix STUBBY_RT debug STUBBY_RT_CLI1 ",

test/harness

@@ -5,6 +5,7 @@ import functools
 import logging
 import multiprocessing
 import os.path
+import pdb
 import queue
 import re
 import shlex
@@ -16,8 +17,11 @@ import threading
 import tempfile
 import textwrap
 import time
+import traceback
 import yaml
 
+import prettytable
+
 
 MODE_NVRAM = 'nvram'
 MODE_UEFI_SHELL = 'efi-shell'
@@ -915,10 +919,17 @@ class TestError(Exception):
 
 
 class Validator:
-    def __init__(self, results_d):
-        self.results_d = results_d
-        self.testinfo = load_test_data(path_join(results_d, "tests.yaml"))
+    def __init__(self, args):
+        self.args = args
+        self.results_d = args.results
+        self.testinfo = load_test_data(path_join(self.results_d, "tests.yaml"))
         self._tests = None
+        self._table = prettytable.PrettyTable()
+        columns = ["Name", "Passed", "Errors"]
+        if self.args.show_config_column:
+            columns.append("Config")
+        self._table.field_names = columns
+        self._table.align = "l"
 
     def tests(self):
         tests = {}
@@ -945,12 +956,7 @@ class Validator:
             except TestError as e:
                 errors[ast] = e
 
-        if len(errors) == 0:
-            print(f"{name}: PASS")
-        else:
-            print("%s: FAIL (%s)" % (name, ",".join(errors.keys())))
-
-        return errors
+        return len(errors) == 0, errors
 
     def validate_all(self):
         errors = {}
@@ -959,16 +965,26 @@ class Validator:
             if not os.path.isdir(trdir):
                 print(f"no results for test {name}")
                 continue
-            found = self.validate_test(tdata)
-            if found:
-                errors[name] = found
+            result, errs = self.validate_test(tdata)
+            if errs:
+                errors[name] = errs
+
+            row = [name, result, ",".join(errs.keys())]
+            if self.args.show_config_column:
+                row.append(tdata)
+            self._table.add_row(row)
 
         return errors
 
+    def print_results(self):
+        print("== Results ==")
+        print(self._table)
+
 
-def validate_results(results_d):
-    vdator = Validator(results_d)
+def validate_results(cliargs):
+    vdator = Validator(cliargs)
     errors = vdator.validate_all()
+    vdator.print_results()
     if not errors:
         return 0
 
@@ -991,14 +1007,14 @@ def main_run(cliargs):
     finally:
         runner.cleanup()
 
-    errors = validate_results(cliargs.results)
+    errors = validate_results(cliargs)
     if errors:
         return 1
     return 0
 
 
 def main_validate(args):
-    validate_results(args.results)
+    validate_results(args)
 
 
 def main():
@@ -1014,6 +1030,9 @@ def main():
     s.add_argument(
         "-r", "--results", action="store", default="./results",
         help="Write results output to dir")
+    s.add_argument(
+        "-C", "--show-config-column", action="store_true", default=False,
+        help="Display the 'Config' column in validate results")
     s.add_argument("testdata", help="The yaml formated test definition file")
     s.set_defaults(handler=main_run)
 
@@ -1035,4 +1054,11 @@ def main():
 
 
 if __name__ == "__main__":
-    sys.exit(main())
+    ret = 0
+    try:
+        ret = main()
+    except Exception:
+        traceback.print_exc()
+        pdb.post_mortem()
+        ret = 1
+    sys.exit(ret)