Re: [PATCH v1 0/2] Add Altera SoCFPGA Crypto Service (FCS) driver

[Hang Suan Wang]

On 6/7/2026 8:33 pm, Dinh Nguyen wrote:
> 
> 
> On 7/1/26 02:39, hang.suan.wang@xxxxxxxxxx wrote:
>> From: Hang Suan Wang <hang.suan.wang@xxxxxxxxxx>
>>
>> This series adds support for the Altera SoCFPGA Crypto Service (FCS), the
>> runtime cryptographic interface provided by the Secure Device Manager
>> (SDM). The SDM is the hardware security controller in Altera SoCFPGA
>> devices. It acts as the root-of-trust device and controls access to
>> built-in cryptographic hardware such as AES, SHA, a true random number
>> generator, and Intel PUF. The SDM is responsible for security-critical
>> functions including secure boot, FPGA bitstream authentication and optional
>> decryption, remote system update, and runtime crypto services. On the HPS
>> side, software reaches the SDM through a mailbox interface exposed in Linux
>> via the stratix10-svc layer, which uses Arm Trusted Firmware SIP SMC calls
>> underneath.
>>
>> The FPGA Crypto Service (FCS) is the runtime crypto interface provided by
>> the SDM. It covers services such as random number generation, AES
>> operations, HMAC/SHA, key management, attestation, and related security
>> functions.
>>
>> This series implements one FCS feature: the Secure Data Object Service
>> (SDOS), which protects sensitive data at rest. With SDOS the SDM encrypts
>> and decrypts data using a key derived from a device-unique root key that
>> never leaves the secure boundary, plus an SDM-generated IV. The host never
>> handles raw key material or IVs: it supplies plaintext and receives an
>> authenticated ciphertext object (and vice versa for decryption). A primary
>> use case is black key provisioning, where operational keys are installed in
>> protected form without ever being exposed in cleartext.
>>
>> The driver reaches the SDM through the existing stratix10-svc mailbox using
>> the Arm Trusted Firmware SIP SMC transport. Data buffers are allocated from
>> the service-layer memory pool, which provides physically-contiguous memory
>> whose physical address is handed to the SDM.
>>
>> The series is organized as follows:
>>   - Patch 1 (prerequisite) extends the stratix10-svc service layer with the
>>     FCS command codes and matching SIP SMC function IDs, adds the Agilex 5
>>     (intel,agilex5-svc) match, and registers a "stratix10-fcs" platform
>>     device that an FCS client driver binds to.
>>
>>   - Patch 2 adds the FCS firmware driver implementing SDOS encrypt/decrypt
>>     and the crypto-session lifecycle, exposed via sysfs. It relies on the
>>     command codes and the device from patch 1, so patch 1 must be applied
>>     first.
>>
>> Testing:
>>   - Built for arm64 (defconfig + CONFIG_ALTERA_SOCFPGA_FCS=m).
>>
> 
> There are a few sashiko comments on this. Can you verify that they are applicable?
> 
> https://sashiko.dev/#/patchset/cover.1782888532.git.hang.suan.wang%40altera.com
> 
> Thanks,
> Dinh


Thanks for the sashiko review. Responses below, grouped by disposition.

=== Fixes coming in v2 ===
1) Async payload not returned (stratix10_svc_async_prepare_response)
Correct. stratix10_svc_async_poll() zeroes data, and prepare_response()
never sets kaddr1 for COMMAND_FCS_CRYPTO_OPEN_SESSION and
COMMAND_FCS_SDOS_DATA_EXT, so the client reads priv->resp = 0. The
session ID and SDOS output length never reach userspace. This is not
about SDM trustworthiness -- the payload register is simply never
forwarded. 
Fixed in v2.

2) wait_for_completion_timeout() units
Correct. SVC_FCS_REQUEST_TIMEOUT_MS is in milliseconds but is passed
directly as jiffies, so the real timeout scales with CONFIG_HZ (2 s at
HZ=1000, 20 s at HZ=100). I'll wrap it with msecs_to_jiffies() and drop
the redundant TIMEOUT define in favor of the named _MS macro. Same fix
for the wait_for_completion_io_timeout() call on the async path.
Fixed in v2.

