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 9388c49d509fda02b652c81dc214900fab6f5e1d..1ac54fbcf29f76e0c6a561dcef02ac3b3a5f616c 100644 (file)
--- 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");