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

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

Hi Kuai,

Sorry, I forgot to mark this series as RFC.

On Mon, Sep 28, 2026 at 20:26 +0000, sashiko-bot@kernel.org wrote:
> 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?
>
I will fix this in v2.

> 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?
>
I will fix this in v2.

> 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.
>
I will fix this in v2.

> +			       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.
>
I will fix this in v2.

> +		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.
>
I will use pr_err_ratelimited() in v2.

> +			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()?
>
get_raid_version() should not take mddev as a parameter; I will fix this
in v2.

> 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.
>
Yes that might happen. I will fix this in v2.

> +		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.
>
I will fix this in v2.

> +	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.
>
I will fix this in v2.

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

-- 
Best Regards,
Abd-Alrhman