Re: [PATCH 4/4] dibs: introduce dibs block device peer device driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider:
- [Critical] dibs: data corruption due to missing request sequence numbers
- [High] dibs: UAF in worker due to unregistering ring buffer before cancel_work_sync
- [High] block: UAF of tag_set and driver data on queue teardown
- [High] block: uninitialized tag_set usage due to early ring buffer registration
- [High] dibs: race condition on device_add_disk state causes NULL dereference
- [High] dibs: UAF of blkpd due to media_gone_work scheduled after cancellation
- [High] dibs: UAF / Race condition accessing unregistered ring buffer from queue_rq
- [High] dibs: TOCTOU bounds check bypass via shared memory mutation
- [High] block: deadlock in dibs_blk_peer_dev_remove during teardown
- [Medium] dibs: UAF of blkpd in worker due to missing cancel_work_sync on probe error
- [Medium] block: API misuse exhausts dynamic major numbers and overflows minor numbers
- [Medium] block: double-unquiesce warning triggered by unsynchronized state access
- [Medium] dibs: Out-of-bounds read accessing message payload length fields
- [Medium] block: disk_name truncation causes sysfs naming collisions
- [Low] block: snprintf truncation causes sysfs name collisions
--

commit c8b558951c6dbf245b6e9d1bfed2adc0596fda35
Author: Julian Ruess <julianr@linux.ibm.com>
Subject: dibs: introduce dibs block device peer device driver

This introduces a virtual block device driver that uses the newly added dibs
peer device bus. It allows access to resources like an ISO file hosted on
another system via dibs. A block device is created when a dibs peer device
of type block is announced.

Hello,

Thanks for the patch. I noticed a few potential issues with the teardown
and communication logic that might need a closer look.

