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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] tty: uninitialized spinlock mr_spinlock in dibs_console
- [High] tty: UAF in IRQ handler due to inverted teardown in dibs_con_peer_dev_remove()
- [High] tty: racy hvc_instantiate() usage during hotplug corrupts global state
- [High] tty: dibs_console: use-after-free of hvc in termid_show()
- [High] dibs: kernel stack information leak in dibs_con_put_chars()
- [High] dibs: TOCTOU double-fetch in IRQ handler leading to OOB read
- [High] dibs: UAF of dcpd in dibs_con_get_chars() racing with device remove
- [Low] tty: sysfs attribute named 'termid' instead of documented 'hvc'
--

commit 1fe9a43373bccffcc93521015b20907f33642f67
Author: Julian Ruess <julianr@linux.ibm.com>

dibs: introduce dibs console peer device driver

This introduces a virtual console device driver over the dibs peer device
bus, allowing tty access across systems in a dibs fabric without requiring
a network setup. A /dev/hvcN device is created for each detected peer.

> diff --git a/drivers/dibs/dibs_console.c b/drivers/dibs/dibs_console.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/dibs/dibs_console.c

[ ... ]

> +static void dibs_con_peer_dev_handle_irq(unsigned int dmbno,
> +					 struct dibs_peer_device *peer_dev,
> +					 u16 dmbemask)
> +{
> +	struct dibs_con_peer_dev *dcpd;
> +	struct dibs_con_pd_msg *msg;
> +	struct winsize winsize;
> +	size_t count;
> +	u16 datalen;
> +
> +	dcpd = peer_dev->drv_priv;
> +
> +	if (!dcpd->connect_msg_enqueued)
> +		dibs_con_peer_dev_send_connect_msg(peer_dev);
> +
> +	while ((msg = (struct dibs_con_pd_msg *)dibs_pbd_msg_recv(&peer_dev->rb))) {
> +		switch (msg->hdr.type) {
> +		case DIBS_CON_DATA_MSG:
> +			datalen = min_t(u16, READ_ONCE(msg->hdr.datalen),
> +					DIBS_CON_PEER_DEV_DATA_MSG_MAX_DATALEN);
> +			count = kfifo_in(&dcpd->fifo, &msg->tdm, datalen);

[Severity: High]
Can a malicious peer concurrently modify msg->hdr.datalen between the bounds
check in dibs_pbd_msg_recv() and this second read?

The double fetch could allow a larger size to be used, meaning kfifo_in()
would read past the end of the mapped ring buffer memory, resulting in an
out-of-bounds read.

This also applies to the DIBS_CON_RESIZE_MSG case below where msg->rm.ws
is read. If datalen was initially 0, accessing the 8-byte payload can cross
the ring buffer boundary.

[ ... ]

> +static ssize_t dibs_con_get_chars(u32 vtermno, u8 *buf, size_t count)
> +{
> +	struct dibs_con_peer_dev *dcpd = NULL;
> +
> +	dcpd = hvc_peer_devs[vtermno];
> +	if (!dcpd)
> +		return 0;
> +
> +	spin_lock(&dcpd->mr_spinlock);

[Severity: High]
Is the mr_spinlock properly initialized before it is acquired here?

In dibs_con_peer_dev_probe(), dcpd is dynamically allocated using kzalloc_obj().
This zero-initializes the memory, but the lock is never properly initialized
with spin_lock_init(). Acquiring a zero-initialized spinlock will cause kernel
crashes on PREEMPT_RT systems or trigger warnings when lock debugging is enabled.

[Severity: High]
Is this pointer dereference safe from concurrent teardown?

In dibs_con_peer_dev_remove(), dcpd is freed without waiting for the khvcd
polling thread to exit. The hvc_remove() function temporarily takes and drops
hp->lock to clear console metadata, but it does not clear the hp->ops pointers.

Meanwhile, __hvc_poll() inside khvcd drops hp->lock to call cond_resched(),
allowing hvc_remove() to proceed and dibs_con_peer_dev_remove() to free dcpd.
When __hvc_poll() reacquires the lock, it jumps back and unconditionally calls
hp->ops->get_chars(), leading to a use-after-free here.

[ ... ]

> +static ssize_t dibs_con_put_chars(u32 vtermno, const u8 *buf, size_t count)
> +{
> +	struct dibs_con_peer_dev *dcpd = NULL;
> +	struct dibs_con_peer_dev_rbe dcpdr;
> +	size_t max_cnt;
> +
> +	dcpd = hvc_peer_devs[vtermno];
> +	if (!dcpd)
> +		return 0;
> +	if (!dcpd->connect_msg_enqueued)
> +		return 0;
> +
> +	max_cnt = dibs_pbd_msg_max_payload(&dcpd->dpd->rb);
> +	if (!max_cnt)
> +		return 0;
> +
> +	count = min(count, max_cnt);
> +	dcpdr.pdm.hdr.version = DIBS_CON_PEER_DEV_HDR_VERSION;
> +	dcpdr.pdm.hdr.type = DIBS_CON_DATA_MSG;
> +	dcpdr.pdm.hdr.datalen = count;
> +	memcpy(&dcpdr.pdm.tdm.data, buf, count);
> +	if (!dibs_pbd_msg_send(&dcpd->dpd->rb, &dcpdr.pdm.hdr))

[Severity: High]
Does this leak uninitialized stack padding to the remote peer?

The dcpdr struct is allocated on the stack and is not zero-initialized.
The memcpy() only writes count bytes into the data buffer.

When dibs_pbd_msg_send() calculates the transmit size using dibs_pbd_msg_size(),
it rounds the total size up to a 4-byte boundary. This will cause
dibs_ring_send() to read and transmit up to 3 bytes of uninitialized stack
memory adjacent to the valid data over the dibs fabric.

[ ... ]

> +static int dibs_con_alloc_hvc(struct dibs_con_peer_dev *dcpd)
> +{
> +	int vtermno;
> +	int ret;
> +
> +	for (vtermno = 0; vtermno < DIBS_CON_MAX_VTERMNOS; vtermno++) {
> +		ret = hvc_instantiate(vtermno, vtermno, &dibs_con_ops);
> +		if (ret)
> +			continue;

[Severity: High]
Is it safe to use hvc_instantiate() dynamically during device probe?

This function is a lockless API designed strictly for sequential early boot
console initialization. If multiple dibs peer devices are probed concurrently,
they will race to read and write the global vtermnos and cons_ops arrays,
leading to corruption where multiple devices claim the same slot.

Additionally, if the subsequent hvc_alloc() call fails, the claimed slot
is never un-instantiated, permanently leaking the console slot.

[ ... ]

> +static ssize_t termid_show(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct dibs_peer_device *dpd;
> +	struct dibs_con_peer_dev *dcpd;
> +
> +	dpd = to_dibs_peer_device(dev);
> +	dcpd = dpd->drv_priv;
> +	if (dcpd->dpd == dpd)
> +		return sysfs_emit(buf, "hvc%d\n", dcpd->hvc->index);

[Severity: High]
Can this dereference a freed pointer during concurrent device teardown?

In dibs_con_peer_dev_remove(), hvc_remove(dcpd->hvc) is called and can
immediately free the hvc_struct if no references are held. The sysfs file is
not removed via sysfs_remove_file() until later, leaving a window where
concurrent readers to the sysfs file can access freed memory here.

> +	return 0;
> +}
> +static DEVICE_ATTR_RO(termid);

[Severity: Low]
This isn't a bug, but the commit message states the hvc device number can be
accessed by reading /sys/bus/dibs_peer/devices/<rgid>/hvc.

The macro DEVICE_ATTR_RO(termid) defines a file named termid instead,
creating an undocumented API mismatch.

[ ... ]

> +static void dibs_con_peer_dev_remove(struct dibs_peer_device *dpd)
> +{
> +	struct dibs_con_peer_dev *dcpd = dpd->drv_priv;
> +
> +	if (dcpd->hvc)
> +		hvc_remove(dcpd->hvc);
> +	hvc_peer_devs[dcpd->vtermno] = NULL;
> +	kfifo_free(&dcpd->fifo);
> +
> +	if (dcpd->ring_registered) {
> +		dibs_pbd_unregister_ring_buffer(dcpd->dpd);

[Severity: High]
Does this teardown ordering create a use-after-free window in the IRQ handler?

The kfifo resource is freed before the ring buffer (which is the interrupt
source) is unregistered. If a message arrives from the peer between these two
calls, the interrupt handler dibs_con_peer_dev_handle_irq() will fire and
call kfifo_in() on the already freed dcpd->fifo buffer, resulting in memory
corruption.

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