Re: [PATCH v3 5/7] zloop: add degrade_element control command

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] block: double-free and UAF in zloop_ctl_add error path
- [Low] block: missing presence check and error message for element_id in zloop_ctl_degrade_element
--

commit 85f6af597dc5c733cb0ab607eae7cb9cac0357b4
Author: Damien Le Moal <dlemoal@kernel.org>

zloop: add degrade_element control command

Allow users to mark storage elements of a zloop device as degraded using
the new "degrade_element" control command. The element to degrade is
indicated using the element_id option.

> diff --git a/drivers/block/zloop.c b/drivers/block/zloop.c
> index e113ecd63c0b2..311588de853fc 100644
> --- a/drivers/block/zloop.c
> +++ b/drivers/block/zloop.c

[ ... ]

> @@ -1384,6 +1890,8 @@ static int zloop_ctl_add(struct zloop_options *opts)
>  	kfree(zlo->base_dir);
>  out_destroy_workqueue:
>  	destroy_workqueue(zlo->workqueue);
> +out_destroy_storage_elements:
> +	kfree(zlo->elements);

[Severity: High]
Does this code introduce a double-free and use-after-free on zlo->elements?

When a failure in zloop_ctl_add() causes execution to jump to the
out_cleanup_disk label (which is located just above this block),
put_disk(zlo->disk) is called. This synchronously triggers the
zloop_free_disk() fops callback. This callback completely frees zlo and
its members, including zlo->elements.

The error path then falls through and accesses the already freed zlo pointer
to sequentially double-free the same resources, including the newly added
zlo->elements shown here.

Can this be triggered from user-space by writing invalid parameters or
encountering resource exhaustion during the device add command on
/dev/zloop-control?

[ ... ]

> @@ -1946,6 +1991,42 @@ static int zloop_ctl_remove(struct zloop_options *opts)
>  	return 0;
>  }
>  
> +static int zloop_ctl_degrade_element(struct zloop_options *opts)
> +{
> +	struct zloop_device *zlo;
> +	int ret = 0;
> +
> +	if (!(opts->mask & ZLOOP_OPT_ID)) {
> +		pr_err("No ID specified for degrade_element\n");
> +		return -EINVAL;
> +	}
> +
> +	if (opts->mask & ~(ZLOOP_OPT_ID | ZLOOP_OPT_ELEMENT_ID)) {
> +		pr_err("Invalid option specified for degrade_element\n");
> +		return -EINVAL;
> +	}

[Severity: Low]
Does this code fail to verify the presence of the ZLOOP_OPT_ELEMENT_ID option?

This bitwise check in zloop_ctl_degrade_element() only validates that no
extra options are present, but it does not enforce that ZLOOP_OPT_ELEMENT_ID
is actually provided. If element_id is omitted, it defaults to 0 and
zloop_degrade_element(zlo, 0) will safely catch this and return -EINVAL.
However, the user receives a silent failure rather than an informative error
message about the missing required argument.

Can we add a check similar to ZLOOP_OPT_ID to ensure ZLOOP_OPT_ELEMENT_ID
is explicitly provided?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007082344.1049179-1-dlemoal@kernel.org?part=5