In the previous commit, bpf_net_context handling was added to 
tun_sendmsg() and do_xdp_generic(), but if you write code like this,
bpf_net_context overlaps in the call trace below, causing various
memory corruptions.

<Call trace>
...
tun_sendmsg() // bpf_net_ctx_set()
  tun_xdp_one()
    do_xdp_generic() // bpf_net_ctx_set() <-- nested
...

This patch removes the bpf_net_context handling that exists in 
do_xdp_generic() and modifies it to handle it in the parent function.

Reported-by: syzbot+44623300f057a28baf1e@syzkaller.appspotmail.com
Fixes: fecef4cd42c6 ("tun: Assign missing bpf_net_context.")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
 drivers/net/tun.c | 3 +++
 net/core/dev.c    | 8 +++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9b24861464bc..095ada4a525e 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1919,10 +1919,12 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 
 	if (skb_xdp) {
 		struct bpf_prog *xdp_prog;
+		struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx;
 		int ret;
 
 		local_bh_disable();
 		rcu_read_lock();
+		bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
 		xdp_prog = rcu_dereference(tun->xdp_prog);
 		if (xdp_prog) {
 			ret = do_xdp_generic(xdp_prog, &skb);
@@ -1932,6 +1934,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 				goto unlock_frags;
 			}
 		}
+		bpf_net_ctx_clear(bpf_net_ctx);
 		rcu_read_unlock();
 		local_bh_enable();
 	}
diff --git a/net/core/dev.c b/net/core/dev.c
index 6ea1d20676fb..26f9fdd66e64 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5126,14 +5126,11 @@ static DEFINE_STATIC_KEY_FALSE(generic_xdp_needed_key);
 
 int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff **pskb)
 {
-	struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx;
-
 	if (xdp_prog) {
 		struct xdp_buff xdp;
 		u32 act;
 		int err;
 
-		bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
 		act = netif_receive_generic_xdp(pskb, &xdp, xdp_prog);
 		if (act != XDP_PASS) {
 			switch (act) {
@@ -5147,13 +5144,11 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff **pskb)
 				generic_xdp_tx(*pskb, xdp_prog);
 				break;
 			}
-			bpf_net_ctx_clear(bpf_net_ctx);
 			return XDP_DROP;
 		}
 	}
 	return XDP_PASS;
 out_redir:
-	bpf_net_ctx_clear(bpf_net_ctx);
 	kfree_skb_reason(*pskb, SKB_DROP_REASON_XDP);
 	return XDP_DROP;
 }
@@ -5475,10 +5470,13 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
 
 	if (static_branch_unlikely(&generic_xdp_needed_key)) {
 		int ret2;
+		struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx;
 
 		migrate_disable();
+		bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
 		ret2 = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog),
 				      &skb);
