crypto: caam - Fix RSA destination buffer handling
v2 cover crypto: caam - Fix RSA destination buffer handling
This series fixes two issues in the rsa-caam driver.
Patch 1 fixes wrong unmap operations where rsa_pub_unmap() was called regardless of the key type.
Patch 2 adds a bounce buffer to handle unaligned RSA destination buffers on DMA-incoherent hardware such as i.MX8, fixing intermittent -EKEYREJECTED errors when loading signed kernel modules.
Changes since v1:
- Patch 1: Add Fixes: tag; pick up Reviewed-by from Sahil Malhotra.
- Patch 2: Move the !bounce_buf check out of do_rsa_bounce_buf() into the caller rsa_bounce_buf_done() so the helper is only entered when a bounce buffer actually exists; add Reported-by/Closes and Tested-by from Martin Kepplinger-Novakovic.
Link to v1:
https://lore.kernel.org/linux-crypto/[email protected]/
Changwei Zou (2):
crypto: caam - Fix wrong unmap operations
crypto: caam - Use bounce buffer for unaligned RSA destination buffers
drivers/crypto/caam/caampkc.c | 123 ++++++++++++++++++++++++++++++---- drivers/crypto/caam/caampkc.h | 6 ++ 2 files changed, 116 insertions(+), 13 deletions(-)
Signature · 1 line
2.43.0
PATCH v2 1/2 crypto: caam - Fix wrong unmap operations
Both akcipher_do_one_req() and akcipher_enqueue_req() call rsa_pub_unmap() regardless of the key type. priv_form only takes values FORM1/FORM2/FORM3 with no distinct public key enumerator.
Use key->d to distinguish public from private key operations, then dispatch to the correct unmap function based on key->priv_form.
caam_rsa_set_priv_key_form() implicitly relies on zero-initialization for priv_form. Set priv_form = FORM1 explicitly at the head of the function for clarity and robustness.
--- drivers/crypto/caam/caampkc.c | 43 +++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 12 deletions(-)
drivers/crypto/caam/caampkc.c +31 -12
@@ -379,6 +379,7 @@ static int akcipher_do_one_req(struct crypto_engine *engine, void *areq)struct crypto_akcipher *tfm =crypto_akcipher_reqtfm (req);struct caam_rsa_req_ctx *req_ctx =akcipher_request_ctx (req);struct caam_rsa_ctx *ctx =akcipher_tfm_ctx_dma (tfm); +struct caam_rsa_key *key = &ctx->key;struct device *jrdev = ctx->dev;u32 *desc = req_ctx->edesc->hw_desc;int ret;
@@ -391,7 +392,21 @@ static int akcipher_do_one_req(struct crypto_engine *engine, void *areq)return ret;if (ret != -EINPROGRESS ) { -rsa_pub_unmap (jrdev, req_ctx->edesc, req); +if (key->d) { +switch (key->priv_form) { +case FORM1 : +rsa_priv_f1_unmap (jrdev, req_ctx->edesc, req); +break ; +case FORM2 : +rsa_priv_f2_unmap (jrdev, req_ctx->edesc, req); +break ; +case FORM3 : +rsa_priv_f3_unmap (jrdev, req_ctx->edesc, req); +break ; + } + }else { +rsa_pub_unmap (jrdev, req_ctx->edesc, req); + }rsa_io_unmap (jrdev, req_ctx->edesc, req);kfree (req_ctx->edesc); }else {
@@ -691,17 +706,19 @@ static int akcipher_enqueue_req(struct device *jrdev, ret =caam_jr_enqueue (jrdev, desc, cbk, req);if ((ret != -EINPROGRESS ) && (ret != -EBUSY )) { -switch (key->priv_form) { -case FORM1 : -rsa_priv_f1_unmap (jrdev, edesc, req); -break ; -case FORM2 : -rsa_priv_f2_unmap (jrdev, edesc, req); -break ; -case FORM3 : -rsa_priv_f3_unmap (jrdev, edesc, req); -break ; -default : +if (key->d) { +switch (key->priv_form) { +case FORM1 : +rsa_priv_f1_unmap (jrdev, edesc, req); +break ; +case FORM2 : +rsa_priv_f2_unmap (jrdev, edesc, req); +break ; +case FORM3 : +rsa_priv_f3_unmap (jrdev, edesc, req); +break ; + } + }else {rsa_pub_unmap (jrdev, edesc, req); }rsa_io_unmap (jrdev, edesc, req);
@@ -992,6 +1009,8 @@ static int caam_rsa_set_priv_key_form(struct caam_rsa_ctx *ctx,size_t q_sz = raw_key->q_sz;unsigned aligned_size; + rsa_key->priv_form =FORM1 ; + rsa_key->p =caam_read_raw_data (raw_key->p, &p_sz);if (!rsa_key->p)return -ENOMEM ;
Signature · 1 line
2.43.0
PATCH v2 2/2 crypto: caam - Use bounce buffer for unaligned RSA destination buffers
In reply to Changwei ZouThe rsa-caam driver directly DMA-maps the destination buffer supplied by the caller via req->dst without checking whether it meets the cacheline alignment requirements of DMA-incoherent hardware such as i.MX8.
On CPUs with non-coherent DMA caches, if the destination buffer shares a cacheline with other data (i.e. it is not cacheline-aligned), cache writeback/invalidation during DMA can corrupt adjacent memory or cause stale data to be read back. This manifests as intermittent -EKEYREJECTED errors when loading signed kernel modules.
When any segment of req->dst is not cacheline-aligned in either its start offset or length, allocate a single contiguous aligned bounce buffer covering the full dst_len rounded up to a cacheline multiple, redirect the operation to it, and copy the result back to the original destination once the hardware has completed successfully.
Introduce a helper sg_is_dma_aligned() that checks both sg->offset and sg->length for cacheline alignment. Checking sg->offset suffices for the start address since physical pages are always page-aligned and PAGE_SIZE is a multiple of dma_get_cache_alignment(). sg->length is also checked to ensure the buffer end does not share a cacheline with adjacent memory.
The intermittent error 'Key was rejected by service' on i.MX8 with CAAM can be triggered when loading signed kernel modules:
for i in $(seq 1 100); do
sudo modprobe xfs 2>&1 && echo "SUCCESS on attempt $i" \
&& sudo rmmod xfs || echo "FAILED on attempt $i"
done--- drivers/crypto/caam/caampkc.c | 80 ++++++++++++++++++++++++++++++++++- drivers/crypto/caam/caampkc.h | 6 +++ 2 files changed, 85 insertions(+), 1 deletion(-)
drivers/crypto/caam/caampkc.c +79 -1
@@ -59,6 +59,43 @@ static void rsa_io_unmap(struct device *dev, struct rsa_edesc *edesc, DMA_TO_DEVICE); } +static int do_rsa_bounce_buf (struct akcipher_request *req, +struct caam_rsa_req_ctx *req_ctx,int req_err) +{ +int nents, err =0 ; + +/* Only copy back to the original destination on success */ +if (!req_err) { + nents =sg_nents_for_len (req_ctx->orig_dst, req->dst_len); +if (nents <0 ) + err = nents; +else if (sg_copy_from_buffer (req_ctx->orig_dst, nents, + req_ctx->bounce_buf, + req->dst_len) != req->dst_len) + err = -EFAULT ; + } + +kfree (req_ctx->bounce_buf); + req_ctx->bounce_buf =NULL ; + req->dst = req_ctx->orig_dst; + +return err; +} + +static inline void rsa_bounce_buf_done (struct akcipher_request *req,int *err) +{ +struct caam_rsa_req_ctx *req_ctx =akcipher_request_ctx (req); +int cperr; + +if (!req_ctx->bounce_buf) +return ; + + cperr =do_rsa_bounce_buf (req, req_ctx, *err); + +if (!*err) + *err = cperr; +} +static void rsa_pub_unmap (struct device *dev,struct rsa_edesc *edesc,struct akcipher_request *req) {
@@ -138,6 +175,7 @@ static void rsa_pub_done(struct device *dev, u32 *desc, u32 err, void *context)rsa_pub_unmap (dev, edesc, req);rsa_io_unmap (dev, edesc, req);kfree (edesc); +rsa_bounce_buf_done (req, &ecode);/* * If no backlog flag, the completion of the request is done
@@ -181,6 +219,7 @@ static void rsa_priv_f_done(struct device *dev, u32 *desc, u32 err,rsa_io_unmap (dev, edesc, req);kfree (edesc); +rsa_bounce_buf_done (req, &ecode);/* * If no backlog flag, the completion of the request is done
@@ -246,6 +285,12 @@ static int caam_rsa_count_leading_zeros(struct scatterlist *sgl,return tbytes - nbytes; } +static inline bool sg_is_dma_aligned (struct scatterlist *sg) +{ +return IS_ALIGNED (sg->offset,dma_get_cache_alignment ()) && +IS_ALIGNED (sg->length,dma_get_cache_alignment ()); +} +static struct rsa_edesc *rsa_edesc_alloc (struct akcipher_request *req,size_t desclen) {
@@ -291,11 +336,34 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req, req_ctx->fixup_src_len); dst_nents =sg_nents_for_len (req->dst, req->dst_len); + req_ctx->bounce_buf =NULL ; + req_ctx->orig_dst = req->dst; +if (req->dst_len >0 ) { +struct scatterlist *sg; +int i; + +for_each_sg (req->dst, sg, dst_nents, i) { +if (!sg_is_dma_aligned (sg)) { + req_ctx->bounce_buf = +kzalloc (ALIGN (req->dst_len, +dma_get_cache_alignment ()), + flags); +if (!req_ctx->bounce_buf) +return ERR_PTR (-ENOMEM ); +sg_init_one (&req_ctx->dst, req_ctx->bounce_buf, + req->dst_len); + req->dst = &req_ctx->dst; + dst_nents =1 ; +break ; + } + } + } + mapped_src_nents =dma_map_sg (dev, req_ctx->fixup_src, src_nents,DMA_TO_DEVICE );if (unlikely (!mapped_src_nents)) {dev_err (dev,"unable to map source\n" ); -return ERR_PTR (-ENOMEM ); + goto bounce_fail; } mapped_dst_nents =dma_map_sg (dev, req->dst, dst_nents,DMA_FROM_DEVICE );
@@ -368,6 +436,10 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,dma_unmap_sg (dev, req->dst, dst_nents,DMA_FROM_DEVICE ); src_fail:dma_unmap_sg (dev, req_ctx->fixup_src, src_nents,DMA_TO_DEVICE ); +bounce_fail: +kfree (req_ctx->bounce_buf); + req_ctx->bounce_buf =NULL ; + req->dst = req_ctx->orig_dst;return ERR_PTR (-ENOMEM ); }
@@ -409,6 +481,7 @@ static int akcipher_do_one_req(struct crypto_engine *engine, void *areq) }rsa_io_unmap (jrdev, req_ctx->edesc, req);kfree (req_ctx->edesc); +rsa_bounce_buf_done (req, &ret); }else { ret =0 ; }
@@ -723,6 +796,7 @@ static int akcipher_enqueue_req(struct device *jrdev, }rsa_io_unmap (jrdev, edesc, req);kfree (edesc); +rsa_bounce_buf_done (req, &ret); }return ret;
@@ -764,6 +838,7 @@ static int caam_rsa_enc(struct akcipher_request *req) init_fail:rsa_io_unmap (jrdev, edesc, req);kfree (edesc); +rsa_bounce_buf_done (req, &ret);return ret; }
@@ -793,6 +868,7 @@ static int caam_rsa_dec_priv_f1(struct akcipher_request *req) init_fail:rsa_io_unmap (jrdev, edesc, req);kfree (edesc); +rsa_bounce_buf_done (req, &ret);return ret; }
@@ -822,6 +898,7 @@ static int caam_rsa_dec_priv_f2(struct akcipher_request *req) init_fail:rsa_io_unmap (jrdev, edesc, req);kfree (edesc); +rsa_bounce_buf_done (req, &ret);return ret; }
@@ -851,6 +928,7 @@ static int caam_rsa_dec_priv_f3(struct akcipher_request *req) init_fail:rsa_io_unmap (jrdev, edesc, req);kfree (edesc); +rsa_bounce_buf_done (req, &ret);return ret; }
drivers/crypto/caam/caampkc.h +6 -0
@@ -103,6 +103,9 @@ struct caam_rsa_ctx {* @src : input scatterlist (stripped of leading zeros) * @fixup_src : input scatterlist (that might be stripped of leading zeros) * @fixup_src_len : length of the fixup_src input scatterlist +* @dst : destination scatterlist backed by bounce buffer (if needed) +* @bounce_buf : DMA-aligned bounce buffer for destination (or NULL) +* @orig_dst : original destination scatterlist (before bounce substitution) * @edesc : s/w-extended rsa descriptor * @akcipher_op_done : callback used when operation is done */
@@ -110,6 +113,9 @@ struct caam_rsa_req_ctx {struct scatterlist src[2 ];struct scatterlist *fixup_src;unsigned int fixup_src_len; +struct scatterlist dst; +u8 *bounce_buf; +struct scatterlist *orig_dst;struct rsa_edesc *edesc;void (*akcipher_op_done)(struct device *jrdev,u32 *desc,u32 err,void *context);
Signature · 1 line
2.43.0
Re: [PATCH v2 2/2] crypto: caam - Use bounce buffer for unaligned RSA destination buffers
Hi Changwei,
On i.MX8M Mini (CAAM era 9, non-coherent DMA, no IOMMU in front of the CAAM), with both patches of this series applied on 6.18.57, rsa-caam gives correct results again for callers whose destination buffer shares a cache line with the request context:
- cfg80211 loads regulatory.db, which was rejected before;
- keyctl padd accepts a valid self-signed certificate, and keyctl pkey_verify accepts 20 valid signatures (both failed with EINVAL before);
- a test module comparing rsa-caam with rsa-generic over 200 public-key operations per case (2048-bit key) reports no wrong result in any case, including a destination placed right after the request context (in place or not), crypto_akcipher_sync_encrypt() and pkcs1(rsa-caam,sha256) verification. Without the series, these cases failed in 198 to 200 of 200 operations, with bytes 0 to 23 of the output holding stale data.
Only the public-key path was exercised. The requests did not use CRYPTO_TFM_REQ_MAY_BACKLOG, so the crypto-engine path fixed by patch 1 was not reached.
I posted a series that addresses the same corruption on the caller side, in crypto_akcipher_sync_prep() and rsassa_pkcs1_verify(), which build these unaligned buffers:
https://lore.kernel.org/linux-crypto/[email protected]/
The two approaches are complementary: with my series, the buffers from these two callers are aligned and need no bounce, while your patch covers any other caller.
Thanks,
Stephane
Re: [PATCH v2 2/2] crypto: caam - Use bounce buffer for unaligned RSA destination buffers
Hi Stephane,
Thank you very much for your help with testing the patch.
In fact, I had previously submitted another patch to fix the bug on the caller side:
https://lore.kernel.org/all/[email protected]/
However, Herbert pointed out:
"In the Crypto API we don't ask the user to provide aligned data. In general it's the driver's responsibility to ensure proper alignment."
https://lore.kernel.org/all/[email protected]/
Therefore, I later switched to a driver-side solution.
I'll include your Tested-by tag when I submit the next version.
Thanks.
Re: [PATCH v2 2/2] crypto: caam - Use bounce buffer for unaligned RSA destination buffers
Hi Changwei,
Quoted 1 line · Changwei Zou
Therefore, I later switched to a driver-side solution.
Thanks for the explanation and the links. I have asked Herbert to drop my series.
Stephane
Herbert Xu
maintainerQuoted 19 lines · Changwei Zou
This series fixes two issues in the rsa-caam driver.
Patch 1 fixes wrong unmap operations where
rsa_pub_unmap()was called regardless of the key type.Patch 2 adds a bounce buffer to handle unaligned RSA destination buffers on DMA-incoherent hardware such as i.MX8, fixing intermittent
-EKEYREJECTEDerrors when loading signed kernel modules.Changes since v1:
- Patch 1: Add Fixes: tag; pick up Reviewed-by from Sahil Malhotra.
- Patch 2: Move the !bounce_buf check out ofdo_rsa_bounce_buf()into the callerrsa_bounce_buf_done()so the helper is only entered when a bounce buffer actually exists; add Reported-by/Closes and Tested-by from Martin Kepplinger-Novakovic.Link to v1:
https://lore.kernel.org/linux-crypto/[email protected]/
Quoted 10 lines
Changwei Zou (2):
crypto: caam - Fix wrong unmap operations
crypto: caam - Use bounce buffer for unaligned RSA destination buffersdrivers/crypto/caam/caampkc.c | 123 ++++++++++++++++++++++++++++++---- drivers/crypto/caam/caampkc.h | 6 ++ 2 files changed, 116 insertions(+), 13 deletions(-)Signature · 1 line
2.43.0
Plesae check
https://sashiko.dev/#/patchset/20261004210200.290119-1-changwei.zou%40canonical.com
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi Herbert,
Thanks for the review.
Quoted 7 lines
If a caller provides an unaligned destination scatterlist where
req->dst_lenis larger than the actual RSA output size (such askey->n_sz), the bounce buffer is zero-initialized and sized toreq->dst_len. [...] Could this corrupt adjacent data by copying zeroes over the trailing capacity of the caller's buffer? Should this copy only the actual hardware output size instead of the full buffer capacity?
There is no out-of-bounds access or adjacent-memory corruption: the copy-back targets the caller's own dst (orig_dst) and is bounded by req->dst_len, the buffer length the caller provided. It reads dst_len bytes from the bounce buffer (sized ALIGN(dst_len, cacheline) >= dst_len) and writes them into that same dst, so neither buffer is overrun.
That said, copying only the actual output size is a sensible improvement, and I've applied it in v3: the copy-back is now bounded to the RSA output size (key->n_sz) instead of req->dst_len. The hardware writes the full modulus-width (n_sz) output - the RSA PDB has no separate output-length field - and caam_rsa_enc()/caam_rsa_dec() guarantee dst_len >= n_sz, so the trailing capacity of the caller's buffer is left untouched.
Another option, if keeping the full dst_len copy is preferred, is to seed the bounce buffer with the caller's original destination contents before the operation (rather than zero-initializing it) and copy all dst_len bytes back afterwards. The hardware overwrites only [0, n_sz) with the result, so [n_sz, dst_len) still holds the caller's original bytes and the full copy-back leaves the trailing region unchanged. I went with the simpler key->n_sz bound for v3, but I'm happy to switch to this if you prefer.
Kind regards,
Changwei