Re: [RFC PATCH v3 2/6] virt: bao: add IPC shared-memory driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Unbinding the device while the misc character device is open causes a Use-After-Free of the driver context and unmapped memory access.
- [High] Missing memory barrier before notifying the peer guest via hypercall, causing potential data corruption due to stale memory observation.
- [High] Missing write-protection enforcement allows userspace to mmap the read region with write permissions, triggering a hypervisor stage-2 fault.
- [Medium] The mmap offset calculation uses `unsigned long`, which overflows on 32-bit architectures, contradicting the author's intent to fix u32 overflows.
- [Medium] The mmap logic fails entirely for the write region if the read region's size is not page-aligned.
- [Medium] Missing `.llseek` operation prevents userspace from resetting the file position, allowing the device to be read or written only once per open.
- [Low] The device label string buffer is too small to hold the maximum device ID, leading to truncation and potential naming collisions.
- [Low] Missing architecture dependencies in Kconfig will cause build failures on unsupported architectures.
--

--- Patch [2]: [RFC PATCH v3 2/6] virt: bao: add IPC shared-memory driver ---
commit 79e4d2a70a45b49aa525420ee0e59987417ec11f
Author: João Peixoto <jpeixoto@osyx.tech>
Subject: virt: bao: add IPC shared-memory driver

Adds a driver for guests running on the Bao static-partitioning hypervisor
to communicate through shared memory. Userspace accesses the regions through
a misc character device using read(), write(), and mmap(), where a write()
notifies the peer guest through an architecture-specific hypercall.

> diff --git a/drivers/virt/bao/ipcshmem/Kconfig b/drivers/virt/bao/ipcshmem/Kconfig
> --- /dev/null
> +++ b/drivers/virt/bao/ipcshmem/Kconfig
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0
> +config BAO_SHMEM
> +	tristate "Bao hypervisor shared memory support"

[Severity: Low]
Does this config need an architecture dependency?

Building this on an unsupported architecture like x86 (e.g., via
allmodconfig) might fail because it unconditionally includes <asm/bao.h>
which only exists for ARM, ARM64, and RISC-V.

[ ... ]
> diff --git a/drivers/virt/bao/ipcshmem/ipcshmem.c b/drivers/virt/bao/ipcshmem/ipcshmem.c
> --- /dev/null
> +++ b/drivers/virt/bao/ipcshmem/ipcshmem.c
[ ... ]
> +static int bao_ipcshmem_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> +	struct bao_ipcshmem *bao = filp->private_data;
> +	unsigned long vsize = vma->vm_end - vma->vm_start;
> +	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;

[Severity: Medium]
Will this offset calculation overflow on 32-bit architectures?

Since unsigned long is 32 bits there, an offset larger than 4GB might cause
an overflow. This seems to contradict the commit message's goal to avoid u32
overflows.

