Re: [PATCH v4 1/3] virt: bao: add IPC shared-memory driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] virt: bao: UAF on module initialization failure
--

commit 2f861bb98100794551d8aebc0473ce9f71733cac
Author: João Peixoto <jpeixoto@osyx.tech>

virt: bao: add IPC shared-memory driver

This commit introduces a new IPC shared-memory driver for the Bao hypervisor.
It allows guests to communicate through read and write regions configured via
the bao_ipcshmem.channels module parameter, exposing each channel as a misc
character device to userspace.

> diff --git a/drivers/virt/bao/ipcshmem/ipcshmem.c b/drivers/virt/bao/ipcshmem/ipcshmem.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/virt/bao/ipcshmem/ipcshmem.c
> @@ -0,0 +1,358 @@
[ ... ]
> +static int __init bao_ipcshmem_init(void)
> +{
> +	char *buf, *p, *desc;
> +	int ret = 0;
> +
> +	if (!channels || !*channels)
> +		return 0;
> +
> +	buf = kstrdup(channels, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	p = buf;
> +	while ((desc = strsep(&p, ";")) != NULL) {
> +		if (!*desc)
> +			continue;
> +		ret = bao_ipcshmem_parse_one(desc);
> +		if (ret) {
> +			pr_err("bad 'channels' descriptor\n");
> +			bao_ipcshmem_remove_all();

[Severity: High]
Can this error path lead to a use-after-free if userspace has already opened a
character device?

When multiple channels are specified in the module parameter,
bao_ipcshmem_init() parses them in a loop. For each successfully parsed
channel, bao_ipcshmem_add() immediately exposes the device to userspace via
misc_register():

bao_ipcshmem_init()
  bao_ipcshmem_parse_one()
    bao_ipcshmem_add()
      misc_register(&bao->miscdev);

If a subsequent channel fails to parse, bao_ipcshmem_init() takes this error
path and calls bao_ipcshmem_remove_all(). This unregisters and frees all
previously registered channels.

If a user-space process (such as udev) opens the first exposed character
device before the initialization failure occurs, the file descriptor remains
active. However, the initialization failure causes the kernel to unload the
module and free its memory. Subsequent system calls on the active file
descriptor would attempt to access the unmapped file_operations structure and
the freed struct bao_ipcshmem data, crashing the system.

Would it be safer to defer calling misc_register() until all channels have
been successfully parsed and added to the list?

> +			break;
> +		}
> +	}
> +
> +	kfree(buf);
> +	return ret;
> +}

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