On Mon, May 8, 2017 at 12:51 AM, Daniel Borkmann <dan...@iogearbox.net> wrote:
> On 05/08/2017 12:26 AM, Jann Horn wrote:
>>
>> On Mon, May 8, 2017 at 12:04 AM, Daniel Borkmann <dan...@iogearbox.net>
>> wrote:
>>>
>>> The patch fixes two things at once:
>>>
>>> 1) It checks the env->allow_ptr_leaks and only prints the map address to
>>> the log if we have the privileges to do so, otherwise it just dumps 0
>>> as we would when kptr_restrict is enabled on %pK. Given the latter is
>>> off by default and not every distro sets it, I don't want to rely on
>>> this, hence the 0 by default for unprivileged.
>>>
>>> 2) Printing of ldimm64 in the verifier log is currently broken in that
>>> we don't print the full immediate, but only the 32 bit part of the
>>> first insn part for ldimm64. Thus, fix this up as well; it's okay to
>>> access, since we verified all ldimm64 earlier already (including just
>>> constants) through replace_map_fd_with_map_ptr().
>>>
>>> Fixes: cbd357008604 ("bpf: verifier (add ability to receive verification
>>> log)")
>>> Reported-by: Jann Horn <ja...@google.com>
>>> Signed-off-by: Daniel Borkmann <dan...@iogearbox.net>
>>
>> [...]
>>>
>>> @@ -362,9 +363,19 @@ static void print_bpf_insn(struct bpf_insn *insn)
>>> insn->code,
>>> bpf_ldst_string[BPF_SIZE(insn->code) >>
>>> 3],
>>> insn->src_reg, insn->imm);
>>> - } else if (BPF_MODE(insn->code) == BPF_IMM) {
>>> - verbose("(%02x) r%d = 0x%x\n",
>>> - insn->code, insn->dst_reg, insn->imm);
>>> + } else if (BPF_MODE(insn->code) == BPF_IMM &&
>>> + BPF_SIZE(insn->code) == BPF_DW) {
>>> + /* At this point, we already made sure that the
>>> second
>>> + * part of the ldimm64 insn is accessible.
>>> + */
>>> + u64 imm = ((u64)(insn + 1)->imm << 32) |
>>> (u32)insn->imm;
>>> + bool map_ptr = insn->src_reg ==
>>> BPF_PSEUDO_MAP_FD;
>>> +
>>> + if (map_ptr && !env->allow_ptr_leaks)
>>> + imm = 0;
>>> +
>>> + verbose("(%02x) r%d = 0x%llx\n", insn->code,
>>> + insn->dst_reg, (unsigned long long)imm);
>>> } else {
>>> verbose("BUG_ld_%02x\n", insn->code);
>>> return;
>>
>>
>> You replaced the `BPF_MODE(insn->code) == BPF_IMM` branch with a
>> `BPF_MODE(insn->code) == BPF_IMM && BPF_SIZE(insn->code) == BPF_DW`
>> branch. Doesn't that break printing normal immediates?
>
>
> What do you mean by 'normal' immediates? You mean loads of imm into
> register, right? The ldimm64 is kind of special treated; for imms
> fitting into 32 bit, there is BPF_MOV64_IMM() and BPF_MOV32_IMM()
> otherwise.
>
> F.e. see the jumptable in __bpf_prog_run(), which is the interpreter.
> All BPF_LD instructions that we have are:
>
> static const void *jumptable[256] = {
> [...]
> [BPF_LD | BPF_ABS | BPF_W] = &&LD_ABS_W,
> [BPF_LD | BPF_ABS | BPF_H] = &&LD_ABS_H,
> [BPF_LD | BPF_ABS | BPF_B] = &&LD_ABS_B,
> [BPF_LD | BPF_IND | BPF_W] = &&LD_IND_W,
> [BPF_LD | BPF_IND | BPF_H] = &&LD_IND_H,
> [BPF_LD | BPF_IND | BPF_B] = &&LD_IND_B,
> [BPF_LD | BPF_IMM | BPF_DW] = &&LD_IMM_DW,
> };
>
> In the print_bpf_insn() under class == BPF_LD, the BPF_ABS and BPF_IND
> are separately handled (load of packet data from skb), and the BPF_IMM
> is the one we're fixing, which only has BPF_DW as an option. I added it
> there since we really only want to see BPF_DW in this branch due to the
> double imm access.
Ah, right, I missed that. Nevermind.
