Re: [gnutls-devel] GnuTLS | WIP: Add Linux kernel AF_ALG backend (!1404)

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | WIP: Add Linux kernel AF_ALG backend (!1404)

Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller commented on a discussion on lib/accelerated/afalg.c:

89
+	struct kcapi_ctx *ctx = _ctx;
90
+
91
+	if (iv_size > kcapi_cipher_ivsize(ctx->handle))
92
+		return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST);
93
+
94
+	memcpy(ctx->iv, iv, iv_size);
95
+
96
+	return 0;
97
+}
98
+
99
+static int afalg_cipher_encrypt(void *_ctx, const void *src, size_t src_size,
100
+				void *dst, size_t dst_size)
101
+{
102
+	struct kcapi_ctx *ctx = _ctx;
103
+
104
+	if (kcapi_cipher_encrypt(ctx->handle, src, src_size, ctx->iv,

Ahh, yes - the issue is not an alignment, the issue is that each encrypt call is a standalone cipher operation. Thus, the cipher state (i.e. the IV update by CBC) is not "carried over" to the next encrypt operation.

Here is the example using your code that fixes the issue:

	assert(kcapi_cipher_init(&handle, "cbc(aes)", 0) == 0);
	assert(kcapi_cipher_setkey(handle, KEY, sizeof(KEY)) == 0);

	iov.iov_base = PLAINTEXT;
	iov.iov_len = 16;
	assert(kcapi_cipher_stream_init_enc(handle, IV, &iov, 1) == 16);

	iov.iov_base = PLAINTEXT + 16;
	assert(kcapi_cipher_stream_update_last(handle, &iov, 1) == 16);

	iov.iov_base = ciphertext;
	iov.iov_len = sizeof(ciphertext);
	assert(kcapi_cipher_stream_op(handle, &iov, 1) == 32);

	kcapi_cipher_destroy(handle);

Take a note on kcapi_cipher_stream_update vs kcapi_cipher_stream_update_last! The _last MUST always be invoked as the final operation.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | WIP: Add Linux kernel AF_ALG backend (!1404)

Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller commented on a discussion on lib/accelerated/afalg.c:

89
+	struct kcapi_ctx *ctx = _ctx;
90
+
91
+	if (iv_size > kcapi_cipher_ivsize(ctx->handle))
92
+		return gnutls_assert_val(GNUTLS_E_INVALID_REQUEST);
93
+
94
+	memcpy(ctx->iv, iv, iv_size);
95
+
96
+	return 0;
97
+}
98
+
99
+static int afalg_cipher_encrypt(void *_ctx, const void *src, size_t src_size,
100
+				void *dst, size_t dst_size)
101
+{
102
+	struct kcapi_ctx *ctx = _ctx;
103
+
104
+	if (kcapi_cipher_encrypt(ctx->handle, src, src_size, ctx->iv,

Side note, if we need the stream API, take care on the documentation, the update call cannot be invoked endlessly without a stream_op inbetween.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | WIP: Add Linux kernel AF_ALG backend (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab
Merge request !1404 was reviewed by Daiki Ueno

Daiki Ueno started a new discussion on lib/accelerated/afalg.c:

406
+
407
+	/*
408
+	 * Older kernels require tag as input. This buffer data is unused

I wonder if this still makes sense; is there a minimum version requirement of the Kernel that libkcapi requires?

Daiki Ueno commented on a discussion on lib/accelerated/afalg.c:

252
+	struct iovec iov[3];
253
+	uint32_t iovlen = 2;
254
+	uint8_t authtmp[auth_size];

Afaik the maximum of AAD for GCM is quite large; maybe we will have to use heap for that?

Daiki Ueno commented on a discussion on lib/accelerated/afalg.c:

253
+	uint32_t iovlen = 2;
254
+	uint8_t authtmp[auth_size];
255
+	uint8_t tagtmp[tag_size];

Here we can use MAX_HASH_SIZE.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | WIP: Add Linux kernel AF_ALG backend (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller commented on a discussion on lib/accelerated/afalg.c:

239
+	}
240
+
241
+	return 0;
242
+}
243
+
244
+static int afalg_aead_decrypt(void *_ctx,
245
+			      const void *nonce, size_t nonce_size,
246
+			      const void *auth, size_t auth_size,
247
+			      size_t tag_size,
248
+			      const void *encr, size_t encr_size,
249
+			      void *plain, size_t plain_size)
250
+{
251
+	struct kcapi_aead_ctx *ctx = _ctx;
252
+	struct iovec iov[3];
253
+	uint32_t iovlen = 2;
254
+	uint8_t authtmp[auth_size];

Or a combo of stack and heap?

For example, we could use something like the following that I used in a kernel patch:

        u8 tmpbuf[LRNG_DRNG_BLOCKSIZE] __aligned(LRNG_KCAPI_ALIGN);
        u8 *tmp_large = NULL, *tmp = tmpbuf;
        u32 tmplen = sizeof(tmpbuf);

        /*
         * Satisfy large read requests -- as the common case are smaller
         * request sizes, such as 16 or 32 bytes, avoid a kmalloc overhead for
         * those by using the stack variable of tmpbuf.
         */
        if (!CONFIG_BASE_SMALL && (nbytes > sizeof(tmpbuf))) {
                tmplen = min_t(u32, nbytes, LRNG_DRNG_MAX_REQSIZE);
                tmp_large = kmalloc(tmplen + LRNG_KCAPI_ALIGN, GFP_KERNEL);
                if (!tmp_large)
                        tmplen = sizeof(tmpbuf);
                else
                        tmp = PTR_ALIGN(tmp_large, LRNG_KCAPI_ALIGN);
        }
...
        /* Wipe data just returned from memory */
        if (tmp_large)
                kfree_sensitive(tmp_large);
        else
                memzero_explicit(tmpbuf, sizeof(tmpbuf));

_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | WIP: Add Linux kernel AF_ALG backend (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stephan Mueller commented on a discussion on lib/accelerated/afalg.c:

393
+	/*
394
+	 * Set AAD: IOVECs do not support const, this buffer is guaranteed to be
395
+	 * read-only
396
+	 */
397
+	iov[0].iov_base = (void *)auth;
398
+	iov[0].iov_len = auth_size;
399
+
400
+	/*
401
+	 * Set PT: IOVECs do not support const, this buffer is guaranteed to be
402
+	 * read-only
403
+	 */
404
+	iov[1].iov_base = (void *)plain;
405
+	iov[1].iov_len = plain_size;
406
+
407
+	/*
408
+	 * Older kernels require tag as input. This buffer data is unused

I recommend leaving it. This "older kernel" references:

        uint32_t len = inlen + assoclen;

        if (!handle->flags.ge_v4_9 == true)
                len += taglen;

        return len;

I think it is not wise to require a minimum kernel version for GnuTLS/AF_ALG. IMHO this check is cheap, we should leave this.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | WIP: Add Linux kernel AF_ALG backend (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented on a discussion on lib/accelerated/afalg.c:

308
+	 * read-only
309
+	 */
310
+	iov[1].iov_base = (void *)encr;
311
+	iov[1].iov_len = encr_size;
312
+
313
+	if (kcapi_aead_stream_update_last(ctx->handle, iov, 2) < 0) {
314
+		gnutls_assert();
315
+		return GNUTLS_E_ENCRYPTION_FAILED;
316
+	}
317
+
318
+	/* The kernel may set the AAD, avoid modification of auth */
319
+	iov[0].iov_base = authtmp;
320
+
321
+	/* Set PT buffer to be filled by kernel */
322
+	iov[1].iov_base = plain;
323
+	iov[1].iov_len = plain_size;

The initial motivation of this change is: #308 (comment 527421502) i.e., kcapi_aead_stream_op blocks if the requested output size didn't match.

According to the libkcapi documentation, it seems like a legitimate behavior. I agree that it could cause a buffer overflow; maybe we could tighten the length check, and also merge it with the next if block.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | WIP: Add Linux kernel AF_ALG backend (!1404)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented on a discussion on lib/accelerated/afalg.c:

239
+	}
240
+
241
+	return 0;
242
+}
243
+
244
+static int afalg_aead_decrypt(void *_ctx,
245
+			      const void *nonce, size_t nonce_size,
246
+			      const void *auth, size_t auth_size,
247
+			      size_t tag_size,
248
+			      const void *encr, size_t encr_size,
249
+			      void *plain, size_t plain_size)
250
+{
251
+	struct kcapi_aead_ctx *ctx = _ctx;
252
+	struct iovec iov[3];
253
+	uint32_t iovlen = 2;
254
+	uint8_t authtmp[auth_size];

I guess we could use malloca from Gnulib, if we go with this direction.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel