Re: [PATCH v2 16/19] crypto: cmh - add SLH-DSA/LMS/XMSS (HCQ)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The SLH-DSA, LMS, and XMSS verification and signing functions unconditionally unmap and free DMA-mapped buffers on timeout, leading to potential use-after-free and memory corruption by the hardware.
- [High] LMS and XMSS algorithms fail to implement the `.max_size` callback in their `struct sig_alg` definitions.
- [High] SLH-DSA, LMS, and XMSS algorithms fail to implement the `.digest_size` callback in their `struct sig_alg` definitions.
- [Medium] The PQC signature algorithms incorrectly reject 0-length messages, preventing empty message signing and verification.
--

--- Patch [16]: [PATCH v2 16/19] crypto: cmh - add SLH-DSA/LMS/XMSS (HCQ) ---
commit aa5a6d3ad22af27c0421ed22c5126da35c7caa5b
Author: Alex Ousherovitch <aousherovitch@rambus.com>

crypto: cmh - add SLH-DSA/LMS/XMSS (HCQ)

Register SLH-DSA, LMS, LMS-HSS, XMSS, and XMSS-MT algorithms
using the CMH HCQ core (core ID 0x08).  SLH-DSA is registered as a
sig algorithm with sign and verify support.

> --- a/drivers/crypto/cmh/cmh_pqc_slhdsa.c
> +++ b/drivers/crypto/cmh/cmh_pqc_slhdsa.c
> @@ -54,23 +54,342 @@ static inline struct cmh_slhdsa_tfm_ctx *cmh_slhdsa_ctx(struct crypto_sig *tfm)
>  static int cmh_slhdsa_sign(struct crypto_sig *tfm,
>                             const void *src, unsigned int slen,
>                             void *dst, unsigned int dlen)
>  {
[ ... ]
>         if (ctx->key.mode == CMH_KEY_NONE)
>                 return -EINVAL;
>         if (dlen < sig_sz)
>                 return -EINVAL;
>         if (!slen || slen > SLHDSA_MAX_MSG_LEN)
>                 return -EINVAL;

[Severity: Medium]
Does this incorrectly reject valid 0-length messages? Empty strings are valid 
inputs for SLH-DSA, but this returns -EINVAL when slen is 0. Is this intended 
to prevent a ZERO_SIZE_PTR allocation from kmemdup()? 

>         m_buf = kmemdup(src, slen, GFP_KERNEL);
[ ... ]
>         ret = cmh_tm_submit_sync_tmo(vcq, vcq_count, 1, d.mbx_idx,
>                                      cmh_tm_slow_op_timeout_jiffies());
> 
>         if (!ret) {
>                 /* Sync bounce buffer so CPU sees the DMA-written signature */
>                 cmh_dma_sync_for_cpu(sig_dma, sig_sz, DMA_FROM_DEVICE);
>                 memcpy(dst, sig_buf, sig_sz);
>                 ret = sig_sz;
>         }
> 
> out_unmap:
>         if (sk_buf) {
>                 if (!cmh_dma_map_error(sk_dma))
>                         cmh_dma_unmap_single(sk_dma, sk_sz, DMA_TO_DEVICE);
>                 kfree_sensitive(sk_buf);
>         }
>         if (!cmh_dma_map_error(sig_dma))
>                 cmh_dma_unmap_single(sig_dma, sig_sz, DMA_FROM_DEVICE);
>         if (!cmh_dma_map_error(m_dma))
>                 cmh_dma_unmap_single(m_dma, slen, DMA_TO_DEVICE);
> 
> out_free:
>         kfree(sig_buf);
>         kfree(m_buf);
>         return ret;
>  }

[Severity: Critical]
Are these buffers safely unmapped and freed if the transaction times out?

If cmh_tm_submit_sync_tmo() returns -ETIMEDOUT, the documentation in the TM
subsystem notes that the hardware abort might fail, leaving the hardware actively
writing to the DMA buffers. Because kfree() is called unconditionally on 
sk_buf, sig_buf, and m_buf here, could this allow the hardware to corrupt 
whatever data is subsequently allocated in those physical pages?

> @@ -165,22 +165,58 @@ static int cmh_slhdsa_verify(struct crypto_sig *tfm,
>                               const void *src, unsigned int slen,
>                               const void *digest, unsigned int dlen)
>  {
[ ... ]
>         if (!dlen || dlen > SLHDSA_MAX_MSG_LEN)
>                 return -EINVAL;

[Severity: Medium]
Similar to the sign function, does this prevent verifying 0-length messages 
which are otherwise mathematically valid for SLH-DSA?

>         sig_buf = kmemdup(src, slen, GFP_KERNEL);
[ ... ]
>         /* SLH-DSA verify recomputes hyper-tree hashes -- inherently slow */
>         ret = cmh_tm_submit_sync_tmo(vcq, HCQ_VCQ_CMDS_MIN, 1, d.mbx_idx,
>                                      cmh_tm_slow_op_timeout_jiffies());
> 
> out_unmap:
>         if (!cmh_dma_map_error(pk_dma))
>                 cmh_dma_unmap_single(pk_dma, pk_sz, DMA_TO_DEVICE);
>         if (!cmh_dma_map_error(m_dma))
>                 cmh_dma_unmap_single(m_dma, dlen, DMA_TO_DEVICE);
>         if (!cmh_dma_map_error(sig_dma))
>                 cmh_dma_unmap_single(sig_dma, sig_sz, DMA_TO_DEVICE);
> 
> out_free:
>         kfree(pk_buf);
>         kfree(m_buf);
>         kfree(sig_buf);
>         return ret;
>  }

[Severity: Critical]
Are pk_buf, m_buf, and sig_buf safe to free here on a timeout? 

For the same reasons as above, if cmh_tm_submit_sync_tmo() returns -ETIMEDOUT, 
does unconditionally freeing these buffers risk memory corruption by the 
hardware?

