v1.0.0 #57
Merged
v1.0.0 #57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The host secure boot variable backend code was reorganised. The edk2-compat code was moved to the host directory, and the -m option in secvarctl was added to allow for the selection of the secure boot variable's mode. By default, we generating host backend static library and linked it with secvarctl binary. Also, provided the option to generating and linking of host backend dynamic library Ex: make DYNAMIC_LIB=1 Signed-off-by: Sudhakar Kuppusamy <[email protected]>
In secvarctl, incorporated the generation, read, validation, write, and verify methods for the guest secure boot variables, which use the libstb-secvar backend. it is supporting 9 pre-defined secure boot variables and an arbitrary variables. secure boot variables: 1. PK 2. KEK 3. db 4. dbx 5. grubdb 6. grubdbx 7. sbat 8. moduledb 9. trustedcadb Signed-off-by: Sudhakar Kuppusamy <[email protected]>
unit test cases for generate, read, validate, verify and write Signed-off-by: Sudhakar Kuppusamy <[email protected]>
This explains what is secvarctl and data format, and supported variables introduced. Signed-off-by: Sudhakar Kuppusamy <[email protected]>
A) Using Cmake we can compile different backends and secvarctl using following command
1) Secvarctl compilation with both guest and host backends
- By Default it is building for OPENSSL && Static library
- mkdir build && cd build && cmake ../ . && cmake --build .
- For building Shared Library
- mkdir build && cd build && cmake -DDYNAMIC_LIB=1 ../ . && cmake --build .
- For installing libraries and binaries
-mkdir build && cd build && cmake ../ . && cmake --build . && make install_lib
- For uninstalling libraries and binaries
-mkdir build && cd build && cmake ../ . && cmake --build . && make uninstall
- For Cleanning build
-mkdir build && cd build && cmake ../ . && cmake --build . && make clean_build
2) Secvarctl compilation with Host backend
- By Default it is building for OPENSSL && Static library
-mkdir build && cd build && cmake -DGUEST_BACKEND=0 ../ . && cmake --build .
- For building Shared Library
-mkdir build && cd build && cmake -DGUEST_BACKEND=0 -DDYNAMIC_LIB=1 ../ . && cmake --build .
3) Secvarctl compilation with Guest backend
- By Default it is building for OPENSSL && Static library
-mkdir build && cd build && cmake -DHOST_BACKEND=0 ../ . && cmake --build .
- For building Shared Library
-mkdir build && cd build && cmake -DHOST_BACKEND=0 -DDYNAMIC_LIB=1 ../ . && cmake --build .
4) Separate Host and guest can be build
-For building Host backend
- cd backends/host && mkdir build && cd build && cmake ../ . && cmake --build .
-For building Guest
- cd backends/guest && mkdir build && cd build && cmake ../ . && cmake --build .
-For cleanning build
-After building if want to do cleanup use command "make clean_build"
B) Added secvarctl-cov changes for testing
C) Secvarctl man page installation to man
Signed-off-by: Shubham Pandey <[email protected]>
Added all the required option for building secvarctl using Cmake file. Signed-off-by: Shubham Pandey <[email protected]>
Some headers in libstb-secvar were moved to external/, and so the include paths changed from the initial version secvarctl was developed against. This adds the root of libstb-secvar as an include path, as the include paths in libstb-secvar were changed to be relative to the root.
…a bit Signed-off-by: Eric Richter <[email protected]>
…larger rework Signed-off-by: Eric Richter <[email protected]>
There are some unnecessary splits between guest and host, mostly in the external dependencies. This commit removes the separation between host and guest in the external/ dir, since host and guest will be sharing some of those dependencies anyway. The src/ directory has been removed, since it only contains two files. Those files now live in the root. NOTE: This will not compile on its own, the subsequent commit reworks the entire Makefile-based build system. Signed-off-by: Eric Richter <[email protected]>
Employ a similar strategy as used in libstb-secvar: - use wildcard rules as much as possible - put all build objects in an easy-to-clean directory - don't build backends as libraries Signed-off-by: Eric Richter <[email protected]>
All build artifacts should be in the obj/ directory, with the exception of external/libstb-secvar which has its own ignore rules. Any build objects (*.o, *.d, etc) that still show up in `git status` must then be a build system bug, or a remnant from an older version. Signed-off-by: Eric Richter <[email protected]>
Use Makefile targets for selecting guest/host tests. These can eventually be dynamically called based on top-level support for guest or host secure boot. Also fixes the default path for the secvarctl binary -- eventually all test files should use a standard way of determining what binary to use. Signed-off-by: Eric Richter <[email protected]>
pointer + value will never be NULL. I think the purpose was to make sure that the buffer might be at least of the right size, but checking if an offset to a pointer being NULL is not the way to do that. Remove the check to clear the warning. Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
In order to fix a pair of uninitialized variable warnings from current_esl_size, the function has be reorganized to avoid a giant if/else expression, and set consistent outputs for the pass-by-reference arguments when exiting early. Since current variables supplied with -c should take precedence over variables from a path, the "else" condition for-loop has been moved to the top of the function. If a matching variable is not located in the current variable list, it falls through to checking for the variable in the variable path. Furthermore, all early returns have been replaced with goto out/fails where appropriate, so that the pass-by-reference variables can be returned consistently, which was the source of the uninitialized variable warning. Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
… new bin/ directory Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
A previous rework commit either removed the inclusion of dependency files, or never existed in the first place. Re-add that so changes to headers can properly trigger a rebuild. Also remove the useless `coverage` build target, which current is just a phony alias for `bin/secvarctl-cov`. This will return later when there is actual coverage testing implemented. Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
To make the format target easier, we can hook into the $(SRCS) variable before we add on the external source files, so we definitely format all files that we would normally compile. Headers are located via find -- a reasonable stopgap for now, but should be improved in the future somehow. Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
Pointed out by cppcheck. out_buffer, a pointer to a pointer, is dereferenced and assigned to without first checking validity. Then, the base, top-level pointer value is checked for NULL, after it has already been dereferenced. This commit fixes the two problems by first checking that the out_buffer parameter actually points somewhere, then correcting the NULL check to confirm that the call to malloc actually succeeds. Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
There can only be one PK, therefore the PK cannot be appended to. However, the current error message is not clear, and references the older method of setting the flag. Furthermore, this error is handled in the same way as an invalid argument parse which leads to the whole usage information being printed, which makes finding the error message difficult. This commit rewords the error message to be a little more clear, and moves the special case check for PK appends outside of the rest of the argument validation logic, thus avoiding the extra usage output. Signed-off-by: Eric Richter <[email protected]>
…d of uint8_t Signed-off-by: Eric Richter <[email protected]>
The codebase interchangably uses char*, unsigned char*, uint8_t*, and other types for buffers, leading to a lot of excessive casts when trying to call one function from another. This commit attempts to reduce the amount of needless casts by unifying the "buffer" or "data" type to uint8_t*, leaving char* for strings. Not all cases have been covered, there are some oddballs left over (particularly in the host/skiboot code), that might need a little more work. Overall, this commit shouldn't change any functional code -- only the types in the function signatures. libstb-secvar and the crypto abstractions may need to be evaluated separately. Signed-off-by: Eric Richter <[email protected]>
…f char* Many of the timestamp-related functions in the host edk2 backend take in both a single timestamp and a reference to the contents of the TS variable, which is really an array of timestamps. This is likely why the the type of that array was left as char*, so it wouldn't be ambiguous with the single `struct efi_time*` input parameters. This commit changes the char* usage to be `struct efi_time[]`, which conveys the type information while still being distinct from the single `struct efi_time*` inputs. Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
Reported by cppcheck 2.12.1 Signed-off-by: Eric Richter <[email protected]>
Reported by cppcheck 2.12.1 Signed-off-by: Eric Richter <[email protected]>
…or uint64_t Reported by cppcheck 2.12.1 Signed-off-by: Eric Richter <[email protected]>
The globals used in skiboot proper are not used in secvarctl, thus do not need to be included in this header. These globals are then shadowed in the host backend when simulating updates, causing warnings from cppcheck. Rather than suppress the warnings, comment out the offending extern declarations, which should probably not be there in the first place. Signed-off-by: Eric Richter <[email protected]>
Newer versions of cppcheck apparently don't like it when suppressions are not matched. As currently the newer version of cppcheck and the older version used by CI do not agree on whether certain suppressions are hit, disable the unmatched warning entirely. Signed-off-by: Eric Richter <[email protected]>
Reported by scan-build, introduced in 1f96bad. The PK/append special case check runs unconditionally after the switch statement ends. Therefore, the -n argument may not be supplied, thus args->variable_name may be NULL. The strcmp() call to check if the target variable does not verify that a variable name is set, thus may read from NULL. Add a check for NULL before the strcmp. Signed-off-by: Eric Richter <[email protected]>
Move the existing print_timestamp function to common/util, and add a new helper to handle reading and printing a timestamp from a variable buffer. Prints the timestamp when using the read command, and when using verbose levels in the verify command. This commit is part of a larger work in progress on factoring out loading data from a variable vs a file, see more information in #76. Co-developed-by: Eric Richter <[email protected]> Signed-off-by: Sudhakar Kuppusamy <[email protected]>
-w option available only when use -u with -p. otherwise throwing error message Signed-off-by: Sudhakar Kuppusamy <[email protected]>
In generating trustedcadb auth file, checked to have basicConstraints CA:TRUE for any certificate added to trustedcadb, else returned as invalid certificate. Signed-off-by: Sudhakar Kuppusamy <[email protected]>
Signed-off-by: Sudhakar Kuppusamy <[email protected]>
issue: verify with -u, -p and -w, verify of variable auth file failed if variable not available in /sys/firmware/secvar/vars/. fix: allowing the variable auth file verify and write if variable not available in /sys/firmware/secvar/vars/. Signed-off-by: Sudhakar Kuppusamy <[email protected]>
…e is set Signed-off-by: Eric Richter <[email protected]>
TIMESTAMP_LEN is incredibly misleading. In its current form, it refers to an 8-byte header consisting of a 1-byte version number, and 7 bytes of a timestamp (truncated at pad1). This is obviously confusing as sizeof(timestamp_t) == 16, and this also includes a version byte for some reason. This commit attempts to clean this up a bit, by using the struct defined in libstb-secvar that defines the layout of this header, so that types and sizeof() can be used to get these offset/size values, instead of undocumented mislabled constants. This includes a version bump of libstb-secvar, which includes a commit exposing the variable header struct in pseries.h. Signed-off-by: Eric Richter <[email protected]>
Introduced in 74c5b43 . While reworking buffer types, a header include got added to the top of generic.c (likely automatically placed by an editor). This header does not seem to be present in the alpine build environment, thus causing the static builds to fail. Since this header is not even needed, it can be removed. Signed-off-by: Eric Richter <[email protected]>
When building the read test cases, the script looks for files with the file extension ".cert", when all the test data uses the suffix ".crt". Therefore, no ".crt" files have actually been tested. Signed-off-by: Eric Richter <[email protected]>
When using `read -c <file.crt>`, secvarctl only reads the data and does not require nor assign a variable name. Therefore, the call to `is_trustedcadb_variable()` errors as it expects a valid non-NULL string as an argument. Check that variable_name is non-NULL before passing to the trustedcadb variable check. Introduced in: 0d274ba Guest/generate: trustedcadb variable allow only CA certificates Fixes: #78 Reported-by: R Nageswara Sastry <[email protected]> Signed-off-by: Eric Richter <[email protected]>
Argp calls the `parse_options()` function once per argument, therefore it is possible that the condition for printing the error may be reached if there are more arguments to parse. Move the check to after arguments have been parsed, so that the check is only made after the complete argument stream has been parsed in entirety. Fixes: #80 Reported-by: R Nageswara Sastry <[email protected]> Signed-off-by: Eric Richter <[email protected]>
…arsing
Currently, it is ASSUMED but not enforced that the first argument to
`secvarctl generate` is a format specifier in the form `a:b`, where a,b
correspond to the formats to be generated/converted. This argument parse
logic is applied to ANY bare/positional arguments applied to `generate`,
including repeated ones.
Therefore, any of the following cases can occur:
1. multiple format specifiers can be supplied:
`secvarctl generate a:b b:c c:d -n NAME -c file.crt`
2. invalid/erroneous arguments can be mis-interpreted as a format specifier:
`secvarctl generate a:b -n NAME -c file.crt -a 0`,
where `-a` does not take in an argument, thus yielding "invalid format"
3. format specifiers can be ANYWHERE in the arg list:
`secvarctl generate -n NAME -c file.crt a:b`
This commit addresses case 1 and 2 by only allowing format specifiers to be
declared ONCE in an argument list. Furthermore, the format specifier is
validated while parsing, so in the event the erroneous positional
argument is read in first, the parser will immediately complain.
Case 3 is NOT addressed by this commit, however is a consideration for
future refactoring of the argument parsing logic.
Signed-off-by: Eric Richter <[email protected]>
…type Currently, assertCmdFalse only checks that a command exited with a nonzero return code, but not WHY it returned nonzero. That means, this assert is perfectly happy with a command exiting with SIGSEGV, passing the test. This commit changes two things: 1. check for exit via a signal 2. force ASAN to abort on error, causing an exit via SIGABRT This way, SIGSEGVs should be caught on assertCmdFalse cases, meaning that we should be asserting that our command handles invalid input correctly instead of crashing. ASAN needs to specifically be configured to abort, otherwise it will return a nonzero return code that might just be picked up as a "correct error" by mistake. Signed-off-by: Eric Richter <[email protected]>
Signed-off-by: Eric Richter <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PR contains all of the changes needed to support guest secureboot in secvarctl, numerous fixes and refactors, CI improvements, and more. This will bring the new version to
v1.0.0For more detailed changes, see the respective pull requests:
v1.0.0-rc1
--version, display supported backends #55v1.0.0-rc2
v1.0.0-rc3
Current dependencies:
Pre-release artifacts are available in the releases section, with the latest one available here. This PR will be used to track any bugs from release candidate builds.