> +	phys_addr_t region_phys;
> +	size_t region_size;
> +
> +	if (!vsize)
> +		return -EINVAL;
> +
> +	/*
> +	 * The read region is exposed at offset 0 and the write region right
> +	 * after it. A single mapping cannot span both regions, since they are
> +	 * not guaranteed to be physically contiguous.
> +	 */
> +	if (offset < bao->read_size) {
> +		region_phys = bao->read_phys;
> +		region_size = bao->read_size;

[Severity: High]
Can a user map the read region with write permissions here?

If userspace opens the device O_RDWR and maps this region with PROT_WRITE,
the kernel will create writable PTEs because vma->vm_page_prot is used
directly during remap_pfn_range().

If the hypervisor maps the read region as read-only at Stage-2, a subsequent
write from userspace could trigger a Stage-2 permission fault and crash the
guest VM. Does this need a check to enforce !(vma->vm_flags & VM_WRITE)?

> +	} else if (offset < bao->read_size + bao->write_size) {
> +		offset -= bao->read_size;

[Severity: Medium]
What happens if the device tree specifies a read_size that is not
page-aligned?

Subtracting an unaligned read_size from a page-aligned offset would result
in an unaligned offset. This would then cause the PAGE_ALIGNED(region_phys)
check below to fail unconditionally, preventing userspace from mapping the
write region.

> +		region_phys = bao->write_phys;
> +		region_size = bao->write_size;
> +	} else {
> +		return -EINVAL;
> +	}
[ ... ]
> +static ssize_t bao_ipcshmem_write(struct file *filp, const char __user *buf,
> +				  size_t count, loff_t *ppos)
> +{
[ ... ]
> +	if (copy_from_user(bao->write_base + *ppos, buf, count))
> +		return -EFAULT;
> +
> +	*ppos += count;
> +
> +	/* Notify Bao hypervisor */
> +	bao_ipcshmem_hypercall(BAO_IPCSHMEM_HYPERCALL_ID, bao->id);

[Severity: High]
Is a memory barrier needed here before notifying the peer guest?

The hypercall provides a Context Synchronization Event, but it does not
guarantee that prior memory stores to the MEMREMAP_WB (cacheable) memory are
globally visible.

Could the peer guest observe stale data if the local CPU's store buffer
hasn't drained before the hypervisor wakes it up?

> +	return count;
> +}
[ ... ]
> +static const struct file_operations bao_ipcshmem_fops = {
> +	.owner = THIS_MODULE,
> +	.read = bao_ipcshmem_read,
> +	.write = bao_ipcshmem_write,
> +	.mmap = bao_ipcshmem_mmap,
> +	.open = bao_ipcshmem_open,
> +	.release = bao_ipcshmem_release,
> +};

[Severity: Medium]
Does this file operations structure need an .llseek callback (such as
default_llseek)?

Without it, the VFS will clear the FMODE_LSEEK flag during open(). Any
subsequent lseek() calls will return -ESPIPE. Once a read() or write() hits
the end of the shared memory region, userspace might be completely unable to
rewind the offset without closing and reopening the character device.

> +static int bao_ipcshmem_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	bao = devm_kzalloc(dev, sizeof(*bao), GFP_KERNEL);
> +	if (!bao)
> +		return -ENOMEM;
> +
> +	bao->read_base = devm_memremap(dev, read_res->start,
> +				       resource_size(read_res), MEMREMAP_WB);
> +	if (IS_ERR(bao->read_base))
> +		return PTR_ERR(bao->read_base);
[ ... ]
> +	bao->id = id;
> +	bao->read_phys = read_res->start;
> +	bao->read_size = resource_size(read_res);

[Severity: Medium]
Will resource_size() be truncated here on 32-bit systems with LPAE?

The resource_size() helper returns a 64-bit resource_size_t, but read_size
(and write_size) are defined as size_t in the bao_ipcshmem struct, which
will be 32 bits on those platforms.

> +	bao->write_phys = write_res->start;
> +	bao->write_size = resource_size(write_res);
> +
> +	scnprintf(bao->label, BAO_IPCSHMEM_NAME_LEN, "baoipc%u", id);

[Severity: Low]
Can this label string be truncated?

If the bao,id from the device tree is a 10-digit number (e.g., 4294967295),
the formatted string requires 17 bytes including the null terminator. Since
BAO_IPCSHMEM_NAME_LEN is 16, the last digit could be truncated, potentially
leading to naming collisions.

[ ... ]
> +static void bao_ipcshmem_remove(struct platform_device *pdev)
> +{
> +	struct bao_ipcshmem *bao = platform_get_drvdata(pdev);
> +
> +	misc_deregister(&bao->miscdev);
> +}

[Severity: High]
Can this result in a use-after-free if the driver is unbound while the
character device is still open?

While misc_deregister() removes the device, it does not revoke existing open
file descriptors. When bao_ipcshmem_remove() returns, devres will
automatically free the bao structure and unmap the MEMREMAP_WB regions.

If a user process subsequently calls read, write, or mmap on the open file
descriptor, could it dereference the freed bao structure and access unmapped
memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786010512.git.jpeixoto@osyx.tech?part=2