The patch series fixes a possible racy use-after-free scenario described
in 2/2: if j1939_can_rx_register() fails then the concurrent thread may
have already read the invalid priv structure.
The 1/2 makes j1939_netdev_lock a mutex so that access to
j1939_can_rx_register() can be serialized without changing GFP_KERNEL to
GFP_ATOMIC inside can_rx_register(). This seems to be safe.
Note that the patch series has been tested only via Syzkaller and not with
a real device.
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 <[email protected]>
Signed-off-by: Fedor Pchelkin <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Fedor Pchelkin <[email protected]>
Tested-by: Oleksij Rempel <[email protected]>
Acked-by: Oleksij Rempel <[email protected]>
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
>
>
>
--
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 26.05.2023 20:19:08, Fedor Pchelkin wrote:
> The patch series fixes a possible racy use-after-free scenario described
> in 2/2: if j1939_can_rx_register() fails then the concurrent thread may
> have already read the invalid priv structure.
>
> The 1/2 makes j1939_netdev_lock a mutex so that access to
> j1939_can_rx_register() can be serialized without changing GFP_KERNEL to
> GFP_ATOMIC inside can_rx_register(). This seems to be safe.
>
> Note that the patch series has been tested only via Syzkaller and not with
> a real device.
Applied to linux-can + adding stable on Cc.
Thanks,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |