Re: [PATCH v2 3/3] tty: hvcs: replace __get_free_page() with kmalloc()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Failure in kthread_run leaves global variables dangling and pointing to an ERR_PTR, bypassing initialization checks on subsequent probes (leading to UAF) and causing a kernel panic on module unload.
--

commit b1538891a24d98413046b6d1a4fb966407282fe0
Author: Mike Rapoport (Microsoft) <rppt@kernel.org>

tty: hvcs: replace __get_free_page() with kmalloc()

This patch replaces the use of __get_free_page() and free_page() with
kmalloc() and kfree() respectively for the hvcs_pi_buff allocation. It
simplifies the API usage and removes the need for size tracking during
deallocation.

Link: https://lore.kernel.org/all/635405e4-9423-4a25-a6e7-e03c8ea0bcbe@redhat.com

> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> index d32b1e3c50bfd..b94d59fa36274 100644
> --- a/drivers/tty/hvc/hvcs.c
> +++ b/drivers/tty/hvc/hvcs.c
[ ... ]
> @@ -1486,7 +1486,7 @@ static int hvcs_initialize(void)
>  	return 0;
>  
>  kthread_fail:
> -	free_page((unsigned long)hvcs_pi_buff);
> +	kfree(hvcs_pi_buff);

[Severity: High]
This isn't a bug introduced by this patch, but it looks like the error handling 
here leaves global variables dangling.

If kthread_run() fails earlier in hvcs_initialize(), it returns an ERR_PTR 
which is assigned to the global hvcs_task pointer. In this error path, 
hvcs_pi_buff is freed, but neither hvcs_task nor hvcs_pi_buff are reset to 
NULL.

Since hvcs_task is not NULL (it holds the ERR_PTR), a subsequent call to 
hvcs_initialize() might bypass initialization:

	if (hvcs_task) {
		mutex_unlock(&hvcs_init_mutex);
		return 0;
	}

Could this lead to use-after-free bugs if later code tries to use the freed 
hvcs_pi_buff?

[ ... ]
> @@ -1528,7 +1528,7 @@ static void __exit hvcs_module_exit(void)
>  	kthread_stop(hvcs_task);
>  
>  	spin_lock(&hvcs_pi_lock);
> -	free_page((unsigned long)hvcs_pi_buff);
> +	kfree(hvcs_pi_buff);

[Severity: High]
This is a pre-existing issue, but does this cleanup path attempt to dereference 
an invalid pointer if hvcs_initialize() previously failed during kthread_run()?

If hvcs_task contains an ERR_PTR, calling kthread_stop(hvcs_task) could cause 
an invalid dereference.

Also, because hvcs_pi_buff was already freed in the kthread_fail path and never 
set to NULL, would this result in a double free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-tty-v2-0-88cd58c3c640@kernel.org?part=3