| Message ID | 1701252962-63418-1-git-send-email-alibuda@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/netfilter: bpf: avoid leakage of skb | expand |
D. Wythe <alibuda@linux.alibaba.com> wrote: > From: "D. Wythe" <alibuda@linux.alibaba.com> > > A malicious eBPF program can interrupt the subsequent processing of > a skb by returning an exceptional retval, and no one will be responsible > for releasing the very skb. How? The bpf verifier is supposed to reject nf bpf programs that return a value other than accept or drop. If this is a real bug, please also figure out why 006c0e44ed92 ("selftests/bpf: add missing netfilter return value and ctx access tests") failed to catch it. > Moreover, normal programs can also have the demand to return NF_STOLEN, No, this should be disallowed already. > net/netfilter/nf_bpf_link.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c > index e502ec0..03c47d6 100644 > --- a/net/netfilter/nf_bpf_link.c > +++ b/net/netfilter/nf_bpf_link.c > @@ -12,12 +12,29 @@ static unsigned int nf_hook_run_bpf(void *bpf_prog, struct sk_buff *skb, > const struct nf_hook_state *s) > { > const struct bpf_prog *prog = bpf_prog; > + unsigned int verdict; > struct bpf_nf_ctx ctx = { > .state = s, > .skb = skb, > }; > > - return bpf_prog_run(prog, &ctx); > + verdict = bpf_prog_run(prog, &ctx); > + switch (verdict) { > + case NF_STOLEN: > + consume_skb(skb); > + fallthrough; This can't be right. STOLEN really means STOLEN (free'd, redirected, etc, "skb" MUST be "leaked". Which is also why the bpf program is not allowed to return it.
On 11/29/23 9:18 PM, Florian Westphal wrote: > D. Wythe <alibuda@linux.alibaba.com> wrote: >> From: "D. Wythe" <alibuda@linux.alibaba.com> >> >> A malicious eBPF program can interrupt the subsequent processing of >> a skb by returning an exceptional retval, and no one will be responsible >> for releasing the very skb. > How? The bpf verifier is supposed to reject nf bpf programs that > return a value other than accept or drop. > > If this is a real bug, please also figure out why > 006c0e44ed92 ("selftests/bpf: add missing netfilter return value and ctx access tests") > failed to catch it. Hi Florian, You are right, i make a mistake.. , it's not a bug.. And my origin intention was to allow ebpf progs to return NF_STOLEN, we are trying to modify some netfilter modules via ebpf, and some scenarios require the use of NF_STOLEN, but from your description, it seems that at least currently, you do not want to return NF_STOLEN, until there is a helper for sonsume_skb(), right ? Again, very sorry to bother you. Best wishes, D. Wythe. >> Moreover, normal programs can also have the demand to return NF_STOLEN, > No, this should be disallowed already. > >> net/netfilter/nf_bpf_link.c | 19 ++++++++++++++++++- >> 1 file changed, 18 insertions(+), 1 deletion(-) >> >> diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c >> index e502ec0..03c47d6 100644 >> --- a/net/netfilter/nf_bpf_link.c >> +++ b/net/netfilter/nf_bpf_link.c >> @@ -12,12 +12,29 @@ static unsigned int nf_hook_run_bpf(void *bpf_prog, struct sk_buff *skb, >> const struct nf_hook_state *s) >> { >> const struct bpf_prog *prog = bpf_prog; >> + unsigned int verdict; >> struct bpf_nf_ctx ctx = { >> .state = s, >> .skb = skb, >> }; >> >> - return bpf_prog_run(prog, &ctx); >> + verdict = bpf_prog_run(prog, &ctx); >> + switch (verdict) { >> + case NF_STOLEN: >> + consume_skb(skb); >> + fallthrough; > This can't be right. STOLEN really means STOLEN (free'd, > redirected, etc, "skb" MUST be "leaked". > > Which is also why the bpf program is not allowed to return it.
D. Wythe <alibuda@linux.alibaba.com> wrote: > And my origin intention was to allow ebpf progs to return NF_STOLEN, we are > trying to modify some netfilter modules via ebpf, > and some scenarios require the use of NF_STOLEN, but from your description, NF_STOLEN can only be supported via a trusted helper, as least as far as I understand. Otherwise verifier would have to guarantee that any branch that returns NF_STOLEN has released the skb, or passed it to a function that will release the skb in the near future.
On 11/29/23 10:47 PM, Florian Westphal wrote: > D. Wythe <alibuda@linux.alibaba.com> wrote: >> And my origin intention was to allow ebpf progs to return NF_STOLEN, we are >> trying to modify some netfilter modules via ebpf, >> and some scenarios require the use of NF_STOLEN, but from your description, > NF_STOLEN can only be supported via a trusted helper, as least as far as > I understand. > > Otherwise verifier would have to guarantee that any branch that returns > NF_STOLEN has released the skb, or passed it to a function that will > release the skb in the near future. Thank you very much for your help. I now understand the difficulty here. The verifier cannot determine whether the consume_skb() was executed or not, when the return value goes to NF_STOLEN. We may use NF_DROP at first, it won't be make much difference for us now. Also, do you have any plans to support this helper? Best wishes, D. Wythe
diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c index e502ec0..03c47d6 100644 --- a/net/netfilter/nf_bpf_link.c +++ b/net/netfilter/nf_bpf_link.c @@ -12,12 +12,29 @@ static unsigned int nf_hook_run_bpf(void *bpf_prog, struct sk_buff *skb, const struct nf_hook_state *s) { const struct bpf_prog *prog = bpf_prog; + unsigned int verdict; struct bpf_nf_ctx ctx = { .state = s, .skb = skb, }; - return bpf_prog_run(prog, &ctx); + verdict = bpf_prog_run(prog, &ctx); + switch (verdict) { + case NF_STOLEN: + consume_skb(skb); + fallthrough; + case NF_ACCEPT: + case NF_DROP: + case NF_QUEUE: + /* restrict the retval of the ebpf programs */ + break; + default: + /* force it to be dropped */ + verdict = NF_DROP_ERR(-EINVAL); + break; + } + + return verdict; } struct bpf_nf_link {