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?
