[Linux-kernel-mentees] [PATCH] net/rose: Fix NULL pointer dereference in rose_send_frame()

[Peilin Ye]

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

[syzbot]

Hello,

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

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

Tested on:

commit: 15bc20c6 Merge tag 'tty-5.9-rc3' of git://git.kernel.org/p..
git tree: upstream
kernel config: https://syzkaller.appspot.com/x/.config?x=39ee3d48e855d05c
dashboard link: https://syzkaller.appspot.com/bug?extid=7078ae989d857fe17988
compiler: gcc (GCC) 10.1.0-syz 20200507
patch: https://syzkaller.appspot.com/x/patch.diff?x=16b0f669900000

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

[Peilin Ye]

1) Creates a socket().

2) Does an `SIOCSIFFLAGS` ("set interface flags", 0x8914) ioctl() on that
socket, with `ifr->ifr_name` set to "rose0" and `ifr->ifr_flags` set
to 0x201 (`IFF_UP` and `IFF_ALLMULTI`).

3) A connect() to that socket. This connect() triggers the
"autobinding", see rose_connect():

(v5.9-rc3 net/rose/af_rose.c:779)
if (sock_flag(sk, SOCK_ZAPPED)) { /* Must bind first - autobinding in this may or may not work */
sock_reset_flag(sk, SOCK_ZAPPED);

if ((dev = rose_dev_first()) == NULL) {
err = -ENETUNREACH;
goto out_release;
}

user = ax25_findbyuid(current_euid());
if (!user) {
err = -EINVAL;
goto out_release;
}

then it errors out since `user` is NULL.

4) Another connect() to the same socket...This one jumps over the
"autobinding" part and make its way all down to the for-loop:

(v5.9-rc3 net/rose/af_rose.c:837)
if (sk->sk_state == TCP_SYN_SENT) {
DEFINE_WAIT(wait);

for (;;) {
prepare_to_wait(sk_sleep(sk), &wait,
TASK_INTERRUPTIBLE);
if (sk->sk_state != TCP_SYN_SENT)
break;
if (!signal_pending(current)) {
release_sock(sk);
schedule();
lock_sock(sk);
continue;
}
err = -ERESTARTSYS;
break;
}
finish_wait(sk_sleep(sk), &wait);

if (err)
goto out_release;
}

Finally it times out. In rose_loopback_timer():

(v5.9-rc3 net/rose/rose_loopback.c:98)

if (frametype == ROSE_CALL_REQUEST) {
if ((dev = rose_dev_get(dest)) != NULL) {
if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
^^^^^^^^^^^^^^^^^^^^
kfree_skb(skb);
} else {
kfree_skb(skb);
}
} else {
kfree_skb(skb);
}

rose_loopback_timer() calls rose_rx_call_request() calls
rose_transmit_clear_request() calls rose_send_frame(), which
dereferences `rose_loopback_neigh->dev`.

However, initialized in rose_add_loopback_neigh(),
`rose_loopback_neigh->dev` is NULL:

(v5.9-rc3 net/rose/rose_route.c:369)
void rose_add_loopback_neigh(void)
{
struct rose_neigh *sn;

rose_loopback_neigh = kmalloc(sizeof(struct rose_neigh), GFP_KERNEL);
if (!rose_loopback_neigh)
return;
sn = rose_loopback_neigh;

sn->callsign = null_ax25_address;
sn->digipeat = NULL;
sn->ax25 = NULL;
sn->dev = NULL;
^^^^^^^ ^^^^

I couldn't figure out how to fix it properly, and why does
rose_loopback_timer() call rose_rx_call_request() with
`rose_loopback_neigh` even if its `dev` is NULL, but basically this is
what the reproducer is doing.

Thank you,
Peilin Ye