| 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: Thu, 1 Jun 2023 17:00:11 +0530
From: Komal Bajaj <quic_kbajaj@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: Andy Gross <agross@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<devicetree@...r.kernel.org>
Subject: Re: [PATCH v3 01/10] nvmem: qfprom: Add support for secure reading
On 5/28/2023 2:20 AM, Bjorn Andersson wrote:
> On Fri, May 12, 2023 at 05:51:25PM +0530, Komal Bajaj wrote:
>> For some of the Qualcomm SoC's, it is possible that
>> some of the fuse regions or entire qfprom region is
>> protected from non-secure access. In such situations,
>> linux will have to use secure calls to read the region.
>> With that motivation, add the support of reading secure
>> regions in qfprom driver. Ensuring the address to read
>> is word aligned since our secure I/O only supports word
>> size I/O.
>>
>> Signed-off-by: Komal Bajaj <quic_kbajaj@...cinc.com>
>> ---
>> drivers/nvmem/Kconfig | 1 +
>> drivers/nvmem/qfprom.c | 69 +++++++++++++++++++++++++++++++++---------
>> 2 files changed, 55 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index b291b27048c7..3d896ba29b89 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -209,6 +209,7 @@ config NVMEM_QCOM_QFPROM
>> tristate "QCOM QFPROM Support"
>> depends on ARCH_QCOM || COMPILE_TEST
>> depends on HAS_IOMEM
>> + select QCOM_SCM
>> help
>> Say y here to enable QFPROM support. The QFPROM provides access
>> functions for QFPROM data to rest of the drivers via nvmem interface.
>> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
>> index c1e893c8a247..20662e2d3732 100644
>> --- a/drivers/nvmem/qfprom.c
>> +++ b/drivers/nvmem/qfprom.c
>> @@ -16,6 +16,7 @@
>> #include <linux/pm_runtime.h>
>> #include <linux/property.h>
>> #include <linux/regulator/consumer.h>
>> +#include <linux/firmware/qcom/qcom_scm.h>
>>
>> /* Blow timer clock frequency in Mhz */
>> #define QFPROM_BLOW_TIMER_OFFSET 0x03c
>> @@ -59,21 +60,22 @@ struct qfprom_soc_data {
>> /**
>> * struct qfprom_priv - structure holding qfprom attributes
>> *
>> - * @qfpraw: iomapped memory space for qfprom-efuse raw address space.
>> - * @qfpconf: iomapped memory space for qfprom-efuse configuration address
>> - * space.
>> + * @qfpraw: iomapped memory space for qfprom-efuse raw address space.
>> + * @qfpconf: iomapped memory space for qfprom-efuse configuration address space.
> Adjusting the indentation makes it unnecessarily hard to see what you
> actually changed.
Okay, will maintain the existing indentation.
>
>> * @qfpcorrected: iomapped memory space for qfprom corrected address space.
>> - * @qfpsecurity: iomapped memory space for qfprom security control space.
>> - * @dev: qfprom device structure.
>> - * @secclk: Clock supply.
>> - * @vcc: Regulator supply.
>> - * @soc_data: Data that for things that varies from SoC to SoC.
>> + * @qfpsecurity: iomapped memory space for qfprom security control space.
>> + * @qfpseccorrected: starting physical address for qfprom secure corrected address space.
>> + * @dev: qfprom device structure.
>> + * @secclk: Clock supply.
>> + * @vcc: Regulator supply.
>> + * @soc_data: Data that for things that varies from SoC to SoC.
>> */
>> struct qfprom_priv {
>> void __iomem *qfpraw;
>> void __iomem *qfpconf;
>> void __iomem *qfpcorrected;
>> void __iomem *qfpsecurity;
>> + phys_addr_t qfpseccorrected;
>> struct device *dev;
>> struct clk *secclk;
>> struct regulator *vcc;
>> @@ -99,10 +101,12 @@ struct qfprom_touched_values {
>> *
>> * @keepout: Array of keepout regions for this SoC.
>> * @nkeepout: Number of elements in the keepout array.
>> + * @secure: Is qfprom region for this SoC protected from non-secure access.
>> */
>> struct qfprom_soc_compatible_data {
>> const struct nvmem_keepout *keepout;
>> unsigned int nkeepout;
>> + bool secure;
>> };
>>
>> static const struct nvmem_keepout sc7180_qfprom_keepout[] = {
>> @@ -334,6 +338,34 @@ static int qfprom_reg_read(void *context,
>> return 0;
>> }
>>
>> +static int qfprom_sec_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
>> +{
>> + struct qfprom_priv *priv = context;
>> + u8 *val = _val;
>> + int buf_start, buf_end, index, i = 0;
>> + char *buffer;
>> + u32 read_val;
>> +
>> + buf_start = ALIGN_DOWN(reg, 4);
>> + buf_end = ALIGN(reg + bytes, 4);
>> + buffer = kzalloc(buf_end - buf_start, GFP_KERNEL);
>> + if (!buffer)
>> + return -ENOMEM;
> I don't you need all these variables, the full temp buffer or the two
> memcpy... I think something like this should do the trick:
>
> unsigned int i;
> u8 *val = _val;
> u8 tmp[4];
>
> for (i = 0; i < bytes; i++, reg++)
> if (i == 0 || reg % 4 == 0)
> qcom_scm_io_readl(qfpseccorrected + (reg & ~3), tmp);
>
> val[i] = tmp[reg & 3];
> }
Thanks, for this. Will make the suggested change in the next patch series.
>> +
>> + for (index = buf_start; index < buf_end; index += 4, i += 4) {
>> + if (qcom_scm_io_readl(priv->qfpseccorrected + index, &read_val)) {
>> + dev_err(priv->dev, "Couldn't access feature register\n");
> What's a "feature register"?
It's a fuse register that we are trying to read here.
Thanks
Komal
>
> Regards,
> Bjorn
>
>> + kfree_sensitive(buffer);
>> + return -EINVAL;
>> + }
>> + memcpy(buffer + i, &read_val, 4);
>> + }
>> +
>> + memcpy(val, buffer + reg % 4, bytes);
>> + kfree_sensitive(buffer);
>> + return 0;
>> +}
>> +
>> static void qfprom_runtime_disable(void *data)
>> {
>> pm_runtime_disable(data);
>> @@ -373,13 +405,6 @@ static int qfprom_probe(struct platform_device *pdev)
>> if (!priv)
>> return -ENOMEM;
>>
>> - /* The corrected section is always provided */
>> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - priv->qfpcorrected = devm_ioremap_resource(dev, res);
>> - if (IS_ERR(priv->qfpcorrected))
>> - return PTR_ERR(priv->qfpcorrected);
>> -
>> - econfig.size = resource_size(res);
>> econfig.dev = dev;
>> econfig.priv = priv;
>>
>> @@ -390,6 +415,20 @@ static int qfprom_probe(struct platform_device *pdev)
>> econfig.nkeepout = soc_data->nkeepout;
>> }
>>
>> + /* The corrected section is always provided */
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + if (soc_data && soc_data->secure) {
>> + priv->qfpseccorrected = res->start;
>> + econfig.reg_read = qfprom_sec_reg_read;
>> + } else {
>> + priv->qfpcorrected = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(priv->qfpcorrected))
>> + return PTR_ERR(priv->qfpcorrected);
>> + }
>> +
>> + econfig.size = resource_size(res);
>> +
>> /*
>> * If more than one region is provided then the OS has the ability
>> * to write.
>> --
>> 2.17.1
>>
Powered by blists - more mailing lists