> diff --git a/drivers/dibs/dibs_blk.c b/drivers/dibs/dibs_blk.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/dibs/dibs_blk.c
> @@ -0,0 +1,798 @@
[ ... ]
> +static blk_status_t dibs_blk_queue_rq(struct blk_mq_hw_ctx *hctx,
> +				      const struct blk_mq_queue_data *bd)
> +{
[ ... ]
> +	blkpd = bd->rq->q->disk->private_data;
> +
> +	if (!blkpd->device_add_disk || !blkpd->ring_registered)
> +		return BLK_STS_IOERR;

[Severity: High]
Could concurrent userspace I/O access the unregistered ring buffer here?

In dibs_blk_peer_dev_remove(), the driver calls
dibs_pbd_unregister_ring_buffer() to unmap and free the ring buffer memory
before calling blk_mark_disk_dead() and del_gendisk().

Because the queue is still active, concurrent userspace I/O can enter
dibs_blk_queue_rq() while the ring buffer is being unregistered. This
lockless check might pass before the ring_registered flag is cleared,
causing dibs_pbd_msg_send() to attempt writing to the freed memory.

[ ... ]
> +static void dibs_blk_unquiesce(struct dibs_blk_peer_dev *blkpd)
> +{
> +	if (!blkpd->blk.quiesced)
> +		return;
> +
> +	blkpd->blk.quiesced = false;

[Severity: Medium]
Could this unsynchronized state access trigger a double-unquiesce warning?

If dibs_blk_unquiesce() is called concurrently from media_gone_work and
handle_disk_detach, both threads might read quiesced as true and proceed
to call blk_mq_unquiesce_queue().

This second call would drop q->quiesce_depth below zero, triggering a
WARN_ON_ONCE(q->quiesce_depth <= 0) in blk_mq_unquiesce_queue().

[ ... ]
> +static enum blk_eh_timer_return dibs_blk_timeout(struct request *req)
> +{
[ ... ]
> +	blk_mq_end_request(req, BLK_STS_IOERR);
> +	schedule_work(&blkpd->media_gone_work);

[Severity: High]
Can media_gone_work be scheduled after it has been cancelled?

In dibs_blk_peer_dev_remove(), cancel_work_sync(&blkpd->media_gone_work)
is called before iterating the tags to cancel inflight requests. Before
those requests are cancelled, a timer could expire, triggering this
timeout handler.

This explicitly calls schedule_work(&blkpd->media_gone_work) after the
cancellation has finished. The newly scheduled work will execute later and
dereference the freed blkpd structure.

[ ... ]
> +static void dibs_blk_handle_data_msg(struct dibs_blk_peer_dev *blkpd,
> +				     void *current_data, u64 data_len,
> +				     u64 total_bytes_of_request)
> +{
[ ... ]
> +	req = blkpd->current_request;
> +	if (!req) {
> +		spin_unlock_irqrestore(&blkpd->req_lock, flags);
> +		pr_warn("Received response for unknown request\n");
> +		return;
> +	}
> +
> +	blkpd->last_completion = jiffies;
> +	blkpd->first_timeout = 0;
> +
> +	if (total_bytes_of_request != blk_rq_bytes(req)) {

[Severity: Critical]
How does the driver guarantee this response belongs to the current request?

The dibs block protocol appears to lack transaction IDs or sequence numbers.
If a block request times out, dibs_blk_timeout() clears current_request
and completes it with an error. The block layer can immediately dispatch a
new request, assigning it to current_request.

When the remote peer later sends the delayed data response for the original
request, this code blindly accepts it for the new current_request because
the lengths often match for identical block sizes. This would cause stale
data from the old request to be copied into the new request's bio buffers,
corrupting filesystem data.

[ ... ]
> +static void dibs_blk_media_change_work(struct work_struct *work)
> +{
> +	struct dibs_blk_peer_dev *blkpd;
> +	int ret;
> +
> +	blkpd = container_of(work, struct dibs_blk_peer_dev, media_change_work);
> +
> +	if (!blkpd->device_add_disk) {
> +		blkpd->blk.gd = blk_mq_alloc_disk(&blkpd->blk.tag_set,
> +						  &dibs_blk_lim, &blkpd->blk);
[ ... ]
> +		blkpd->blk.gd->first_minor =
> +			blkpd->device_index * MAX_GPT_PARTITIONS;

[Severity: Medium]
Will this multiplication lead to minor number overflow?

In probe(), the driver allocates a dynamic major number per device via
register_blkdev(0, "dibs_blk") instead of doing this once in module init,
which exhausts the small pool of available dynamic major numbers.

Additionally, device_index unconditionally increments via dibs_blk_idx++.
Because the minor number is calculated as device_index * MAX_GPT_PARTITIONS,
after 8192 probes the minor number exceeds the maximum limit, causing
device_add_disk() to fail or overwrite the major number.

[ ... ]
> +		blkpd->blk.gd->minors = MAX_GPT_PARTITIONS;
> +		blkpd->blk.gd->events = DISK_EVENT_MEDIA_CHANGE;
> +
> +		set_disk_ro(blkpd->blk.gd, 1);
> +		snprintf(blkpd->blk.gd->disk_name, 10, "dibs_blk%d",
> +			 blkpd->device_index);

[Severity: Medium]
Is the size parameter of 10 too small for double-digit indices?

The prefix dibs_blk consumes 8 characters. For any device_index >= 10,
the string requires at least 11 characters including the null terminator.
Because the size limit is hardcoded to 10 instead of DISK_NAME_LEN,
snprintf truncates the output to 9 characters plus the null terminator.

This means devices with an index >= 10 will be truncated and named
dibs_blk1, guaranteeing sysfs naming collisions and initialization
failures when multiple devices are attached or hotplugged.

[ ... ]
> +		dibs_blk_update_capacity(blkpd);
> +		blkpd->device_add_disk = 1;
> +
> +		ret = device_add_disk(&blkpd->dpd->dev, blkpd->blk.gd, NULL);
> +		if (ret) {
> +			pr_err("%s: device_add_disk failed: %d\n", __func__,
> +			       ret);
> +			blkpd->device_add_disk = 0;
> +			put_disk(blkpd->blk.gd);
> +			blkpd->blk.gd = NULL;
> +			return;
> +		}
> +	}
> +
> +	dibs_blk_update_capacity(blkpd);
> +	disk_force_media_change(blkpd->blk.gd);
> +	dibs_blk_rescan_partitions(blkpd);
> +}

[Severity: High]
Could exposing the device_add_disk flag early lead to a race condition?

If device_add_disk() fails, the error path resets the flag to 0 and frees
the gendisk. Concurrently, dibs_blk_handle_disk_detach() reads the flag
locklessly. If it sees the flag as 1 before the failure path resets it, it
calls disk_force_media_change(blkpd->blk.gd).

This would access the pointer while it is being freed or after it is
NULLed, causing a crash.

[ ... ]
> +static void dibs_blk_proc_recv_msg_work(struct work_struct *work)
> +{
[ ... ]
> +	while ((msg = (struct dibs_blk_peer_dev_msg *)dibs_pbd_msg_recv(&peer_dev->rb))) {
> +		switch (msg->hdr.type) {
> +		case DIBS_BLK_DATA_MSG:
> +			current_data = msg->blkdm.data;
> +			data_len = READ_ONCE(msg->blkdm.datalen);

[Severity: Medium]
Should the code validate the message bounds before accessing payload fields?

While dibs_pbd_msg_recv() ensures the ring buffer holds at least
msg->hdr.datalen bytes, this code accesses READ_ONCE(msg->blkdm.datalen)
and READ_ONCE(msg->blkmcm.size) without verifying that msg->hdr.datalen is
large enough to contain the full struct dibs_blk_data_msg or struct
dibs_blk_media_change_msg.

If a peer sends a valid message type but a tiny header length, reading
these payload fields accesses memory beyond the validated message bounds,
potentially causing a page fault.

[ ... ]
> +			total_bytes_of_request =
> +				READ_ONCE(msg->blkdm.total_bytes_of_req);
> +
> +			if (sizeof(msg->blkdm) + data_len >
> +			    READ_ONCE(msg->hdr.datalen))
> +				break;

[Severity: High]
Is there a TOCTOU vulnerability here when re-reading datalen from shared memory?

Because msg resides in the shared ring buffer mapped to the remote peer, a
malicious peer can concurrently mutate msg->hdr.datalen to a large value
after the initial boundary validation but during this check.

This bypasses the validation, causing dibs_blk_handle_data_msg() to copy
data_len bytes from the shared memory pointer, reading past the end of the
mapped buffer and leaking adjacent kernel memory.

[ ... ]
> +static int dibs_blk_peer_dev_probe(struct dibs_peer_device *dpd)
> +{
[ ... ]
> +	ret = dibs_pbd_register_ring_buffer("dibs_blk", dpd,
> +					    DIBS_BLK_PEER_DEV_BLK_RING_SIZE);
> +	if (ret) {
> +		pr_err("%s: failed to register ring buffer\n", __func__);
> +		goto err_free;
> +	}
> +
> +	blkpd->ring_registered = true;
> +	dibs_ring_set_rdmb_tok(&dpd->rb, dpd->rdmb_tok);

[Severity: High]
Does activating the remote communication channels here allow the remote
peer to access uninitialized block layer structures?

The remote device can immediately send a DIBS_BLK_MEDIA_CHANGE message,
triggering the IRQ handler which schedules proc_recv_msg_work and then
media_change_work.

Since media_change_work calls blk_mq_alloc_disk(&blkpd->blk.tag_set, ...),
if this runs before probe() reaches blk_mq_alloc_tag_set(), the tag_set is
uninitialized memory, leading to a crash in the block layer.

[ ... ]
> +	blkpd->blk.major = register_blkdev(0, "dibs_blk");
[ ... ]
> +err_unregister_ring:
> +	if (blkpd->ring_registered)
> +		dibs_pbd_unregister_ring_buffer(dpd);
> +err_free:
> +	dpd->drv_priv = NULL;
> +	kfree(blkpd);
> +	return ret;
> +}

[Severity: Medium]
Is there a missing cancel_work_sync() before freeing blkpd on this error path?

If a step after ring buffer registration fails (like register_blkdev), the
code jumps to err_unregister_ring and frees blkpd.

Because the IRQ handler could have already scheduled proc_recv_msg_work,
failing to call cancel_work_sync(&blkpd->proc_recv_msg_work) means the
work could execute later on the freed blkpd structure.

[ ... ]
> +static void dibs_blk_peer_dev_remove(struct dibs_peer_device *dpd)
> +{
> +	struct dibs_blk_peer_dev *blkpd = dpd->drv_priv;
> +
> +	if (blkpd->list_added) {
> +		mutex_lock(&blkpd_mutex);
> +		list_del(&blkpd->list);
> +		mutex_unlock(&blkpd_mutex);
> +		blkpd->list_added = false;
> +	}
> +
> +	if (blkpd->ring_registered) {
> +		dibs_pbd_unregister_ring_buffer(blkpd->dpd);
> +		blkpd->ring_registered = false;
> +	}
> +
> +	cancel_work_sync(&blkpd->proc_recv_msg_work);

[Severity: High]
Would tearing down the ring buffer before stopping the worker lead to a
use-after-free?

In both dibs_blk_peer_dev_remove() and dibs_blk_peer_dev_disconnect(), the
driver unregisters the ring buffer before calling
cancel_work_sync(&blkpd->proc_recv_msg_work).

Because the background worker accesses the ring buffer via
dibs_pbd_msg_recv(), if the worker is running concurrently, tearing down
the ring buffer from underneath it causes a use-after-free.

[ ... ]
> +	cancel_work_sync(&blkpd->media_change_work);

[Severity: High]
Can this removal path deadlock by waiting on a work item blocked on the
quiesced queue?

When the device is disconnected, dibs_blk_peer_dev_disconnect() quiesces
the queue but does not cancel media_change_work. If media_change_work
runs, it calls bdev_disk_changed() which issues synchronous read I/O to
scan partitions.

Since the queue is quiesced, these read requests block indefinitely. When
dibs_blk_peer_dev_remove() subsequently calls cancel_work_sync() for
media_change_work, it hangs forever waiting for the blocked work. The
queue is not unquiesced until later in this function, resulting in a
permanent deadlock.

[ ... ]
> +	if (blkpd->device_add_disk) {
> +		blk_mark_disk_dead(blkpd->blk.gd);
> +		dibs_blk_unquiesce(blkpd);
> +		blk_mq_tagset_busy_iter(&blkpd->blk.tag_set,
> +					dibs_blk_cancel_request, blkpd);
> +		del_gendisk(blkpd->blk.gd);
> +		put_disk(blkpd->blk.gd);
> +		blkpd->blk.gd = NULL;
> +		blkpd->device_add_disk = 0;
> +	}
> +
> +	if (blkpd->tag_set_allocated)
> +		blk_mq_free_tag_set(&blkpd->blk.tag_set);
> +
> +	if (blkpd->blkdev_registered)
> +		unregister_blkdev(blkpd->blk.major, "dibs_blk");
> +
> +	kfree(blkpd);
> +}

[Severity: High]
Is it safe to free tag_set and driver data while the block layer may still
retain a reference to the queue?

This code drops the driver's reference using put_disk(), followed
immediately by blk_mq_free_tag_set() and kfree(blkpd). If a userspace
process holds the block device open, the gendisk and request_queue remain
alive.

When the user later closes the device, blk_release_queue() will attempt to
access the queue's tag_set to free hardware queues. Since tag_set points
to blkpd->blk.tag_set inside the freed blkpd struct, this causes a
use-after-free. The tag set and private data must be managed via the
gendisk's .free_disk callback.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-console_server-v1-0-636b11ebf191@linux.ibm.com?part=4