Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix map value pruning check #4010

Closed
wants to merge 2 commits into from
Closed

Fix map value pruning check #4010

wants to merge 2 commits into from

Conversation

kernel-patches-bot
Copy link

Pull request for series with
subject: Fix map value pruning check
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607

@kernel-patches-bot
Copy link
Author

Upstream branch: 4b45cd8
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kernel-patches-bot
Copy link
Author

Upstream branch: 4b45cd8
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kernel-patches-bot
Copy link
Author

Upstream branch: 1f6e04a
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kernel-patches-bot
Copy link
Author

Upstream branch: 9cbd48d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kernel-patches-bot
Copy link
Author

Upstream branch: ed1fe1b
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kernel-patches-bot
Copy link
Author

Upstream branch: 5fd2a60
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kernel-patches-bot
Copy link
Author

Upstream branch: 47df8a2
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kernel-patches-bot
Copy link
Author

Upstream branch: 47df8a2
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kernel-patches-bot
Copy link
Author

Upstream branch: 6f04180
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kernel-patches-bot
Copy link
Author

Upstream branch: 11a695a
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kernel-patches-bot
Copy link
Author

Upstream branch: 8be602d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kkdwivedi
bpf: Fix state pruning check for PTR_TO_MAP_VALUE
0fed5ac 
Currently, the verifier preserves reg->id for PTR_TO_MAP_VALUE when it
points to a map value that contains a bpf_spin_lock element (see the
logic in reg_may_point_to_spin_lock and how it is used to skip setting
reg->id to 0 in mark_ptr_or_null_reg). This gives a unique lock ID for
each critical section begun by a bpf_spin_lock helper call.

The same reg->id is matched with env->active_spin_lock during unlock to
determine whether bpf_spin_unlock is called for the same bpf_spin_lock
object.

However, regsafe takes a different approach to safety checks currently.
The comparison of reg->id was explicitly skipped in the commit being
fixed with the reasoning that the reg->id value should have no bearing
on the safety of the program if the old state was verified to be safe.

This however is demonstrably not true (with a selftest having the
verbose working test case in a later commit), with the following pseudo
code:

	r0 = bpf_map_lookup_elem(&map, ...); // id=1
	r6 = r0;
	r0 = bpf_map_lookup_elem(&map, ...); // id=2
	r7 = r0;

	bpf_spin_lock(r1=r6);
	if (cond) // unknown scalar, hence verifier cannot predict branch
		r6 = r7;
	p:
	bpf_spin_unlock(r1=r7);

In the first exploration path, we would want the verifier to skip
over the r6 = r7 assignment so that it reaches BPF_EXIT and the
state branches counter drops to 0 and it becomes a safe verified
state.

The branch target 'p' acts a pruning point, hence states will be
compared. If the old state was verified without assignment, it has
r6 with id=1, but the new state will have r6 with id=2. The other
parts of register, stack, and reference state and any other verifier
state compared in states_equal remain unaffected by the assignment.

Now, when the memcmp fails for r6, the verifier drops to the switch case
and simply memcmp until the id member, and requires the var_off to be
more permissive in the current state. Once establishing this fact, it
returns true and search is pruned.

Essentially, we end up calling unlock for a bpf_spin_lock that was never
locked whenever the condition is true at runtime.

To fix this, also include id in the memcmp comparison. Since ref_obj_id
is never set for PTR_TO_MAP_VALUE, change the offsetof to be until that
member.

Note that by default the reg->id in case of PTR_TO_MAP_VALUE should be 0
(without PTR_MAYBE_NULL), so it should only really impact cases where a
bpf_spin_lock is present in the map element.

Fixes: d83525c ("bpf: introduce bpf_spin_lock")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Acked-by: Stanislav Fomichev <sdf@google.com>
@kernel-patches-bot
Copy link
Author

Upstream branch: 8be602d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=694607
version: 1

@kkdwivedi
selftests/bpf: Add pruning test case for bpf_spin_lock
41020cf 
Test that when reg->id is not same for the same register of type
PTR_TO_MAP_VALUE between current and old explored state, we currently
return false from regsafe and continue exploring.

Without the fix in prior commit, the test case fails.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Acked-by: Stanislav Fomichev <sdf@google.com>
@kernel-patches-bot
Copy link
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=694607 expired. Closing PR.

@kernel-patches-bot kernel-patches-bot deleted the series/694607=>bpf branch November 27, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bpf changes-requested V1
Projects
None yet
Milestone
No milestone
2 participants
@kernel-patches-bot @kkdwivedi