On Tue, Jan 4, 2022 at 9:16 AM Kris Van Hees [off-list ref] wrote:

I ran into a problem due to this patch.  Specifically, the test in the
__check_func_call() function is flaweed because it can actually mis-interpret
a regular BPF-to-BPF pseudo-call as a callback call.

Consider the conditional in the code:

        if (insn->code == (BPF_JMP | BPF_CALL) &&
            insn->imm == BPF_FUNC_timer_set_callback) {

The BPF_FUNC_timer_set_callback has value 170.  This means that if you have
a BPF program that contains a pseudo-call with an instruction delta of 170,
this conditional will be found to be true by the verifier, and it will
interpret the pseudo-call as a callback.  This leads to a mess with the
verification of the program because it makes the wrong assumptions about the
nature of this call.

As far as I can see, the solution is simple.  Include an explicit check to
ensure that src_reg is not a pseudo-call.  I.e. make the conditional:

        if (insn->code == (BPF_JMP | BPF_CALL) &&
            insn->src_reg != BPF_PSEUDO_CALL &&
            insn->imm == BPF_FUNC_timer_set_callback) {

It is of course a pretty rare case that this would go wrong, but since my
code makes extensive use of BPF-to-BPF pseudo-calls, it was only a matter of
time before I would run into a call with instruction delta 170.

Great catch. All makes sense.
Could you please submit an official patch ?
Checking for insn->src_reg == 0 is probably better,
since src_reg can be BPF_PSEUDO_KFUNC_CALL as well
though __check_func_call is not called for it.