From: Jan Engelhardt <jengelh@inai.de>
Date: Sat, 30 Dec 2023 17:47:05 +0000 (+0100)
Subject: xt_pknock: update for shash API
X-Git-Tag: v3.26~3
X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=a5b9baeee87c17ec79113cb51e4316e6ce468253;p=thirdparty%2Fxtables-addons.git

xt_pknock: update for shash API

Bug report states:

``crypto.desc`` is used to hold the ``hmac(sha256)`` transform such
that it can be fed to ``crypto_shash_update`` et al. It seems that
those functions require extra memory after the ``shash_desc``. With
the current layout, usage of ``&crypto.desc`` with the
``crypto_shash_*`` functions causes memory corruption which most
often crashes in netfilter after the pknock match filter has
returned.

By removing ``crypto.desc`` and instead using ``SHASH_DESC_ON_STACK``
within ``has_secret``, the issue can be avoided. See other
SHASH_DESC_ON_STACK uses elsewhere in the kernel source.

Additionally, ``crypto_shash_init`` needs to be called before the
first ``crypto_shash_update``.

Fixes: v2.10-7-g7af1b97
---

diff --git a/extensions/pknock/xt_pknock.c b/extensions/pknock/xt_pknock.c
index 9388c49..1ac54fb 100644
--- a/extensions/pknock/xt_pknock.c
+++ b/extensions/pknock/xt_pknock.c
@@ -105,11 +105,10 @@ static struct {
 	const char *algo;
 	struct crypto_shash *tfm;
 	unsigned int size;
-	struct shash_desc desc;
+	// SHASH_DESC_ON_STACK part 1
+	char desc[sizeof(struct shash_desc) + HASH_MAX_DESCSIZE] __aligned(__alignof__(struct shash_desc));
 } crypto = {
 	.algo	= "hmac(sha256)",
-	.tfm	= NULL,
-	.size	= 0
 };
 
 module_param(rule_hashsize, int, S_IRUGO);
@@ -721,6 +720,8 @@ has_secret(const unsigned char *secret, unsigned int secret_len, uint32_t ipsrc,
 	bool fret = false;
 	uint64_t x;
 	unsigned int epoch_min;
+	/* Concurrent use fenced off by a caller which holds list_lock. */
+	struct shash_desc *shash = (void *)crypto.desc; // SHASH_DESC_ON_STACK part 2
 
 	if (payload_len == 0)
 		return false;
@@ -741,16 +742,12 @@ has_secret(const unsigned char *secret, unsigned int secret_len, uint32_t ipsrc,
 		printk("crypto_hash_setkey() failed ret=%d\n", ret);
 		goto out;
 	}
-
-	/*
-	 * The third parameter is the number of bytes INSIDE the sg!
-	 * 4 bytes IP (32 bits) +
-	 * 4 bytes int epoch_min (32 bits)
-	 */
-	if ((ret = crypto_shash_update(&crypto.desc, (const void *)&ipsrc, sizeof(ipsrc))) != 0 ||
-	    (ret = crypto_shash_update(&crypto.desc, (const void *)&epoch_min, sizeof(epoch_min))) != 0 ||
-	    (ret = crypto_shash_final(&crypto.desc, result)) != 0) {
-		printk("crypto_shash_update/final() failed ret=%d\n", ret);
+	shash->tfm = crypto.tfm;
+	if ((ret = crypto_shash_init(shash)) != 0 ||
+	    (ret = crypto_shash_update(shash, (const void *)&ipsrc, sizeof(ipsrc))) != 0 ||
+	    (ret = crypto_shash_update(shash, (const void *)&epoch_min, sizeof(epoch_min))) != 0 ||
+	    (ret = crypto_shash_final(shash, result)) != 0) {
+		printk("crypto_shash_init/update/final() failed ret=%d\n", ret);
 		goto out;
 	}
 	crypt_to_hex(hexresult, result, crypto.size);
@@ -1082,8 +1079,6 @@ static int __init xt_pknock_mt_init(void)
 	}
 
 	crypto.size = crypto_shash_digestsize(crypto.tfm);
-	crypto.desc.tfm = crypto.tfm;
-
 	pde = proc_mkdir("xt_pknock", init_net.proc_net);
 	if (pde == NULL) {
 		pr_err("proc_mkdir() error in _init().\n");