+		bpf_net_ctx_clear(bpf_net_ctx);
 		migrate_enable();
 
 		if (ret2 != XDP_PASS) {
--

Jeongjun Park wrote:
> In the previous commit, bpf_net_context handling was added to 
> tun_sendmsg() and do_xdp_generic(), but if you write code like this,
> bpf_net_context overlaps in the call trace below, causing various
> memory corruptions.

I'm no expert on this code, but commit 401cb7dae813 that introduced
bpf_net_ctx_set explicitly states that nested calls are allowed.

And the function does imply that:

static inline struct bpf_net_context *bpf_net_ctx_set(struct bpf_net_context *bpf_net_ctx)
{
        struct task_struct *tsk = current;

        if (tsk->bpf_net_context != NULL)
                return NULL;
        bpf_net_ctx->ri.kern_flags = 0;

        tsk->bpf_net_context = bpf_net_ctx;
        return bpf_net_ctx;
}


 
> <Call trace>
> ...
> tun_sendmsg() // bpf_net_ctx_set()
>   tun_xdp_one()
>     do_xdp_generic() // bpf_net_ctx_set() <-- nested
> ...
> 
> This patch removes the bpf_net_context handling that exists in 
> do_xdp_generic() and modifies it to handle it in the parent function.

Is tun_xdp_one missing? That also calls do_xdp_generic.

On 7/25/24 04:43, Willem de Bruijn wrote:
> Jeongjun Park wrote:
>> In the previous commit, bpf_net_context handling was added to
>> tun_sendmsg() and do_xdp_generic(), but if you write code like this,
>> bpf_net_context overlaps in the call trace below, causing various
>> memory corruptions.
> 
> I'm no expert on this code, but commit 401cb7dae813 that introduced
> bpf_net_ctx_set explicitly states that nested calls are allowed.
> 
> And the function does imply that:
> 
> static inline struct bpf_net_context *bpf_net_ctx_set(struct bpf_net_context *bpf_net_ctx)
> {
>          struct task_struct *tsk = current;
> 
>          if (tsk->bpf_net_context != NULL)
>                  return NULL;
>          bpf_net_ctx->ri.kern_flags = 0;
> 
>          tsk->bpf_net_context = bpf_net_ctx;
>          return bpf_net_ctx;
> }

I agree with Willem, the ctx nesting looks legit generally speaking. 
@Jeongjun: you need to track down more accurately the issue root cause 
and include such info into the commit message.

Skimming over the code I *think* do_xdp_generic() is not cleaning the 
nested context in all the paths before return and that could cause the 
reported issue.

Thanks,

Paolo

Paolo Abeni wrote:
>
> On 7/25/24 04:43, Willem de Bruijn wrote:
> > Jeongjun Park wrote:
> >> In the previous commit, bpf_net_context handling was added to
> >> tun_sendmsg() and do_xdp_generic(), but if you write code like this,
> >> bpf_net_context overlaps in the call trace below, causing various
> >> memory corruptions.
> >
> > I'm no expert on this code, but commit 401cb7dae813 that introduced
> > bpf_net_ctx_set explicitly states that nested calls are allowed.
> >
> > And the function does imply that:
> >
> > static inline struct bpf_net_context *bpf_net_ctx_set(struct bpf_net_context *bpf_net_ctx)
> > {
> >          struct task_struct *tsk = current;
> >
> >          if (tsk->bpf_net_context != NULL)
> >                  return NULL;
> >          bpf_net_ctx->ri.kern_flags = 0;
> >
> >          tsk->bpf_net_context = bpf_net_ctx;
> >          return bpf_net_ctx;
> > }
>
> I agree with Willem, the ctx nesting looks legit generally speaking.
> @Jeongjun: you need to track down more accurately the issue root cause
> and include such info into the commit message.
>
> Skimming over the code I *think* do_xdp_generic() is not cleaning the
> nested context in all the paths before return and that could cause the
> reported issue.

Thanks to your comment, I re-read the code and found the root cause.
I will send a patch for that bug.

Regards,
Jeongjun Park

Willem de Bruijn wrote:
> I'm no expert on this code, but commit 401cb7dae813 that introduced
> bpf_net_ctx_set explicitly states that nested calls are allowed.
>
> And the function does imply that:
>
> static inline struct bpf_net_context *bpf_net_ctx_set(struct bpf_net_context *bpf_net_ctx)
> {
>         struct task_struct *tsk = current;
>
>         if (tsk->bpf_net_context != NULL)
>                 return NULL;
>         bpf_net_ctx->ri.kern_flags = 0;
>
>         tsk->bpf_net_context = bpf_net_ctx;
>         return bpf_net_ctx;
> }

I'm not an expert on this code either. As you said, there is a 
possibility that the bug is not caused by overlapping calls, but various
memory corruptions are occurring due to the handling of bpf_net_context 
in do_xdp_generic. Therefore, it is appropriate to modify it to handle
it in the parent function rather than in do_xdp_generic.

> Is tun_xdp_one missing? That also calls do_xdp_generic.

This is no problem since tun_xdp_one is only called from tun_sendmsg 
and tun_sendmsg already does the bpf_net_context handling.

Regards,
Jeongjun Park.