lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <30dc4254-ff2b-4bd3-aea8-1be8da11fb8e@linaro.org>
Date: Mon, 19 Aug 2024 11:15:32 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Bjorn Andersson <quic_bjorande@...cinc.com>,
 Sebastian Reichel <sre@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konrad.dybcio@...aro.org>,
 Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Johan Hovold <johan+linaro@...nel.org>, Chris Lew
 <quic_clew@...cinc.com>, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
 Stephen Boyd <swboyd@...omium.org>, Amit Pundir <amit.pundir@...aro.org>,
 linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
 Johan Hovold <johan@...nel.org>, stable@...r.kernel.org
Subject: Re: [PATCH 1/3] soc: qcom: pmic_glink: Fix race during initialization

On 19/08/2024 01:17, 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@...aro.org>
> Closes: https://lore.kernel.org/all/CAMi1Hd2_a7TjA7J9ShrAbNOd_CoZ3D87twmO5t+nZxC9sX18tA@mail.gmail.com/
> Reported-by: Johan Hovold <johan@...nel.org>
> Closes: https://lore.kernel.org/all/ZqiyLvP0gkBnuekL@hovoldconsulting.com/
> Reported-by: Stephen Boyd <swboyd@...omium.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@...r.kernel.org
> Signed-off-by: Bjorn Andersson <quic_bjorande@...cinc.com>
> ---
>   drivers/power/supply/qcom_battmgr.c   | 16 ++++++++++------
>   drivers/soc/qcom/pmic_glink.c         | 28 ++++++++++++++++++----------
>   drivers/soc/qcom/pmic_glink_altmode.c | 17 +++++++++++------
>   drivers/usb/typec/ucsi/ucsi_glink.c   | 16 ++++++++++------
>   include/linux/soc/qcom/pmic_glink.h   | 11 ++++++-----
>   5 files changed, 55 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
> index 49bef4a5ac3f..df90a470c51a 100644
> --- a/drivers/power/supply/qcom_battmgr.c
> +++ b/drivers/power/supply/qcom_battmgr.c
> @@ -1387,12 +1387,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_new_client(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_register_client(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 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,
> +						     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_new_client);
> +
> +void pmic_glink_register_client(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_register_client);
>   
>   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 1e0808b3cb93..e4f5059256e5 100644
> --- 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_new_client(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_register_client(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 16c328497e0b..ac53a81c2a81 100644
> --- 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_new_client(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_register_client(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 fd124aa18c81..aedde76d7e13 100644
> --- 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_new_client(struct device *dev,
> +						     unsigned int id,
> +						     void (*cb)(const void *, size_t, void *),
> +						     void (*pdr)(void *, int),
> +						     void *priv);
> +void pmic_glink_register_client(struct pmic_glink_client *client);
>   
>   #endif
> 

Reviewed-by: Neil Armstrong <neil.armstrong@...aro.org>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