On Tue, Jan 4, 2022 at 9:16 AM Kris Van Hees <kris.van.hees@xxxxxxxxxx> 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.
