On 7/9/26 3:01 AM, Alexis Lothoré (eBPF Foundation) wrote:
> When the verifier patches an ebpf program with bpf_patch_insn_data, it
> then calls adjust_insn_aux_data to make sure that insn_aux_data takes
> into account the newly inserted patch. Some of the data offset is pretty
> straightforward to deduce, it is for example the case for
> indirect_target, as any patch affecting indirect calls will
> systematically move the original instruction to the end of the new
> patch.
I think an additional KASAN-specific argument to adjust_insn_aux_data()
is not a good idea. It's threaded through ~30 call sites, and it's
been error-prone too: you had to fix the offsets a few times already.
indirect_target needs no help from the callers, so it's not really the
same pattern. Similar for the other aux fields: seen is broadcast and
zext_dst is re-derived.
>
> In order to introduce KASAN support for eBPF JIT, we need to mark any
> load/store instruction that accesses non-stack memory, but updating this
> new marking after a patch is not as straightforward as for indirect
> calls: the original BPF_ST/BPF_STX/BPF_LDX can be at the beginning, at
> the end or somewhere in the middle of the new patch: we then need some
> additional info to properly update this marking.
I don't think we need to track the exact offset here.
It looks like .non_stack_access is set to is_mem_insn(insn + off) in
every single case *except* for when a stack access happens to be in
the middle of a patch.
Given that the cost of getting the flag "wrong" is an unnecessary
kasan check only for that case, I think it'll be cleaner to just
unconditionally do:
data[off].non_stack_access = is_mem_insn(insn + off);
in adjust_insn_aux_data()
And then the code change can be folded in patch #2
The only problem with this I can think of is that in reality *most*
stack accesses go trough that special case, which would defeat the
purpose of the .non_stack_access flag. This can be checked empirically
however.
Anything else I'm missing here?
>
> Add a new parameter to bpf_patch_insn_data and adjust_insn_aux_data to
> convey the info about the new location in the patch of the original
> instruction. This info does not always make sense depending on the
> generated patch (eg, some bpf helpers being inlined by the verifier, and
> so turned into multiple new instructions without any remaining BPF_CALL),
> the parameter can then be set to -1. It will be used in next patches to
> properly handle insn_aux_data adjustment for the new KASAN
> instrumentation
>
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <[email protected]>
> ---
> Changes in v5:
> - fix a few incorrect offset values
>
> Changes in v4:
> - define insn_off_in_patch inside convert_ctx_accesses main loop to
> avoid old value leakage
>
> Changes in v3:
> - new patch
> ---
> include/linux/filter.h | 10 +++--
> kernel/bpf/core.c | 2 +-
> kernel/bpf/fixups.c | 100
> ++++++++++++++++++++++++++++++++-----------------
> 3 files changed, 74 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 14acb2455746..1ebcd247ef4e 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1210,13 +1210,17 @@ struct bpf_prog *bpf_patch_insn_single(struct
> bpf_prog *prog, u32 off,
>
> #ifdef CONFIG_BPF_SYSCALL
> struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 off,
> - const struct bpf_insn *patch, u32 len);
> + const struct bpf_insn *patch, u32 len,
> + s32 insn_off_in_patch);
> struct bpf_insn_aux_data *bpf_dup_insn_aux_data(struct bpf_verifier_env
> *env);
> void bpf_restore_insn_aux_data(struct bpf_verifier_env *env,
> struct bpf_insn_aux_data *orig_insn_aux);
> #else
> -static inline struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env
> *env, u32 off,
> - const struct bpf_insn
> *patch, u32 len)
> +static inline struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env
> *env,
> + u32 off,
> + const struct bpf_insn *patch,
> + u32 len,
> + s32 insn_off_in_patch)
> {
> return ERR_PTR(-ENOTSUPP);
> }
> [...]
>
>
