| Message ID | 20230526171910.227615-2-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: j1939: avoid possible use-after-free when j1939_can_rx_register fails | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On Fri, May 26, 2023 at 08:19:09PM +0300, Fedor Pchelkin wrote: > It turns out access to j1939_can_rx_register() needs to be serialized, > otherwise j1939_priv can be corrupted when parallel threads call > j1939_netdev_start() and j1939_can_rx_register() fails. This issue is > thoroughly covered in other commit which serializes access to > j1939_can_rx_register(). > > Change j1939_netdev_lock type to mutex so that we do not need to remove > GFP_KERNEL from can_rx_register(). > > j1939_netdev_lock seems to be used in normal contexts where mutex usage > is not prohibited. > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. > > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") > Suggested-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> Tested-by: Oleksij Rempel <o.rempel@pengutronix.de> Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> Thank you! > --- > Note that it has been only tested via Syzkaller and not with real > hardware. > > net/can/j1939/main.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c > index 821d4ff303b3..6ed79afe19a5 100644 > --- a/net/can/j1939/main.c > +++ b/net/can/j1939/main.c > @@ -126,7 +126,7 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data) > #define J1939_CAN_ID CAN_EFF_FLAG > #define J1939_CAN_MASK (CAN_EFF_FLAG | CAN_RTR_FLAG) > > -static DEFINE_SPINLOCK(j1939_netdev_lock); > +static DEFINE_MUTEX(j1939_netdev_lock); > > static struct j1939_priv *j1939_priv_create(struct net_device *ndev) > { > @@ -220,7 +220,7 @@ static void __j1939_rx_release(struct kref *kref) > j1939_can_rx_unregister(priv); > j1939_ecu_unmap_all(priv); > j1939_priv_set(priv->ndev, NULL); > - spin_unlock(&j1939_netdev_lock); > + mutex_unlock(&j1939_netdev_lock); > } > > /* get pointer to priv without increasing ref counter */ > @@ -248,9 +248,9 @@ static struct j1939_priv *j1939_priv_get_by_ndev(struct net_device *ndev) > { > struct j1939_priv *priv; > > - spin_lock(&j1939_netdev_lock); > + mutex_lock(&j1939_netdev_lock); > priv = j1939_priv_get_by_ndev_locked(ndev); > - spin_unlock(&j1939_netdev_lock); > + mutex_unlock(&j1939_netdev_lock); > > return priv; > } > @@ -260,14 +260,14 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev) > struct j1939_priv *priv, *priv_new; > int ret; > > - spin_lock(&j1939_netdev_lock); > + mutex_lock(&j1939_netdev_lock); > priv = j1939_priv_get_by_ndev_locked(ndev); > if (priv) { > kref_get(&priv->rx_kref); > - spin_unlock(&j1939_netdev_lock); > + mutex_unlock(&j1939_netdev_lock); > return priv; > } > - spin_unlock(&j1939_netdev_lock); > + mutex_unlock(&j1939_netdev_lock); > > priv = j1939_priv_create(ndev); > if (!priv) > @@ -277,20 +277,20 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev) > spin_lock_init(&priv->j1939_socks_lock); > INIT_LIST_HEAD(&priv->j1939_socks); > > - spin_lock(&j1939_netdev_lock); > + mutex_lock(&j1939_netdev_lock); > priv_new = j1939_priv_get_by_ndev_locked(ndev); > if (priv_new) { > /* Someone was faster than us, use their priv and roll > * back our's. > */ > kref_get(&priv_new->rx_kref); > - spin_unlock(&j1939_netdev_lock); > + mutex_unlock(&j1939_netdev_lock); > dev_put(ndev); > kfree(priv); > return priv_new; > } > j1939_priv_set(ndev, priv); > - spin_unlock(&j1939_netdev_lock); > + mutex_unlock(&j1939_netdev_lock); > > ret = j1939_can_rx_register(priv); > if (ret < 0) > @@ -308,7 +308,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev) > > void j1939_netdev_stop(struct j1939_priv *priv) > { > - kref_put_lock(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock); > + kref_put_mutex(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock); > j1939_priv_put(priv); > } > > -- > 2.34.1 > > >
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c index 821d4ff303b3..6ed79afe19a5 100644 --- a/net/can/j1939/main.c +++ b/net/can/j1939/main.c @@ -126,7 +126,7 @@ static void j1939_can_recv(struct sk_buff *iskb, void *data) #define J1939_CAN_ID CAN_EFF_FLAG #define J1939_CAN_MASK (CAN_EFF_FLAG | CAN_RTR_FLAG) -static DEFINE_SPINLOCK(j1939_netdev_lock); +static DEFINE_MUTEX(j1939_netdev_lock); static struct j1939_priv *j1939_priv_create(struct net_device *ndev) { @@ -220,7 +220,7 @@ static void __j1939_rx_release(struct kref *kref) j1939_can_rx_unregister(priv); j1939_ecu_unmap_all(priv); j1939_priv_set(priv->ndev, NULL); - spin_unlock(&j1939_netdev_lock); + mutex_unlock(&j1939_netdev_lock); } /* get pointer to priv without increasing ref counter */ @@ -248,9 +248,9 @@ static struct j1939_priv *j1939_priv_get_by_ndev(struct net_device *ndev) { struct j1939_priv *priv; - spin_lock(&j1939_netdev_lock); + mutex_lock(&j1939_netdev_lock); priv = j1939_priv_get_by_ndev_locked(ndev); - spin_unlock(&j1939_netdev_lock); + mutex_unlock(&j1939_netdev_lock); return priv; } @@ -260,14 +260,14 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev) struct j1939_priv *priv, *priv_new; int ret; - spin_lock(&j1939_netdev_lock); + mutex_lock(&j1939_netdev_lock); priv = j1939_priv_get_by_ndev_locked(ndev); if (priv) { kref_get(&priv->rx_kref); - spin_unlock(&j1939_netdev_lock); + mutex_unlock(&j1939_netdev_lock); return priv; } - spin_unlock(&j1939_netdev_lock); + mutex_unlock(&j1939_netdev_lock); priv = j1939_priv_create(ndev); if (!priv) @@ -277,20 +277,20 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev) spin_lock_init(&priv->j1939_socks_lock); INIT_LIST_HEAD(&priv->j1939_socks); - spin_lock(&j1939_netdev_lock); + mutex_lock(&j1939_netdev_lock); priv_new = j1939_priv_get_by_ndev_locked(ndev); if (priv_new) { /* Someone was faster than us, use their priv and roll * back our's. */ kref_get(&priv_new->rx_kref); - spin_unlock(&j1939_netdev_lock); + mutex_unlock(&j1939_netdev_lock); dev_put(ndev); kfree(priv); return priv_new; } j1939_priv_set(ndev, priv); - spin_unlock(&j1939_netdev_lock); + mutex_unlock(&j1939_netdev_lock); ret = j1939_can_rx_register(priv); if (ret < 0) @@ -308,7 +308,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev) void j1939_netdev_stop(struct j1939_priv *priv) { - kref_put_lock(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock); + kref_put_mutex(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock); j1939_priv_put(priv); }
It turns out access to j1939_can_rx_register() needs to be serialized, otherwise j1939_priv can be corrupted when parallel threads call j1939_netdev_start() and j1939_can_rx_register() fails. This issue is thoroughly covered in other commit which serializes access to j1939_can_rx_register(). Change j1939_netdev_lock type to mutex so that we do not need to remove GFP_KERNEL from can_rx_register(). j1939_netdev_lock seems to be used in normal contexts where mutex usage is not prohibited. Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol") Suggested-by: Alexey Khoroshilov <khoroshilov@ispras.ru> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> --- Note that it has been only tested via Syzkaller and not with real hardware. net/can/j1939/main.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)