Static Analysis Fixes #44

erichte-ibm · 2023-07-24T14:41:10Z

This PR adds a new make cppcheck build target to run the cppcheck static analysis tool on the source files. The rest of this PR is a fix for every single complaint it had, including a few found from scan-build.

Some highlights:

a LOT of incorrect format strings, mostly using %d with unsigned integers (%u). Commit messages started getting lazier with these.
a worrying amount of unsigned < 0 comparisons. May warrant a larger audit of all size checks and size arithmetic, I fear there may be possible cases of unsigned wrapping.
a few function overhauls, as the errors that were reported were too messy to fix in a simple manner. These should have more detailed commit messages

NOTE: A subsequent PR will re-enable Actions, and will run both of these tools.

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]>

…e type Signed-off-by: Eric Richter <[email protected]>

rc is a relatively useless variable in this function unless there actually is an error. Therefore, initialize it to SUCCESS, and return it at the end rather than only referencing it in the while loop. Signed-off-by: Eric Richter <[email protected]>

…same #ifdef Without this, those two variables trigger an static analysis warning when not compiling with the defined flag. Also include those as a requirement under the flag so to temporarily clear the warning. That flag should still be deleted eventually. Signed-off-by: Eric Richter <[email protected]>

cppcheck flags variables that can be reduced in scope, meaning they are declared in say the top-level of a function, but only used in the scope of a loop. This in itself isn't an issue, as I personally prefer seeing all the variables used in the function as a whole up at the top, however it does highlight a larger issue with the function as a whole -- FAR too much indentation and probably repeat logic. This function should *really* be broken down into helper functions so that it need not be one massive while loop. For now, reduce the variables to the scopes that they are used in. Signed-off-by: Eric Richter <[email protected]>

This function needlessly uses near-identical logic for PK and KEK, only changing what variables are referenced. Therefore, factor out the common logic, to dramatically reduce code size and indentation levels. Signed-off-by: Eric Richter <[email protected]>

get_pk_and_kek_from_path_var previously contained one giant `if` block with all of the logic. Invert the logic and return early, so that the rest of the body can have the indentation reduced. Also return an error code instead of SUCCESS if that condition is not met, since apparently it just happily returns on through without doing anything. Signed-off-by: Eric Richter <[email protected]>

Signed-off-by: Eric Richter <[email protected]>

Fix check of esl_data_size against -1, which will never happen. Since this appears to be checking that there is enough space in the esl buffer for the timestamp header + data, instead just check that the size is at least the length of a timestamp. If the size is less than a timestamp, it's probably garbage data. Signed-off-by: Eric Richter <[email protected]>

Signed-off-by: Eric Richter <[email protected]>

…C to that scope Signed-off-by: Eric Richter <[email protected]>

I'm going to be honest, I have no idea where that code came from, or why it was there in the first place. It appears the host equivalent to this function also has code after the break, but cppcheck isn't flagging that for some reason. Adding a default: to the switch breaks a test. Removing the initial break breaks a test. None of this make sense, so just remove the offending code, and improve the test cases later -- maybe we'll find out why this is so miserable later. Signed-off-by: Eric Richter <[email protected]>

Signed-off-by: Eric Richter <[email protected]>

For some reason the for loop uses an assigment as its condition. I'm going to ignore that and address the portability issue of casting a pointer to void then doing arithmetic on a non-known size for `void *`. Just increment the pointer if you are only going to add the size of the value anyway. Signed-off-by: Eric Richter <[email protected]>

Signed-off-by: Eric Richter <[email protected]>

… is trying to be loaded Signed-off-by: Eric Richter <[email protected]>

Signed-off-by: Eric Richter <[email protected]>

…_single_esl esl_size is declared and only used twice: once to make a redundant check, and then again at the end of the function to be assigned to a pass-by-reference parameter. Remove the variable, redundant check, and use the source value directly at the end. Signed-off-by: Eric Richter <[email protected]>

Signed-off-by: Eric Richter <[email protected]>

Same change as before, see the following commit for more details: a0a778e guest/validate: remove needless esl_size variable alias from validate_single_esl Signed-off-by: Eric Richter <[email protected]>

Signed-off-by: Eric Richter <[email protected]>

… zero Signed-off-by: Eric Richter <[email protected]>

