CodeQL

[andyhhp]

Several RFCs here.

• Are we happy swapping M32=y/n for BITS=32/64 in the first place? I can fix the CodeQL bug while retaining M32= but I don't really like leaving M32= in place.
• Do we want to run security-and-quality (i.e. everything) by default? It shows up 40 issues, most of which I think are false positives, but can probably be fixed by doing certain things in more standard ways.

@dpsmith Along with merging this, we should disable the LGTM plugin for the repo.

https://github.blog/2022-08-15-the-next-step-for-lgtm-com-github-code-scanning/ for full info.

[krystian-hebel]

I'm all for BITS, its much clearer than M32=n.

As for CodeQL, it has all false positives from LGTM and some more, most notably it doesn't seem to understand noreturn. Also, I'd like to know what happened here https://github.com/TrenchBoot/secure-kernel-loader/security/code-scanning/31, namely where sizeof(tpm_sha1_digest) comes from and why sizeof() result isn't size_t. If we can get rid of false positives like the last one by running different set of tests I think we should do it, because I honestly don't see how we can fix that memcpy() call, it is already as standard as it can be.

On the positive side, information is presented in much more readable way than LGTM did it.

[andyhhp]

I have a suspicion that all of this fallout stems from https://github.com/TrenchBoot/secure-kernel-loader/security/code-scanning/14 and the likes of https://github.com/TrenchBoot/secure-kernel-loader/security/code-scanning/17 where CodeQL is clearly getting confused as to what a size_t is, and/or failing to spot the declarations of the string functions.

My best guess is that it's either something about not parsing -include properly to the compiler or getting confused given the magic in include/string.h. All of the incompatible argument issues are complaining about integer promotion.

The two "real" issues are the high severity ones. https://github.com/TrenchBoot/secure-kernel-loader/security/code-scanning/18 is a false positive, due to the identified code being unreachable in all cases where mle_header isn't initialised. https://github.com/TrenchBoot/secure-kernel-loader/security/code-scanning/40 is intentional, and something a static analyser ought to flag. It probably wants a semantic code comment explaining why this case is correct.

[andyhhp]

I've opened github/codeql#10600 for the noreturn issue, and github/codeql#10601 for the size_t issue.

[macpijan]

@krystian-hebel @andyhhp

I've opened github/codeql#10600 for the noreturn issue, and github/codeql#10601 for the size_t issue.

Both of these are closed already. Did you have any more comments or suggestions on how to move forward here?

We could push this as part of the task: TrenchBoot/trenchboot-issues#19

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[rossphilipson]

Looked it over...
Reviewed-by: Ross Philipson [email protected]

[dpsmith]

Reviewed-by: Daniel P. Smith [email protected]