3) Freeing the handle on timeout (UAF risk)
The handle is kernel-only; the SDM tracks only an integer
transaction_id, so firmware never touches the handle. But on timeout
stratix10_svc_async_poll() returns -EAGAIN (SDM still BUSY) and the old
code called stratix10_svc_async_done(), freeing the handle and releasing
the transaction_id while the SDM still owned it -- enabling id
reuse/aliasing (and a real UAF once handle->cb is wired up). Fix: on
-EAGAIN, return -ETIMEDOUT without async_done(), leaving the transaction
tracked so its id is not recycled; only terminal OK/ERROR statuses free
the handle. Fully reclaiming a never-completing transaction needs
SVC-layer cancellation, which I'll track as a follow-up.
Fixed in v2.

4) SDOS missing UUID check
Correct. fcs_session_close() validates sdos.suuid against priv->uuid_id,
but fcs_sdos_crypt() does not, so the token is ignored. I'll add the
same uuid_equal() check early in fcs_sdos_crypt(). Clarification: the
attrs are DEVICE_ATTR_WO (root-only by default), so it is not an
unprivileged bypass, but the missing check is still wrong and weakens
per-session ownership.
Fixed in v2.

5) Premature free of DMA buffer on timeout
Correct. fcs_svc_send_request() will return a distinct code
(-EINPROGRESS) for the in-flight abort so fcs_sdos_crypt() skips freeing
s_buf/d_buf on that path (leaking them safely with the abandoned
transaction) while still freeing them on genuine send failures.
Fixed in v2.

6) NULL-deref / singleton priv
Agreed. The mutex cannot guard this since it lives inside priv, so
mutex_lock(&priv->lock) is exactly what dereferences NULL. FCS is
inherently single-instance (one SDM / one SVC_CLIENT_FCS channel), so
fcs_init() will return -EBUSY if priv is already set (checked before
allocating, so a rejected second probe cannot NULL a bound instance),
plus a NULL-guard in fcs_acquire_cmd_ctx(). This also covers the
devm_kzalloc() overwrite/orphan concern.
Fixed in v2.

7) Uninitialized priv->platform
Correct, it was never assigned and always returned 0. Since there is no
platform-detection source wired up, I've removed the platform sysfs
attribute and its supporting code (fcs_get_platform(), the platform
field, the unused AGILEX5_PLAT define) rather than ship a stub. It can be
reintroduced with real detection later.
Fixed in v2.

8) DEVICE_ATTR on a raw kobject (wrong first arg / panic)
Right, attaching device_attributes to a standalone kobject means the
callbacks get a struct kobject * where they expect struct device *.
Fixed: attributes are now attached to the platform device via
.dev_groups, and the manual kobject_create_and_add() is removed. This
also resolves the related comments about using pdev->dev.kobj instead of
a global /sys/kernel/fcs_sysfs directory, and the sysfs_create_groups()
type mismatch.

9) fw_np refcount in stratix10_svc_init()
of_find_matching_node() does drop the reference on fw_np, so it is used
ref-less by of_platform_populate(). This is pre-existing and unrelated to
this series, and causes no observable failure because the firmware node
is static (non-OF_DYNAMIC) and never freed. I'd prefer to address it as
a separate standalone fix to keep this series focused.

10) Dropping arg[3] in the sync copy
Intentional. stratix10_svc_send() copies only arg[0..2] into
stratix10_svc_data, the FIFO entry for the synchronous mailbox thread,
whose commands use at most arg[0..1]. arg[3] is used solely by the async
SMC path (stratix10_svc_async_send()), which reads p_msg->arg[3]
directly and never goes through the FIFO struct. No command loses data.

11) Overwriting priv->session_id on repeated open
The previous session is not leaked: the SDM rejects opening a new
session while one is active, so fcs_session_open() takes the priv->status
error path and returns before the memcpy into priv->session_id; the
existing session_id/uuid_id are preserved and remain closable.

12) Passing a struct fcs_cmd_context * through the sysfs buffer
Acknowledged; this is not standard sysfs ASCII I/O. It is the current
design choice for this interface, since the controls are mostly scalar
and independent.

13) Raw pointers in the copied-in struct / 32-bit compat
Acknowledged; the context embeds native pointers, so layout and pointer
widths differ between 32-bit userspace and a 64-bit kernel. That is the
current design.