Re: [PATCH v3 17/19] Documentation: ioctl: add CMH ioctl documentation and register 'J'

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] UAPI Architecture Anti-Pattern: Bypassing standard cryptographic subsystems with a massive custom ioctl interface.
- [High] Privilege Separation and Access Control Violation in device node.
- [Medium] Misleading UAPI Naming in SM2 ECDH Hash struct leads to API misuse.
- [Medium] Hardware Error Management Anti-Pattern: Exposing read-to-clear error state directly to user-space.
--

--- Patch [17]: [PATCH v3 17/19] Documentation: ioctl: add CMH ioctl documentation and register 'J' ---
commit dcc392785ff594e59facfd1248a64d489fc2a2fd
Author: Alex Ousherovitch <[email protected]>

Documentation: ioctl: add CMH ioctl documentation and register 'J'

This commit adds documentation for the /dev/cmh_mgmt misc device ioctls
used by the Rambus CryptoManager Hub (CMH) hardware crypto accelerator. It
covers key management, derivation, public-key cryptography, post-quantum
cryptography, and hardware management. It also registers the ioctl magic
number 'J' (0x4A) in ioctl-number.rst.

> diff --git a/Documentation/userspace-api/ioctl/cmh_mgmt.rst b/Documentation/userspace-api/ioctl/cmh_mgmt.rst
> new file mode 100644
> index 0000000000000..d17a77975df6c
> --- /dev/null
> +++ b/Documentation/userspace-api/ioctl/cmh_mgmt.rst

[ ... ]

> +Introduction
> +============
> +
> +The ``/dev/cmh_mgmt`` character device provides user-space access to key
> +management, key derivation, public-key, and post-quantum cryptographic
> +operations on the CryptoManager Hub (CMH) hardware accelerator.
> +
> +The device is created by the ``cmh`` kernel module as a ``misc_device``.
> +All operations are synchronous -- the ioctl blocks until the hardware
> +completes.  Opening the device requires ``CAP_SYS_ADMIN``.

[Severity: High]
Could the global permission check on the device node create a privilege
escalation regression?

Looking at cmh_mgmt_open() in drivers/crypto/cmh/cmh_mgmt.c:

    if (!capable(CAP_SYS_ADMIN))
        return -EPERM;

If an administrator opens /dev/cmh_mgmt and delegates the file descriptor
to an unprivileged process for cryptographic data-plane operations, the
lack of per-ioctl capability checks means that process can escalate its
access to invoke system management operations like DRBG_CONFIG or EAC_READ.

Alternatively, if delegation is not used, this structure forces standard
applications to run as root just to access the crypto hardware.

[ ... ]

> +CMH_IOCTL_SM2_ECDH_HASH
> +~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Derive the shared key from the shared point and ZA digests.
> +
> +:Direction: ``_IOWR``
> +:Number: 0x35
> +:Argument: ``struct cmh_ioctl_sm2_ecdh_hash``
> +
> +::
> +
> +  struct cmh_ioctl_sm2_ecdh_hash {
> +      __u32 version;
> +      __u32 __reserved;
> +      __u64 peer_id_digest;    /* ptr to Z_A -- initiator's digest (32B) */
> +      __u64 id_digest;         /* ptr to Z_B -- responder's digest (32B) */

[Severity: Medium]
Will this struct naming lead to API misuse and silent protocol failures?

The names peer_id_digest and id_digest naturally imply relative ownership,
but the documentation requires absolute protocol ordering. If a developer
implements the initiator role using the uapi struct fields as named, they
must pass their own digest as peer_id_digest, which contradicts the naming.

[ ... ]

> +Hardware Management ioctls
> +==========================
> +
> +CMH_IOCTL_EAC_READ
> +-------------------
> +
> +Read and clear the hardware Error and Alarm Controller registers.

[ ... ]

> +The eSW atomically reads and clears the registers on each call.
> +Successive reads show only new events since the last read.

[Severity: Medium]
Does exposing read-to-clear error state directly to user-space risk silent
loss of hardware error events?

Looking at cmh_mgmt_eac_read() in drivers/crypto/cmh/cmh_mgmt.c:

    vcq_add_eac_read(&vcq[1], rsp_dma, sizeof(*rsp));

The driver unconditionally fetches and clears the state from hardware
without multiplexing or caching it for other listeners. If multiple
processes, or a daemon and a debugging tool, invoke the EAC_READ ioctl,
each caller steals events from the others, preventing reliable error
monitoring or telemetry.

[ ... ]

> +Relationship to the in-kernel crypto API
> +=========================================
> +
> +The main reason these operations are exposed as ioctls, rather than
> +through the standard in-kernel crypto API, is the CMH datastore key
> +model: an ioctl can operate on a *datastore-referenced* (hardware-held)

[Severity: High]
Does bypassing standard cryptographic subsystems introduce a fragmentation
regression for the user-space ecosystem?

By bypassing standard kernel abstractions like AF_ALG or the keys subsystem
in favor of a vendor-specific uapi, this approach commits the kernel to
permanently maintaining a massive set of custom ioctls and requires
applications to implement vendor-specific code instead of relying on
standard interfaces.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=17