RE: [PATCH 1/2] crypto: caam - add support for gcm(aes)
On Thu, 9 Oct 2014 17:54:09 +0300
Tudor Ambarus tudor.amba...@freescale.com wrote:
+ /*
+ * Job Descriptor and Shared Descriptors
+ * must all fit into the 64-word Descriptor h/w Buffer
+ */
+ if (DESC_GCM_DEC_LEN + DESC_JOB_IO_LEN +
+ ctx-enckeylen = CAAM_DESC_BYTES_MAX)
+ keys_fit_inline = true;
we need to reset the encrypt descriptor's keys_fit_inline setting to false
before re-evaluating for decrypt.
[TA] Agreed.
+ /* Galois Counter Mode */
+ {
+ .name = gcm(aes),
+ .driver_name = gcm-aes-caam,
+ .blocksize = 1,
+ .type = CRYPTO_ALG_TYPE_AEAD,
+ .template_aead = {
+ .setkey = gcm_setkey,
+ .setauthsize = gcm_setauthsize,
+ .encrypt = aead_encrypt,
+ .decrypt = aead_decrypt,
+ .givencrypt = NULL,
+ .geniv = built-in,
+ .ivsize = 12,
+ .maxauthsize = 16,
AES_BLOCK_SIZE
[TA] I think we shall not change the blocksize value to AES_BLOCK_SIZE.
GCM uses a block cipher as a stream cipher. It generates encryption blocks,
which are then XORed with the plaintext blocks to get the ciphertext. Just as
with other stream ciphers, flipping a bit in the ciphertext produces a flipped
bit in the plaintext at the same location.
Thanks,
Kim
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2] crypto: caam - add support for rfc4106(gcm(aes))
On Thu, 9 Oct 2014 17:54:10 +0300
Tudor Ambarus tudor.amba...@freescale.com wrote:
+static int rfc4106_set_sh_desc(struct crypto_aead *aead)
...
+ /*
+ * Job Descriptor and Shared Descriptors
+ * must all fit into the 64-word Descriptor h/w Buffer
+ */
+ if (DESC_RFC4106_DEC_LEN + DESC_JOB_IO_LEN +
+ ctx-enckeylen = CAAM_DESC_BYTES_MAX)
+ key_fit_inline = true;
we need to reset encrypt descriptor's keys_fit_inline setting to
false before doing this.
Also, the singular of keys_fit_inline is key_fits_inline, but
I'd prefer we not gratuitously rename the variable from the rest of
the driver's keys_fit_inline for consistency's sake, thanks.
[TA] Agreed.
+ /*
+ * Job Descriptor and Shared Descriptors
+ * must all fit into the 64-word Descriptor h/w Buffer
+ */
+ if (DESC_RFC4106_GIVENC_LEN + DESC_JOB_IO_LEN +
+ ctx-split_key_pad_len + ctx-enckeylen =
+ CAAM_DESC_BYTES_MAX)
+ key_fit_inline = true;
we need to reset the variable here too.
[TA] Agreed.
+static int rfc4106_setauthsize(struct crypto_aead *authenc,
+unsigned int authsize)
+{
+ struct caam_ctx *ctx = crypto_aead_ctx(authenc);
+
+ switch (authsize) {
+ case 8:
+ case 12:
+ case 16:
+ break;
+ default:
+ return -EINVAL;
+ }
the h/w can handle more authsizes than that, so we
shouldn't be blocking it from doing so here.
[TA] rfc4106 says that Implementations MUST support a full-length 16-octet
ICV, and MAY support 8 or 12 octet ICVs, and MUST NOT support other ICV
lengths.
Do we want to support other ICV lengths?
@@ -2601,6 +2986,23 @@ static struct caam_alg_template driver_algs[] = {
OP_ALG_AAI_HMAC_PRECOMP,
.alg_op = OP_ALG_ALGSEL_SHA512 | OP_ALG_AAI_HMAC,
},
+ {
+ .name = rfc4106(gcm(aes)),
+ .driver_name = rfc4106-gcm-aes-caam,
+ .blocksize = 1,
+ .type = CRYPTO_ALG_TYPE_AEAD,
+ .template_aead = {
+ .setkey = rfc4106_setkey,
+ .setauthsize = rfc4106_setauthsize,
+ .encrypt = aead_encrypt,
+ .decrypt = aead_decrypt,
+ .givencrypt = aead_givencrypt,
+ .geniv = built-in,
+ .ivsize = 8,
+ .maxauthsize = 16,
AES_BLOCK_SIZE
[TA] I don't think we should change the blocksize value to AES_BLOCK_SIZE.
Thank you,
Tudor
Thanks,
Kim
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.
On 10/09/2014 07:12 PM, Tadeusz Struk wrote:
On 10/09/2014 02:42 PM, Prarit Bhargava wrote:
I don't think cpu hotplug matters here. This is one (probe) time
determination if the configuration is optimal or not and if it makes
sense to use this accelerator or not.
It absolutely matters. num_online_cpus() *changes* depending on the # of
cpus.
Sure, but I still think that we are safe here.
No, you're not. Dropping a single CPU changes num_online_cpus(), which results
in
static uint8_t adf_get_dev_node_id(struct pci_dev *pdev)
{
unsigned int bus_per_cpu = 0;
struct cpuinfo_x86 *c = cpu_data(num_online_cpus() - 1); this
being different.
if (!c-phys_proc_id)
return 0;
bus_per_cpu = 256 / (c-phys_proc_id + 1); this being different
if (bus_per_cpu != 0)
return pdev-bus-number / bus_per_cpu; and this being
different
return 0;
}
P.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.
On 10/10/2014 04:23 AM, Prarit Bhargava wrote:
Sure, but I still think that we are safe here.
No, you're not. Dropping a single CPU changes num_online_cpus(), which
results in
static uint8_t adf_get_dev_node_id(struct pci_dev *pdev)
{
unsigned int bus_per_cpu = 0;
struct cpuinfo_x86 *c = cpu_data(num_online_cpus() - 1); this
being different.
if (!c-phys_proc_id)
return 0;
bus_per_cpu = 256 / (c-phys_proc_id + 1); this being different
if (bus_per_cpu != 0)
return pdev-bus-number / bus_per_cpu; and this being
different
return 0;
}
You forgot to explain how this is not safe.
T.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] crypto: caam - add support for rfc4106(gcm(aes))
On Fri, 10 Oct 2014 05:10:55 -0500
Ambarus Tudor-Dan-B38632 tudor.amba...@freescale.com wrote:
On Thu, 9 Oct 2014 17:54:10 +0300
Tudor Ambarus tudor.amba...@freescale.com wrote:
+static int rfc4106_setauthsize(struct crypto_aead *authenc,
+ unsigned int authsize)
+{
+ struct caam_ctx *ctx = crypto_aead_ctx(authenc);
+
+ switch (authsize) {
+ case 8:
+ case 12:
+ case 16:
+ break;
+ default:
+ return -EINVAL;
+ }
the h/w can handle more authsizes than that, so we
shouldn't be blocking it from doing so here.
[TA] rfc4106 says that Implementations MUST support a full-length 16-octet
ICV, and MAY support 8 or 12 octet ICVs, and MUST NOT support other ICV
lengths.
Do we want to support other ICV lengths?
how much the linux kernel wants to enforce the RFC is up to the
higher levels in its crypto API and other front end interfaces such
as XFRM. So those checks for RFC compliance should be performed
before they reach the implementation (driver).
Meanwhile, drivers themselves should accurately represent the h/w
capabilities, whether they meet and/or exceed the RFC's constraints
or not, IMO. This way, if/when the RFC gets extended to become
open to a wider range of values, the driver needs no changes.
@@ -2601,6 +2986,23 @@ static struct caam_alg_template driver_algs[] = {
OP_ALG_AAI_HMAC_PRECOMP,
.alg_op = OP_ALG_ALGSEL_SHA512 | OP_ALG_AAI_HMAC,
},
+ {
+ .name = rfc4106(gcm(aes)),
+ .driver_name = rfc4106-gcm-aes-caam,
+ .blocksize = 1,
+ .type = CRYPTO_ALG_TYPE_AEAD,
+ .template_aead = {
+ .setkey = rfc4106_setkey,
+ .setauthsize = rfc4106_setauthsize,
+ .encrypt = aead_encrypt,
+ .decrypt = aead_decrypt,
+ .givencrypt = aead_givencrypt,
+ .geniv = built-in,
+ .ivsize = 8,
+ .maxauthsize = 16,
AES_BLOCK_SIZE
[TA] I don't think we should change the blocksize value to AES_BLOCK_SIZE.
sorry, I meant just .maxauthsize = AES_BLOCK_SIZE.
Thanks,
Kim
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] crypto: caam - add support for gcm(aes)
On Fri, 10 Oct 2014 03:47:18 -0500
Ambarus Tudor-Dan-B38632 tudor.amba...@freescale.com wrote:
On Thu, 9 Oct 2014 17:54:09 +0300
Tudor Ambarus tudor.amba...@freescale.com wrote:
+ /* Galois Counter Mode */
+ {
+ .name = gcm(aes),
+ .driver_name = gcm-aes-caam,
+ .blocksize = 1,
+ .type = CRYPTO_ALG_TYPE_AEAD,
+ .template_aead = {
+ .setkey = gcm_setkey,
+ .setauthsize = gcm_setauthsize,
+ .encrypt = aead_encrypt,
+ .decrypt = aead_decrypt,
+ .givencrypt = NULL,
+ .geniv = built-in,
+ .ivsize = 12,
+ .maxauthsize = 16,
AES_BLOCK_SIZE
[TA] I think we shall not change the blocksize value to AES_BLOCK_SIZE.
GCM uses a block cipher as a stream cipher. It generates encryption blocks,
which are then XORed with the plaintext blocks to get the ciphertext. Just as
with other stream ciphers, flipping a bit in the ciphertext produces a
flipped bit in the plaintext at the same location.
Sorry, I meant just .maxauthsize = AES_BLOCK_SIZE.
Kim
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.
On 10/10/2014 09:25 AM, Tadeusz Struk wrote:
On 10/10/2014 04:23 AM, Prarit Bhargava wrote:
Sure, but I still think that we are safe here.
No, you're not. Dropping a single CPU changes num_online_cpus(), which
results in
static uint8_t adf_get_dev_node_id(struct pci_dev *pdev)
{
unsigned int bus_per_cpu = 0;
struct cpuinfo_x86 *c = cpu_data(num_online_cpus() - 1); this
being different.
if (!c-phys_proc_id)
return 0;
bus_per_cpu = 256 / (c-phys_proc_id + 1); this being different
if (bus_per_cpu != 0)
return pdev-bus-number / bus_per_cpu; and this being
different
return 0;
}
You forgot to explain how this is not safe.
Sorry, I thought I did explain it. My apologies.
So let's say you boot the system and load the driver. At this time,
num_online_cpus@boot = 4 . Crunch through the math above, and you reference the
cpuinfo_x86 struct for cpu 3 (the fourth cpu), and the calculation takes into
account c-phys_proc_id.
So let's say now you boot the system and disable a cpu. In this case, now
num_online_cpus@module_load = 3. Crunch through the math above and you're
referncing a different cpuinfo_x86 struct for cpu 2. That may or may not point
at the same c-phys_proc_id. That changes the calculation and gives an
incorrect value.
In addition to that I haven't even talked about the possibility of hot-adding
and hot-removing cpus in sockets which changes the numbering scheme completely.
In short, that calcuation is wrong. Don't use it; stick with the widely
accepted and used dev_to_node of the pci_dev. It is used in other cases IIRC to
determine the numa location of the device. It shouldn't be any different for
this driver.
P.
T.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
