| Message ID | 20200822072934.4394-1-giovanni.cabiddu@intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: qat - aead cipher length should be block multiple | expand |
On Sat, 22 Aug 2020 at 09:29, Giovanni Cabiddu <giovanni.cabiddu@intel.com> wrote: > > From: Dominik Przychodni <dominik.przychodni@intel.com> > > Include an additional check on the cipher length to prevent undefined > behaviour from occurring upon submitting requests which are not a > multiple of AES_BLOCK_SIZE. > > Fixes: d370cec32194 ("crypto: qat - Intel(R) QAT crypto interface") > Signed-off-by: Dominik Przychodni <dominik.przychodni@intel.com> > Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com> I only looked at the patch, and not at the entire file, but could you explain which AES based AEAD implementations require the input length to be a multiple of the block size? CCM and GCM are both CTR based, and so any input length should be supported for at least those modes. > --- > drivers/crypto/qat/qat_common/qat_algs.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c > index 72753b84dc95..d552dbcfe0a0 100644 > --- a/drivers/crypto/qat/qat_common/qat_algs.c > +++ b/drivers/crypto/qat/qat_common/qat_algs.c > @@ -828,6 +828,11 @@ static int qat_alg_aead_dec(struct aead_request *areq) > struct icp_qat_fw_la_bulk_req *msg; > int digst_size = crypto_aead_authsize(aead_tfm); > int ret, ctr = 0; > + u32 cipher_len; > + > + cipher_len = areq->cryptlen - digst_size; > + if (cipher_len % AES_BLOCK_SIZE != 0) > + return -EINVAL; > > ret = qat_alg_sgl_to_bufl(ctx->inst, areq->src, areq->dst, qat_req); > if (unlikely(ret)) > @@ -842,7 +847,7 @@ static int qat_alg_aead_dec(struct aead_request *areq) > qat_req->req.comn_mid.src_data_addr = qat_req->buf.blp; > qat_req->req.comn_mid.dest_data_addr = qat_req->buf.bloutp; > cipher_param = (void *)&qat_req->req.serv_specif_rqpars; > - cipher_param->cipher_length = areq->cryptlen - digst_size; > + cipher_param->cipher_length = cipher_len; > cipher_param->cipher_offset = areq->assoclen; > memcpy(cipher_param->u.cipher_IV_array, areq->iv, AES_BLOCK_SIZE); > auth_param = (void *)((u8 *)cipher_param + sizeof(*cipher_param)); > @@ -871,6 +876,9 @@ static int qat_alg_aead_enc(struct aead_request *areq) > u8 *iv = areq->iv; > int ret, ctr = 0; > > + if (areq->cryptlen % AES_BLOCK_SIZE != 0) > + return -EINVAL; > + > ret = qat_alg_sgl_to_bufl(ctx->inst, areq->src, areq->dst, qat_req); > if (unlikely(ret)) > return ret; > -- > 2.26.2 >
On Sat, Aug 22, 2020 at 02:04:10PM +0100, Ard Biesheuvel wrote: > On Sat, 22 Aug 2020 at 09:29, Giovanni Cabiddu > <giovanni.cabiddu@intel.com> wrote: > > > > From: Dominik Przychodni <dominik.przychodni@intel.com> > > > > Include an additional check on the cipher length to prevent undefined > > behaviour from occurring upon submitting requests which are not a > > multiple of AES_BLOCK_SIZE. > > > > Fixes: d370cec32194 ("crypto: qat - Intel(R) QAT crypto interface") > > Signed-off-by: Dominik Przychodni <dominik.przychodni@intel.com> > > Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com> > > I only looked at the patch, and not at the entire file, but could you > explain which AES based AEAD implementations require the input length > to be a multiple of the block size? CCM and GCM are both CTR based, > and so any input length should be supported for at least those modes. This is only for AES CBC as the qat driver supports only authenc(hmac(sha1),cbc(aes)), authenc(hmac(sha256),cbc(aes)) and authenc(hmac(sha512),cbc(aes)). Regards,
On Fri, 28 Aug 2020 at 12:24, Giovanni Cabiddu <giovanni.cabiddu@intel.com> wrote: > > On Sat, Aug 22, 2020 at 02:04:10PM +0100, Ard Biesheuvel wrote: > > On Sat, 22 Aug 2020 at 09:29, Giovanni Cabiddu > > <giovanni.cabiddu@intel.com> wrote: > > > > > > From: Dominik Przychodni <dominik.przychodni@intel.com> > > > > > > Include an additional check on the cipher length to prevent undefined > > > behaviour from occurring upon submitting requests which are not a > > > multiple of AES_BLOCK_SIZE. > > > > > > Fixes: d370cec32194 ("crypto: qat - Intel(R) QAT crypto interface") > > > Signed-off-by: Dominik Przychodni <dominik.przychodni@intel.com> > > > Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com> > > > > I only looked at the patch, and not at the entire file, but could you > > explain which AES based AEAD implementations require the input length > > to be a multiple of the block size? CCM and GCM are both CTR based, > > and so any input length should be supported for at least those modes. > This is only for AES CBC as the qat driver supports only > authenc(hmac(sha1),cbc(aes)), authenc(hmac(sha256),cbc(aes)) and > authenc(hmac(sha512),cbc(aes)). > Ah right, yes that makes sense.
diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c index 72753b84dc95..d552dbcfe0a0 100644 --- a/drivers/crypto/qat/qat_common/qat_algs.c +++ b/drivers/crypto/qat/qat_common/qat_algs.c @@ -828,6 +828,11 @@ static int qat_alg_aead_dec(struct aead_request *areq) struct icp_qat_fw_la_bulk_req *msg; int digst_size = crypto_aead_authsize(aead_tfm); int ret, ctr = 0; + u32 cipher_len; + + cipher_len = areq->cryptlen - digst_size; + if (cipher_len % AES_BLOCK_SIZE != 0) + return -EINVAL; ret = qat_alg_sgl_to_bufl(ctx->inst, areq->src, areq->dst, qat_req); if (unlikely(ret)) @@ -842,7 +847,7 @@ static int qat_alg_aead_dec(struct aead_request *areq) qat_req->req.comn_mid.src_data_addr = qat_req->buf.blp; qat_req->req.comn_mid.dest_data_addr = qat_req->buf.bloutp; cipher_param = (void *)&qat_req->req.serv_specif_rqpars; - cipher_param->cipher_length = areq->cryptlen - digst_size; + cipher_param->cipher_length = cipher_len; cipher_param->cipher_offset = areq->assoclen; memcpy(cipher_param->u.cipher_IV_array, areq->iv, AES_BLOCK_SIZE); auth_param = (void *)((u8 *)cipher_param + sizeof(*cipher_param)); @@ -871,6 +876,9 @@ static int qat_alg_aead_enc(struct aead_request *areq) u8 *iv = areq->iv; int ret, ctr = 0; + if (areq->cryptlen % AES_BLOCK_SIZE != 0) + return -EINVAL; + ret = qat_alg_sgl_to_bufl(ctx->inst, areq->src, areq->dst, qat_req); if (unlikely(ret)) return ret;