Syzkaller reports the following failure:
BUG: KASAN: use-after-free in kref_put include/linux/kref.h:64 [inline]
BUG: KASAN: use-after-free in j1939_priv_put+0x25/0xa0 net/can/j1939/main.c:172
Write of size 4 at addr ffff888141c15058 by task swapper/3/0
CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.10.144-syzkaller #0
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
Call Trace:
<IRQ>
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x107/0x167 lib/dump_stack.c:118
print_address_description.constprop.0+0x1c/0x220 mm/kasan/report.c:385
__kasan_report mm/kasan/report.c:545 [inline]
kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
check_memory_region_inline mm/kasan/generic.c:186 [inline]
check_memory_region+0x145/0x190 mm/kasan/generic.c:192
instrument_atomic_read_write include/linux/instrumented.h:101 [inline]
atomic_fetch_sub_release include/asm-generic/atomic-instrumented.h:220 [inline]
__refcount_sub_and_test include/linux/refcount.h:272 [inline]
__refcount_dec_and_test include/linux/refcount.h:315 [inline]
refcount_dec_and_test include/linux/refcount.h:333 [inline]
kref_put include/linux/kref.h:64 [inline]
j1939_priv_put+0x25/0xa0 net/can/j1939/main.c:172
j1939_sk_sock_destruct+0x44/0x90 net/can/j1939/socket.c:374
__sk_destruct+0x4e/0x820 net/core/sock.c:1784
rcu_do_batch kernel/rcu/tree.c:2485 [inline]
rcu_core+0xb35/0x1a30 kernel/rcu/tree.c:2726
__do_softirq+0x289/0x9a3 kernel/softirq.c:298
asm_call_irq_on_stack+0x12/0x20
</IRQ>
__run_on_irqstack arch/x86/include/asm/irq_stack.h:26 [inline]
run_on_irqstack_cond arch/x86/include/asm/irq_stack.h:77 [inline]
do_softirq_own_stack+0xaa/0xe0 arch/x86/kernel/irq_64.c:77
invoke_softirq kernel/softirq.c:393 [inline]
__irq_exit_rcu kernel/softirq.c:423 [inline]
irq_exit_rcu+0x136/0x200 kernel/softirq.c:435
sysvec_apic_timer_interrupt+0x4d/0x100 arch/x86/kernel/apic/apic.c:1095
asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:635
Allocated by task 1141:
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
kasan_set_track mm/kasan/common.c:56 [inline]
__kasan_kmalloc.constprop.0+0xc9/0xd0 mm/kasan/common.c:461
kmalloc include/linux/slab.h:552 [inline]
kzalloc include/linux/slab.h:664 [inline]
j1939_priv_create net/can/j1939/main.c:131 [inline]
j1939_netdev_start+0x111/0x860 net/can/j1939/main.c:268
j1939_sk_bind+0x8ea/0xd30 net/can/j1939/socket.c:485
__sys_bind+0x1f2/0x260 net/socket.c:1645
__do_sys_bind net/socket.c:1656 [inline]
__se_sys_bind net/socket.c:1654 [inline]
__x64_sys_bind+0x6f/0xb0 net/socket.c:1654
do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x61/0xc6
Freed by task 1141:
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
kasan_set_track+0x1c/0x30 mm/kasan/common.c:56
kasan_set_free_info+0x1b/0x30 mm/kasan/generic.c:355
__kasan_slab_free+0x112/0x170 mm/kasan/common.c:422
slab_free_hook mm/slub.c:1542 [inline]
slab_free_freelist_hook+0xad/0x190 mm/slub.c:1576
slab_free mm/slub.c:3149 [inline]
kfree+0xd9/0x3b0 mm/slub.c:4125
j1939_netdev_start+0x5ee/0x860 net/can/j1939/main.c:300
j1939_sk_bind+0x8ea/0xd30 net/can/j1939/socket.c:485
__sys_bind+0x1f2/0x260 net/socket.c:1645
__do_sys_bind net/socket.c:1656 [inline]
__se_sys_bind net/socket.c:1654 [inline]
__x64_sys_bind+0x6f/0xb0 net/socket.c:1654
do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x61/0xc6
It can be caused by this scenario:
CPU0 CPU1
j1939_sk_bind(socket0, ndev0, ...)
j1939_netdev_start()
j1939_sk_bind(socket1, ndev0, ...)
j1939_netdev_start()
mutex_lock(&j1939_netdev_lock)
j1939_priv_set(ndev0, priv)
mutex_unlock(&j1939_netdev_lock)
if (priv_new)
kref_get(&priv_new->rx_kref)
return priv_new;
/* inside j1939_sk_bind() */
jsk->priv = priv
j1939_can_rx_register(priv) // fails
j1939_priv_set(ndev, NULL)
kfree(priv)
j1939_sk_sock_destruct()
j1939_priv_put() // <- uaf
To avoid this, call j1939_can_rx_register() under j1939_netdev_lock so
that a concurrent thread cannot process j1939_priv before
j1939_can_rx_register() returns.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Fedor Pchelkin <[email protected]>
---
net/can/j1939/main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 6ed79afe19a5..ecff1c947d68 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -290,16 +290,18 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
return priv_new;
}
j1939_priv_set(ndev, priv);
- mutex_unlock(&j1939_netdev_lock);
ret = j1939_can_rx_register(priv);
if (ret < 0)
goto out_priv_put;
+ mutex_unlock(&j1939_netdev_lock);
return priv;
out_priv_put:
j1939_priv_set(ndev, NULL);
+ mutex_unlock(&j1939_netdev_lock);
+
dev_put(ndev);
kfree(priv);
--
2.34.1
Hi Fedor,
On Fri, May 26, 2023 at 08:19:10PM +0300, Fedor Pchelkin wrote:
> Syzkaller reports the following failure:
>
> BUG: KASAN: use-after-free in kref_put include/linux/kref.h:64 [inline]
> BUG: KASAN: use-after-free in j1939_priv_put+0x25/0xa0 net/can/j1939/main.c:172
> Write of size 4 at addr ffff888141c15058 by task swapper/3/0
>
> CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.10.144-syzkaller #0
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
> Call Trace:
> <IRQ>
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x107/0x167 lib/dump_stack.c:118
> print_address_description.constprop.0+0x1c/0x220 mm/kasan/report.c:385
> __kasan_report mm/kasan/report.c:545 [inline]
> kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
> check_memory_region_inline mm/kasan/generic.c:186 [inline]
> check_memory_region+0x145/0x190 mm/kasan/generic.c:192
> instrument_atomic_read_write include/linux/instrumented.h:101 [inline]
> atomic_fetch_sub_release include/asm-generic/atomic-instrumented.h:220 [inline]
> __refcount_sub_and_test include/linux/refcount.h:272 [inline]
> __refcount_dec_and_test include/linux/refcount.h:315 [inline]
> refcount_dec_and_test include/linux/refcount.h:333 [inline]
> kref_put include/linux/kref.h:64 [inline]
> j1939_priv_put+0x25/0xa0 net/can/j1939/main.c:172
> j1939_sk_sock_destruct+0x44/0x90 net/can/j1939/socket.c:374
> __sk_destruct+0x4e/0x820 net/core/sock.c:1784
> rcu_do_batch kernel/rcu/tree.c:2485 [inline]
> rcu_core+0xb35/0x1a30 kernel/rcu/tree.c:2726
> __do_softirq+0x289/0x9a3 kernel/softirq.c:298
> asm_call_irq_on_stack+0x12/0x20
> </IRQ>
> __run_on_irqstack arch/x86/include/asm/irq_stack.h:26 [inline]
> run_on_irqstack_cond arch/x86/include/asm/irq_stack.h:77 [inline]
> do_softirq_own_stack+0xaa/0xe0 arch/x86/kernel/irq_64.c:77
> invoke_softirq kernel/softirq.c:393 [inline]
> __irq_exit_rcu kernel/softirq.c:423 [inline]
> irq_exit_rcu+0x136/0x200 kernel/softirq.c:435
> sysvec_apic_timer_interrupt+0x4d/0x100 arch/x86/kernel/apic/apic.c:1095
> asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:635
>
> Allocated by task 1141:
> kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
> kasan_set_track mm/kasan/common.c:56 [inline]
> __kasan_kmalloc.constprop.0+0xc9/0xd0 mm/kasan/common.c:461
> kmalloc include/linux/slab.h:552 [inline]
> kzalloc include/linux/slab.h:664 [inline]
> j1939_priv_create net/can/j1939/main.c:131 [inline]
> j1939_netdev_start+0x111/0x860 net/can/j1939/main.c:268
> j1939_sk_bind+0x8ea/0xd30 net/can/j1939/socket.c:485
> __sys_bind+0x1f2/0x260 net/socket.c:1645
> __do_sys_bind net/socket.c:1656 [inline]
> __se_sys_bind net/socket.c:1654 [inline]
> __x64_sys_bind+0x6f/0xb0 net/socket.c:1654
> do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
> entry_SYSCALL_64_after_hwframe+0x61/0xc6
>
> Freed by task 1141:
> kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
> kasan_set_track+0x1c/0x30 mm/kasan/common.c:56
> kasan_set_free_info+0x1b/0x30 mm/kasan/generic.c:355
> __kasan_slab_free+0x112/0x170 mm/kasan/common.c:422
> slab_free_hook mm/slub.c:1542 [inline]
> slab_free_freelist_hook+0xad/0x190 mm/slub.c:1576
> slab_free mm/slub.c:3149 [inline]
> kfree+0xd9/0x3b0 mm/slub.c:4125
> j1939_netdev_start+0x5ee/0x860 net/can/j1939/main.c:300
> j1939_sk_bind+0x8ea/0xd30 net/can/j1939/socket.c:485
> __sys_bind+0x1f2/0x260 net/socket.c:1645
> __do_sys_bind net/socket.c:1656 [inline]
> __se_sys_bind net/socket.c:1654 [inline]
> __x64_sys_bind+0x6f/0xb0 net/socket.c:1654
> do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
> entry_SYSCALL_64_after_hwframe+0x61/0xc6
>
> It can be caused by this scenario:
>
> CPU0 CPU1
> j1939_sk_bind(socket0, ndev0, ...)
> j1939_netdev_start()
> j1939_sk_bind(socket1, ndev0, ...)
> j1939_netdev_start()
> mutex_lock(&j1939_netdev_lock)
> j1939_priv_set(ndev0, priv)
> mutex_unlock(&j1939_netdev_lock)
> if (priv_new)
> kref_get(&priv_new->rx_kref)
> return priv_new;
> /* inside j1939_sk_bind() */
> jsk->priv = priv
> j1939_can_rx_register(priv) // fails
> j1939_priv_set(ndev, NULL)
> kfree(priv)
> j1939_sk_sock_destruct()
> j1939_priv_put() // <- uaf
>
> To avoid this, call j1939_can_rx_register() under j1939_netdev_lock so
> that a concurrent thread cannot process j1939_priv before
> j1939_can_rx_register() returns.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Signed-off-by: Fedor Pchelkin <[email protected]>
> ---
> net/can/j1939/main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index 6ed79afe19a5..ecff1c947d68 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
> @@ -290,16 +290,18 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
> return priv_new;
> }
> j1939_priv_set(ndev, priv);
> - mutex_unlock(&j1939_netdev_lock);
>
> ret = j1939_can_rx_register(priv);
> if (ret < 0)
> goto out_priv_put;
>
> + mutex_unlock(&j1939_netdev_lock);
> return priv;
>
> out_priv_put:
> j1939_priv_set(ndev, NULL);
> + mutex_unlock(&j1939_netdev_lock);
> +
> dev_put(ndev);
> kfree(priv);
>
> --
> 2.34.1
>
>
Thank you for your investigation. How about this change?
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -285,8 +285,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
*/
kref_get(&priv_new->rx_kref);
spin_unlock(&j1939_netdev_lock);
- dev_put(ndev);
- kfree(priv);
+ j1939_priv_put(priv);
return priv_new;
}
j1939_priv_set(ndev, priv);
@@ -300,8 +299,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
out_priv_put:
j1939_priv_set(ndev, NULL);
- dev_put(ndev);
- kfree(priv);
+ j1939_priv_put(priv);
return ERR_PTR(ret);
}
If I see it correctly, the problem is kfree() which is called without respecting
the ref counting. If CPU1 has priv_new, refcounting is increased. The priv will
not be freed on this place.
Can you please test it?
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Oleksij,
thanks for the reply!
On Fri, May 26, 2023 at 08:15:00PM +0200, Oleksij Rempel wrote:
> Hi Fedor,
>
> On Fri, May 26, 2023 at 08:19:10PM +0300, Fedor Pchelkin wrote:
>
>
> Thank you for your investigation. How about this change?
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
> @@ -285,8 +285,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
> */
> kref_get(&priv_new->rx_kref);
> spin_unlock(&j1939_netdev_lock);
> - dev_put(ndev);
> - kfree(priv);
> + j1939_priv_put(priv);
I don't think that's good because the priv which is directly freed here is
still local to the thread, and parallel threads don't have any access to
it. j1939_priv_create() has allocated a fresh priv and called dev_hold()
so dev_put() and kfree() here are okay.
> return priv_new;
> }
> j1939_priv_set(ndev, priv);
> @@ -300,8 +299,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
>
> out_priv_put:
> j1939_priv_set(ndev, NULL);
> - dev_put(ndev);
> - kfree(priv);
> + j1939_priv_put(priv);
>
> return ERR_PTR(ret);
> }
>
> If I see it correctly, the problem is kfree() which is called without respecting
> the ref counting. If CPU1 has priv_new, refcounting is increased. The priv will
> not be freed on this place.
With your suggestion, I think it doesn't work correctly if
j1939_can_rx_register() fails and we go to out_priv_put. The priv is kept
but the parallel thread which may have already grabbed it thinks that
j1939_can_rx_register() has succeeded when actually it hasn't succeed.
Moreover, j1939_priv_set() makes it NULL on error path so that priv cannot
be accessed from ndev.
I also considered the alternatives where we don't have to serialize access
to j1939_can_rx_register() and subsequently introduce mutex. But with
current j1939_netdev_start() implementation I can't see how to fix the
racy bug without it.
>
> Can you please test it?
>
> Regards,
> Oleksij
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Fedor,
On Fri, May 26, 2023 at 09:50:26PM +0300, Fedor Pchelkin wrote:
> Hi Oleksij,
>
> thanks for the reply!
>
> On Fri, May 26, 2023 at 08:15:00PM +0200, Oleksij Rempel wrote:
> > Hi Fedor,
> >
> > On Fri, May 26, 2023 at 08:19:10PM +0300, Fedor Pchelkin wrote:
> >
> >
> > Thank you for your investigation. How about this change?
> > --- a/net/can/j1939/main.c
> > +++ b/net/can/j1939/main.c
> > @@ -285,8 +285,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
> > */
> > kref_get(&priv_new->rx_kref);
> > spin_unlock(&j1939_netdev_lock);
> > - dev_put(ndev);
> > - kfree(priv);
> > + j1939_priv_put(priv);
>
> I don't think that's good because the priv which is directly freed here is
> still local to the thread, and parallel threads don't have any access to
> it. j1939_priv_create() has allocated a fresh priv and called dev_hold()
> so dev_put() and kfree() here are okay.
>
> > return priv_new;
> > }
> > j1939_priv_set(ndev, priv);
> > @@ -300,8 +299,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
> >
> > out_priv_put:
> > j1939_priv_set(ndev, NULL);
> > - dev_put(ndev);
> > - kfree(priv);
> > + j1939_priv_put(priv);
> >
> > return ERR_PTR(ret);
> > }
> >
> > If I see it correctly, the problem is kfree() which is called without respecting
> > the ref counting. If CPU1 has priv_new, refcounting is increased. The priv will
> > not be freed on this place.
>
> With your suggestion, I think it doesn't work correctly if
> j1939_can_rx_register() fails and we go to out_priv_put. The priv is kept
> but the parallel thread which may have already grabbed it thinks that
> j1939_can_rx_register() has succeeded when actually it hasn't succeed.
> Moreover, j1939_priv_set() makes it NULL on error path so that priv cannot
> be accessed from ndev.
>
> I also considered the alternatives where we don't have to serialize access
> to j1939_can_rx_register() and subsequently introduce mutex. But with
> current j1939_netdev_start() implementation I can't see how to fix the
> racy bug without it.
Ok, it make sense.
I'll try to do some testing next week. If i'll forget it, please feel
free to ping me.
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Sat, May 27, 2023 at 07:57:04AM +0200, Oleksij Rempel wrote:
> Hi Fedor,
>
> On Fri, May 26, 2023 at 09:50:26PM +0300, Fedor Pchelkin wrote:
> > Hi Oleksij,
> >
> > thanks for the reply!
> >
> > On Fri, May 26, 2023 at 08:15:00PM +0200, Oleksij Rempel wrote:
> > > Hi Fedor,
> > >
> > > On Fri, May 26, 2023 at 08:19:10PM +0300, Fedor Pchelkin wrote:
> > >
> > >
> > > Thank you for your investigation. How about this change?
> > > --- a/net/can/j1939/main.c
> > > +++ b/net/can/j1939/main.c
> > > @@ -285,8 +285,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
> > > */
> > > kref_get(&priv_new->rx_kref);
> > > spin_unlock(&j1939_netdev_lock);
> > > - dev_put(ndev);
> > > - kfree(priv);
> > > + j1939_priv_put(priv);
> >
> > I don't think that's good because the priv which is directly freed here is
> > still local to the thread, and parallel threads don't have any access to
> > it. j1939_priv_create() has allocated a fresh priv and called dev_hold()
> > so dev_put() and kfree() here are okay.
> >
> > > return priv_new;
> > > }
> > > j1939_priv_set(ndev, priv);
> > > @@ -300,8 +299,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
> > >
> > > out_priv_put:
> > > j1939_priv_set(ndev, NULL);
> > > - dev_put(ndev);
> > > - kfree(priv);
> > > + j1939_priv_put(priv);
> > >
> > > return ERR_PTR(ret);
> > > }
> > >
> > > If I see it correctly, the problem is kfree() which is called without respecting
> > > the ref counting. If CPU1 has priv_new, refcounting is increased. The priv will
> > > not be freed on this place.
> >
> > With your suggestion, I think it doesn't work correctly if
> > j1939_can_rx_register() fails and we go to out_priv_put. The priv is kept
> > but the parallel thread which may have already grabbed it thinks that
> > j1939_can_rx_register() has succeeded when actually it hasn't succeed.
> > Moreover, j1939_priv_set() makes it NULL on error path so that priv cannot
> > be accessed from ndev.
> >
> > I also considered the alternatives where we don't have to serialize access
> > to j1939_can_rx_register() and subsequently introduce mutex. But with
> > current j1939_netdev_start() implementation I can't see how to fix the
> > racy bug without it.
>
> Ok, it make sense.
>
> I'll try to do some testing next week. If i'll forget it, please feel
> free to ping me.
Got it, thank you.
>
> Regards,
> Oleksij
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Fri, May 26, 2023 at 08:19:10PM +0300, Fedor Pchelkin wrote:
> Syzkaller reports the following failure:
>
> BUG: KASAN: use-after-free in kref_put include/linux/kref.h:64 [inline]
> BUG: KASAN: use-after-free in j1939_priv_put+0x25/0xa0 net/can/j1939/main.c:172
> Write of size 4 at addr ffff888141c15058 by task swapper/3/0
>
> CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.10.144-syzkaller #0
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
> Call Trace:
> <IRQ>
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x107/0x167 lib/dump_stack.c:118
> print_address_description.constprop.0+0x1c/0x220 mm/kasan/report.c:385
> __kasan_report mm/kasan/report.c:545 [inline]
> kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
> check_memory_region_inline mm/kasan/generic.c:186 [inline]
> check_memory_region+0x145/0x190 mm/kasan/generic.c:192
> instrument_atomic_read_write include/linux/instrumented.h:101 [inline]
> atomic_fetch_sub_release include/asm-generic/atomic-instrumented.h:220 [inline]
> __refcount_sub_and_test include/linux/refcount.h:272 [inline]
> __refcount_dec_and_test include/linux/refcount.h:315 [inline]
> refcount_dec_and_test include/linux/refcount.h:333 [inline]
> kref_put include/linux/kref.h:64 [inline]
> j1939_priv_put+0x25/0xa0 net/can/j1939/main.c:172
> j1939_sk_sock_destruct+0x44/0x90 net/can/j1939/socket.c:374
> __sk_destruct+0x4e/0x820 net/core/sock.c:1784
> rcu_do_batch kernel/rcu/tree.c:2485 [inline]
> rcu_core+0xb35/0x1a30 kernel/rcu/tree.c:2726
> __do_softirq+0x289/0x9a3 kernel/softirq.c:298
> asm_call_irq_on_stack+0x12/0x20
> </IRQ>
> __run_on_irqstack arch/x86/include/asm/irq_stack.h:26 [inline]
> run_on_irqstack_cond arch/x86/include/asm/irq_stack.h:77 [inline]
> do_softirq_own_stack+0xaa/0xe0 arch/x86/kernel/irq_64.c:77
> invoke_softirq kernel/softirq.c:393 [inline]
> __irq_exit_rcu kernel/softirq.c:423 [inline]
> irq_exit_rcu+0x136/0x200 kernel/softirq.c:435
> sysvec_apic_timer_interrupt+0x4d/0x100 arch/x86/kernel/apic/apic.c:1095
> asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:635
>
> Allocated by task 1141:
> kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
> kasan_set_track mm/kasan/common.c:56 [inline]
> __kasan_kmalloc.constprop.0+0xc9/0xd0 mm/kasan/common.c:461
> kmalloc include/linux/slab.h:552 [inline]
> kzalloc include/linux/slab.h:664 [inline]
> j1939_priv_create net/can/j1939/main.c:131 [inline]
> j1939_netdev_start+0x111/0x860 net/can/j1939/main.c:268
> j1939_sk_bind+0x8ea/0xd30 net/can/j1939/socket.c:485
> __sys_bind+0x1f2/0x260 net/socket.c:1645
> __do_sys_bind net/socket.c:1656 [inline]
> __se_sys_bind net/socket.c:1654 [inline]
> __x64_sys_bind+0x6f/0xb0 net/socket.c:1654
> do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
> entry_SYSCALL_64_after_hwframe+0x61/0xc6
>
> Freed by task 1141:
> kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
> kasan_set_track+0x1c/0x30 mm/kasan/common.c:56
> kasan_set_free_info+0x1b/0x30 mm/kasan/generic.c:355
> __kasan_slab_free+0x112/0x170 mm/kasan/common.c:422
> slab_free_hook mm/slub.c:1542 [inline]
> slab_free_freelist_hook+0xad/0x190 mm/slub.c:1576
> slab_free mm/slub.c:3149 [inline]
> kfree+0xd9/0x3b0 mm/slub.c:4125
> j1939_netdev_start+0x5ee/0x860 net/can/j1939/main.c:300
> j1939_sk_bind+0x8ea/0xd30 net/can/j1939/socket.c:485
> __sys_bind+0x1f2/0x260 net/socket.c:1645
> __do_sys_bind net/socket.c:1656 [inline]
> __se_sys_bind net/socket.c:1654 [inline]
> __x64_sys_bind+0x6f/0xb0 net/socket.c:1654
> do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
> entry_SYSCALL_64_after_hwframe+0x61/0xc6
>
> It can be caused by this scenario:
>
> CPU0 CPU1
> j1939_sk_bind(socket0, ndev0, ...)
> j1939_netdev_start()
> j1939_sk_bind(socket1, ndev0, ...)
> j1939_netdev_start()
> mutex_lock(&j1939_netdev_lock)
> j1939_priv_set(ndev0, priv)
> mutex_unlock(&j1939_netdev_lock)
> if (priv_new)
> kref_get(&priv_new->rx_kref)
> return priv_new;
> /* inside j1939_sk_bind() */
> jsk->priv = priv
> j1939_can_rx_register(priv) // fails
> j1939_priv_set(ndev, NULL)
> kfree(priv)
> j1939_sk_sock_destruct()
> j1939_priv_put() // <- uaf
>
> To avoid this, call j1939_can_rx_register() under j1939_netdev_lock so
> that a concurrent thread cannot process j1939_priv before
> j1939_can_rx_register() returns.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Signed-off-by: Fedor Pchelkin <[email protected]>
Tested-by: Oleksij Rempel <[email protected]>
Acked-by: Oleksij Rempel <[email protected]>
Thank you!
> ---
> net/can/j1939/main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index 6ed79afe19a5..ecff1c947d68 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
> @@ -290,16 +290,18 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
> return priv_new;
> }
> j1939_priv_set(ndev, priv);
> - mutex_unlock(&j1939_netdev_lock);
>
> ret = j1939_can_rx_register(priv);
> if (ret < 0)
> goto out_priv_put;
>
> + mutex_unlock(&j1939_netdev_lock);
> return priv;
>
> out_priv_put:
> j1939_priv_set(ndev, NULL);
> + mutex_unlock(&j1939_netdev_lock);
> +
> dev_put(ndev);
> kfree(priv);
>
> --
> 2.34.1
>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Fri, Jun 02, 2023 at 02:35:19PM +0200, Oleksij Rempel wrote:
> On Fri, May 26, 2023 at 08:19:10PM +0300, Fedor Pchelkin wrote:
> > Syzkaller reports the following failure:
> >
> > BUG: KASAN: use-after-free in kref_put include/linux/kref.h:64 [inline]
> > BUG: KASAN: use-after-free in j1939_priv_put+0x25/0xa0 net/can/j1939/main.c:172
> > Write of size 4 at addr ffff888141c15058 by task swapper/3/0
> >
> > CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.10.144-syzkaller #0
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
> > Call Trace:
> > <IRQ>
> > __dump_stack lib/dump_stack.c:77 [inline]
> > dump_stack+0x107/0x167 lib/dump_stack.c:118
> > print_address_description.constprop.0+0x1c/0x220 mm/kasan/report.c:385
> > __kasan_report mm/kasan/report.c:545 [inline]
> > kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
> > check_memory_region_inline mm/kasan/generic.c:186 [inline]
> > check_memory_region+0x145/0x190 mm/kasan/generic.c:192
> > instrument_atomic_read_write include/linux/instrumented.h:101 [inline]
> > atomic_fetch_sub_release include/asm-generic/atomic-instrumented.h:220 [inline]
> > __refcount_sub_and_test include/linux/refcount.h:272 [inline]
> > __refcount_dec_and_test include/linux/refcount.h:315 [inline]
> > refcount_dec_and_test include/linux/refcount.h:333 [inline]
> > kref_put include/linux/kref.h:64 [inline]
> > j1939_priv_put+0x25/0xa0 net/can/j1939/main.c:172
> > j1939_sk_sock_destruct+0x44/0x90 net/can/j1939/socket.c:374
> > __sk_destruct+0x4e/0x820 net/core/sock.c:1784
> > rcu_do_batch kernel/rcu/tree.c:2485 [inline]
> > rcu_core+0xb35/0x1a30 kernel/rcu/tree.c:2726
> > __do_softirq+0x289/0x9a3 kernel/softirq.c:298
> > asm_call_irq_on_stack+0x12/0x20
> > </IRQ>
> > __run_on_irqstack arch/x86/include/asm/irq_stack.h:26 [inline]
> > run_on_irqstack_cond arch/x86/include/asm/irq_stack.h:77 [inline]
> > do_softirq_own_stack+0xaa/0xe0 arch/x86/kernel/irq_64.c:77
> > invoke_softirq kernel/softirq.c:393 [inline]
> > __irq_exit_rcu kernel/softirq.c:423 [inline]
> > irq_exit_rcu+0x136/0x200 kernel/softirq.c:435
> > sysvec_apic_timer_interrupt+0x4d/0x100 arch/x86/kernel/apic/apic.c:1095
> > asm_sysvec_apic_timer_interrupt+0x12/0x20 arch/x86/include/asm/idtentry.h:635
> >
> > Allocated by task 1141:
> > kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
> > kasan_set_track mm/kasan/common.c:56 [inline]
> > __kasan_kmalloc.constprop.0+0xc9/0xd0 mm/kasan/common.c:461
> > kmalloc include/linux/slab.h:552 [inline]
> > kzalloc include/linux/slab.h:664 [inline]
> > j1939_priv_create net/can/j1939/main.c:131 [inline]
> > j1939_netdev_start+0x111/0x860 net/can/j1939/main.c:268
> > j1939_sk_bind+0x8ea/0xd30 net/can/j1939/socket.c:485
> > __sys_bind+0x1f2/0x260 net/socket.c:1645
> > __do_sys_bind net/socket.c:1656 [inline]
> > __se_sys_bind net/socket.c:1654 [inline]
> > __x64_sys_bind+0x6f/0xb0 net/socket.c:1654
> > do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
> > entry_SYSCALL_64_after_hwframe+0x61/0xc6
> >
> > Freed by task 1141:
> > kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
> > kasan_set_track+0x1c/0x30 mm/kasan/common.c:56
> > kasan_set_free_info+0x1b/0x30 mm/kasan/generic.c:355
> > __kasan_slab_free+0x112/0x170 mm/kasan/common.c:422
> > slab_free_hook mm/slub.c:1542 [inline]
> > slab_free_freelist_hook+0xad/0x190 mm/slub.c:1576
> > slab_free mm/slub.c:3149 [inline]
> > kfree+0xd9/0x3b0 mm/slub.c:4125
> > j1939_netdev_start+0x5ee/0x860 net/can/j1939/main.c:300
> > j1939_sk_bind+0x8ea/0xd30 net/can/j1939/socket.c:485
> > __sys_bind+0x1f2/0x260 net/socket.c:1645
> > __do_sys_bind net/socket.c:1656 [inline]
> > __se_sys_bind net/socket.c:1654 [inline]
> > __x64_sys_bind+0x6f/0xb0 net/socket.c:1654
> > do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46
> > entry_SYSCALL_64_after_hwframe+0x61/0xc6
> >
> > It can be caused by this scenario:
> >
> > CPU0 CPU1
> > j1939_sk_bind(socket0, ndev0, ...)
> > j1939_netdev_start()
> > j1939_sk_bind(socket1, ndev0, ...)
> > j1939_netdev_start()
> > mutex_lock(&j1939_netdev_lock)
> > j1939_priv_set(ndev0, priv)
> > mutex_unlock(&j1939_netdev_lock)
> > if (priv_new)
> > kref_get(&priv_new->rx_kref)
> > return priv_new;
> > /* inside j1939_sk_bind() */
> > jsk->priv = priv
> > j1939_can_rx_register(priv) // fails
> > j1939_priv_set(ndev, NULL)
> > kfree(priv)
> > j1939_sk_sock_destruct()
> > j1939_priv_put() // <- uaf
> >
> > To avoid this, call j1939_can_rx_register() under j1939_netdev_lock so
> > that a concurrent thread cannot process j1939_priv before
> > j1939_can_rx_register() returns.
> >
> > Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> >
> > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> > Signed-off-by: Fedor Pchelkin <[email protected]>
>
> Tested-by: Oleksij Rempel <[email protected]>
> Acked-by: Oleksij Rempel <[email protected]>
>
> Thank you!
>
Great!
Thanks for testing the patches!