[Patch net] tipc: fix the skb_unshare() in tipc_buf_append()

[Patch net] tipc: fix the skb_unshare() in tipc_buf_append()

[Cong Wang]

skb_unshare() drops a reference count on the old skb unconditionally,
so in the failure case, we end up freeing the skb twice here.
And because the skb is allocated in fclone and cloned by caller
tipc_msg_reassemble(), the consequence is actually freeing the
original skb too, thus triggered the UAF by syzbot.

Fix this by replacing this skb_unshare() with skb_cloned()+skb_copy().

Fixes: ff48b6222e65 ("tipc: use skb_unshare() instead in tipc_buf_append()")
Reported-and-tested-by: syzbot+e96a7ba46281824cc46a@syzkaller.appspotmail.com
Cc: Xin Long <lucien.xin@gmail.com>
Cc: Jon Maloy <jmaloy@redhat.com>
Cc: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/tipc/msg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 52e93ba4d8e2..681224401871 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -150,7 +150,8 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
 	if (fragid == FIRST_FRAGMENT) {
 		if (unlikely(head))
 			goto err;
-		frag = skb_unshare(frag, GFP_ATOMIC);
+		if (skb_cloned(frag))
+			frag = skb_copy(frag, GFP_ATOMIC);
 		if (unlikely(!frag))
 			goto err;
 		head = *headbuf = frag;
-- 
2.28.0

Re: [Patch net] tipc: fix the skb_unshare() in tipc_buf_append()

[Xin Long]

On Thu, Oct 8, 2020 at 12:12 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> skb_unshare() drops a reference count on the old skb unconditionally,
> so in the failure case, we end up freeing the skb twice here.
> And because the skb is allocated in fclone and cloned by caller
> tipc_msg_reassemble(), the consequence is actually freeing the
> original skb too, thus triggered the UAF by syzbot.
Do you mean:
                frag = skb_clone(skb, GFP_ATOMIC);
frag = skb_unshare(frag) will free the 'skb' too?

>
> Fix this by replacing this skb_unshare() with skb_cloned()+skb_copy().
>
> Fixes: ff48b6222e65 ("tipc: use skb_unshare() instead in tipc_buf_append()")
> Reported-and-tested-by: syzbot+e96a7ba46281824cc46a@syzkaller.appspotmail.com
> Cc: Xin Long <lucien.xin@gmail.com>
> Cc: Jon Maloy <jmaloy@redhat.com>
> Cc: Ying Xue <ying.xue@windriver.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/tipc/msg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 52e93ba4d8e2..681224401871 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -150,7 +150,8 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
>         if (fragid == FIRST_FRAGMENT) {
>                 if (unlikely(head))
>                         goto err;
> -               frag = skb_unshare(frag, GFP_ATOMIC);
> +               if (skb_cloned(frag))
> +                       frag = skb_copy(frag, GFP_ATOMIC);
>                 if (unlikely(!frag))
>                         goto err;
>                 head = *headbuf = frag;
> --
> 2.28.0
>

Re: [Patch net] tipc: fix the skb_unshare() in tipc_buf_append()

[Cong Wang]

On Thu, Oct 8, 2020 at 1:45 AM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Thu, Oct 8, 2020 at 12:12 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > skb_unshare() drops a reference count on the old skb unconditionally,
> > so in the failure case, we end up freeing the skb twice here.
> > And because the skb is allocated in fclone and cloned by caller
> > tipc_msg_reassemble(), the consequence is actually freeing the
> > original skb too, thus triggered the UAF by syzbot.
> Do you mean:
>                 frag = skb_clone(skb, GFP_ATOMIC);
> frag = skb_unshare(frag) will free the 'skb' too?

Yes, more precisely, I mean:

new = skb_clone(old)
kfree_skb(new)
kfree_skb(new)

would free 'old' eventually when 'old' is a fast clone. The skb_clone()
sets ->fclone_ref to 2 and returns the clone, whose skb->fclone is
SKB_FCLONE_CLONE. So, the first call of kfree_skbmem() will
just decrease ->fclone_ref by 1, but the second call will trigger
kmem_cache_free() which frees _both_  skb's.

Thanks.

Re: [Patch net] tipc: fix the skb_unshare() in tipc_buf_append()

[Xin Long]

On Fri, Oct 9, 2020 at 1:45 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Thu, Oct 8, 2020 at 1:45 AM Xin Long <lucien.xin@gmail.com> wrote:
> >
> > On Thu, Oct 8, 2020 at 12:12 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > skb_unshare() drops a reference count on the old skb unconditionally,
> > > so in the failure case, we end up freeing the skb twice here.
> > > And because the skb is allocated in fclone and cloned by caller
> > > tipc_msg_reassemble(), the consequence is actually freeing the
> > > original skb too, thus triggered the UAF by syzbot.
> > Do you mean:
> >                 frag = skb_clone(skb, GFP_ATOMIC);
> > frag = skb_unshare(frag) will free the 'skb' too?
>
> Yes, more precisely, I mean:
>
> new = skb_clone(old)
> kfree_skb(new)
> kfree_skb(new)
>
> would free 'old' eventually when 'old' is a fast clone. The skb_clone()
> sets ->fclone_ref to 2 and returns the clone, whose skb->fclone is
> SKB_FCLONE_CLONE. So, the first call of kfree_skbmem() will
> just decrease ->fclone_ref by 1, but the second call will trigger
> kmem_cache_free() which frees _both_  skb's.
Thanks. Didn't notice kfree_skb 'buf' on the err path.

Reviewed-by: Xin Long <lucien.xin@gmail.com>

Re: [Patch net] tipc: fix the skb_unshare() in tipc_buf_append()

[Jakub Kicinski]

On Wed,  7 Oct 2020 21:12:50 -0700 Cong Wang wrote:
> skb_unshare() drops a reference count on the old skb unconditionally,
> so in the failure case, we end up freeing the skb twice here.
> And because the skb is allocated in fclone and cloned by caller
> tipc_msg_reassemble(), the consequence is actually freeing the
> original skb too, thus triggered the UAF by syzbot.
> 
> Fix this by replacing this skb_unshare() with skb_cloned()+skb_copy().
> 
> Fixes: ff48b6222e65 ("tipc: use skb_unshare() instead in tipc_buf_append()")
> Reported-and-tested-by: syzbot+e96a7ba46281824cc46a@syzkaller.appspotmail.com
> Cc: Xin Long <lucien.xin@gmail.com>
> Cc: Jon Maloy <jmaloy@redhat.com>
> Cc: Ying Xue <ying.xue@windriver.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued for stable, thank you!