netdev - Re: [PATCH v5 bpf-next 4/6] bpf, x64: rework pro/epilogue and tailcall handling in JIT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e0c1f8c5-cd73-48eb-7c92-fcf755319173@iogearbox.net>
Date:   Wed, 29 Jul 2020 00:07:52 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>, ast@...nel.org
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, bjorn.topel@...el.com,
        magnus.karlsson@...el.com
Subject: Re: [PATCH v5 bpf-next 4/6] bpf, x64: rework pro/epilogue and
 tailcall handling in JIT

On 7/24/20 7:35 PM, Maciej Fijalkowski wrote:
> This commit serves two things:
> 1) it optimizes BPF prologue/epilogue generation
> 2) it makes possible to have tailcalls within BPF subprogram
> 
> Both points are related to each other since without 1), 2) could not be
> achieved.
> 
[...]
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 6fe6491fa17a..e9d62a60134b 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -750,6 +750,7 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
>   				    struct bpf_prog *old,
>   				    struct bpf_prog *new)
>   {
> +	u8 *old_addr, *new_addr, *old_bypass_addr;
>   	struct prog_poke_elem *elem;
>   	struct bpf_array_aux *aux;
>   
> @@ -800,13 +801,47 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
>   			if (poke->tail_call.map != map ||
>   			    poke->tail_call.key != key)
>   				continue;
> +			/* protect against un-updated poke descriptors since
> +			 * we could fill them from subprog and the same desc
> +			 * is present on main's program poke tab
> +			 */
> +			if (!poke->tailcall_bypass || !poke->tailcall_target ||
> +			    !poke->bypass_addr)
> +				continue;

Thinking more about this, this check here is not sufficient. You basically need this here
given you copy all poke descs over to each of the subprogs in jit_subprogs(). So for those
that weren't handled by the subprog have the above addresses as NULL. But in jit_subprogs()
once we filled out the target addresses for the bpf-in-bpf calls we loop over each subprog
and do the extra/final pass in the JIT to complete the images. However, nothing protects
bpf_tail_call_direct_fixup() as far as I can see from patching at the NULL addr if there is
a target program loaded in the map at the given key. That will most likely blow up and hit
the BUG_ON().

Instead of these above workarounds, did you try to go the path to only copy over the poke
descs that are relevant for the individual subprog (but not all the others)?

Thanks,
Daniel