Re: [PATCH v3] killswitch: add per-function short-circuit mitigation primitive

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling __module_address() without rcu_read_lock() or module_mutex in ks_module_notify() triggers lockdep splats and risks a Use-After-Free.
- [Medium] The command parser in ks_control_write() rigidly fails with -EINVAL if there are multiple spaces or tabs between arguments.
--

commit 41367b8b3bd9a102ede7118d0216e23f583793b2
Author: Sasha Levin <sashal@kernel.org>

killswitch: add per-function short-circuit mitigation primitive

This commit introduces a killswitch mitigation primitive, which allows
an administrator to force a given function to return a specified value
without executing its body. This is intended to mitigate security issues
until a patch can be deployed. It is operated via a securityfs interface.

> diff --git a/kernel/killswitch.c b/kernel/killswitch.c
> new file mode 100644
> index 0000000000000..7f509c62ea748
> --- /dev/null
> +++ b/kernel/killswitch.c

[ ... ]

> +static int ks_module_notify(struct notifier_block *nb, unsigned long action,
> +			    void *data)
> +{
> +	struct module *mod = data;
> +	struct ks_attr *attr, *n;
> +
> +	if (action != MODULE_STATE_GOING)
> +		return NOTIFY_DONE;
> +
> +	mutex_lock(&ks_lock);
> +	list_for_each_entry_safe(attr, n, &ks_engaged_list, list) {
> +		if (!attr->kp.addr ||
> +		    __module_address((unsigned long)attr->kp.addr) != mod)

Does this need to be wrapped in an rcu_read_lock() and rcu_read_unlock()?

The __module_address() function traverses the RCU-protected module latch tree
and requires the caller to hold module_mutex or be within an RCU read-side
critical section.

The module subsystem drops the module_mutex before invoking the blocking
notifier chain, so this code executes without it.

Can this trigger a lockdep warning or risk a use-after-free if concurrent
operations modify the tree during traversal?

> +			continue;
> +
> +		pr_warn("killswitch: %s mitigation lost: module %s unloading; re-engage after reload if still needed\n",
> +			attr->kp.symbol_name, mod->name);

[ ... ]

> +static ssize_t ks_control_write(struct file *file, const char __user *ubuf,
> +				size_t count, loff_t *ppos)
> +{
> +	char *buf, *cur, *verb, *sym, *retstr;
> +	long retval = 0;
> +	int ret;

[ ... ]

> +	cur = strim(buf);
> +	verb = strsep(&cur, " \t\n");
> +	if (!verb || !*verb) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (!strcmp(verb, "disengage_all")) {
> +		mutex_lock(&ks_lock);
> +		ks_disengage_all_locked();
> +		mutex_unlock(&ks_lock);
> +		ret = count;
> +		goto out;
> +	}
> +
> +	sym = strsep(&cur, " \t\n");
> +	if (!sym || !*sym) {

Is it possible for valid input to be rejected here if there are consecutive
spaces between tokens?

If a user or script writes "engage  my_func  0" with multiple spaces,
strsep() replaces the first space with a null terminator and returns an empty
string for the subsequent space.

The check !*sym would then interpret this empty token as invalid input and
abort with -EINVAL.

Would it be better to use skip_spaces() or a loop to handle natural variations
in whitespace?

> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (!strcmp(verb, "disengage")) {
> +		ret = __ks_disengage(sym);
> +		ret = ret ? ret : count;
> +		goto out;
> +	}
> +
> +	if (strcmp(verb, "engage")) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	retstr = strsep(&cur, " \t\n");
> +	if (!retstr || !*retstr) {

Could this also fail similarly if there are consecutive spaces before the
return value token?

> +		ret = -EINVAL;
> +		goto out;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517134858.146569-1-sashal@kernel.org?part=1