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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Key payloads detached during update or revocation and pending RCU destruction are skipped, leaking secrets into the crash dump.
- [High] Lockless reading of `big_key` payloads during non-atomic updates causes torn reads, leading to massive out-of-bounds writes during panic zeroization.
--

--- Patch [4]: [PATCH 4/4] security/keys: zeroize key payloads before kdump ---
commit 5ba36d14be79dc23bd8b584d1f1a143c55ab30ba
Author: Jan Sebastian Götte <[email protected]>

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. Implement the zeroize op for
various 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]
Does this code risk an out-of-bounds write if a panic happens during a key
update?

If big_key_update() is called, it updates the payload via
generic_key_instantiate(), which writes the new buffer pointer to data[0] and
then updates the length in data[3]:

security/keys/key.c:generic_key_instantiate() {
    rcu_assign_keypointer(key, prep->payload.data[0]);
    key->payload.data[1] = prep->payload.data[1];
    key->payload.data[2] = prep->payload.data[2];
    key->payload.data[3] = prep->payload.data[3];
}

If a panic stops the writer CPU after rcu_assign_keypointer() but before the
length is updated, could big_key_zeroize() read the new, smaller buffer pointer
but use the old, larger length, causing memzero_explicit() to corrupt memory
during the crash dump process?

> +	}
> +}
> +
>  /*
>   * Register key type
>   */
[ ... ]
> 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;
> +	}
> +
> +	for (node = rb_first(&key_serial_tree); node; node = rb_next(node)) {

[Severity: High]
Does this loop miss key payloads that have been detached but not yet freed?

When a key is updated, the old payload is displaced and queued for RCU free:

security/keys/user_defined.c:user_update() {
    ...
    rcu_assign_keypointer(key, prep->payload.data[0]);
    prep->payload.data[0] = NULL;
    
    if (zap)
        call_rcu(&zap->rcu, user_free_payload_rcu);
}

If a panic occurs before the RCU grace period completes, the old payload
remains in memory but is no longer reachable via the active keys in
key_serial_tree. Will these displaced payloads containing secrets be leaked
into the crash dump since they are skipped by this zeroization loop?

> +		struct key *key = rb_entry(node, struct key, serial_node);
> +
> +		if (key->type == &key_type_keyring ||
> +		    key->state == KEY_IS_UNINSTANTIATED)
> +			continue;
> +
> +		/* custom zeroize since free'ing isn't safe at this point */
> +		if (key->type->zeroize)
> +			key->type->zeroize(key);
> +	}
> +	/* off to kexec()! */
> +	return NOTIFY_DONE;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4