[Deepin-Kernel-SIG] [linux 6.6.y] [FROMLIST] [Security] crypto: af_alg - Drop support for off-CPU cryptography#1805
Conversation
Reviewer's GuideRestricts AF_ALG to use only in-kernel, synchronous, software crypto implementations by hardcoding a crypto algorithm mask and simplifying the AF_ALG bind interface, while updating all AF_ALG users and its userspace documentation accordingly. Sequence diagram for AF_ALG bind path using restricted crypto implementationssequenceDiagram
actor Userspace
participant AF_ALG_Socket as AF_ALG_socket
participant AF_ALG_Core as alg_bind
participant AF_ALG_Type as af_alg_type
participant CryptoAPI
Userspace->>AF_ALG_Socket: bind(salg_name, salg_feat, salg_mask)
AF_ALG_Socket->>AF_ALG_Core: alg_bind(sock, sockaddr_alg)
AF_ALG_Core->>AF_ALG_Type: bind(salg_name)
note over AF_ALG_Type: aead_bind/hash_bind/rng_bind/skcipher_bind
AF_ALG_Type->>CryptoAPI: crypto_alloc_* (name, 0, AF_ALG_CRYPTOAPI_MASK)
CryptoAPI-->>AF_ALG_Type: crypto_instance
AF_ALG_Type-->>AF_ALG_Core: private
AF_ALG_Core-->>AF_ALG_Socket: bind result
AF_ALG_Socket-->>Userspace: success or error
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
AF_ALG is deprecated and exposed to unprivileged userspace. Only use the least buggy algorithm implementations: the pure software ones. This removes one of the main advantages of AF_ALG, which is the ability to use it with off-CPU accelerators. However, using off-CPU accelerators has huge overheads, both in performance and attack surface. I have yet to see real-world, performance-critical workloads where using an accelerator via AF_ALG is actually a win over doing cryptography in userspace. If using an off-CPU accelerator really does turn out to be a win, a new API should be developed that is actually a good fit for it. Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> [WangYuli: Rewirte userspace-if.rst because of conflicts] Link: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=7524070f26d8d347c26787dc297fb844baa26abf Signed-off-by: WangYuli <wangyl5933@chinaunicom.cn>
3b0cdf2 to
3dee94d
Compare
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- AF_ALG_CRYPTOAPI_MASK duplicates FSCRYPT_CRYPTOAPI_MASK semantics; consider centralizing this mask definition in a shared header to avoid future divergence between the two.
- Since alg_bind now ignores salg_feat and salg_mask and always applies AF_ALG_CRYPTOAPI_MASK, it would be helpful to explicitly mention in the UAPI-facing docs or comments that these sockaddr fields are no longer honored and are effectively ignored.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- AF_ALG_CRYPTOAPI_MASK duplicates FSCRYPT_CRYPTOAPI_MASK semantics; consider centralizing this mask definition in a shared header to avoid future divergence between the two.
- Since alg_bind now ignores salg_feat and salg_mask and always applies AF_ALG_CRYPTOAPI_MASK, it would be helpful to explicitly mention in the UAPI-facing docs or comments that these sockaddr fields are no longer honored and are effectively ignored.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR tightens the AF_ALG userspace crypto interface by preventing selection of off-CPU / driver-backed Crypto API implementations, aiming to reduce attack surface and avoid problematic accelerator drivers. It also simplifies the AF_ALG type binding callback interface and updates the userspace documentation to reflect the new constraints.
Changes:
- Simplify
struct af_alg_type’sbind()callback to accept only the algorithm name (droppingtype/maskparameters). - Enforce software-only / safer Crypto API implementation selection by allocating with a fixed
AF_ALG_CRYPTOAPI_MASK. - Add documentation note warning that hardware accelerator access via AF_ALG is removed.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| include/crypto/if_alg.h | Changes AF_ALG type bind API and introduces AF_ALG_CRYPTOAPI_MASK for restricting implementations. |
| Documentation/crypto/userspace-if.rst | Documents removal of hardware accelerator exposure via AF_ALG. |
| crypto/algif_skcipher.c | Allocates skcipher with fixed mask to exclude off-CPU/driver-only implementations. |
| crypto/algif_rng.c | Allocates RNG with fixed mask to exclude off-CPU/driver-only implementations. |
| crypto/algif_hash.c | Allocates ahash with fixed mask to exclude off-CPU/driver-only implementations. |
| crypto/algif_aead.c | Allocates AEAD with fixed mask to exclude off-CPU/driver-only implementations. |
| crypto/af_alg.c | Updates bind path to use the simplified bind(name) callback signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * This is the same as FSCRYPT_CRYPTOAPI_MASK in fs/crypto/fscrypt_private.h. | ||
| * In additions to the motivations there, this API is exposed to userspace | ||
| * that might not be fully trusted. |
AF_ALG is deprecated and exposed to unprivileged userspace. Only use the least buggy algorithm implementations: the pure software ones.
This removes one of the main advantages of AF_ALG, which is the ability to use it with off-CPU accelerators. However, using off-CPU accelerators has huge overheads, both in performance and attack surface. I have yet to see real-world, performance-critical workloads where using an accelerator via AF_ALG is actually a win over doing cryptography in userspace.
If using an off-CPU accelerator really does turn out to be a win, a new API should be developed that is actually a good fit for it.
[WangYuli: Rewirte userspace-if.rst because of conflicts]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=7524070f26d8d347c26787dc297fb844baa26abf
Summary by Sourcery
Restrict AF_ALG userspace crypto interface to software-based Crypto API implementations and update the API and docs accordingly.
Enhancements: