Explicit raw_tp NULL arguments #4776
Closed
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
![kernel-patches-daemon-bpf-rc[bot]](https://avatars.githubusercontent.com/u/70728002?s=60&v=4){kind=small}
This patch reverts commit cb4158c ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL"). The patch was well-intended and meant to be as a stop-gap fixing branch prediction when the pointer may actually be NULL at runtime. Eventually, it was supposed to be replaced by an automated script or compiler pass detecting possibly NULL arguments and marking them accordingly. However, it caused two main issues observed for production programs and failed to preserve backwards compatibility. First, programs relied on the verifier not exploring == NULL branch when pointer is not NULL, thus they started failing with a 'dereference of scalar' error. Next, allowing raw_tp arguments to be modified surfaced the warning in the verifier that warns against reg->off when PTR_MAYBE_NULL is set. More information, context, and discusson on both problems is available in [0]. Overall, this approach had several shortcomings, and was the fixes would further complicate the verifier's logic, and the entire masking scheme would have to be removed eventually anyway. Hence, revert the patch in preparation of a better fix avoiding these issues. [0]: https://lore.kernel.org/bpf/[email protected] Reported-by: Manu Bretelle <[email protected]> Fixes: cb4158c ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL") Signed-off-by: Kumar Kartikeya Dwivedi <[email protected]>
…ing" This patch reverts commit d798ce3 ("selftests/bpf: Add tests for raw_tp null handling") since it's parent commit cb4158c ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL") is also being reverted. Fixes: d798ce3 ("selftests/bpf: Add tests for raw_tp null handling") Signed-off-by: Kumar Kartikeya Dwivedi <[email protected]>
Arguments to a raw tracepoint are tagged as trusted, which carries the semantics that the pointer will be non-NULL. However, in certain cases, a raw tracepoint argument may end up being NULL. More context about this issue is available in [0]. Thus, there is a discrepancy between the reality, that raw_tp arguments can actually be NULL, and the verifier's knowledge, that they are never NULL, causing explicit NULL checks to be deleted, and accesses to such pointers potentially crashing the kernel. A previous attempt [1], i.e. the second fixed commit, was made to simulate symbolic execution as if in most accesses, the argument is a non-NULL raw_tp, except for conditional jumps. This tried to suppress branch prediction while preserving compatibility, but surfaced issues with production programs that were difficult to solve without increasing verifier complexity. A more complete discussion of issues and fixes is available at [2]. Fix this by maintaining an explicit, incomplete list of tracepoints where the arguments are known to be NULL, and mark the positional arguments as PTR_MAYBE_NULL. Additionally, capture the tracepoints where arguments are known to be PTR_ERR, and mark these arguments as scalar values to prevent potential dereference. In the future, an automated pass will be used to produce such a list, or insert __nullable annotations automatically for tracepoints. Anyhow, this is an attempt to close the gap until the automation lands, and reflets the current best known list according to Jiri's analysis in [3]. [0]: https://lore.kernel.org/bpf/[email protected] [1]: https://lore.kernel.org/all/[email protected] [2]: https://lore.kernel.org/bpf/[email protected] [3]: https://lore.kernel.org/bpf/Z1d-qbCdtJqg6Er4@krava Reported-by: Juri Lelli <[email protected]> # original bug Reported-by: Manu Bretelle <[email protected]> # bugs in masking fix Fixes: 3f00c52 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs") Fixes: cb4158c ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL") Co-developed-by: Jiri Olsa <[email protected]> Signed-off-by: Jiri Olsa <[email protected]> Signed-off-by: Kumar Kartikeya Dwivedi <[email protected]>
Add bash and python scripts that process the RAW_TP_NULL_ARGS list in kernel/bpf/btf.c and produce selftests automatically. This is not done automatically on build as the file requires some human-guided post processing (like disabling certain tests for which we don't enable CONFIG options), and only needs to be run once when growing the list. Once the list generation becomes automatic in kernel/bpf/btf.c, the script can run on build, or be modified if we support __nullable BTF type tags in the future to parse them and generate tests accordingly. The tests basically ensure the pointer is marked or_null_, and likewise for raw_tp_scalar.c case (where it needs to be marked scalar). Enable enough config options to cover all but 4 without increasing build time significantly. By enabling AFS, CACHEFILES, and INFINIBAND, we gain enough coverage to cover distinct cases and positional arguments. The config is modified to include some new options to test most options, but driver tracepoints that include too much stuff are disabled manually after the script produces it's output. Whenever adding a new RAW_TP_NULL_ARGS specification or removing one, the developer can run this script to update the selftest, reject hunks removing the comments around disabled tracepoints, and accept other hunks that are relevant for the newly added tracepoint. There are some tracepoints manually hardcoded in btf_ctx_access that have an IS_ERR argument type. For now, these are manually encoded in raw_tp_scalar.c, but if the list grows, we can introduce a new mask for IS_ERR args, an ERR_ARG() macro, and augment the script to generate tests for these cases and ensure argument is marked scalar. Signed-off-by: Kumar Kartikeya Dwivedi <[email protected]>
|
Upstream branch: 7d0d673 |
|
At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=916606 expired. Closing PR. |
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.
Pull request for series with
subject: Explicit raw_tp NULL arguments
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=916606