ACK/cmnt: [CVE-2017-9150][X/Y/joule/aws/gke][PATCH 1/1] bpf: don't let ldimm64 leak map addresses on unprivileged

[Stefan Bader]

On 21.06.2017 16:13, Wen-chien Jesse Sung wrote:
> From: Daniel Borkmann <daniel at iogearbox.net>
> 
> CVE-2017-9150
> 
> 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: 1be7f75d1668 ("bpf: enable non-root eBPF programs")
> Fixes: cbd357008604 ("bpf: verifier (add ability to receive verification log)")
> Reported-by: Jann Horn <jannh at google.com>
> Signed-off-by: Daniel Borkmann <daniel at iogearbox.net>
> Acked-by: Alexei Starovoitov <ast at kernel.org>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (backported from commit 0d0e57697f162da4aa218b5feafe614fb666db07)
> Signed-off-by: Wen-chien Jesse Sung <jesse.sung at canonical.com>

Acked-by: Stefan Bader <stefan.bader at canonical.com>

> ---

The backport appear to look right. But this to be targeted only fo X/Y. Any of
the derivatives (or backports) will automatically get it when rebasing.

Thanks,
-Stefan

>  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 2cbfba7..85de509 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -313,7 +313,8 @@ static const char *const bpf_jmp_string[16] = {
>  	[BPF_EXIT >> 4] = "exit",
>  };
>  
> -static void print_bpf_insn(struct bpf_insn *insn)
> +static void print_bpf_insn(const struct verifier_env *env,
> +			   const struct bpf_insn *insn)
>  {
>  	u8 class = BPF_CLASS(insn->code);
>  
> @@ -377,9 +378,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;
> @@ -1758,7 +1769,7 @@ static int do_check(struct verifier_env *env)
>  
>  		if (log_level) {
>  			verbose("%d: ", insn_idx);
> -			print_bpf_insn(insn);
> +			print_bpf_insn(env, insn);
>  		}
>  
>  		if (class == BPF_ALU || class == BPF_ALU64) {
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20170622/ad3874cd/attachment.sig>