* [PATCH net] bpf: fix ri->map prog pointer on bpf_prog_realloc
@ 2017-09-19 1:16 Daniel Borkmann
2017-09-19 1:43 ` Alexei Starovoitov
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Borkmann @ 2017-09-19 1:16 UTC (permalink / raw)
To: davem; +Cc: john.fastabend, ast, netdev, Daniel Borkmann
Commit 109980b894e9 ("bpf: don't select potentially stale
ri->map from buggy xdp progs") passed the pointer to the prog
itself to be loaded into r4 prior on bpf_redirect_map() helper
call, so that we can store the owner into ri->map_owner out of
the helper.
Issue with that is that the actual address of the prog is still
subject to change when subsequent rewrites occur, e.g. through
patching other helper functions or constant blinding. Thus, we
really need to take prog->aux as the address we're holding, and
then during runtime fetch the actual pointer via aux->prog. This
also works with prog clones as they share the same aux and fixup
pointer to self after blinding finished.
Fixes: 109980b894e9 ("bpf: don't select potentially stale ri->map from buggy xdp progs")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/verifier.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 799b245..243c09f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4205,9 +4205,17 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
}
if (insn->imm == BPF_FUNC_redirect_map) {
- u64 addr = (unsigned long)prog;
+ /* Note, we cannot use prog directly as imm as subsequent
+ * rewrites would still change the prog pointer. The only
+ * stable address we can use is aux, which also works with
+ * prog clones during blinding.
+ */
+ u64 addr = (unsigned long)prog->aux;
+ const int r4 = BPF_REG_4;
struct bpf_insn r4_ld[] = {
- BPF_LD_IMM64(BPF_REG_4, addr),
+ BPF_LD_IMM64(r4, addr),
+ BPF_LDX_MEM(BPF_DW, r4, r4,
+ offsetof(struct bpf_prog_aux, prog)),
*insn,
};
cnt = ARRAY_SIZE(r4_ld);
--
1.9.3
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net] bpf: fix ri->map prog pointer on bpf_prog_realloc
2017-09-19 1:16 [PATCH net] bpf: fix ri->map prog pointer on bpf_prog_realloc Daniel Borkmann
@ 2017-09-19 1:43 ` Alexei Starovoitov
2017-09-19 7:23 ` Daniel Borkmann
0 siblings, 1 reply; 3+ messages in thread
From: Alexei Starovoitov @ 2017-09-19 1:43 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, john.fastabend, ast, netdev
On Tue, Sep 19, 2017 at 03:16:44AM +0200, Daniel Borkmann wrote:
> Commit 109980b894e9 ("bpf: don't select potentially stale
> ri->map from buggy xdp progs") passed the pointer to the prog
> itself to be loaded into r4 prior on bpf_redirect_map() helper
> call, so that we can store the owner into ri->map_owner out of
> the helper.
>
> Issue with that is that the actual address of the prog is still
> subject to change when subsequent rewrites occur, e.g. through
> patching other helper functions or constant blinding. Thus, we
> really need to take prog->aux as the address we're holding, and
> then during runtime fetch the actual pointer via aux->prog. This
> also works with prog clones as they share the same aux and fixup
> pointer to self after blinding finished.
>
> Fixes: 109980b894e9 ("bpf: don't select potentially stale ri->map from buggy xdp progs")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> kernel/bpf/verifier.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 799b245..243c09f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4205,9 +4205,17 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
> }
>
> if (insn->imm == BPF_FUNC_redirect_map) {
> - u64 addr = (unsigned long)prog;
> + /* Note, we cannot use prog directly as imm as subsequent
> + * rewrites would still change the prog pointer. The only
> + * stable address we can use is aux, which also works with
> + * prog clones during blinding.
> + */
good catch. extra load at runtime sucks, but I don't see better solution.
> + u64 addr = (unsigned long)prog->aux;
> + const int r4 = BPF_REG_4;
> struct bpf_insn r4_ld[] = {
> - BPF_LD_IMM64(BPF_REG_4, addr),
> + BPF_LD_IMM64(r4, addr),
> + BPF_LDX_MEM(BPF_DW, r4, r4,
> + offsetof(struct bpf_prog_aux, prog)),
needs to be BPF_FIELD_SIZEOF(struct bpf_prog_aux, prog) to work on 32-bit
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] bpf: fix ri->map prog pointer on bpf_prog_realloc
2017-09-19 1:43 ` Alexei Starovoitov
@ 2017-09-19 7:23 ` Daniel Borkmann
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Borkmann @ 2017-09-19 7:23 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: davem, john.fastabend, ast, netdev
On 09/19/2017 03:43 AM, Alexei Starovoitov wrote:
> On Tue, Sep 19, 2017 at 03:16:44AM +0200, Daniel Borkmann wrote:
>> Commit 109980b894e9 ("bpf: don't select potentially stale
>> ri->map from buggy xdp progs") passed the pointer to the prog
>> itself to be loaded into r4 prior on bpf_redirect_map() helper
>> call, so that we can store the owner into ri->map_owner out of
>> the helper.
>>
>> Issue with that is that the actual address of the prog is still
>> subject to change when subsequent rewrites occur, e.g. through
>> patching other helper functions or constant blinding. Thus, we
>> really need to take prog->aux as the address we're holding, and
>> then during runtime fetch the actual pointer via aux->prog. This
>> also works with prog clones as they share the same aux and fixup
>> pointer to self after blinding finished.
>>
>> Fixes: 109980b894e9 ("bpf: don't select potentially stale ri->map from buggy xdp progs")
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>> kernel/bpf/verifier.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 799b245..243c09f 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -4205,9 +4205,17 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
>> }
>>
>> if (insn->imm == BPF_FUNC_redirect_map) {
>> - u64 addr = (unsigned long)prog;
>> + /* Note, we cannot use prog directly as imm as subsequent
>> + * rewrites would still change the prog pointer. The only
>> + * stable address we can use is aux, which also works with
>> + * prog clones during blinding.
>> + */
>
> good catch. extra load at runtime sucks, but I don't see better solution.
>
>> + u64 addr = (unsigned long)prog->aux;
>> + const int r4 = BPF_REG_4;
>> struct bpf_insn r4_ld[] = {
>> - BPF_LD_IMM64(BPF_REG_4, addr),
>> + BPF_LD_IMM64(r4, addr),
>> + BPF_LDX_MEM(BPF_DW, r4, r4,
>> + offsetof(struct bpf_prog_aux, prog)),
>
> needs to be BPF_FIELD_SIZEOF(struct bpf_prog_aux, prog) to work on 32-bit
Good point, will spin a v2. Thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-09-19 7:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-19 1:16 [PATCH net] bpf: fix ri->map prog pointer on bpf_prog_realloc Daniel Borkmann
2017-09-19 1:43 ` Alexei Starovoitov
2017-09-19 7:23 ` Daniel Borkmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).