Re: [PATCH 3/7] block: add storage element management ioctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] block: missing queue freeze during storage element reconfiguration
- [High] block: integer truncation in invalidate_inode_pages2_range() skips cache invalidation
- [High] block: heap out-of-bounds read and info leak in blkdev_report_storage_elements_ioctl()
- [High] block: loop-carried dependency in disk_wait_for_se_mgmt_completion() causes heap overflow
- [Medium] block: unbounded memory allocation in blkdev_report_storage_elements_ioctl()
- [Medium] block: silent OOM failure in disk_wait_for_se_mgmt_completion() bypasses wait
--

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

block: add storage element management ioctls

This commit introduces new ioctl commands for zoned block devices to allow
userspace management of storage elements. It provides interfaces for getting
the number of elements, reporting their status, and removing or restoring
degraded elements.

> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index b713512f7d48f..62b9cf21e9adf 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
[ ... ]
> @@ -2794,6 +2795,69 @@ int bdev_report_storage_elements(struct block_device *bdev,
[ ... ]
> +static int blkdev_report_storage_elements_ioctl(struct block_device *bdev,
> +						void __user *argp)
> +{
> +	struct blk_storage_elements_report rep;
> +	struct blk_storage_element *elements;
> +	unsigned int nr_elements;
> +	unsigned long retc;
> +	int ret;
> +
> +	if (!argp)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&rep, argp,
> +			   sizeof(struct blk_storage_elements_report)))
> +		return -EFAULT;
> +
> +	if (!rep.nr_elements)
> +		return -EINVAL;
> +
> +	nr_elements = rep.nr_elements;
> +	elements = kzalloc_objs(struct blk_storage_element, nr_elements);
> +	if (!elements)
> +		return -ENOMEM;

[Severity: Medium]
Is it safe to use kzalloc_objs() here with an unbounded user-provided count?

A maliciously crafted ioctl call with a huge rep.nr_elements will request an
allocation exceeding KMALLOC_MAX_SIZE. This will trigger a kernel
WARN_ON_ONCE stack trace and fail with -ENOMEM. Repeated calls by an
unprivileged user with read access can spam dmesg and degrade system
performance.

> +
> +	ret = bdev_report_storage_elements(bdev, &nr_elements, elements);
> +	if (ret)
> +		goto free_elements;
> +
> +	retc = copy_to_user(argp + sizeof(struct blk_storage_elements_report),
> +			    elements,
> +			    sizeof(struct blk_storage_element) * nr_elements);

[Severity: High]
Can this copy_to_user() read out of bounds and leak kernel heap memory?

If the hardware reports more elements than the user originally requested,
bdev_report_storage_elements() updates nr_elements to the larger count.
This larger nr_elements is then used to determine the copy byte length,
which copies past the end of the elements allocation and leaks uninitialized
adjacent kernel heap memory to userspace.

[ ... ]
> @@ -2847,6 +2911,22 @@ int bdev_remove_storage_element(struct block_device *bdev,
>  }
>  EXPORT_SYMBOL_GPL(bdev_remove_storage_element);
>  
> +static int blkdev_remove_storage_element_ioctl(struct block_device *bdev,
> +					blk_mode_t mode, void __user *argp)
> +{
> +	unsigned int element_id;
> +
> +	if (!(mode & BLK_OPEN_WRITE))
> +		return -EBADF;
> +	if (bdev_read_only(bdev))
> +		return -EPERM;
> +
> +	if (get_user(element_id, (unsigned int __user *)argp))
> +		return -EFAULT;
> +
> +	return bdev_remove_storage_element(bdev, element_id);
> +}

[Severity: High]
Does bdev_remove_storage_element() fail to hold the queue freeze?

In bdev_remove_storage_element(), the queue is unfrozen prematurely:

	memflags = blk_mq_freeze_queue(disk->queue);
	blk_mq_unfreeze_queue(disk->queue, memflags);

The block layer subsystem guide requires maintaining a queue freeze across
reconfiguration or teardown. blk_revalidate_disk_zones() assumes the queue
is actively frozen when called from a reconfiguration context.

Since the ioctl does not implicitly serialize I/O, concurrent userspace I/O
can be submitted while the zone structure is being invalidated and rewritten.
This allows in-flight BIOs to read torn configuration state or cause
use-after-free.


[Severity: High]
Does bdev_remove_storage_element() pass bytes instead of a page index to
invalidate_inode_pages2_range()?

	invalidate_inode_pages2_range(bdev->bd_mapping, 0,
				      get_capacity(disk) << SECTOR_SHIFT);

The end parameter expects a page index (pgoff_t). On 32-bit systems where
pgoff_t is a 32-bit unsigned long, passing a byte capacity for any disk
exceeding 4GB overflows the parameter.

For example, exactly 4GB evaluates to 0, invalidating only the 0th page.
This leaves the rest of the page cache entirely stale, causing silent data
corruption when userspace later reads from the modified block device.


[Severity: High]
Does disk_wait_for_se_mgmt_completion() have a loop-carried dependency that
causes a heap overflow?

When called by bdev_remove_storage_element(), the elements buffer is
allocated once before entering the polling loop. Inside the loop:

	while (1) {
		/*
		 * Check if we have storage elements being removed or restored.
		 */
		nr_se = nr_elements;
		ret = (disk->fops->se_ops->report_elements)(disk,
						&nr_elements, elements);

If the device dynamically increases the number of reported elements,
report_elements updates nr_elements to the higher value.

In the next loop iteration, this larger nr_elements is passed back into
report_elements, which will write the larger number of elements into the
originally sized elements heap buffer, causing an out-of-bounds heap
overwrite.


[Severity: Medium]
Does disk_wait_for_se_mgmt_completion() bypass the wait on OOM?

	elements = kzalloc_objs(struct blk_storage_element, nr_elements);
	if (!elements)
		return;

If kzalloc_objs() fails, the function silently returns without completing its
waiting loop.

Under memory pressure, this allows the caller to immediately proceed and
invoke blk_revalidate_disk_zones() while the device is actively busy
mutating its zone configuration in the background. This leads to I/O timeouts,
torn reads of zone statuses, or hardware errors.

[ ... ]
> @@ -2886,3 +2966,37 @@ int bdev_restore_storage_elements(struct block_device *bdev)
>  	return blk_revalidate_disk_zones(disk);
>  }
>  EXPORT_SYMBOL_GPL(bdev_restore_storage_elements);
> +
> +static int blkdev_restore_storage_elements_ioctl(struct block_device *bdev,
> +						 blk_mode_t mode)
> +{
> +	if (!(mode & BLK_OPEN_WRITE))
> +		return -EBADF;
> +	if (bdev_read_only(bdev))
> +		return -EPERM;
> +
> +	return bdev_restore_storage_elements(bdev);
> +}

[Severity: High]
Does bdev_restore_storage_elements() also fail to hold the queue freeze?

Similar to the removal path, the queue is unfrozen prematurely:

	memflags = blk_mq_freeze_queue(disk->queue);
	blk_mq_unfreeze_queue(disk->queue, memflags);

This opens a window for concurrent I/Os during restoration.


[Severity: High]
Does bdev_restore_storage_elements() also skip cache invalidation due to
integer truncation?

Similar to the removal path, it passes a byte capacity instead of a page
index:

	invalidate_inode_pages2_range(bdev->bd_mapping, 0,
				      get_capacity(disk) << SECTOR_SHIFT);

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