On Mon, 17 Jun 2024 at 15:44, Matt Bobrowski <mattbobrowski@xxxxxxxxxx> wrote:
>
> Adding some new negative selftests which are responsible for asserting
> that the new relaxed fixed offset constraints applicable to BPF
> helpers and kfuncs taking trusted pointer arguments are enforced
> correctly by the BPF verifier.
>
> The BPF programs contained within the new negative selftests are
> mainly responsible for triggering the various branches and checks
> performed within the check_release_arg_reg_off() helper.
>
> Signed-off-by: Matt Bobrowski <mattbobrowski@xxxxxxxxxx>
> ---
With the stuff below addressed, please add:
Acked-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
I believe sockmap map_update and dynptr ones are also providing
positive coverage (given previous versions made them fail), so I think
focusing on negative cases is good enough here. I guess bpf_sk_release
is also covered by existing ones.
Still it'd be good to add a few more cases if you find any, as Eduard
suggested, just to make sure we're not missing anything.
> [...]
> +SEC("tp_btf/task_newtask")
> +__failure
> +__msg("variable trusted_ptr_ access var_off=(0x0; 0xffffffffffffffff) disallowed")
> +int BPF_PROG(trusted_type_match_mismatch_neg_var_off, struct task_struct *task,
> + u64 clone_flags)
> +{
> + s64 var_off = task->start_time;
> + task = bpf_get_current_task_btf();
> +
> + bpf_assert_range(var_off, -64, 64);
> + /* Need one bpf_throw() reference, otherwise BTF gen fails. */
> + if (!task)
> + bpf_throw(1);
It seems s390x is failing because it doesn't have support for throw.
So I'd just drop this selftest, since on second thought I think it
doesn't provide any additional coverage than the pos var_off one.
> +
> + task = (void *)task + var_off;
> + /* Passing a trusted pointer with an incorrect variable offset, type
> + * match will succeed due to reg->off == 0, but the later call to
> + * __check_ptr_off_reg should fail as it's responsible for checking
> + * reg->var_off.
> + */
> + task = bpf_task_acquire(task);
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.45.2.627.g7a2c4fd464-goog
>
> /M
