Re: [PATCH v3 3/3] s390/debug: Fix race between debug area resize and event logging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Preexisting NULL pointer dereference in debug_info_copy() if debug_register_static() fails to allocate memory.
--

commit 1749ca122b22e4769c7efc7f4a52046777b678ef
Author: Mikhail Zaslonko <zaslonko@linux.ibm.com>

s390/debug: Fix race between debug area resize and event logging

Trace functions check for non-NULL id->areas without lock to minimize
overhead. This opens a race window where a NULL pointer dereference
occurs if id->areas is set to NULL after the check and before id->lock
is taken. This commit fixes the race by rechecking id->areas under lock.

> diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
> index cf411f203571b..b5bf8284dbfc8 100644
> --- a/arch/s390/kernel/debug.c
> +++ b/arch/s390/kernel/debug.c
[ ... ]
> @@ -1663,9 +1673,11 @@ static void debug_flush(debug_info_t *id, int area)
>  	unsigned long flags;
>  	int i, j;
>  
> -	if (!id || !id->areas)
> +	if (!id)
>  		return;
>  	raw_spin_lock_irqsave(&id->lock, flags);
> +	if (!id->areas)
> +		goto out;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential NULL
pointer dereference in debug_info_copy() if debug_register_static() fails
to allocate memory.

In arch/s390/kernel/debug.c:debug_register_static(), if the allocation fails:

	if (!copy) {
		pr_err("Registering debug feature %s failed\n", id->name);

		/* Clear pointers to prevent tracing into released initdata. */
		raw_spin_lock_irqsave(&id->lock, flags);
		id->areas = NULL;
		id->active_pages = NULL;
		id->active_entries = NULL;
		raw_spin_unlock_irqrestore(&id->lock, flags);

		return;
	}

This sets id->areas to NULL but leaves id->pages_per_area and id->nr_areas
with their original EARLY_PAGES and EARLY_AREAS values.

Later, if debug_info_copy() is called when a user accesses the exposed
debugfs file:

	for (i = 0; i < in->nr_areas; i++) {
		for (j = 0; j < in->pages_per_area; j++)
			memcpy(rc->areas[i][j], in->areas[i][j], PAGE_SIZE);

Will this cause a NULL pointer dereference when accessing in->areas[i][j]
since in->areas is NULL but the loop still executes based on in->nr_areas?
Should we reset id->pages_per_area and id->nr_areas to 0 on the error path
in debug_register_static(), or add a NULL check in debug_info_copy()?

>  	if (area == DEBUG_FLUSH_ALL) {
>  		id->active_area = 0;
>  		memset(id->active_entries, 0, id->nr_areas * sizeof(int));
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903130733.709001-1-zaslonko@linux.ibm.com?part=3