Re: [PATCH v2 0/2] crypto: caam - Fix RSA destination buffer handling

[Changwei Zou]

Hi Herbert,

Thanks for the review.

> If a caller provides an unaligned destination scatterlist where
> req->dst_len is larger than the actual RSA output size (such as
> key->n_sz), the bounce buffer is zero-initialized and sized to
> req->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