[2/2] crypto: qat - Don't move data inside output buffer

Commit Message

Tadeusz Struk Aug. 14, 2015, 2:21 p.m. UTC

Hi Herbert,
On 08/13/2015 11:26 PM, Herbert Xu wrote:
> On Thu, Aug 13, 2015 at 11:14:11PM -0700, Tadeusz Struk wrote:
>>
>> Right, but we don't need that anymore.
> 
> Why not? If you reduce the size without moving the buffer wouldn't
> it begin with a bunch of zeroes and wouldn't you lose the real bytes
> at the end?

Yes, that was wrong, sorry. The reason I wanted to change it is that
the SW implementation can return a number with leading zeros.
This is because mpi_read_buffer() returns the whole thing.

Because the format of the module signature starts with 0x00, 0x01
the two implementations return a different thing.
For instance SW returned a 512 bytes number starting with 0x00, 0x01
and HW returned 511 bytes number without the 0x00 at the beginning.
Technically both are correct, but then the rsa_signture_verify()
needs to check for both cases, which is not ideal.
To make it return the same thing as SW we can do something like this:

---8<---

Allow for leading zeros in output to make it exactly the same as the
SW implementation.
Change memcpy to memmove because the copy is done within the same buffer.


--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Herbert Xu Aug. 17, 2015, 8:48 a.m. UTC | #1

On Fri, Aug 14, 2015 at 07:21:29AM -0700, Tadeusz Struk wrote:
> 
> Yes, that was wrong, sorry. The reason I wanted to change it is that
> the SW implementation can return a number with leading zeros.
> This is because mpi_read_buffer() returns the whole thing.

I think mpi_read_buffer is broken.  It should return exactly the
same thing as mpi_get_buffer, except that it should do so in the
buffer provided instead of allocating a new one.

So it most certainly should remove all leading zero bytes.

Cheers,

Patch

diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c
index fe352a6..cc450fa 100644
--- a/drivers/crypto/qat/qat_common/qat_asym_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c
@@ -118,7 +118,13 @@  static void qat_rsa_cb(struct icp_qat_fw_pke_resp *resp)
 	struct device *dev = &GET_DEV(req->ctx->inst->accel_dev);
 	int err = ICP_QAT_FW_PKE_RESP_PKE_STAT_GET(
 				resp->pke_resp_hdr.comn_resp_flags);
-	char *ptr = areq->dst;
+#if BYTES_PER_MPI_LIMB == 4
+	u32 *ptr = areq->dst;
+#elif BYTES_PER_MPI_LIMB == 8
+	u64 *ptr = areq->dst;
+#else
+#error please implement for this limb size.
+#endif
 
 	err = (err == ICP_QAT_FW_COMN_STATUS_FLAG_OK) ? 0 : -EINVAL;
 
@@ -140,12 +146,12 @@  static void qat_rsa_cb(struct icp_qat_fw_pke_resp *resp)
 	areq->dst_len = req->ctx->key_sz;
 	/* Need to set the corect length of the output */
 	while (!(*ptr) && areq->dst_len) {
-		areq->dst_len--;
+		areq->dst_len -= sizeof(*ptr);
 		ptr++;
 	}
 
 	if (areq->dst_len != req->ctx->key_sz)
-		memcpy(areq->dst, ptr, areq->dst_len);
+		memmove(areq->dst, ptr, areq->dst_len);
 
 	akcipher_request_complete(areq, err);
 }