[PATCH net] bpf: don't let ldimm64 leak map addresses on unprivileged

[PATCH net] bpf: don't let ldimm64 leak map addresses on unprivileged

[Daniel Borkmann]

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 <jannh@google.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/verifier.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c2ff608..5d19a02 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -298,7 +298,8 @@ static void print_verifier_state(struct bpf_verifier_state *state)
 	[BPF_EXIT >> 4] = "exit",
 };
 
-static void print_bpf_insn(struct bpf_insn *insn)
+static void print_bpf_insn(const struct bpf_verifier_env *env,
+			   const struct bpf_insn *insn)
 {
 	u8 class = BPF_CLASS(insn->code);
 
@@ -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;
@@ -2853,7 +2864,7 @@ static int do_check(struct bpf_verifier_env *env)
 
 		if (log_level) {
 			verbose("%d: ", insn_idx);
-			print_bpf_insn(insn);
+			print_bpf_insn(env, insn);
 		}
 
 		err = ext_analyzer_insn_hook(env, insn_idx, prev_insn_idx);
-- 
1.9.3

Re: [PATCH net] bpf: don't let ldimm64 leak map addresses on unprivileged

[Jann Horn]

On Mon, May 8, 2017 at 12:04 AM, Daniel Borkmann <daniel@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 <jannh@google.com>
> Signed-off-by: Daniel Borkmann <daniel@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?

Re: [PATCH net] bpf: don't let ldimm64 leak map addresses on unprivileged

[Daniel Borkmann]

On 05/08/2017 12:26 AM, Jann Horn wrote:
> On Mon, May 8, 2017 at 12:04 AM, Daniel Borkmann <daniel@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 <jannh@google.com>
>> Signed-off-by: Daniel Borkmann <daniel@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.

Re: [PATCH net] bpf: don't let ldimm64 leak map addresses on unprivileged

[Jann Horn]

On Mon, May 8, 2017 at 12:51 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 05/08/2017 12:26 AM, Jann Horn wrote:
>>
>> On Mon, May 8, 2017 at 12:04 AM, Daniel Borkmann <daniel@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 <jannh@google.com>
>>> Signed-off-by: Daniel Borkmann <daniel@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.

Re: [PATCH net] bpf: don't let ldimm64 leak map addresses on unprivileged

[Daniel Borkmann]

On 05/08/2017 12:04 AM, Daniel Borkmann 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().

This one is also needed for the log (should come first):

Fixes: 1be7f75d1668 ("bpf: enable non-root eBPF programs")

> Fixes: cbd357008604 ("bpf: verifier (add ability to receive verification log)")
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

Thanks,
Daniel

Re: [PATCH net] bpf: don't let ldimm64 leak map addresses on unprivileged

[Alexei Starovoitov]

On Mon, May 08, 2017 at 12:04:09AM +0200, Daniel Borkmann 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 <jannh@google.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

thanks for the fix!

Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH net] bpf: don't let ldimm64 leak map addresses on unprivileged

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Mon,  8 May 2017 00:04:09 +0200

> 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 <jannh@google.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

Applied, with second Fixes: tag added, and queued up for -stable.

Thanks.