Reported by scan-build. ESL is not checked for NULL throughout the function, and is blatantly memcpy'd from at the end from an also unchecked size value. This is however, actually valid, the case of ESL_size = 0, as the memcpy will then not actually read from the buffer. Even better, this is exactly how generating a reset auth file works -- it passes in a NULL buffer with ESL_size set to 0. Therefore, this commit adds a check for that specific error case where ESL is NULL, but ESL_size is nonzero. Also this skips the memcpy at the end in the case of ESL being NULL, just to ensure that any static analyzers are happy about not blatantly memcpy'ing from NULL, even if ESL_size = 0 would prevent it. Signed-off-by: Eric Richter <[email protected]>

Reported by scan-build. auth_data is assumed to not be NULL. Add a check to ensure it is not, so that we don't blatantly continue to pass it around and read from garbage memory. Signed-off-by: Eric Richter <[email protected]>

Reported by scan-build. If getCurrentVars returns an error case, we exit the function early without cleaning up currentVars. Free the memory in the error clause. Signed-off-by: Eric Richter <[email protected]>

…_var Reported by scan-build. Currently, this attempts to free auth_data as returned by get_auth_data() if rc is non-success. get_auth_data() only sets *auth_data if returning successfully. Therefore, auth_data will never be assigned to an allocated buffer in this particular conditional block. Remove the free, as it is not necessary. Functions that return allocated memory should really be marked better. Signed-off-by: Eric Richter <[email protected]>

…UNC enabled Signed-off-by: Eric Richter <[email protected]>

…s for better inline documentation Signed-off-by: Eric Richter <[email protected]>

nick-child-ibm

Ran out of time today but managed to review ~10 commits. Will add more later, thanks!

nick-child-ibm · 2023-07-25T21:32:45Z

backends/guest/common/validate.c

@@ -260,7 +260,7 @@ int validate_esl(const uint8_t *esl_data, size_t esl_data_len)
 	if (!count)
 		return ESL_FAIL;

-	return SUCCESS;
+	return rc;


This actually changes behavior of this function.

Assume that there are more than one ESL's in the chain: Previously, if the first one parsed successfully and the second failed to parse, we would return success. This commit ensures that all ESLs are valid AND that all bytes are accounted for.

This functions behavior should mirror whatever our backends are accepting as a "valid" ESL list (ikik atm machine-ism lol)

Agreed. Changed to reducing the scope of rc instead of persisting it through the whole function. There's no test case that failed for this, but there really should be if this change of behavior is supposed to be important.

This is a weird function though -- why do we handled what seems to be the first one different from the second. Why is a function called validate_esl validating a list, and should it not be breaking and erroring if we're validating a list of esls and one fails? I don't know what the right answer is here, but this is slated for rework.

nick-child-ibm · 2023-07-25T22:01:20Z

backends/guest/common/read.c

