Re: [PATCH 4/4] security/keys: zeroize key payloads before kdump

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Torn payload updates and in-place frees in `big_key_update` cause Use-After-Free and Out-Of-Bounds writes in `big_key_zeroize` during panic.
- [High] Sensitive key types (like NVMe TLS PSK and Ceph keys) are missing zeroization callbacks and will leak in crash dumps.
- [High] Keys removed from the rbtree by the garbage collector but not yet destroyed are missed by the crash zeroization mechanism.
- [High] Keys whose payloads are currently being instantiated are skipped during panic zeroization due to a TOCTOU race with `key->state`.
- [High] The entire crash zeroization mechanism can be reliably bypassed if `key_serial_lock` is held when panic occurs.
--

--- Patch [4]: [PATCH 4/4] security/keys: zeroize key payloads before kdump ---
commit 6f552c7707db0ce9c5a1ed7eeec0a2afb30b26a3
Author: Jan Sebastian Götte <linux@jaseg.de>

security/keys: zeroize key payloads before kdump

When CONFIG_CRASH_ZEROIZE is set, try to erase key payloads on panic before
jumping to the kdump kernel. It implements the zeroize op for the user/logon,
encrypted, trusted, and big_key types.

> diff --git a/security/keys/big_key.c b/security/keys/big_key.c
> index 268f702df380e..ad8537dda70fa 100644
> --- a/security/keys/big_key.c
> +++ b/security/keys/big_key.c

[ ... ]

> @@ -279,6 +282,18 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen)
>  	return ret;
>  }
>  
> +static void big_key_zeroize(struct key *key)
> +{
> +	struct big_key_payload *payload = to_big_key_payload(key->payload);
> +
> +	if (payload->data) {
> +		if (payload->length > BIG_KEY_FILE_THRESHOLD)
> +			memzero_explicit(payload->data, CHACHA20POLY1305_KEY_SIZE);
> +		else
> +			memzero_explicit(payload->data, payload->length);
> +	}
> +}

[Severity: High]
Can this result in a use-after-free or out-of-bounds write if a panic occurs
during big_key_update?

In big_key_update(), the key payload is modified in-place:

security/keys/big_key.c:big_key_update() {
        ...
        if (key_is_positive(key))
                big_key_destroy(key);

        return generic_key_instantiate(key, prep);
}

generic_key_instantiate() updates the payload union sequentially. If a panic
occurs exactly when the payload pointer is freed, or when the new pointer is
written but payload->length is still the old length, this zeroize function
will read the torn state asynchronously and potentially write zeroes to freed
memory or beyond the bounds of the new buffer.

> diff --git a/security/keys/key.c b/security/keys/key.c
> index 091ee084bc300..4f0e8e579cebd 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c

[ ... ]

> @@ -1268,6 +1269,44 @@ void unregister_key_type(struct key_type *ktype)
>  }
>  EXPORT_SYMBOL(unregister_key_type);
>  
> +#ifdef CONFIG_CRASH_ZEROIZE
> +/* Called far into vpanic from crash_core.c with other CPUs stopped and
> + * preemption disabled
> + */
> +static int key_crash_zeroize(struct notifier_block *nb, unsigned long action,
> +		void *data)
> +{
> +	struct rb_node *node;
> +
> +	/* If we can't acquire the lock, the rbtree might be in an inconsistent
> +	 * state. That's all we can do then, as there's no point to waiting
> +	 * at this stage.
> +	 */
> +	if (!spin_trylock(&key_serial_lock)) {
> +		pr_crit("crash_zeroize: can't acquire key_serial_lock. skipping keyrings.\n");
> +		return NOTIFY_DONE;
> +	}

[Severity: High]
Does this completely bypass zeroization if another CPU holds the lock when
the panic occurs?

Any legitimate action on a separate CPU that briefly acquires key_serial_lock
will cause the panicking CPU to fail this trylock. Could this be used to
reliably bypass the crash zeroize protection for all keys if the panic is
triggered during that window?

> +
> +	for (node = rb_first(&key_serial_tree); node; node = rb_next(node)) {
> +		struct key *key = rb_entry(node, struct key, serial_node);

[Severity: High]
Are keys that are pending destruction missed by this loop?

When a key is unreferenced, key_garbage_collector() removes it from the
key_serial_tree and adds it to a local graveyard list before freeing it:

security/keys/gc.c:key_garbage_collector() {
        ...
        rb_erase(&key->serial_node, &key_serial_tree);
        spin_unlock(&key_serial_lock);

        list_add_tail(&key->graveyard_link, &graveyard);
        ...
}

If a panic happens between the removal from the tree and the actual
destruction (which can involve sleeping for synchronize_rcu), the key
material is still in memory but seems to be skipped here.

> +
> +		if (key->type == &key_type_keyring ||
> +		    key->state == KEY_IS_UNINSTANTIATED)
> +			continue;

[Severity: High]
Can this skip newly instantiated keys due to a race condition?

In __key_instantiate_and_link(), the payload is populated before the state
is updated:

security/keys/key.c:__key_instantiate_and_link() {
        ...
        ret = key->type->instantiate(key, prep);

        if (ret == 0) {
                atomic_inc(&key->user->nikeys);
                mark_key_instantiated(key, 0);
        ...
}

If a panic occurs in the window after the instantiate callback copies the
secret material but before the state changes from KEY_IS_UNINSTANTIATED,
the key material is already present but will be skipped by this check.

> +
> +		/* custom zeroize since free'ing isn't safe at this point */
> +		if (key->type->zeroize)
> +			key->type->zeroize(key);

[Severity: High]
Will this silently leak sensitive keys that lack a zeroize callback?

Several sensitive key types appear to be missing the zeroize callback
definition in this patch, such as nvme_tls_psk_key_type in
drivers/nvme/common/keyring.c and key_type_ceph in net/ceph/crypto.c.

Since this loop skips any type without a zeroize callback, those
cryptographic keys will bypass the crash zeroize mechanism and remain
in crash dumps.

> +	}
> +	/* off to kexec()! */
> +	return NOTIFY_DONE;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731162739.158320-1-linux@jaseg.de?part=4