soc: qcom: pmic_glink: Fix race during initialization

As pointed out by Stephen Boyd it is possible that during initialization
of the pmic_glink child drivers, the protection-domain notifiers fires,
and the associated work is scheduled, before the client registration
returns and as a result the local "client" pointer has been initialized.

The outcome of this is a NULL pointer dereference as the "client"
pointer is blindly dereferenced.

Timeline provided by Stephen:
 CPU0                               CPU1
 ----                               ----
 ucsi->client = NULL;
 devm_pmic_glink_register_client()
  client->pdr_notify(client->priv, pg->client_state)
   pmic_glink_ucsi_pdr_notify()
    schedule_work(&ucsi->register_work)
    <schedule away>
                                    pmic_glink_ucsi_register()
                                     ucsi_register()
                                      pmic_glink_ucsi_read_version()
                                       pmic_glink_ucsi_read()
                                        pmic_glink_ucsi_read()
                                         pmic_glink_send(ucsi->client)
                                         <client is NULL BAD>
 ucsi->client = client // Too late!

This code is identical across the altmode, battery manager and usci
child drivers.

Resolve this by splitting the allocation of the "client" object and the
registration thereof into two operations.

This only happens if the protection domain registry is populated at the
time of registration, which by the introduction of commit '1ebcde047c54
("soc: qcom: add pd-mapper implementation")' became much more likely.

Reported-by: Amit Pundir <amit.pundir@linaro.org>
Closes: https://lore.kernel.org/all/CAMi1Hd2_a7TjA7J9ShrAbNOd_CoZ3D87twmO5t+nZxC9sX18tA@mail.gmail.com/
Reported-by: Johan Hovold <johan@kernel.org>
Closes: https://lore.kernel.org/all/ZqiyLvP0gkBnuekL@hovoldconsulting.com/
Reported-by: Stephen Boyd <swboyd@chromium.org>
Closes: https://lore.kernel.org/all/CAE-0n52JgfCBWiFQyQWPji8cq_rCsviBpW-m72YitgNfdaEhQg@mail.gmail.com/
Fixes: 58ef4ece1e41 ("soc: qcom: pmic_glink: Introduce base PMIC GLINK driver")
Cc: stable@vger.kernel.org
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Tested-by: Amit Pundir <amit.pundir@linaro.org>
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Tested-by: Johan Hovold <johan+linaro@kernel.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
Link: https://lore.kernel.org/r/20240820-pmic-glink-v6-11-races-v3-1-eec53c750a04@quicinc.com
Signed-off-by: Bjorn Andersson <andersson@kernel.org>

diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
index 46f36dcb185c366aec6652017e1c64f9b0822ae1..7cf19a39d98605ffebd890eabb0ab69c0409a3b7 100644 (file)
--- a/drivers/power/supply/qcom_battmgr.c
+++ b/drivers/power/supply/qcom_battmgr.c
@@ -1385,12 +1385,16 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev,
                                             "failed to register wireless charing power supply\n");
        }
 
-       battmgr->client = devm_pmic_glink_register_client(dev,
-                                                         PMIC_GLINK_OWNER_BATTMGR,
-                                                         qcom_battmgr_callback,
-                                                         qcom_battmgr_pdr_notify,
-                                                         battmgr);
-       return PTR_ERR_OR_ZERO(battmgr->client);
+       battmgr->client = devm_pmic_glink_client_alloc(dev, PMIC_GLINK_OWNER_BATTMGR,
+                                                      qcom_battmgr_callback,
+                                                      qcom_battmgr_pdr_notify,
+                                                      battmgr);
+       if (IS_ERR(battmgr->client))
+               return PTR_ERR(battmgr->client);
+
+       pmic_glink_client_register(battmgr->client);
+
+       return 0;
 }
 
 static const struct auxiliary_device_id qcom_battmgr_id_table[] = {

diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
index 9ebc0ba359477ae0bf3195764d6fabf16cca84dc..53b176d04fbdfeb60e07085db0c83b293ef58e35 100644 (file)
--- a/drivers/soc/qcom/pmic_glink.c
+++ b/drivers/soc/qcom/pmic_glink.c
@@ -66,15 +66,14 @@ static void _devm_pmic_glink_release_client(struct device *dev, void *res)
        spin_unlock_irqrestore(&pg->client_lock, flags);
 }
 
-struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev,
-                                                         unsigned int id,
-                                                         void (*cb)(const void *, size_t, void *),
-                                                         void (*pdr)(void *, int),
-                                                         void *priv)
+struct pmic_glink_client *devm_pmic_glink_client_alloc(struct device *dev,
+                                                      unsigned int id,
+                                                      void (*cb)(const void *, size_t, void *),
+                                                      void (*pdr)(void *, int),
+                                                      void *priv)
 {
        struct pmic_glink_client *client;
        struct pmic_glink *pg = dev_get_drvdata(dev->parent);
-       unsigned long flags;
 
        client = devres_alloc(_devm_pmic_glink_release_client, sizeof(*client), GFP_KERNEL);
        if (!client)
@@ -85,6 +84,18 @@ struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev,
        client->cb = cb;
        client->pdr_notify = pdr;
        client->priv = priv;
+       INIT_LIST_HEAD(&client->node);
+
+       devres_add(dev, client);
+
+       return client;
+}
+EXPORT_SYMBOL_GPL(devm_pmic_glink_client_alloc);
+
+void pmic_glink_client_register(struct pmic_glink_client *client)
+{
+       struct pmic_glink *pg = client->pg;
+       unsigned long flags;
 
        mutex_lock(&pg->state_lock);
        spin_lock_irqsave(&pg->client_lock, flags);
@@ -95,11 +106,8 @@ struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev,
        spin_unlock_irqrestore(&pg->client_lock, flags);
        mutex_unlock(&pg->state_lock);
 
-       devres_add(dev, client);
-
-       return client;
 }
-EXPORT_SYMBOL_GPL(devm_pmic_glink_register_client);
+EXPORT_SYMBOL_GPL(pmic_glink_client_register);
 
 int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len)
 {

diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
index 1e0808b3cb93eaf96d6eb704366a2136962accae..463b1c5288318dff89c4079f1af76893546ec7aa 100644 (file)
--- a/drivers/soc/qcom/pmic_glink_altmode.c
+++ b/drivers/soc/qcom/pmic_glink_altmode.c
@@ -520,12 +520,17 @@ static int pmic_glink_altmode_probe(struct auxiliary_device *adev,
                        return ret;
        }
 
-       altmode->client = devm_pmic_glink_register_client(dev,
-                                                         altmode->owner_id,
-                                                         pmic_glink_altmode_callback,
-                                                         pmic_glink_altmode_pdr_notify,
-                                                         altmode);
-       return PTR_ERR_OR_ZERO(altmode->client);
+       altmode->client = devm_pmic_glink_client_alloc(dev,
+                                                      altmode->owner_id,
+                                                      pmic_glink_altmode_callback,
+                                                      pmic_glink_altmode_pdr_notify,
+                                                      altmode);
+       if (IS_ERR(altmode->client))
+               return PTR_ERR(altmode->client);
+
+       pmic_glink_client_register(altmode->client);
+
+       return 0;
 }
 
 static const struct auxiliary_device_id pmic_glink_altmode_id_table[] = {

diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index 16c328497e0b8d342263aca81904f0ca7b6f6933..f6f4fae40399344a3f582a11c4175a17850afa59 100644 (file)
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -367,12 +367,16 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
                ucsi->port_orientation[port] = desc;
        }
 
-       ucsi->client = devm_pmic_glink_register_client(dev,
-                                                      PMIC_GLINK_OWNER_USBC,
-                                                      pmic_glink_ucsi_callback,
-                                                      pmic_glink_ucsi_pdr_notify,
-                                                      ucsi);
-       return PTR_ERR_OR_ZERO(ucsi->client);
+       ucsi->client = devm_pmic_glink_client_alloc(dev, PMIC_GLINK_OWNER_USBC,
+                                                   pmic_glink_ucsi_callback,
+                                                   pmic_glink_ucsi_pdr_notify,
+                                                   ucsi);
+       if (IS_ERR(ucsi->client))
+               return PTR_ERR(ucsi->client);
+
+       pmic_glink_client_register(ucsi->client);
+
+       return 0;
 }
 
 static void pmic_glink_ucsi_remove(struct auxiliary_device *adev)

diff --git a/include/linux/soc/qcom/pmic_glink.h b/include/linux/soc/qcom/pmic_glink.h
index fd124aa18c81a4ee633e52087ebe191ccc029d52..7cddf10277528ef219ebd4f9f1abed4c3da97c25 100644 (file)
--- a/include/linux/soc/qcom/pmic_glink.h
+++ b/include/linux/soc/qcom/pmic_glink.h
@@ -23,10 +23,11 @@ struct pmic_glink_hdr {
 
 int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len);
 
-struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev,
-                                                         unsigned int id,
-                                                         void (*cb)(const void *, size_t, void *),
-                                                         void (*pdr)(void *, int),
-                                                         void *priv);
+struct pmic_glink_client *devm_pmic_glink_client_alloc(struct device *dev,
+                                                      unsigned int id,
+                                                      void (*cb)(const void *, size_t, void *),
+                                                      void (*pdr)(void *, int),
+                                                      void *priv);
+void pmic_glink_client_register(struct pmic_glink_client *client);
 
 #endif