Skip to content

Commit

Permalink
Merge pull request #44 from raharper/feat/add-test-for-builtin-no-rt-…
Browse files Browse the repository at this point in the history 
…no-marker

Add testcase for shim builtin cmdline with no runtime and no marker
  • Loading branch information
@raharper
raharper authored Mar 5, 2024
2 parents 0e93f8f + 3839268 commit ee55074
Show file tree
Hide file tree
Showing 6 changed files with 280 additions and 78 deletions.
6 changes: 4 additions & 2 deletions .github/workflows/build.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/
Expand Down Expand Up @@ -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
Expand Down
136 changes: 108 additions & 28 deletions kcmdline.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ EFI_STATUS check_cmdline(CONST CHAR8 *cmdline, UINTN cmdline_len, CHAR16 *errmsg
CHAR8 c = '\0';
CHAR8 *buf = NULL;
CHAR8 *tokens[MAX_TOKENS];
EFI_STATUS status = EFI_SUCCESS;
EFI_STATUS status = EFI_SECURITY_VIOLATION;
int i;
int start = -1;
int num_toks = 0;
Expand All @@ -57,6 +57,13 @@ EFI_STATUS check_cmdline(CONST CHAR8 *cmdline, UINTN cmdline_len, CHAR16 *errmsg

*errmsg = '\0';

if (cmdline_len == 0) {
UnicodeSPrint(errmsg, errmsg_len,
L"Empty commandline not allowed: length=0");
status = EFI_SECURITY_VIOLATION;
goto out;
}

CopyMem(buf, cmdline, cmdline_len);
buf[cmdline_len] = '\0';

Expand Down Expand Up @@ -94,13 +101,22 @@ EFI_STATUS check_cmdline(CONST CHAR8 *cmdline, UINTN cmdline_len, CHAR16 *errmsg
num_toks++;
}

// do not allow an empty command line
if (num_toks == 0) {
UnicodeSPrint(errmsg, errmsg_len,
L"Empty commandline not allowed: no tokens found");
status = EFI_SECURITY_VIOLATION;
goto out;
}
for (i=0; i < num_toks; i++) {
if (!is_allowed(tokens[i])) {
UnicodeSPrint(errmsg, errmsg_len, L"token not allowed: %a", tokens[i]);
status = EFI_SECURITY_VIOLATION;
goto out;
}
}

status = EFI_SUCCESS;
out:

if (buf != NULL) {
Expand All @@ -114,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,
Expand Down Expand Up @@ -167,25 +183,42 @@ EFI_STATUS get_cmdline(
part1_len = 0;
part2_len = 0;

/*
* .-------------------------------------------------------------------.
* | case | builtin | runtime | builtin_has_marker | sb mode | status |
* |------|---------|---------|--------------------|----------|--------|
* | a | True | False | False | insecure | ok |
* | b | True | False | True | 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 |
* `-------------------------------------------------------------------'
*/

if (builtin_len != 0) {
// cases: a, b, c, d, e, f, g, h
p = strstra(builtin, marker);
if (p == NULL) {
// there was no marker in builtin cmdline
if (secure) {
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;
}
} else {
// insecure and no marker. act as if marker was at end.
CopyMem(part1, builtin, builtin_len);
part1_len = builtin_len + 1;
*(part1 + part1_len - 1) = ' ';
*(part1 + part1_len) = '\0';
// cases: a, c, e, g
if (runtime_len != 0) {
// 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, e
CopyMem(part1, builtin, builtin_len);
part1_len = builtin_len;
} else {
// cases: b, d, f, h
// builtin has a marker, check that there is only one.
if (strstra(p + marker_len, marker) != NULL) {
status = EFI_INVALID_PARAMETER;
Expand All @@ -208,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)
Expand All @@ -224,31 +265,67 @@ EFI_STATUS get_cmdline(
goto out;
}

// Print(L"part1=%a|\npart2=%a|\nruntime=%a|\n", part1, part2, runtime);
status = check_cmdline(runtime, runtime_len, errbuf, errbuf_buflen);
/* Print(L"builtin=%a len=%d|part1=%a len=%d|\npart2=%a len=%d|\nruntime=%a len=%d|\n",
builtin, builtin_len, part1, part1_len, part2, part2_len, runtime, runtime_len); */
if (runtime_len > 0) {
status = check_cmdline(runtime, runtime_len, errbuf, errbuf_buflen);

// EFI_SECURITY_VIOLATION is allowed if insecure boot, so continue on.
if (EFI_ERROR(status)) {
if (status != EFI_SECURITY_VIOLATION || secure) {
goto out;
// EFI_SECURITY_VIOLATION is allowed if insecure boot, so continue on.
if (EFI_ERROR(status)) {
if (status != EFI_SECURITY_VIOLATION || secure) {
goto out;
}
}
} else {
// if we have don't have a runtime component, then we need to check
// "builtin" cmdline, but builtin may have the marker, but won't show
// up in the two parts that will be joined later
if (part1_len > 0) {
status = check_cmdline(part1, part1_len, errbuf, errbuf_buflen);

// EFI_SECURITY_VIOLATION is allowed if insecure boot, so continue on.
if (EFI_ERROR(status)) {
if (status != EFI_SECURITY_VIOLATION || secure) {
goto out;
}
}
}

if (part2_len > 0) {
status = check_cmdline(part2, part2_len, errbuf, errbuf_buflen);

// EFI_SECURITY_VIOLATION is allowed if insecure boot, so continue on.
if (EFI_ERROR(status)) {
if (status != EFI_SECURITY_VIOLATION || secure) {
goto out;
}
}
}
}

// At this point, part1 and part2 are set so we can just concatenate part1, runtime, part3
// At this point, part1 and part2 are set so we can just concatenate part1, runtime, part2
UINTN clen = part1_len + runtime_len + part2_len;
CHAR8 *cbuf;
cbuf = AllocatePool(clen + 1);
if (cbuf == NULL) {
status = EFI_OUT_OF_RESOURCES;
goto out;
}
//Print(L"copying part1 len=%d into cbuf len=%d\n", part1_len, clen);
CopyMem(cbuf, part1, part1_len);
CopyMem(cbuf+part1_len, runtime, runtime_len);
CopyMem(cbuf+part1_len+runtime_len, part2, part2_len);

if (runtime_len > 0) {
CopyMem(cbuf+part1_len, runtime, runtime_len);
if (part2_len > 0) {
CopyMem(cbuf+part1_len+runtime_len, part2, part2_len);
}
}
cbuf[clen] = '\0';
*cmdline = cbuf;
*cmdline_len = clen;

//Print(L"finalized cmdline=%a len=%d\n", cbuf, clen);
//FIXME: should we check_cmdline on the final composition?
out:
if (errbuf != NULL && errbuf[0] == '\0') {
FreePool(errbuf);
Expand Down Expand Up @@ -308,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) {
Expand Down
4 changes: 2 additions & 2 deletions test-cmdline.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ typedef struct {

TestData tests[] = {
{EFI_SUCCESS, "console=ttyS0"},
{EFI_SUCCESS, ""},
{EFI_SUCCESS, " "},
{EFI_SUCCESS, "quiet"},
{EFI_SUCCESS, " root=atomix console=ttyS0"},
{EFI_SUCCESS, " root=atomix console=/dev/ttyS0 "},
{EFI_SUCCESS, "root=atomix console=/dev/ttyS0"},
{EFI_SUCCESS, "crashkernel=256M"},
{EFI_SUCCESS, "crashkernel=128M"},
{EFI_SECURITY_VIOLATION, ""},
{EFI_SECURITY_VIOLATION, " "},
{EFI_SECURITY_VIOLATION, "crashkernel.off=128M"},
{EFI_SECURITY_VIOLATION, "root=atomix init=/bin/bash debug"},
{EFI_SECURITY_VIOLATION, "init=/bin/bash"},
Expand Down
41 changes: 23 additions & 18 deletions test-get-cmdline.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,24 @@ TestData tests[] = {
"root=atomix STUBBY_RT_CLI1 more",
"console=ttyS0",
"root=atomix console=ttyS0 more"},
// no builtin, use runtime.
// builtin, not runtime, no token
{EFI_SUCCESS, true,
"root=atomix console=ttyS0 verbose",
"",
"root=atomix console=ttyS0 verbose"},
// no builtin, use runtime.
{EFI_INVALID_PARAMETER, true,
"",
"root=atomix verbose",
"root=atomix verbose"},
// no builtin no runtime means empty.
{EFI_SUCCESS, true,
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
Expand Down Expand Up @@ -62,7 +67,7 @@ TestData tests[] = {
"root=atomix STUBBY_RT_CLI1 more2",
"console=ttyS0 more1",
ResultNotChecked},
// marker not a full token
// marker not a full token
{EFI_INVALID_PARAMETER, false,
"root=atomix STUBBY_RT_CLI1=abcd more",
"console=ttyS0",
Expand All @@ -72,30 +77,30 @@ 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 ",
"console=ttyS0",
ResultNotChecked},
ResultNotChecked},
// namespace for marker found twice in builtin insecure
{EFI_INVALID_PARAMETER, false,
"root=atomix STUBBY_RT debug STUBBY_RT_CLI1 ",
"console=ttyS0",
ResultNotChecked},
ResultNotChecked},
// namespace appears in runtime
{EFI_INVALID_PARAMETER, false,
"root=atomix debug STUBBY_RT_CLI1",
"console=ttyS0 STUBBY_RT",
ResultNotChecked},
ResultNotChecked},
};


BOOLEAN do_get_cmdline(TestData td) {
BOOLEAN do_get_cmdline(int testnum, TestData td) {
EFI_STATUS status;
CHAR16 *errmsg;
CHAR8 *found;
Expand All @@ -115,7 +120,7 @@ BOOLEAN do_get_cmdline(TestData td) {
StatusToString(status_exp, td.expstatus);

if (status != td.expstatus) {
Print(L"expected status '%ls' found '%ls'\n", status_found, status_exp);
Print(L"test %d: expected status '%ls' found '%ls'\n", testnum, status_found, status_exp);
Print(L"errmsg = %ls\n", errmsg);
return false;
}
Expand Down Expand Up @@ -148,11 +153,11 @@ int main()
int passes = 0, fails = 0;

for (int i=0; i<num; i++) {
if (do_get_cmdline(tests[i])) {
if (do_get_cmdline(i, tests[i])) {
passes++;
} else {
fails++;
Print(L"test %d FAIL\n", i);
Print(L"test %d: FAIL\n", i);
}
}

Expand Down
Loading

0 comments on commit ee55074

Please sign in to comment.