Re: [PATCH v2 1/3] soc: qcom: pmic_glink: Fix race during initialization

Johan Hovold
2024-08-20T06:53:52Z

On Mon, Aug 19, 2024 at 01:07:45PM -0700, Bjorn Andersson wrote:
> 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>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> index 9ebc0ba35947..58ec91767d79 100644
> --- 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_new_client(struct device *dev,

Please consider renaming this one

devm_pmic_glink_alloc_client()

(or devm_pmic_glink_client_alloc()) which is more conventional for
kernel code than using "new".

> + 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)

Looks good otherwise:

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

Johan