| 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
| ||
|
Message-ID: <a0181586-817c-e093-990f-5f95c10aadbd@linaro.org>
Date: Thu, 16 Mar 2023 18:27:33 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Mukesh Ojha <quic_mojha@...cinc.com>, 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
On 03/03/2023 09:46, Mukesh Ojha wrote:
> 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.
No. The check will validate the value's length against MAX_DLOAD_LEN.
But it will not safeguard your code. download_mode's size is less than
MAX_DLOAD_LEN.
--
With best wishes
Dmitry
Powered by blists - more mailing lists