Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

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>

authored by

Bjorn Andersson and committed by
Bjorn Andersson
3568affc 924fc22c

+55 -33
+10 -6
drivers/power/supply/qcom_battmgr.c
··· 1385 1385 "failed to register wireless charing power supply\n"); 1386 1386 } 1387 1387 1388 - battmgr->client = devm_pmic_glink_register_client(dev, 1389 - PMIC_GLINK_OWNER_BATTMGR, 1390 - qcom_battmgr_callback, 1391 - qcom_battmgr_pdr_notify, 1392 - battmgr); 1393 - return PTR_ERR_OR_ZERO(battmgr->client); 1388 + battmgr->client = devm_pmic_glink_client_alloc(dev, PMIC_GLINK_OWNER_BATTMGR, 1389 + qcom_battmgr_callback, 1390 + qcom_battmgr_pdr_notify, 1391 + battmgr); 1392 + if (IS_ERR(battmgr->client)) 1393 + return PTR_ERR(battmgr->client); 1394 + 1395 + pmic_glink_client_register(battmgr->client); 1396 + 1397 + return 0; 1394 1398 } 1395 1399 1396 1400 static const struct auxiliary_device_id qcom_battmgr_id_table[] = {
+18 -10
drivers/soc/qcom/pmic_glink.c
··· 66 66 spin_unlock_irqrestore(&pg->client_lock, flags); 67 67 } 68 68 69 - struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev, 70 - unsigned int id, 71 - void (*cb)(const void *, size_t, void *), 72 - void (*pdr)(void *, int), 73 - void *priv) 69 + struct pmic_glink_client *devm_pmic_glink_client_alloc(struct device *dev, 70 + unsigned int id, 71 + void (*cb)(const void *, size_t, void *), 72 + void (*pdr)(void *, int), 73 + void *priv) 74 74 { 75 75 struct pmic_glink_client *client; 76 76 struct pmic_glink *pg = dev_get_drvdata(dev->parent); 77 - unsigned long flags; 78 77 79 78 client = devres_alloc(_devm_pmic_glink_release_client, sizeof(*client), GFP_KERNEL); 80 79 if (!client) ··· 84 85 client->cb = cb; 85 86 client->pdr_notify = pdr; 86 87 client->priv = priv; 88 + INIT_LIST_HEAD(&client->node); 89 + 90 + devres_add(dev, client); 91 + 92 + return client; 93 + } 94 + EXPORT_SYMBOL_GPL(devm_pmic_glink_client_alloc); 95 + 96 + void pmic_glink_client_register(struct pmic_glink_client *client) 97 + { 98 + struct pmic_glink *pg = client->pg; 99 + unsigned long flags; 87 100 88 101 mutex_lock(&pg->state_lock); 89 102 spin_lock_irqsave(&pg->client_lock, flags); ··· 106 95 spin_unlock_irqrestore(&pg->client_lock, flags); 107 96 mutex_unlock(&pg->state_lock); 108 97 109 - devres_add(dev, client); 110 - 111 - return client; 112 98 } 113 - EXPORT_SYMBOL_GPL(devm_pmic_glink_register_client); 99 + EXPORT_SYMBOL_GPL(pmic_glink_client_register); 114 100 115 101 int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len) 116 102 {
+10 -6
drivers/usb/typec/ucsi/ucsi_glink.c
··· 367 367 ucsi->port_orientation[port] = desc; 368 368 } 369 369 370 - ucsi->client = devm_pmic_glink_register_client(dev, 371 - PMIC_GLINK_OWNER_USBC, 372 - pmic_glink_ucsi_callback, 373 - pmic_glink_ucsi_pdr_notify, 374 - ucsi); 375 - return PTR_ERR_OR_ZERO(ucsi->client); 370 + ucsi->client = devm_pmic_glink_client_alloc(dev, PMIC_GLINK_OWNER_USBC, 371 + pmic_glink_ucsi_callback, 372 + pmic_glink_ucsi_pdr_notify, 373 + ucsi); 374 + if (IS_ERR(ucsi->client)) 375 + return PTR_ERR(ucsi->client); 376 + 377 + pmic_glink_client_register(ucsi->client); 378 + 379 + return 0; 376 380 } 377 381 378 382 static void pmic_glink_ucsi_remove(struct auxiliary_device *adev)
+6 -5
include/linux/soc/qcom/pmic_glink.h
··· 23 23 24 24 int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len); 25 25 26 - struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev, 27 - unsigned int id, 28 - void (*cb)(const void *, size_t, void *), 29 - void (*pdr)(void *, int), 30 - void *priv); 26 + struct pmic_glink_client *devm_pmic_glink_client_alloc(struct device *dev, 27 + unsigned int id, 28 + void (*cb)(const void *, size_t, void *), 29 + void (*pdr)(void *, int), 30 + void *priv); 31 + void pmic_glink_client_register(struct pmic_glink_client *client); 31 32 32 33 #endif