The verifier currently does not allow overwriting a referenced dynptr's stack slot to prevent resource leak. This is because referenced dynptr holds additional resources that requires calling specific helpers to release. This limitation can be relaxed when there are multiple copies of the same dynptr. Whether it is the orignial dynptr or one of its clones, as long as there exists at least one other dynptr with the same ref_obj_id (to be used to release the reference), its stack slot should be allowed to be overwritten. Suggested-by: Andrii Nakryiko Signed-off-by: Amery Hung --- kernel/bpf/verifier.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 9523db3fe90d..147495099a23 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -934,8 +934,27 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env, spi = spi + 1; if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) { - verbose(env, "cannot overwrite referenced dynptr\n"); - return -EINVAL; + int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id; + int ref_cnt = 0; + + /* + * A referenced dynptr can be overwritten only if there is at + * least one other dynptr sharing the same ref_obj_id, + * ensuring the reference can still be properly released. + */ + for (i = 0; i < state->allocated_stack / BPF_REG_SIZE; i++) { + if (state->stack[i].slot_type[0] != STACK_DYNPTR) + continue; + if (!state->stack[i].spilled_ptr.dynptr.first_slot) + continue; + if (state->stack[i].spilled_ptr.ref_obj_id == ref_obj_id) + ref_cnt++; + } + + if (ref_cnt <= 1) { + verbose(env, "cannot overwrite referenced dynptr\n"); + return -EINVAL; + } } mark_stack_slot_scratched(env, spi); -- 2.52.0 Test overwriting referenced dynptr and clones to make sure it is only allow when there is at least one other dynptr with the same ref_obj_id. Also make sure slice is still invalidated after the dynptr's stack slot is destroyed. Signed-off-by: Amery Hung --- .../testing/selftests/bpf/progs/dynptr_fail.c | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c index 8f2ae9640886..b62773ce5219 100644 --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c @@ -1993,3 +1993,118 @@ int test_dynptr_reg_type(void *ctx) global_call_bpf_dynptr((const struct bpf_dynptr *)current); return 0; } + +/* Overwriting a referenced dynptr is allowed if a clone still holds the ref */ +SEC("?raw_tp") +__success +int dynptr_overwrite_ref_with_clone(void *ctx) +{ + struct bpf_dynptr ptr, clone; + + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr); + + bpf_dynptr_clone(&ptr, &clone); + + /* Overwrite the original - clone still holds the ref */ + *(volatile __u8 *)&ptr = 0; + + bpf_ringbuf_discard_dynptr(&clone, 0); + + return 0; +} + +/* Overwriting the last referenced dynptr should still be rejected */ +SEC("?raw_tp") +__failure __msg("cannot overwrite referenced dynptr") +int dynptr_overwrite_ref_last_clone(void *ctx) +{ + struct bpf_dynptr ptr, clone; + + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr); + + bpf_dynptr_clone(&ptr, &clone); + + /* Overwrite the original - clone still holds the ref, OK */ + *(volatile __u8 *)&ptr = 0; + + /* Overwrite the last holder - this should fail */ + *(volatile __u8 *)&clone = 0; + + return 0; +} + +/* Overwriting a clone should be allowed if the original still holds the ref */ +SEC("?raw_tp") +__success +int dynptr_overwrite_clone_with_original(void *ctx) +{ + struct bpf_dynptr ptr, clone; + + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr); + + bpf_dynptr_clone(&ptr, &clone); + + /* Overwrite the clone - original still holds the ref */ + *(volatile __u8 *)&clone = 0; + + bpf_ringbuf_discard_dynptr(&ptr, 0); + + return 0; +} + +/* Data slices from the destroyed dynptr should be invalidated */ +SEC("?raw_tp") +__failure __msg("invalid mem access 'scalar'") +int dynptr_overwrite_ref_invalidate_slice(void *ctx) +{ + struct bpf_dynptr ptr, clone; + int *data; + + bpf_ringbuf_reserve_dynptr(&ringbuf, val, 0, &ptr); + + data = bpf_dynptr_data(&ptr, 0, sizeof(val)); + if (!data) + return 0; + + bpf_dynptr_clone(&ptr, &clone); + + /* Overwrite the original - clone holds the ref */ + *(volatile __u8 *)&ptr = 0; + + /* data was from the original dynptr, should be invalid now */ + *data = 123; + + return 0; +} + +/* + * Data slices from a dynptr clone should remain valid after + * overwriting the original dynptr + */ +SEC("?raw_tp") +__success +int dynptr_overwrite_ref_clone_slice_valid(void *ctx) +{ + struct bpf_dynptr ptr, clone; + int *data; + + bpf_ringbuf_reserve_dynptr(&ringbuf, val, 0, &ptr); + + bpf_dynptr_clone(&ptr, &clone); + + data = bpf_dynptr_data(&clone, 0, sizeof(val)); + if (!data) { + bpf_ringbuf_discard_dynptr(&clone, 0); + return 0; + } + + /* Overwrite the original - clone holds the ref */ + *(volatile __u8 *)&ptr = 0; + + /* data is from the clone, should still be valid */ + *data = 123; + + bpf_ringbuf_discard_dynptr(&clone, 0); + + return 0; +} -- 2.52.0