| 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
| ||
|
Date: Fri, 3 Mar 2023 13:16:41 +0530
From: Mukesh Ojha <quic_mojha@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
<agross@...nel.org>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>
CC: <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] firmware: qcom_scm: Refactor code to support
multiple download mode
Thanks for your time in checking this..
On 3/1/2023 4:29 PM, Dmitry Baryshkov wrote:
> On 01/03/2023 11:55, Mukesh Ojha wrote:
>> Currently on Qualcomm SoC, download_mode is enabled if
>> CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected.
>>
>> Refactor the code such that it supports multiple download
>> modes and drop CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT config
>> instead, give interface to set the download mode from
>> module parameter.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@...cinc.com>
>> ---
>> Changes in v2:
>> - Passed download mode as parameter
>> - Accept human accepatable download mode string.
>> - enable = !!dload_mode
>> - Shifted module param callback to somewhere down in
>> the file so that it no longer need to know the
>> prototype of qcom_scm_set_download_mode()
>> - updated commit text.
>>
>> drivers/firmware/Kconfig | 11 --------
>> drivers/firmware/qcom_scm.c | 65
>> ++++++++++++++++++++++++++++++++++++++-------
>> 2 files changed, 56 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index b59e304..ff7e9f3 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -215,17 +215,6 @@ config MTK_ADSP_IPC
>> config QCOM_SCM
>> tristate
>> -config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
>> - bool "Qualcomm download mode enabled by default"
>> - depends on QCOM_SCM
>> - help
>> - A device with "download mode" enabled will upon an unexpected
>> - warm-restart enter a special debug mode that allows the user to
>> - "download" memory content over USB for offline postmortem
>> analysis.
>> - The feature can be enabled/disabled on the kernel command line.
>> -
>> - Say Y here to enable "download mode" by default.
>> -
>> config SYSFB
>> bool
>> select BOOT_VESA_SUPPORT
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index c9f1fad..ca07208 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -17,17 +17,22 @@
>> #include <linux/clk.h>
>> #include <linux/reset-controller.h>
>> #include <linux/arm-smccc.h>
>> +#include <linux/kstrtox.h>
>> #include "qcom_scm.h"
>> -static bool download_mode =
>> IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>> -module_param(download_mode, bool, 0);
>> -
>> #define SCM_HAS_CORE_CLK BIT(0)
>> #define SCM_HAS_IFACE_CLK BIT(1)
>> #define SCM_HAS_BUS_CLK BIT(2)
>> #define QCOM_DOWNLOAD_MODE_MASK 0x30
>> +#define QCOM_DOWNLOAD_FULLDUMP 0x10
>> +#define QCOM_DOWNLOAD_NODUMP 0x0
>> +
>> +#define MAX_DLOAD_LEN 8
>> +
>> +static char download_mode[] = "off";
>> +static u32 dload_mode;
>> struct qcom_scm {
>> struct device *dev;
>> @@ -417,8 +422,9 @@ static int __qcom_scm_set_dload_mode(struct device
>> *dev, bool enable)
>> return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
>> }
>> -static void qcom_scm_set_download_mode(bool enable)
>> +static void qcom_scm_set_download_mode(u32 dload_mode)
>> {
>> + bool enable = !!dload_mode;
>> bool avail;
>> int ret = 0;
>> u32 val;
>> @@ -438,7 +444,7 @@ static void qcom_scm_set_download_mode(bool enable)
>> val &= ~QCOM_DOWNLOAD_MODE_MASK;
>> if (enable)
>> - val |= QCOM_SCM_BOOT_SET_DLOAD_MODE;
>> + val |= dload_mode;
>> ret = qcom_scm_io_writel(__scm->dload_mode_addr, val);
>> } else {
>> @@ -1338,6 +1344,47 @@ bool qcom_scm_is_available(void)
>> }
>> EXPORT_SYMBOL(qcom_scm_is_available);
>> +static int set_dload_mode(const char *val, const struct kernel_param
>> *kp)
>> +{
>> + int ret;
>> +
>> + if (!strncmp(val, "full", strlen("full"))) {
>> + dload_mode = QCOM_DOWNLOAD_FULLDUMP;
>> + } else if (!strncmp(val, "off", strlen("off"))) {
>> + dload_mode = QCOM_DOWNLOAD_NODUMP;
>> + } else {
>> + if (kstrtouint(val, 0, &dload_mode) ||
>> + !(dload_mode == 0 || dload_mode == 1)) {
>> + pr_err("unknown download mode\n");
>> + return -EINVAL;
>> + }
>> +
>> + }
>> +
>> + ret = param_set_copystring(val, kp);
>> + if (ret)
>> + return ret;
>> +
>> + if (__scm)
>> + qcom_scm_set_download_mode(dload_mode);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct kernel_param_ops dload_mode_param_ops = {
>> + .set = set_dload_mode,
>> + .get = param_get_string,
>
> Please follow the param_get_bool approach and return sanitized data
> here. In other words, "full" / "none" depending on the dload_mode
> instead of storing the passed string and returning it later.
>
This could be done.
>> +};
>> +
>> +static struct kparam_string dload_mode_param = {
>> + .string = download_mode,
>> + .maxlen = MAX_DLOAD_LEN,
>
> I think writing "full" to this param might overwrite some kernel data.
> "00000000" should be even worse.
There is check in param_set_copystring() which would avoid that.
>
>> +};
>> +
>> +module_param_cb(download_mode, &dload_mode_param_ops,
>> &dload_mode_param, 0644);
>> +MODULE_PARM_DESC(download_mode,
>> + "Download mode: off/full or 0/1/off/on for existing users");
>
> Nit: on is not supported even for existing users.
Agree. just 0/1 would do fine there.
-Mukesh
>
>> +
>> static int qcom_scm_probe(struct platform_device *pdev)
>> {
>> struct qcom_scm *scm;
>> @@ -1418,12 +1465,12 @@ static int qcom_scm_probe(struct
>> platform_device *pdev)
>> __get_convention();
>> /*
>> - * If requested enable "download mode", from this point on warmboot
>> + * If download mode is requested, from this point on warmboot
>> * will cause the boot stages to enter download mode, unless
>> * disabled below by a clean shutdown/reboot.
>> */
>> - if (download_mode)
>> - qcom_scm_set_download_mode(true);
>> + if (dload_mode)
>> + qcom_scm_set_download_mode(dload_mode);
>> return 0;
>> }
>> @@ -1431,7 +1478,7 @@ static int qcom_scm_probe(struct platform_device
>> *pdev)
>> static void qcom_scm_shutdown(struct platform_device *pdev)
>> {
>> /* Clean shutdown, disable download mode to allow normal restart */
>> - qcom_scm_set_download_mode(false);
>> + qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
>> }
>> static const struct of_device_id qcom_scm_dt_match[] = {
>
Powered by blists - more mailing lists