| 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: <4674b202-ec33-e20a-85a8-4591679488ac@loongson.cn>
Date: Mon, 18 Nov 2024 09:50:46 +0800
From: Zhao Qunqin <zhaoqunqin@...ngson.cn>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: arnd@...db.de, olof@...om.net, soc@...ts.linux.dev,
linux-kernel@...r.kernel.org, loongarch@...ts.linux.dev
Subject: Re: [PATCH] soc: loongson: add Loongson Security Module driver
在 2024/11/16 下午5:31, Christophe JAILLET 写道:
> Le 16/11/2024 à 09:57, Qunqin Zhao a écrit :
>> This driver supports Loongson Security Module, which
>> provides the control for it's hardware encryption
>> acceleration child devices.
>>
>> Only ACPI firmware is supported.
>>
>> Signed-off-by: Qunqin Zhao <zhaoqunqin@...ngson.cn>
>> ---
>> MAINTAINERS | 7 +
>> drivers/soc/loongson/Kconfig | 9 +
>> drivers/soc/loongson/Makefile | 1 +
>> drivers/soc/loongson/loongson_se.c | 542 +++++++++++++++++++++++++++++
>> include/soc/loongson/se.h | 135 +++++++
>> 5 files changed, 694 insertions(+)
>> create mode 100644 drivers/soc/loongson/loongson_se.c
>> create mode 100644 include/soc/loongson/se.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index fdeb3d12c..85fff2eb7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -13379,6 +13379,13 @@ S: Maintained
>> F: Documentation/devicetree/bindings/hwinfo/loongson,ls2k-chipid.yaml
>> F: drivers/soc/loongson/loongson2_guts.c
>> +LOONGSON SECURITY MODULE DRIVER
>> +M: Qunqin Zhao <zhaoqunqin@...ngson.cn>
>> +L: loongarch@...ts.linux.dev
>> +S: Maintained
>> +F: drivers/soc/loongson/loongson_se.c
>> +F: include/soc/loongson/se.h
>> +
>> LOONGSON-2 SOC SERIES PM DRIVER
>> M: Yinbo Zhu <zhuyinbo@...ngson.cn>
>> L: linux-pm@...r.kernel.org
>> diff --git a/drivers/soc/loongson/Kconfig b/drivers/soc/loongson/Kconfig
>> index 368344943..93ef1d205 100644
>> --- a/drivers/soc/loongson/Kconfig
>> +++ b/drivers/soc/loongson/Kconfig
>> @@ -27,3 +27,12 @@ config LOONGSON2_PM
>> Disk), ACPI S5 (Soft Shutdown) and supports multiple wake-up
>> methods
>> (USB, GMAC, PWRBTN, etc.). This driver was to add power
>> management
>> controller support that base on dts for Loongson-2 series SoCs.
>> +
>> +config LOONGSON_SE
>> + tristate "LOONGSON SECURITY MODULE Interface"
>> + depends on LOONGARCH && ACPI
>> + help
>> + The Loongson security module provides the control for hardware
>> + encryption acceleration devices. Each device uses at least one
>> + channel to interacts with security module, and each channel may
>
> s/interacts/interact/
>
>> + has its own buffer provided by security module.
>
> s/has/have/
>
>> diff --git a/drivers/soc/loongson/Makefile
>> b/drivers/soc/loongson/Makefile
>> index 4118f50f5..503075042 100644
>> --- a/drivers/soc/loongson/Makefile
>> +++ b/drivers/soc/loongson/Makefile
>> @@ -5,3 +5,4 @@
>> obj-$(CONFIG_LOONGSON2_GUTS) += loongson2_guts.o
>> obj-$(CONFIG_LOONGSON2_PM) += loongson2_pm.o
>> +obj-$(CONFIG_LOONGSON_SE) += loongson_se.o
>> diff --git a/drivers/soc/loongson/loongson_se.c
>> b/drivers/soc/loongson/loongson_se.c
>> new file mode 100644
>> index 000000000..d85db8423
>> --- /dev/null
>> +++ b/drivers/soc/loongson/loongson_se.c
>
> ...
>
>> +static int loongson_se_get_res(struct loongson_se *se, u32 int_bit,
>> u32 cmd,
>> + struct se_data *res)
>> +{
>> + int err = 0;
>> +
>> + res->int_bit = int_bit;
>> +
>> + if (se_get_response(se, res)) {
>> + dev_err(se->dev, "Int 0x%x get response fail.\n", int_bit);
>> + return -EFAULT;
>> + }
>> +
>> + /* Check response */
>> + if (res->u.res.cmd == cmd)
>> + err = 0;
>
> Not needed, err is already 0.
>
>> + else {
>> + dev_err(se->dev, "Response cmd is 0x%x, not expect cmd
>> 0x%x.\n",
>> + res->u.res.cmd, cmd);
>> + err = -EFAULT;
>> + }
>> +
>> + return err;
>> +}
>
> ...
>
>> +static int loongson_se_set_msg(struct lsse_ch *ch)
>> +{
>> + struct loongson_se *se = ch->se;
>> + struct se_data req = {0};
>> + struct se_data res = {0};
>> + int err;
>> +
>> + req.int_bit = SE_INT_SETUP;
>> + req.u.gcmd.cmd = SE_CMD_SETMSG;
>> + /* MSG off */
>> + req.u.gcmd.info[0] = ch->id;
>> + req.u.gcmd.info[1] = ch->smsg - se->mem_base;
>> + req.u.gcmd.info[2] = ch->msg_size;
>> +
>> + dev_dbg(se->dev, "Set Channel %d msg off 0x%x, msg size %d\n",
>> + ch->id, req.u.gcmd.info[1], req.u.gcmd.info[2]);
>> +
>> + err = se_send_genl_cmd(se, &req, &res, 5);
>> + if (res.u.res.cmd_ret)
>
> In the fnction above, we test (err || ...)
> Is it needed here as well (or can it be removed in se_send_genl_cmd())
>
> Also, below se_send_genl_cmd() errors are checked only with err.
> Should it be consistent?
The above function may need to be tried again, so both values need to be
checked.
However, this function only needs to return an error value.
Others agreed.
Thanks.
>
>> + return res.u.res.cmd_ret;
>> +
>> + return err;
>> +}
>
> ...
>
>> +void se_deinit_ch(struct lsse_ch *ch)
>> +{
>> + struct loongson_se *se = ch->se;
>> + unsigned long flag;
>> + int first, nr;
>> + int id = ch->id;
>> +
>> + if (!se) {
>> + pr_err("SE has bot been initialized\n");
>> + return;
>> + }
>> +
>> + if (id == 0 || id > SE_CH_MAX) {
>> + dev_err(se->dev, "Channel number %d is invalid\n", id);
>> + return;
>> + }
>> +
>> + if (!se_ch_status(se, BIT(id))) {
>> + dev_err(se->dev, "Channel number %d has not been
>> initialized\n", id);
>> + return;
>> + }
>> +
>> + spin_lock_irqsave(&se->dev_lock, flag);
>> +
>> + se->ch_status &= ~BIT(ch->id);
>> +
>> + first = (ch->data_buffer - se->mem_base) / PAGE_SIZE;
>> + nr = round_up(ch->data_size, PAGE_SIZE) / PAGE_SIZE;
>> + bitmap_clear(se->mem_map, first, nr);
>> +
>> + first = (ch->smsg - se->mem_base) / PAGE_SIZE;
>> + nr = round_up(ch->msg_size, PAGE_SIZE) / PAGE_SIZE;
>> + bitmap_clear(se->mem_map, first, nr);
>> +
>> + se_disable_int(se, ch->int_bit);
>> +
>> + spin_unlock_irqrestore(&se->dev_lock, flag);
>> +
>
> Uneeded empty line
>
>> +}
>> +EXPORT_SYMBOL_GPL(se_deinit_ch);
>> +
>> +static int loongson_se_probe(struct platform_device *pdev)
>> +{
>> + struct loongson_se *se;
>> + struct device *dev = &pdev->dev;
>> + int nr_irq, irq, err, size;
>> +
>> + se = devm_kmalloc(dev, sizeof(*se), GFP_KERNEL);
>> + if (!se)
>> + return -ENOMEM;
>> + se->dev = dev;
>> + dev_set_drvdata(dev, se);
>> + init_completion(&se->cmd_completion);
>> + spin_lock_init(&se->cmd_lock);
>> + spin_lock_init(&se->dev_lock);
>> + /* Setup DMA buffer */
>> + dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>> + if (device_property_read_u32(dev, "dmam_size", &size))
>> + return -ENODEV;
>> + size = roundup_pow_of_two(size);
>> + se->mem_base = dmam_alloc_coherent(dev, size, &se->mem_addr,
>> GFP_KERNEL);
>> + if (!se->mem_base)
>> + return -ENOMEM;
>> + memset(se->mem_base, 0, size);
>
> I don't think that it is needed. dmam_alloc_coherent() should return
> zeroed memory.
>
>> + se->mem_map_pages = size / PAGE_SIZE;
>> + se->mem_map = devm_bitmap_zalloc(dev, se->mem_map_pages,
>> GFP_KERNEL);
>> + if (!se->mem_map)
>> + return -ENOMEM;
>> +
>> + se->base = devm_platform_ioremap_resource(pdev, 0);
>> + if (IS_ERR(se->base))
>> + return PTR_ERR(se->base);
>> +
>> + nr_irq = platform_irq_count(pdev);
>> + if (nr_irq <= 0)
>> + return -ENODEV;
>> + while (nr_irq) {
>> + irq = platform_get_irq(pdev, --nr_irq);
>> + if (irq < 0)
>> + return -ENODEV;
>> + /* Use the same interrupt handler address.
>> + * Determine which irq it is accroding
>> + * SE_S2LINT_STAT register.
>> + */
>> + err = devm_request_irq(dev, irq, se_irq, 0,
>> + "loongson-se", se);
>> + if (err)
>> + dev_err(dev, "failed to request irq: %d\n", err);
>> + }
>> +
>> + err = se_init_hw(se, se->mem_addr, size);
>> + if (err)
>> + se_disable_hw(se);
>> +
>> + return err;
>> +}
>> +
>> +static struct acpi_device_id loongson_se_acpi_match[] = {
>
> const?
>
>> + {"LOON0011", 0},
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, loongson_se_acpi_match);
>
> ...
>
> CJ
Powered by blists - more mailing lists