draft: sanitize_check_bounds test#8966
Closed
luisgerhorst wants to merge 1 commit intokernel-patches:bpf-next_basefrom
Closed
draft: sanitize_check_bounds test#8966luisgerhorst wants to merge 1 commit intokernel-patches:bpf-next_basefrom
luisgerhorst wants to merge 1 commit intokernel-patches:bpf-next_basefrom
Conversation
luisgerhorst
force-pushed
the
scb
branch
3 times, most recently
from
9028adc to
7e17351
Compare
kernel-patches-daemon-bpf
bot
force-pushed
the
bpf-next_base
branch
2 times, most recently
from
49174bd to
f3a4188
Compare
kernel-patches-daemon-bpf
bot
force-pushed
the
bpf-next_base
branch
3 times, most recently
from
9857a0f to
b93b30b
Compare
luisgerhorst
force-pushed
the
scb
branch
3 times, most recently
from
0f543ec to
d1427b7
Compare
kernel-patches-daemon-bpf
bot
force-pushed
the
bpf-next_base
branch
4 times, most recently
from
e85324d to
61c8df2
Compare
As is, it appears as if ptr alu is allowed for everything except
PTR_TO_{STACK,MAP_VALUE} if one only looks at sanitize_check_bounds().
However, this is misleading as the function only works together with
retrieve_ptr_limit() and the two must be kept in sync. This patch
documents the interdependency and adds a check to ensure they stay in
sync.
Because the preceeding switch returns -EACCES for every opcode except
for ADD/SUB, the sanitize_needed() following the sanitize_check_bounds()
call is always true if reached. This means, unless
sanitize_check_bounds() detected that the pointer goes OOB because of
the ADD/SUB (and returns -EACCES), sanitize_ptr_alu() always executes
after sanitize_check_bounds().
In sanitize_ptr_alu(commit_window = true), we always run
retrieve_ptr_limit() unless:
* can_skip_alu_sanititation() is true, noteably `BPF_SRC(insn->code) ==
BPF_K`. BPF_K is fine because it means that there is no scalar
register (which could be subject to spec. scalar confusion due to
Spectre v4) that goes into the alu op. The ptr reg can not be subject
to v4-based value confusion due to the nospec added. Thus, in this
case it would have been fine to also skip sanitize_check_bounds().
* We are on a speculative path (`vstate->speculative`). In the second
"commit" phase, sanitize_ptr_alu() always just returns 0. This makes
sense because there are no alu sanitization limits to be learned from
speculative paths. Furthermore, because the sanitization will ensure
that ptr arith stays in (architectural) bounds, the
sanitize_check_bounds() on the spec. path could also be skipped.
TODO, through case two, is it fine that we allow alu with other ptr type
(non-stack/ap) on spec path
retrieve_ptr_limit() only allows the ALU op if the involved ptr reg (can
be either src or dst for ADD) is PTR_TO_STACK or PTR_TO_MAP_VALUE.
Otherwise it returns -EOPNOTSUPP.
In summary, sanitize_check_bounds() returning 0 for
non-PTR_TO_{STACK,MAP_VALUE} is fine because retrieve_ptr_limit() also
runs for all relevant cases and prevents unsafe ops.
We therefore allow unsanitized ptr arith with 64-bit ADD/SUB for
* ptr -=/+= imm32, i.e. `BPF_SRC(insn->code) == BPF_K`
* PTR_TO_{STACK,MAP_VALUE} -= scalar
* PTR_TO_{STACK,MAP_VALUE} += scalar
* scalar += PTR_TO_{STACK,MAP_VALUE}
if the requirements from retrieve_ptr_limit() AND
sanitize_check_bounds() hold.
To document this interdependency between sanitize_check_bounds() and
retrieve_ptr_limit(), add a verifier_bug_if() to make sure they stay in
sync.
Further background info:
* There are two calls to sanitize_ptr_alu(), on the first call we spawn
the path to simulate truncation and
TODO, why spawn the path before simulating the alu op
TODO, why two calls to sanitize_ptr_alu
Signed-off-by: Luis Gerhorst <luis.gerhorst@fau.de>
kernel-patches-daemon-bpf
bot
force-pushed
the
bpf-next_base
branch
10 times, most recently
from
9fa5029 to
148f936
Compare
|
Automatically cleaning up stale PR; feel free to reopen if needed |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.