Re: [PATCH 2/2] md: add a control device for array management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] md: MD_STOP_ARRAY mutual exclusion bypass via control device
- [High] md: md_ctl_exit is marked __init and omitted from md_exit()
- [High] md: self-deadlock in get_mddev_dev and get_mddev_uuid via mddev_put
- [Medium] md: binary UUID lookup fails for UUIDs starting with 0x00
- [Medium] md: missing null-termination enforcement allows stack OOB read
- [Low] md: direct cast from __u64 to user pointer causes warnings
- [Low] md: uninitialized pointer passed to get_raid_version()
- [Low] md: unknown ioctl flags return -ENOTTY instead of -EINVAL
- [Low] md: unprivileged un-ratelimited pr_err allows log flooding
--

commit dd168c18138734c235c0b8b1c097fa7a18b6baa8
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
Subject: md: add a control device for array management

This patch introduces a misc character device, /dev/md-control, to allow
MD management ioctls without holding a block device reference. New 64-bit
safe ioctls are provided that specify the target array by name, UUID, or
device number.

>     The new ioctls use the existing MD_MAJOR ioctl type, starting at 0x40,
>     with an MD_ prefix. Every request structure has a flags field. Unknown
>     flags are rejected with -EINVAL.

[Severity: Low]
The implementation of ctl_flags_valid() below actually returns -ENOTTY for
unknown flags. Should the implementation be updated to match this documented
behavior?

diff --git a/drivers/md/md-ctl.c b/drivers/md/md-ctl.c
new file mode 100644
index 0000000000000..84923b0a2fa3c
--- /dev/null
+++ b/drivers/md/md-ctl.c
@@ -0,0 +1,597 @@
[ ... ]
+static struct mddev *get_mddev_dev(dev_t dev)
+{
+	struct mddev *mddev, *ret = NULL;
+
+	if (MINOR(dev) >= (1 << MINORBITS))
+		return ERR_PTR(-EINVAL);
+
+	if (MAJOR(dev) != MD_MAJOR)
+		dev &= ~((1 << MdpMinorShift) - 1);
+
+	spin_lock(&all_mddevs_lock);
+	list_for_each_entry(mddev, &all_mddevs, all_mddevs) {
+		if (mddev->unit == dev) {
+			ret = mddev_get(mddev);
+			if (ret && !mddev->gendisk) {
+				mddev_put(mddev);
[Severity: High]
Does calling mddev_put() while holding all_mddevs_lock cause a self-deadlock?

If the reference count was 0 before mddev_get(), mddev_put() will drop it
back to 0 and internally call atomic_dec_and_lock(&mddev->active,
&all_mddevs_lock). Since the current CPU already holds the non-recursive
all_mddevs_lock, this will deadlock the system. This pattern also appears
in get_mddev_uuid().

+				ret = ERR_PTR(-EBUSY);
+			}
+			break;
+		}
+	}
+	spin_unlock(&all_mddevs_lock);
+
+	return ret ? ret : ERR_PTR(-ENODEV);
+}
+
+static struct mddev *get_mddev_name(const char *name)
+{
+	unsigned long minor;
+	struct mddev *mddev, *ret = NULL;
+
+	if (strncmp(name, "md", 2))
+		return ERR_PTR(-EINVAL);
+
+	if (name[2] != '_' && (!isdigit(name[2]) ||
+			       kstrtoul(&name[2], 10, &minor) ||
[Severity: Medium]
Can this result in an out-of-bounds read on the kernel stack?

The name field is populated via copy_from_user() on the md_ctl structure
without enforcing null-termination. If a user populates the entire 32-byte
name field without a null terminator, kstrtoul() will read past the array
bounds into adjacent stack memory until it finds a non-digit character.

+			       minor > MINORMASK))
+		return ERR_PTR(-EINVAL);
[ ... ]
+static struct mddev *ctl_find_mddev(struct mdu_ioctl *md_ctl)
+{
+	struct mddev *ret = ERR_PTR(-EINVAL);
+
+	if (*md_ctl->uuid) {
[Severity: Medium]
Does this correctly evaluate the binary UUID?

Checking *md_ctl->uuid only evaluates the first byte of the 16-byte array.
If an array is created with a randomly generated UUID that happens to start
with 0x00, the UUID lookup will be skipped and fallback to checking the name
or device number instead.

+		if (*md_ctl->name || md_ctl->dev) {
+			pr_err("md: only supply one of name, uuid or dev number\n");
[Severity: Low]
Could this allow an unprivileged user to flood the kernel log buffer?

Since this is triggered directly by user input via ioctl, using an
un-ratelimited pr_err() here instead of pr_err_ratelimited() or pr_debug()
allows dmesg flooding.

+			return ret;
+		}
[ ... ]
+static long md_ctl_ioctl(struct file *filp, uint cmd, ulong u)
+{
+	int err;
+	unsigned int old_cmd;
+	unsigned int noio_flags = 0;
+	struct mddev *mddev;
+	void __user *arg;
+	struct mdu_ioctl md_ctl;
+
+	err = ctl_ioctl_valid(cmd);
+	if (err)
+		return err;
+
+	if (cmd == MD_RAID_VERSION)
+		return get_raid_version(mddev, (struct mdu_version64 __user *)u);
[Severity: Low]
Does this pass an uninitialized mddev pointer to get_raid_version()?

mddev is evaluated and passed in a register before it is initialized by
ctl_find_mddev(), resulting in undefined behavior.

+
+	if (copy_from_user(&md_ctl, (void __user *)u, sizeof(md_ctl)))
+		return -EFAULT;
+
+	err = ctl_flags_valid(cmd, md_ctl.flags);
+	if (err)
+		return err;
+
+	mddev = ctl_find_mddev(&md_ctl);
+	if (IS_ERR(mddev))
+		return PTR_ERR(mddev);
+
+	old_cmd = try_convert_ioctl_cmd(cmd, md_ctl.flags);
+	if (old_cmd) {
+		err = md_do_ioctl(mddev->gendisk->part0, 0, old_cmd, md_ctl.arg);
[Severity: High]
Does delegating to md_do_ioctl() bypass the MD_STOP_ARRAY mutual exclusion?

When MD_STOP_ARRAY is called here via the control device, md_do_ioctl() calls
mddev_set_closing_and_sync_blockdev(mddev, 1), which assumes the caller holds
the block device open. Because the control ioctl caller doesn't hold the
block device, atomic_read(&mddev->openers) > 1 will evaluate to false if
exactly one other task has the array open, allowing the array to be stopped
while it is actively in use.

+		goto out;
+	}
+
+	arg = (void __user *)md_ctl.arg;
[Severity: Low]
Does this direct cast from __u64 to a pointer cause compiler warnings on
32-bit architectures?

It is recommended to use u64_to_user_ptr() to handle the pointer translation
correctly for compat userspace.

+	switch (cmd) {
[ ... ]
+void __init md_ctl_exit(void)
+{
+	misc_deregister(&md_ctl_misc);
+}
[Severity: High]
Is md_ctl_exit() marked __init by mistake?

Additionally, it appears this function is completely omitted from the
module unload path (md_exit() in md.c doesn't call it). This would leak the
misc device when the md module is unloaded, and any subsequent access to
/dev/md-control would cause a use-after-free panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928201224.3602262-1-abd.masalkhi@gmail.com?part=2