Re: [PATCH 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] Error path in `hvcs_initialize()` leaves `hvcs_task` as an `ERR_PTR` and `hvcs_pi_buff` as a dangling pointer, leading to a kernel panic and double-free on module exit.
--

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

tty: hvcs: replace __get_free_page() with kmalloc()
    
hvcs_initialize() allocates the buffer that receives the partner info
returned by the H_VTERM_PARTNER_INFO hypercall. This buffer can be allocated
with kmalloc() as there's nothing special about it to go directly to the
page allocator.

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 will this error path leave
hvcs_task as an ERR_PTR and hvcs_pi_buff as a dangling pointer?

If hvcs_initialize() fails at kthread_run(), hvcs_task will hold an ERR_PTR
and hvcs_pi_buff is freed here but not nullified.

>  buff_alloc_fail:
>  	tty_unregister_driver(hvcs_tty_driver);
>  register_fail:
[ ... ]
> @@ -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 isn't a bug introduced by this patch, but could module unload cause a
kernel panic and a double-free if initialization previously failed?

Since hvcs_task is left as an ERR_PTR on failure rather than being reset to
NULL, the early return check in hvcs_module_exit() fails to catch it:

drivers/tty/hvc/hvcs.c:hvcs_module_exit() {
	if (!hvcs_task)
		return;
    ...
}

This allows execution to proceed to kthread_stop(hvcs_task), which will
dereference the error pointer.

It also proceeds to double-free the dangling hvcs_pi_buff here, and
eventually passes the NULL hvcs_tty_driver to tty_unregister_driver(),
resulting in a NULL pointer dereference panic.

>  	hvcs_pi_buff = NULL;
>  	spin_unlock(&hvcs_pi_lock);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830-tty-v1-0-0cbe6170649b@kernel.org?part=3