[Devel] [PATCH vz10 20/24] dm-ploop: fix NULL-derefs on the ctr error / fault-injection paths

[Pavel Tikhomirov]

Functionally sound, but worth splitting into two commits.

Reviewed-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>

On 7/6/26 12:59, Konstantin Khorenko wrote:
> Two NULL-dereferences reachable when a ploop_ctr() allocation fails (or is
> made to fail via CONFIG_FAULT_INJECTION):
> 
> 1) ploop_destroy() stops both worker kthreads under a single
>    "if (ploop->kt_worker)" guard and dereferences ploop->kt_allocator->task
>    unconditionally. ploop_ctr() creates kt_worker first and kt_allocator
>    second (a second ploop_worker_create()); if that second create fails,
>    the error unwind calls ploop_destroy() with kt_worker set but
>    kt_allocator still NULL, so kthread_stop(ploop->kt_allocator->task)
>    NULL-derefs. Guard the kt_allocator teardown with its own NULL check
>    (the following kfree() is already NULL-safe).
> 
> 2) ploop_get_delta_file() is annotated ALLOW_ERROR_INJECTION(..., ERRNO_NULL)
>    but only ever returns a valid file or ERR_PTR(); its sole caller,
>    ploop_add_deltas_stack(), checks IS_ERR() only and then passes the file
>    to vfs_fsync(). fail_function's default retval for ERRNO_NULL is 0 (NULL),
>    so default-configured injection returns NULL, slips past IS_ERR(), and
>    NULL-derefs in vfs_fsync(). The function never legitimately returns NULL,
>    so the correct injection class is ERRNO (error pointers only).
> 
> Fixes: 69aa8c83a32e ("dm-ploop: make filespace preallocations async")
> Fixes: a73b362376db ("dm-ploop: Allow fault injection for all ploop functions")
> Feature: dm-ploop: ploop target driver
> https://virtuozzo.atlassian.net/browse/VSTOR-137234
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> ---
>  drivers/md/dm-ploop-target.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
> index 0de0e84d440d..3b589579741c 100644
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c
> @@ -188,7 +188,9 @@ static void ploop_destroy(struct ploop *ploop)
>  
>  		/* waits for the thread to stop */
>  		kthread_stop(ploop->kt_worker->task);
> -		kthread_stop(ploop->kt_allocator->task);
> +		/* kt_allocator may be NULL if ploop_ctr() failed early */
> +		if (ploop->kt_allocator)
> +			kthread_stop(ploop->kt_allocator->task);
>  
>  		WARN_ON(!llist_empty(&ploop->pios[PLOOP_LIST_PREPARE]));
>  		WARN_ON(!llist_empty(&ploop->llresubmit_pios));
> @@ -255,7 +257,7 @@ static struct file *ploop_get_delta_file(struct ploop *ploop, int fd)
>  	return ERR_PTR(ret);
>  
>  }
> -ALLOW_ERROR_INJECTION(ploop_get_delta_file, ERRNO_NULL);
> +ALLOW_ERROR_INJECTION(ploop_get_delta_file, ERRNO);
>  
>  static int ploop_check_top_delta(struct ploop *ploop, struct file *file)
>  {

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.