> @@ -313,24 +313,64 @@ SLHDSA_INIT(HCQ_SLHDSA_SHA2_256F)
>  #define SLHDSA_ALG(name, drv, ps_val) {                                        \
>                 .sign           = cmh_slhdsa_sign,                      \
>                 .verify         = cmh_slhdsa_verify,                    \
>                 .set_pub_key    = cmh_slhdsa_set_pub_key,               \
>                 .set_priv_key   = cmh_slhdsa_set_priv_key,              \
>                 .key_size       = cmh_slhdsa_key_size,                  \
>                 .max_size       = cmh_slhdsa_max_size,                  \
>                 .init           = cmh_slhdsa_init_##ps_val,             \
>                 .exit           = cmh_slhdsa_exit,                      \
>                 .base = {                                               \

[Severity: High]
Is the .digest_size callback missing from this algorithm definition?

When .digest_size is not provided, crypto_sig_digestsize() falls back to the
public key size. For SLH-DSA, could this cause valid sign/verify operations to
be prematurely rejected with -EINVAL by the asymmetric_keys subsystem if the 
payload is larger than the small public key?

> --- a/drivers/crypto/cmh/cmh_pqc_lms.c
> +++ b/drivers/crypto/cmh/cmh_pqc_lms.c
> @@ -49,20 +49,60 @@ static int cmh_lms_verify(struct crypto_sig *tfm,
>                            const void *src, unsigned int slen,
>                            const void *digest, unsigned int dlen)
>  {
[ ... ]
>         if (!dlen || dlen > LMS_MAX_MSG_LEN)
>                 return -EINVAL;

[Severity: Medium]
Does this incorrectly reject valid 0-length messages for LMS?

>         sig_buf = kmemdup(src, slen, GFP_KERNEL);
[ ... ]
>         /* LMS verify traverses Merkle hash chains -- inherently slow */
>         ret = cmh_tm_submit_sync_tmo(vcq, LMS_VCQ_CMDS, 1, d.mbx_idx,
>                                      cmh_tm_slow_op_timeout_jiffies());
> 
> out_unmap:
>         if (!cmh_dma_map_error(pk_dma))
>                 cmh_dma_unmap_single(pk_dma, ctx->pub_key_len, DMA_TO_DEVICE);
>         if (!cmh_dma_map_error(m_dma))
>                 cmh_dma_unmap_single(m_dma, dlen, DMA_TO_DEVICE);
>         if (!cmh_dma_map_error(sig_dma))
>                 cmh_dma_unmap_single(sig_dma, slen, DMA_TO_DEVICE);
> 
> out_free:
>         kfree(pk_buf);
>         kfree(m_buf);
>         kfree(sig_buf);
>         return ret;
>  }

[Severity: Critical]
Does unconditionally freeing pk_buf, m_buf, and sig_buf on a timeout risk
use-after-free and memory corruption by the hardware?

> @@ -164,36 +164,36 @@ static void cmh_lms_exit(struct crypto_sig *tfm)
>  static struct sig_alg cmh_lms_algs[] = {
>         {
>                 .verify         = cmh_lms_verify,
>                 .set_pub_key    = cmh_lms_set_pub_key,
>                 .key_size       = cmh_lms_key_size,
>                 .init           = cmh_lms_init,
>                 .exit           = cmh_lms_exit,
>                 .base = {

[Severity: High]
Are both the .max_size and .digest_size callbacks missing here?

Without .max_size, crypto_sig_maxsize() defaults to the public key size. 
Because LMS signatures are much larger than their public keys, won't this 
cause standard verification requests to fail with -EINVAL? 

Similarly, missing .digest_size may cause operations to be rejected when the 
message size exceeds the public key size.

> --- a/drivers/crypto/cmh/cmh_pqc_xmss.c
> +++ b/drivers/crypto/cmh/cmh_pqc_xmss.c
> @@ -49,20 +49,60 @@ static int cmh_xmss_verify(struct crypto_sig *tfm,
>                             const void *src, unsigned int slen,
>                             const void *digest, unsigned int dlen)
>  {
[ ... ]
>         if (!dlen || dlen > XMSS_MAX_MSG_LEN)
>                 return -EINVAL;

[Severity: Medium]
Does this incorrectly reject valid 0-length messages for XMSS?

>         sig_buf = kmemdup(src, slen, GFP_KERNEL);
[ ... ]
>         /* XMSS verify traverses Merkle hash chains -- inherently slow */
>         ret = cmh_tm_submit_sync_tmo(vcq, XMSS_VCQ_CMDS, 1, d.mbx_idx,
>                                      cmh_tm_slow_op_timeout_jiffies());
> 
> out_unmap:
>         if (!cmh_dma_map_error(pk_dma))
>                 cmh_dma_unmap_single(pk_dma, ctx->pub_key_len, DMA_TO_DEVICE);
>         if (!cmh_dma_map_error(m_dma))
>                 cmh_dma_unmap_single(m_dma, dlen, DMA_TO_DEVICE);
>         if (!cmh_dma_map_error(sig_dma))
>                 cmh_dma_unmap_single(sig_dma, slen, DMA_TO_DEVICE);
> 
> out_free:
>         kfree(pk_buf);
>         kfree(m_buf);
>         kfree(sig_buf);
>         return ret;
>  }

[Severity: Critical]
Does unconditionally freeing pk_buf, m_buf, and sig_buf on a timeout risk
use-after-free and memory corruption by the hardware?

> @@ -164,36 +164,36 @@ static void cmh_xmss_exit(struct crypto_sig *tfm)
>  static struct sig_alg cmh_xmss_algs[] = {
>         {
>                 .verify         = cmh_xmss_verify,
>                 .set_pub_key    = cmh_xmss_set_pub_key,
>                 .key_size       = cmh_xmss_key_size,
>                 .init           = cmh_xmss_init,
>                 .exit           = cmh_xmss_exit,
>                 .base = {

[Severity: High]
Are both the .max_size and .digest_size callbacks missing here?

As with LMS, missing .max_size means crypto_sig_maxsize() defaults to the 
public key size, preventing validation of XMSS signatures because they are 
larger than their public keys. Missing .digest_size may also break validation 
for any payload larger than the public key.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709203037.1884436-1-skrishnamoorthy@rambus.com?part=16