[Linux-kernel-mentees] [PATCH syzbot net-next] rxrpc: Fix use-after-free write in rxrpc_connect_call()

[Peilin Ye]

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master

From: Hillf Danton <hda...@sina.com>

Put bundle on error to keep its usage count balanced.

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+d57aaf...@syzkaller.appspotmail.com

Tested on:

commit: b652d2a5 Add linux-next specific files for 20200918
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
kernel config: https://syzkaller.appspot.com/x/.config?x=b619002350aa0542
dashboard link: https://syzkaller.appspot.com/bug?extid=d57aaf84dd8a550e6d91
compiler: gcc (GCC) 10.1.0-syz 20200507
patch: https://syzkaller.appspot.com/x/patch.diff?x=124d1e73900000

Note: testing is done by a robot and is best-effort only.

[Dan Carpenter]

The commit message is not clear and I can't immediately verify what
the patch is trying to do. Looking through the git log, I think this
might already be fixed by commit f1b449748760 ("rxrpc: Fix an overget of
the conn bundle when setting up a client conn")

regards,
dan carpenter

[Peilin Ye]

Hi all,

I forgot to Cc: the list in a previous email. Sorry for the noise.

I am not very sure, but syzbot reproduced the bug on ed6d9b02 ("ionic: fix
up debugfs after queue swap", committed 2020-09-14 16:55:54 -0700),
which is ahead of f1b449748760 (committed 2020-09-14 16:18:59 +0100),
see dashboard:
https://syzkaller.appspot.com/bug?id=177d7a2cfdd4dfe796aca877c943fb6799a9a3a9

I myself haven't looked at this issue yet. I saw Hillf Danton sent this
diff in reply to the original syzbot email, so I picked it up and sent
it to syzbot for test. I will take a look at it later.

Thank you,
Peilin Ye

[Hillf Danton]

On Fri, 18 Sep 2020 18:52:16 Dan Carpenter wrote:
>
> The commit message is not clear and I can't immediately verify what
> the patch is trying to do.

Our fault and say sorry to you for it.

You can find the uaf syzbot report at the link below,

https://lore.kernel.org/lkml/0000000000005b...@google.com/

and I sent a diff in reply to it. This is the thread Peilin raised and
asked syzbot to test the diff posted mainly for David to see if I'm
in the right direction.

> Looking through the git log, I think this
> might already be fixed by commit f1b449748760 ("rxrpc: Fix an overget of
> the conn bundle when setting up a client conn")

Thanks for taking a look at it, Dan.

Quite likely if the syzbot report did not cover the commit above.

btw, I'm curious about why the mentees tag is necessary. Are you now taking
some intern job at some office like google.

Hillf

[Dan Carpenter]

On Fri, Sep 18, 2020 at 07:35:39AM -0400, Peilin Ye wrote:
> Hi all,
>
> I forgot to Cc: the list in a previous email. Sorry for the noise.

No no. Not at all.

>
> On Fri, Sep 18, 2020 at 01:50:02PM +0300, Dan Carpenter wrote:
> > The commit message is not clear and I can't immediately verify what
> > the patch is trying to do. Looking through the git log, I think this
> > might already be fixed by commit f1b449748760 ("rxrpc: Fix an overget of
> > the conn bundle when setting up a client conn")
>
> I am not very sure, but syzbot reproduced the bug on ed6d9b02 ("ionic: fix
> up debugfs after queue swap", committed 2020-09-14 16:55:54 -0700),
> which is ahead of f1b449748760 (committed 2020-09-14 16:18:59 +0100),
> see dashboard:
> https://syzkaller.appspot.com/bug?id=177d7a2cfdd4dfe796aca877c943fb6799a9a3a9
>
> I myself haven't looked at this issue yet. I saw Hillf Danton sent this
> diff in reply to the original syzbot email, so I picked it up and sent
> it to syzbot for test. I will take a look at it later.

So the original bug was a double free. The rxrpc_prep_call() takes a
reference to the bundle. Originally the reference was always released
before returning. Now we only release it on the error path.

That will definitely make syzbot happy, but it possibly introduces a
leak that syzbot can't detect. Fortunately, this code is new and simple
enough so David Howells should be able to tell us what he intended here.

That said, I don't think the patch is correct.

net/rxrpc/conn_client.c
696 int rxrpc_connect_call(struct rxrpc_sock *rx,
697 struct rxrpc_call *call,
698 struct rxrpc_conn_parameters *cp,
699 struct sockaddr_rxrpc *srx,
700 gfp_t gfp)
701 {
702 struct rxrpc_bundle *bundle;
703 struct rxrpc_net *rxnet = cp->local->rxnet;
704 int ret = 0;
705
706 _enter("{%d,%lx},", call->debug_id, call->user_call_ID);
707
708 rxrpc_discard_expired_client_conns(&rxnet->client_conn_reaper);
709
710 bundle = rxrpc_prep_call(rx, call, cp, srx, gfp);
711 if (IS_ERR(bundle)) {
712 ret = PTR_ERR(bundle);
713 goto out;
714 }

We've incremented the bundle refcoutn.

715
716 if (call->state == RXRPC_CALL_CLIENT_AWAIT_CONN) {

Assume this condition is false.

717 ret = rxrpc_wait_for_channel(bundle, call, gfp);
718 if (ret < 0)
719 goto wait_failed;
720 }
721
722 granted_channel:
723 /* Paired with the write barrier in rxrpc_activate_one_channel(). */
724 smp_rmb();

That means we are done and we haven't added bundle to any lists so we
should decrement the reference code.

725
726 out_put_bundle:
727 rxrpc_put_bundle(bundle);

But with the patch then we don't decrement it so it seems like a leak.

728 out:
729 _leave(" = %d", ret);
730 return ret;

regards,
dan carpenter

[Peilin Ye]

Ah, I am one of the "Linux Kernel Mentorship Program" mentees. It is my
full-time job from July to September to (try to) fix bugs in the kernel,
and I am supposed to include that tag in the title for every patch I
send during the mentorship.

More information can be found in this email:
https://lore.kernel.org/linux-kernel-mentees/202008211417...@kroah.com/

Thank you,
Peilin Ye

[Peilin Ye]

+ Cc: David Howells <dhow...@redhat.com>

Thank you for the explanation! That makes sense - I'm not familiar with
the code, I'll read the file and think about it.

> 728 out:
> 729 _leave(" = %d", ret);
> 730 return ret;

Peilin Ye

[Peilin Ye]

Hi all,

FWIW, here's a decoded syzbot reproducer:

---
fd = socket(AF_RXRPC, SOCK_DGRAM, PF_INET);

#define RXRPC_EXCLUSIVE_CONNECTION 3 /* Deprecated; use RXRPC_EXCLUSIVE_CALL instead */
setsockopt(fd, SOL_RXRPC, RXRPC_EXCLUSIVE_CONNECTION, NULL, 0);

struct sockaddr_rxrpc srx = {
.srx_family = AF_RXRPC,
.srx_service = 0,
.transport_type = SOCK_DGRAM,
.transport_len = 0x10,
.transport.sin = {
.sin_family = AF_INET,
.sin_port = 0,
.sin_addr = 0x1414ac,
},
};
connect(fd, &srx, sizeof(srx));

struct {
struct cmsghdr hdr;
unsigned long user_call_ID;
} cmsg = {
.hdr = {
.cmsg_len = sizeof(struct cmsghdr) + sizeof(unsigned long),
.cmsg_level = SOL_RXRPC,
.cmsg_type = RXRPC_USER_CALL_ID,
},
.user_call_ID = 0x77,
};
struct mmsghdr msgvec = {
.msg_hdr = {
.msg_control = &cmsg,
.msg_controllen = sizeof(cmsg),
},
};
sendmmsg(fd, &msgvec, 1, 0);

close(fd);
--

Thank you,
Peilin Ye