Re: [PATCH v1 2/2] firmware: socfpga-fcs: add Altera SoCFPGA FCS driver with SDOS
From: Hang Suan Wang
Date: Thu Jul 09 2026 - 09:26:29 EST
thank you for review. I had address all the comment.
On 8/7/2026 2:41 am, Dinh Nguyen wrote:
>
>
> On 7/1/26 02:39, hang.suan.wang@xxxxxxxxxx wrote:
>> From: Hang Suan Wang <hang.suan.wang@xxxxxxxxxx>
>>
>> Add the Altera SoCFPGA Crypto Service (FCS) driver, which exposes the
>> Secure Data Object Service (SDOS) encrypt/decrypt operation and its crypto
>> session lifecycle to non-secure host software.
>>
>> The SDOS is the FCS feature that protects data at rest: the SDM encrypts
>> and decrypts using a key derived from a device-unique SDOS root key plus an
>> SDM-generated IV, so the host never handles raw key material or IVs. It
>> only submits plaintext it already owns and receives authenticated
>> ciphertext objects managed by the SDM. A primary use case is black key
>> provisioning, where operational keys are installed without ever appearing
>> in cleartext.
>>
>> The driver is a standalone module and describes no hardware of its own.
>> It binds by name to the "stratix10-fcs" platform device that the
>> stratix10-svc driver registers in code, so no device-tree node is needed,
>> and detects the SoC by matching the service-layer compatible. It exposes
>> the following sysfs attributes. The SDOS requests are issued to the SDM
>> through the stratix10-svc asynchronous SIP SMC. Source and destination
>> buffers are taken from the service-layer memory pool so the SDM can reach
>> them via physical or SMMU-remapped addresses.
>>
>> For encryption the SDM returns a structured object (metadata, IV, HMAC,
>> ciphertext); for decryption it validates the HMAC, recovers the parameters
>> from the object header, and enforces a 64-bit owner ID so that only the
>> creator of an object can decrypt it.
>>
>> Signed-off-by: Hang Suan Wang <hang.suan.wang@xxxxxxxxxx>
>> ---
>> MAINTAINERS | 8 +
>> drivers/firmware/Kconfig | 16 +
>> drivers/firmware/Makefile | 2 +
>> drivers/firmware/socfpga-fcs-core.c | 589 +++++++++++++++++++++
>> drivers/firmware/socfpga-fcs.c | 294 ++++++++++
>> include/linux/firmware/intel/socfpga-fcs.h | 134 +++++
>> 6 files changed, 1043 insertions(+)
>> create mode 100644 drivers/firmware/socfpga-fcs-core.c
>> create mode 100644 drivers/firmware/socfpga-fcs.c
>> create mode 100644 include/linux/firmware/intel/socfpga-fcs.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 15011f5752a9..72b12b32f8fe 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -946,6 +946,14 @@ ALPS PS/2 TOUCHPAD DRIVER
>> R: Pali Rohár <pali@xxxxxxxxxx>
>> F: drivers/input/mouse/alps.*
>> +ALTERA FCS DRIVER
>> +M: Hang Suan Wang <hang.suan.wang@xxxxxxxxxx>
>> +M: Genevieve Chan <genevieve.chan@xxxxxxxxxx>
>> +L: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> +S: Maintained
>> +F: drivers/firmware/socfpga-fcs*
>> +F: include/linux/firmware/intel/socfpga-fcs*
>> +
>> ALTERA MAILBOX DRIVER
>> M: Tien Sung Ang <tiensung.ang@xxxxxxxxxx>
>> S: Maintained
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index 12dc70254842..9a70def6932a 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -172,6 +172,22 @@ config INTEL_STRATIX10_RSU
>> Say Y here if you want Intel RSU support.
>> +config ALTERA_SOCFPGA_FCS
>> + tristate "Altera SoCFPGA Crypto Service (FCS) configuration"
>> + depends on INTEL_STRATIX10_SERVICE
>> + default n
>> + help
>> + Altera SoCFPGA Crypto Service (FCS) driver exposes interfaces access
>> + through the Intel Service Layer to user space via sysfs device
>> + attribute nodes. It exposes the crypto and key-management services
>> + of the Secure Device Manager (SDM) to the host software stack and
>> + requests are forwarded to Arm Trusted Firmware. The SDM then
>> + executes or authorizes them using device-rooted security resources.
>> + Protected key material remains within the secure firmware boundary
>> + and is not directly exposed to non-secure host software.
>> +
>> + Say Y here if you want Altera SoCFPGA FCS support.
>> +
>> config MTK_ADSP_IPC
>> tristate "MTK ADSP IPC Protocol driver"
>> depends on MTK_ADSP_MBOX
>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>> index 4ddec2820c96..e9f52f0e5f7a 100644
>> --- a/drivers/firmware/Makefile
>> +++ b/drivers/firmware/Makefile
>> @@ -10,6 +10,8 @@ obj-$(CONFIG_EDD) += edd.o
>> obj-$(CONFIG_DMIID) += dmi-id.o
>> obj-$(CONFIG_INTEL_STRATIX10_SERVICE) += stratix10-svc.o
>> obj-$(CONFIG_INTEL_STRATIX10_RSU) += stratix10-rsu.o
>> +obj-$(CONFIG_ALTERA_SOCFPGA_FCS) += altera-fcs.o
>> +altera-fcs-y := socfpga-fcs.o socfpga-fcs-core.o
>> obj-$(CONFIG_ISCSI_IBFT_FIND) += iscsi_ibft_find.o
>> obj-$(CONFIG_ISCSI_IBFT) += iscsi_ibft.o
>> obj-$(CONFIG_FIRMWARE_MEMMAP) += memmap.o
>> diff --git a/drivers/firmware/socfpga-fcs-core.c b/drivers/firmware/socfpga-fcs-core.c
>> new file mode 100644
>> index 000000000000..62460fd0e9e4
>> --- /dev/null
>> +++ b/drivers/firmware/socfpga-fcs-core.c
>> @@ -0,0 +1,589 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2026 Altera Corporation
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/of.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/firmware/intel/socfpga-fcs.h>
>> +#include <linux/firmware/intel/stratix10-svc-client.h>
>> +
>> +#define OWNER_ID_OFFSET 12
>> +#define OWNER_ID_SIZE 8
>> +
>> +#define SDOS_DECRYPTION_REPROVISION_KEY_WARN 0x102
>> +#define SDOS_DECRYPTION_NOT_LATEST_KEY_WARN 0x103
>> +
>> +#define MSG_RETRY 3
>> +#define RETRY_SLEEP_MS 1
>> +#define TIMEOUT 1000
>> +
>> +static struct socfpga_fcs_priv *priv;
>> +
>> +/**
>> + * fcs_atf_version_callback() - service-layer callback for the ATF version query
>> + * @client: pointer to the stratix10-svc client
>> + * @data: pointer to the service-layer callback data
>> + *
>> + * Store the returned Arm Trusted Firmware version (or mailbox error) in @priv
>> + * and signal completion to the waiting caller.
>> + */
>> +static void fcs_atf_version_callback(struct stratix10_svc_client *client,
>> + struct stratix10_svc_cb_data *data)
>> +{
>> + struct socfpga_fcs_priv *p = client->priv;
>> +
>> + p->status = data->status;
>> + if (data->status == BIT(SVC_STATUS_OK)) {
>> + p->status = 0;
>> + p->atf_version[0] = *((unsigned int *)data->kaddr1);
>> + p->atf_version[1] = *((unsigned int *)data->kaddr2);
>> + p->atf_version[2] = *((unsigned int *)data->kaddr3);
>> + } else if (data->status == BIT(SVC_STATUS_ERROR)) {
>> + p->status = *((unsigned int *)data->kaddr1);
>> + dev_err(client->dev, "mbox_error=0x%x\n", p->status);
>> + }
>> +
>> + complete(&p->completion);
>> +}
>> +
>> +/**
>> + * fcs_async_callback() - completion callback for an async service request
>> + * @ptr: pointer to the completion to signal
>> + */
>> +static void fcs_async_callback(void *ptr)
>> +{
>> + if (ptr)
>> + complete(ptr);
>> +}
>> +
>> +/**
>> + * fcs_svc_send_request() - build and send an FCS command to the service layer
>> + * @command: FCS command code to dispatch
>> + * @timeout: time to wait for completion, in jiffies
>> + *
>> + * Build the service-layer message for @command and send it through the
>> + * stratix10-svc service driver, using the synchronous path for the ATF version
>> + * query and the asynchronous mailbox path (with retries) for the remaining
>> + * commands.
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>> +static int fcs_svc_send_request(enum fcs_command_code command,
>> + unsigned long timeout)
>> +{
>> + struct fcs_cmd_context *k_ctx = &priv->k_ctx;
>> + struct stratix10_svc_cb_data data;
>> + struct completion completion;
>> + void *handle = NULL;
>> + int status, index;
>> + int ret = 0;
>> + struct stratix10_svc_client_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
>> +
>> + priv->status = 0;
>> + priv->resp = 0;
>> +
>> + switch (command) {
>> + case FCS_DEV_CRYPTO_OPEN_SESSION:
>> + pr_debug("Sending command: COMMAND_FCS_CRYPTO_OPEN_SESSION\n");
>> + msg->command = COMMAND_FCS_CRYPTO_OPEN_SESSION;
>> + break;
>> +
>> + case FCS_DEV_CRYPTO_CLOSE_SESSION:
>> + pr_debug("Sending command: COMMAND_FCS_CRYPTO_CLOSE_SESSION with session_id: 0x%x\n",
>> + priv->session_id);
>> + msg->arg[0] = priv->session_id;
>> + msg->command = COMMAND_FCS_CRYPTO_CLOSE_SESSION;
>> + break;
>> +
>> + case FCS_DEV_ATF_VERSION:
>> + pr_debug("Sending command: COMMAND_SMC_ATF_BUILD_VER\n");
>> + msg->command = COMMAND_SMC_ATF_BUILD_VER;
>> + priv->client.receive_cb = fcs_atf_version_callback;
>
> You've set the receive_cb here for this case, what if calls to the other cases come in? Wouldn't it just go to fcs_atf_version_callback()?
>
Agreed it's fragile to leave a persistent client field set from inside
the switch, though.
Will fix in v2 by set receive_cb right before the sync send and
clear it to NULL right after, so it is non-NULL only for that
transaction and a future sync caller can't inherit a stale callback.
>
>> + break;
>> +
>> + case FCS_DEV_SDOS_DATA_EXT:
>> + pr_debug("Sending command: COMMAND_FCS_SDOS_DATA_EXT with session_id: 0x%x, context_id: 0x%x, op_mode: 0x%x, own: 0x%llx\n",
>> + priv->session_id, k_ctx->sdos.context_id,
>> + k_ctx->sdos.op_mode, k_ctx->sdos.own);
>> + msg->arg[0] = priv->session_id;
>> + msg->arg[1] = k_ctx->sdos.context_id;
>> + msg->arg[2] = k_ctx->sdos.op_mode;
>> + msg->arg[3] = k_ctx->sdos.own;
>> + msg->payload = k_ctx->sdos.src;
>> + msg->payload_length = k_ctx->sdos.src_size;
>> + msg->payload_output = k_ctx->sdos.dst;
>> + msg->payload_length_output = *k_ctx->sdos.dst_size;
>> + msg->command = COMMAND_FCS_SDOS_DATA_EXT;
>> + break;
>> +
>> + default:
>> + pr_err("Unknown command: 0x%x\n", command);
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + if (ret) {
>> + kfree(msg);
>> + return ret;
>> + }
>> +
>> + if (command == FCS_DEV_ATF_VERSION) {
>> + reinit_completion(&priv->completion);
>> +
>> + ret = stratix10_svc_send(priv->chan, msg);
>> + if (ret) {
>> + pr_err("failed to send message to service channel\n");
>> + goto fun_ret;
>> + }
>> +
>> + if (!wait_for_completion_timeout(&priv->completion, timeout)) {
>> + pr_err("svc timeout to get completed status\n");
>> + ret = -ETIMEDOUT;
>> + }
>> +fun_ret:
>> + kfree(msg);
>> + return ret;
>> + }
>> +
>> + init_completion(&completion);
>
> You're mixing a local stack completion with a priv->completion. Doesn't this local declaration of completion get destroy when there's a timeout?And if fcs_async_callback() gets fired, your local completion is no longer valid.
>
Agree, not hit today only because handle->cb is never invoked
(completions are poll-delivered), but it's fragile.
Fixed in v2: dropped the stack completion and reuse the device-lifetime
priv->completion (already used by the sync path; commands are serialized
under priv->lock, one in-flight), so a late complete() lands on a valid
object.
>
>> +
>> + for (index = 0; index < MSG_RETRY; index++) {
>> + status = stratix10_svc_async_send(priv->chan, msg, &handle,
>> + fcs_async_callback,
>> + &completion);
>> + if (status == 0)
>> + break;
>> + msleep(RETRY_SLEEP_MS);
>> + }
>> +
>> + if (!handle || status != 0) {
>> + pr_err("Failed to send async message\n");
>> + kfree(msg);
>> + return -ETIMEDOUT;
>> + }
>> +
>> + ret = wait_for_completion_io_timeout(&completion, (TIMEOUT));
>> + if (ret > 0)
>> + pr_debug("Received async interrupt\n");
>> + else
>> + pr_err("timeout occurred while waiting for async message\n");
>
> So if a timeout happens,
>> +
>> + ret = stratix10_svc_async_poll(priv->chan, handle, &data);
>
> you fall straight into here. There's a reason why the timeout happened, this code ignoring that reason. Shouldn't you handle the timeout?
>
the timeout is only a hint. The wait_for_completion_io_timeout() result is
intentionally a hint - the authoritative status comes from the SDM via
stratix10_svc_async_poll(), so we poll even after a timeout.
will fix in v2 to poll result: STATUS_BUSY (-EAGAIN) -> return -EINPROGRESS
and abort without recycling the transaction id or freeing the in-flight DMA
buffers; any other poll error is propagated;
>> + if (ret) {
>> + pr_err("Failed to poll async message\n");
>> + goto out;
>> + }
>> +
>> + priv->status = data.status;
>> +
>> + if (data.kaddr1)
>> + priv->resp = *((u32 *)data.kaddr1);
>> + else
>> + priv->resp = 0;
>> +
>> +out:
>> + stratix10_svc_async_done(priv->chan, handle);
>> + kfree(msg);
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * fcs_session_open() - open an FCS crypto service session
>> + * @k_ctx: pointer to the kernel-side FCS command context
>> + *
>> + * Request a new session from the SDM, generate the session UUID and copy it,
>> + * together with the mailbox status, back to user space.
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>> +int fcs_session_open(struct fcs_cmd_context *const k_ctx)
>> +{
>> + int ret = 0;
>> +
>> + ret = fcs_svc_send_request(FCS_DEV_CRYPTO_OPEN_SESSION,
>> + SVC_FCS_REQUEST_TIMEOUT_MS);
>> + if (ret) {
>> + pr_err("Failed to send the cmd=%d,ret=%d\n",
>> + FCS_DEV_CRYPTO_OPEN_SESSION, ret);
>> + return ret;
>> + }
>> +
>> + if (priv->status) {
>> + ret = -EIO;
>> + pr_err("Mailbox error, Failed to open session ret: %d\n", ret);
>> + goto copy_mbox_status;
>> + }
>> +
>> + uuid_gen(&priv->uuid_id);
>> +
>> + memcpy(&priv->session_id, &priv->resp, sizeof(priv->session_id));
>> +
>> + ret = copy_to_user(k_ctx->open_session.suuid, &priv->uuid_id,
>> + sizeof(uuid_t)) ? -EFAULT : 0;
>> + if (ret) {
>> + pr_err("Failed to copy session ID to user suuid addr: %p ret: %d\n",
>> + k_ctx->open_session.suuid, ret);
>> + goto copy_mbox_status;
>
> Don't need this goto.
>
right, will remove in v2
>> + }
>> +
>> +copy_mbox_status:
>> + if (copy_to_user(k_ctx->error_code_addr, &priv->status,
>> + sizeof(priv->status))) {
>> + pr_err("Failed to copy mail box status code to user\n");
>> + /* surface the copy failure only if nothing failed earlier */
>> + if (!ret)
>> + ret = -EFAULT;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * fcs_session_close() - close an FCS crypto service session
>> + * @k_ctx: pointer to the kernel-side FCS command context
>> + *
>> + * Validate the caller-supplied session UUID, ask the SDM to close the session
>> + * and copy the mailbox status back to user space.
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>> +int fcs_session_close(struct fcs_cmd_context *const k_ctx)
>> +{
>> + int ret = 0;
>> + struct fcs_cmd_context ctx;
>> +
>> + memcpy(&ctx, k_ctx, sizeof(struct fcs_cmd_context));
>> +
>> + if (!uuid_equal(&priv->uuid_id, &k_ctx->close_session.suuid)) {
>> + ret = -EINVAL;
>> + pr_err("Session UUID Mismatch ret: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = fcs_svc_send_request(FCS_DEV_CRYPTO_CLOSE_SESSION,
>> + SVC_FCS_REQUEST_TIMEOUT_MS);
>> + if (ret) {
>> + pr_err("Failed to send the cmd=%d,ret=%d\n",
>> + FCS_DEV_CRYPTO_CLOSE_SESSION, ret);
>> + return ret;
>> + }
>> +
>> + memset(&priv->uuid_id, 0, sizeof(uuid_t));
>> + priv->session_id = 0;
>> + if (priv->status) {
>> + ret = -EIO;
>> + pr_err("Mailbox error, Failed to close session ret: %d\n", ret);
>> + }
>> +
>> + if (copy_to_user(ctx.error_code_addr, &priv->status,
>> + sizeof(priv->status))) {
>> + pr_err("Failed to copy mail box status code to user\n");
>> + /* surface the copy failure only if nothing failed earlier */
>> + if (!ret)
>> + ret = -EFAULT;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * fcs_get_atf_version() - return the cached Arm Trusted Firmware version
>> + * @version: array of three u32 entries to receive the major, minor and patch
>> + * version numbers
>> + */
>> +void fcs_get_atf_version(u32 *version)
>> +{
>> + memcpy(version, priv->atf_version, sizeof(priv->atf_version));
>> +}
>> +
>> +/**
>> + * fcs_sdos_crypt() - perform an SDOS encrypt or decrypt operation
>> + * @k_ctx: pointer to the kernel-side FCS command context
>> + *
>> + * Allocate service-layer source and destination buffers, copy the input from
>> + * user space, drive the SDOS data command and copy the result and length back
>> + * to user space. The operation direction is selected by @k_ctx->sdos.op_mode.
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>> +int fcs_sdos_crypt(struct fcs_cmd_context *const k_ctx)
>> +{
>> + void *s_buf = NULL, *d_buf = NULL;
>> + struct fcs_cmd_context ctx;
>> + u32 output_size;
>> + u32 dst_cap;
>> + u64 owner_id;
>> + int ret = 0;
>> + char *temp;
>> +
>> + memcpy(&ctx, k_ctx, sizeof(struct fcs_cmd_context));
>> +
>> + if (!ctx.sdos.dst || !ctx.sdos.dst_size)
>> + return -EINVAL;
>> +
>> + /* Caller-provided output buffer capacity (in/out parameter) */
>> + if (copy_from_user(&dst_cap, ctx.sdos.dst_size, sizeof(dst_cap)))
>> + return -EFAULT;
>> +
>> + if (ctx.sdos.op_mode) {
>> + output_size = SDOS_ENCRYPTED_MAX_SZ;
>> + /* encrypt: input is header + plaintext */
>> + if (k_ctx->sdos.src_size < SDOS_DECRYPTED_MIN_SZ ||
>> + k_ctx->sdos.src_size > SDOS_DECRYPTED_MAX_SZ) {
>
> You've already copied k_ctx to a local ctx, you should consistently use it ctx.
>
Agree, will fix in v2 the read/validation accesses in fcs_sdos_crypt()
to use the local ctx snapshot (src_size checks, buffer sizing,
copy_from_user src). I'll apply the same ctx cleanup to fcs_session_close()
>
>> + pr_err("Invalid SDOS src_size %u\n", k_ctx->sdos.src_size);
>> + return -EINVAL;
>> + }
>> + } else {
>> + output_size = SDOS_DECRYPTED_MAX_SZ;
>> + /* decrypt: input is header + plaintext + HMAC */
>> + if (k_ctx->sdos.src_size < SDOS_ENCRYPTED_MIN_SZ ||
>> + k_ctx->sdos.src_size > SDOS_ENCRYPTED_MAX_SZ) {
>> + pr_err("Invalid SDOS src_size %u\n", k_ctx->sdos.src_size);
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + s_buf = stratix10_svc_allocate_memory(priv->chan, k_ctx->sdos.src_size);
>> + if (IS_ERR(s_buf)) {
>> + ret = -ENOMEM;
>> + pr_err("Failed to allocate memory for SDOS input data kernel buffer ret: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + k_ctx->sdos.dst_size = &output_size;
>
> output_size is declared locally on a stack, but you're assigning the address to a long-living structure. That leaks the stack address and will leave you with a dangling pointer.
>
Agree, k_ctx points at priv->k_ctx (device-lifetime), so storing
&output_size (stack) into k_ctx->sdos.dst_size leaves a dangling pointer
once fcs_sdos_crypt() returns.
Fixed in v2: added a device-lifetime backing field priv->sdos_output_size
and point dst_size at that instead of a stack variable, so the staged
pointer can't dangle. Access stays serialized under priv->lock.
>
>> +
>> + d_buf = stratix10_svc_allocate_memory(priv->chan, *k_ctx->sdos.dst_size);
>> + if (IS_ERR(d_buf)) {
>> + ret = -ENOMEM;
>> + pr_err("Failed to allocate memory for SDOS output kernel buffer ret: %d\n", ret);
>> + goto free_sbuf;
>> + }
>> +
>> + /* Copy the user space input data to the input data kernel buffer */
>> + ret = copy_from_user(s_buf, k_ctx->sdos.src,
>> + k_ctx->sdos.src_size) ? -EFAULT : 0;
>> + if (ret) {
>> + pr_err("Failed to copy SDOS data from user to kernel buffer ret: %d\n", ret);
>> + goto free_dbuf;
>> + }
>> +
>> + /* Get Owner ID from buf */
>> + temp = (uint8_t *)s_buf;
>
> temp is only used here, just declare as a (uint8_t *) to start with.
>
Agreed - temp was only a byte cursor into s_buf, so in v2 I dropped it
and cast inline.