@@ -124,14 +124,17 @@ int print_variables(const uint8_t *buffer, size_t buffer_size, const uint8_t *va
 {
 	int rc;
 	ssize_t esl_data_size = buffer_size, cert_size;


agreed, we should split this into smaller functions.

But if you are going to reduce scope of variables then you might as well go all the way:

data_size is useless and can be removed and replaced with cert_size

cert_size, esd can be declared in the second loop

signature_type, sig_list can go in first loop

I don't think offset is being used properly in this function at all, in the first loop we use it for offset to the next ESL header, in the second, loop we use it as an offset to the ESL data. I doubt this works if there are ever two appended ESL structures (which I believe is possible with dbx, unless something changed)

As this was mostly to silence cppcheck warnings, most of the rest of the scope reductions will probably be removed with a function rewrite. Skipping for now.

nick-child-ibm · 2023-07-28T18:39:43Z

backends/guest/common/verify.c

+
+		free(auth_data);
+
+		if (rc != SUCCESS)


Just a note on this function as a whole, this is not due to the work in this PR, but it looks like this (poorly-named) function performs updates on all PK and KEK updates first then it processes all other variables in verify_update_variable. This differs from host boot processing flow (which handles updates in chronological order). We talked offline and there was speculation as to whether guest boot could even process more than update at once (since it handles updates in realtime). We need to make sure that this is clarified and this function reflects what the backend actually does. If it can only accept one update at a time then we need to have secvarctl reflect that.

nick-child-ibm · 2023-07-28T18:46:22Z

backends/guest/common/verify.c

 		}

+		rc = update_variable((uint8_t *)args->update_variable[i],


I could have sworn this function was implemented in libstb-sevar but it appears it is defined in this file. I see libstb-secvar implements pseries_update_variable. At some point we should look to use that function instead. The more libstb-secvar functions that we can use in secvarctl, the more useful secvarctl will be.

There's... multiple functions like this that more or less do the same thing. There's also a wrapper function in libstb-secvar with 15+ arguments that packs the struct args. I'll look into this with the next overhaul PR that hopefully removes a lot of these redundant wrappers.

nick-child-ibm · 2023-07-28T18:50:16Z

backends/guest/common/verify.c

-		free(esl_data_path);
+	if (args->variable_path == NULL) {
+		prlog(PR_ERR, "Path is not set, possibly a bug?");
+		return -1; // Throwaway error, this function likely needs a rewrite


Was going to complain about generic -1 value but I see where you are coming from. The only use of the function is:

if (args->variable_path != NULL) rc = get_pk_and_kek_from_path_var(.........

So I agree with what you have here.

I'm planning an eventual overhaul of internal secvarctl error codes -- there should at least be a generic unknown error and more memory error types. Will likely end up using standard errno where possible, but that needs a whole treewide rework.

backends/guest/guest_svc_verify.c

nick-child-ibm · 2023-07-28T19:49:04Z

This looks really good thanks for doing all of this.

I left some minor nits, maybe only need to verify the validate_esl comment and perhaps fix that argp usage note. The rest should just be jotted down somewhere for future work.

I have more cppcheck errors showing up when I run make cppcheck, I am going to fix those up (if necessary) and then open a PR against your branch. If yours gets merged before then than I will just open against guest-devel.

Thanks again!

Signed-off-by: Eric Richter <[email protected]>

To fix a scope reduction warning from cppcheck, a previous commit persisted the use of rc throughout the whole function. However, this introduces an unintended side effect of being pickier about Change the function to reduce the scope of rc instead to avoid the unintended side-effect. Signed-off-by: Eric Richter <[email protected]>

erichte-ibm added 30 commits July 24, 2023 09:30

Makefile: add cppcheck static analysis target similar to libstb-secvar's

82a374b

guest/generate: remove always true conditions as reported by cppcheck

2262f6b

Signed-off-by: Eric Richter <[email protected]>

guest/generate: unsigned (size_t) variables cannot be less than zero

69c9284

Signed-off-by: Eric Richter <[email protected]>

guest/read: change printf format specifier to %zu to match variable type

f4a14f0

Signed-off-by: Eric Richter <[email protected]>

guest/valdiate: change printf format specific to %zu to match variabl…

540d15b

…e type Signed-off-by: Eric Richter <[email protected]>

guest/verify: reduce the scope of the loop index

a30dbe7

Signed-off-by: Eric Richter <[email protected]>

guest/read: size_t = %zu

5789c41

Signed-off-by: Eric Richter <[email protected]>

guest/read: reduce scope of variables that are only used by WRITE_FUN…

652864b

…C to that scope Signed-off-by: Eric Richter <[email protected]>

generic: remove unusued function get_leading_whitespace

ee5f417

Signed-off-by: Eric Richter <[email protected]>

host/generate: fix invalid malloc return check

b0588e2

Signed-off-by: Eric Richter <[email protected]>

backends/host: unsigned integers still can't be less than zero

cb12c58

Signed-off-by: Eric Richter <[email protected]>

host/read: reduce the scope of variables in printReadable

45081f4

Signed-off-by: Eric Richter <[email protected]>

host/validate: reduce scope of variables

fd03f0d

Signed-off-by: Eric Richter <[email protected]>

host/validate: add a NULL check to the **x509 parameter of parseX509

109a579

Signed-off-by: Eric Richter <[email protected]>

host/verify: remove assignment with no effect

470ef5c

Signed-off-by: Eric Richter <[email protected]>

host/validate: change ts_ptr to const in timestamp_is_empty

71ee02a

Signed-off-by: Eric Richter <[email protected]>

host/verify: remove redundant check, as <= 1 implies !

81758e8

Signed-off-by: Eric Richter <[email protected]>

guest: use system endian.h, as it's entirely ambiguous which endian.h…

d702bd4

… is trying to be loaded Signed-off-by: Eric Richter <[email protected]>

Makefile: fix include pathing for cppcheck

1146beb

Signed-off-by: Eric Richter <[email protected]>

guest/generate: fix format string typing (int -> uint)

d5cc405

Signed-off-by: Eric Richter <[email protected]>

guest/read: fix format string typing

6c7366a

Signed-off-by: Eric Richter <[email protected]>

erichte-ibm added 17 commits July 24, 2023 09:30

guest/validate: fix format string typing

b606dee

Signed-off-by: Eric Richter <[email protected]>

guest/validate: unsigned integers still can't be less than zero

161591c

Signed-off-by: Eric Richter <[email protected]>

guest/verify: remove redundant check, unsigned less than 0, etc etc

16e257b

Signed-off-by: Eric Richter <[email protected]>

more unsigned printf fixes

0ccabc9

Signed-off-by: Eric Richter <[email protected]>

host/validate: %zu

c96d802

Signed-off-by: Eric Richter <[email protected]>

host/validate: remove useless eslsize alias

947ab14

Same change as before, see the following commit for more details: a0a778e guest/validate: remove needless esl_size variable alias from validate_single_esl Signed-off-by: Eric Richter <[email protected]>

host/validate: always free a buffer that always exists

4cf8030

Signed-off-by: Eric Richter <[email protected]>

host/validate: unsigned still can't be less than zero

391cd0d

Signed-off-by: Eric Richter <[email protected]>

guest/read: remove conditional that is always true in read_auth

d2c4ed2

Signed-off-by: Eric Richter <[email protected]>

host/read: standard for unsigned unchanged, still cannot be less than…

5518ca2

… zero Signed-off-by: Eric Richter <[email protected]>

guest/read: check auth_data is not NULL in read_auth

49cbda7

Reported by scan-build. auth_data is assumed to not be NULL. Add a check to ensure it is not, so that we don't blatantly continue to pass it around and read from garbage memory. Signed-off-by: Eric Richter <[email protected]>

host/verify: fix potential memory leak in setupBanks

8f6703e

Reported by scan-build. If getCurrentVars returns an error case, we exit the function early without cleaning up currentVars. Free the memory in the error clause. Signed-off-by: Eric Richter <[email protected]>

host/generate: fix unsigned printf format strings with CRYPTO_WRITE_F…

b0da6ee

…UNC enabled Signed-off-by: Eric Richter <[email protected]>

Makefile/cppcheck: suppress false positives, reorganize cppcheck flag…

9878d6f

…s for better inline documentation Signed-off-by: Eric Richter <[email protected]>

nick-child-ibm reviewed Jul 25, 2023

View reviewed changes

nick-child-ibm reviewed Jul 28, 2023

View reviewed changes

erichte-ibm added 2 commits August 1, 2023 11:39

guest/verify: display usage if arguments fail to validate

1e4202d

Signed-off-by: Eric Richter <[email protected]>

This was referenced Aug 1, 2023

Replace valgrind with ASAN/UBSAN, overhaul debug build #45

Merged

Re-enable GitHub Actions CI #46

Merged

ASAN Fixes #47

Merged

nick-child-ibm merged commit e77b8c3 into open-power:guest-devel Aug 3, 2023

erichte-ibm mentioned this pull request Sep 27, 2023

v1.0.0 #57

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static Analysis Fixes #44

Static Analysis Fixes #44

erichte-ibm commented Jul 24, 2023

nick-child-ibm left a comment

nick-child-ibm Jul 25, 2023

erichte-ibm Aug 1, 2023

erichte-ibm Aug 1, 2023

nick-child-ibm Jul 25, 2023

erichte-ibm Aug 1, 2023

nick-child-ibm Jul 28, 2023

nick-child-ibm Jul 28, 2023

erichte-ibm Aug 1, 2023

nick-child-ibm Jul 28, 2023

erichte-ibm Aug 1, 2023

nick-child-ibm commented Jul 28, 2023

Static Analysis Fixes #44

Static Analysis Fixes #44

Conversation

erichte-ibm commented Jul 24, 2023

nick-child-ibm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nick-child-ibm commented Jul 28, 2023