Re: [RFC PATCH 11/13] mm/kwatch: add debugfs control plane

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Tasks that exit or are killed while inside a probed function permanently leak their kwatch_ctx_pool slot and leave their hardware breakpoint globally armed.
- [High] Missing kernel lockdown check allows root to bypass confidentiality restrictions by setting hardware breakpoints on arbitrary kernel addresses.
- [Medium] Lockless access to the global `kwatch_config` and `watching_active` variables in `kwatch_dbgfs_read` causes data races and torn reads.
- [Low] The KWATCH config entry selects KPROBES, KRETPROBES, and STACKTRACE without inheriting their architecture-specific dependencies, causing unmet dependency warnings.
--

--- Patch [11]: [RFC PATCH 11/13] mm/kwatch: add debugfs control plane ---
commit fe07df22dbe76645eaac13880523515d1694aec8
Author: Jinchao Wang <wangjinchao600@gmail.com>

mm/kwatch: add debugfs control plane

Wire the pieces together behind a single debugfs file,
/sys/kernel/debug/kwatch/config. Writing a key=value configuration
string stops any active session and starts a new one; reading shows
the active configuration and the nmi_rejected counter. An open-count
guard keeps the file single-open and a mutex serializes
start/stop/auto-stop against each other.

> diff --git a/mm/kwatch/Kconfig b/mm/kwatch/Kconfig
> new file mode 100644
> index 0000000000000..b1c37a829dd53
> --- /dev/null
> +++ b/mm/kwatch/Kconfig
> @@ -0,0 +1,17 @@
> +config KWATCH
> +	tristate "Kernel Watch Framework"
> +	depends on PERF_EVENTS && HAVE_HW_BREAKPOINT && DEBUG_FS
> +	depends on HAVE_REINSTALL_HW_BREAKPOINT
> +	select KPROBES
> +	select KRETPROBES
> +	select STACKTRACE

[Severity: Low]
Should this Kconfig entry inherit architecture-specific dependencies for
KPROBES and STACKTRACE?

Selecting these without depending on HAVE_KPROBES and STACKTRACE_SUPPORT
can cause unmet dependency warnings and potential build failures on
architectures that do not support them.

[ ... ]
> diff --git a/mm/kwatch/core.c b/mm/kwatch/core.c
> new file mode 100644
> index 0000000000000..548d0cdd08126
> --- /dev/null
> +++ b/mm/kwatch/core.c
> @@ -0,0 +1,325 @@
[ ... ]
> +static int kwatch_start_watching(void)
> +{
> +	int ret;
[ ... ]
> +	ret = kwatch_hwbp_prealloc(kwatch_config.max_watch,
> +				   kwatch_config.access_type);
> +	if (ret) {
> +		pr_err("kwatch_hwbp_prealloc ret: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = kwatch_tsk_ctx_prealloc(kwatch_config.max_concurrency);
> +	if (ret) {
> +		kwatch_hwbp_free();
> +		return ret;
> +	}
> +
> +	ret = kwatch_probe_start(&kwatch_config);

[Severity: High]
If a task exits or is killed while inside a probed function, could this leak
the kwatch_ctx_pool slot and leave hardware breakpoints permanently armed?

When a task terminates, the kretprobe framework flushes instances without
executing the exit handlers (like kwatch_lifecycle_exit() in probe.c).
This skips the kwatch_tsk_ctx_put() call, leaving the slot occupied
and the HWBP armed until the kwatch session is manually stopped.

[ ... ]
> +static ssize_t kwatch_dbgfs_read(struct file *file, char __user *user_buf,
> +				 size_t count, loff_t *ppos)
> +{
> +	char *out_buf;
> +	size_t len = 0;
> +	ssize_t ret;
> +
> +	out_buf = kzalloc(MAX_CONFIG_STR_LEN, GFP_KERNEL);
> +	if (!out_buf)
> +		return -ENOMEM;
> +
> +	if (watching_active) {
> +		len += scnprintf(out_buf + len, MAX_CONFIG_STR_LEN - len,

[Severity: Medium]
Is it safe to access watching_active and format kwatch_config here without
holding kwatch_dbgfs_mutex?

Since concurrent writes (via kwatch_dbgfs_write) zero out kwatch_config
while holding the mutex, this lockless read could result in data races
or torn reads of the configuration state.

[ ... ]
> +static ssize_t kwatch_dbgfs_write(struct file *file, const char __user *buffer,
> +				  size_t count, loff_t *ppos)
> +{
> +	char *input_alloc;
> +	char *parse_str;
> +	int ret;
[ ... ]
> +	ret = kwatch_config_parse(parse_str, &kwatch_config);
> +	if (ret) {
> +		pr_err("Failed to parse config %d\n", ret);
> +		goto out;
> +	}
> +
> +	ret = kwatch_start_watching();

[Severity: High]
Could a missing lockdown check here allow root to bypass confidentiality
restrictions?

Since this enables setting hardware breakpoints on arbitrary memory addresses
provided by userspace, it might allow inspecting kernel memory in violation
of LOCKDOWN_CONFIDENTIALITY. perf_event_open() normally enforces
security_locked_down() checks, but register_wide_hw_breakpoint() does not.

> +	if (ret) {
> +		pr_err("Failed to start watching with %d\n", ret);
> +		goto out;
> +	}
> +
> +	ret = count;
> +
> +out:
> +	mutex_unlock(&kwatch_dbgfs_mutex);
> +	kfree(input_alloc);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714182243.10687-1-wangjinchao600@gmail.com?part=11