Re: [PATCH v3 11/13] libmultipath: Add support for block device IOCTL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional dereference of optional `fops->ioctl` leads to NULL pointer dereference.
- [High] Bypass of the underlying block device's custom `.compat_ioctl` handler.
--

commit 3f49637890144f68bc3af4119c61a0b330200c0e
Author: John Garry <john.g.garry@oracle.com>

libmultipath: Add support for block device IOCTL

Add mpath_bdev_ioctl() as a multipath block device IOCTL handler. This
handler calls into the mpath_device bdev fops handler. Like what is done
for cdev IOCTL handler, use .ioctl_begin and .ioctl_finish methods.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index d335074eb5bcd..4e4b347875500 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -515,6 +515,38 @@ static void mpath_bdev_release(struct gendisk *disk)
>  	mpath_put_head(mpath_head);
>  }
>  
> +static int mpath_bdev_ioctl(struct block_device *bdev, blk_mode_t mode,
> +		    unsigned int cmd, unsigned long arg)
> +{
> +	struct gendisk *disk = bdev->bd_disk;
> +	struct mpath_head *mpath_head = mpath_gendisk_to_head(disk);
> +	struct mpath_device *mpath_device;
> +	int srcu_idx, err;
> +	void *unlocked_ioctl_data = NULL;
> +
> +	srcu_idx = srcu_read_lock(&mpath_head->srcu);
> +	mpath_device = mpath_find_path(mpath_head);
> +	if (!mpath_device) {
> +		err = -EWOULDBLOCK;
> +		goto out_unlock;
> +	}
> +
> +	if (mpath_head->mpdt->ioctl_begin)
> +		mpath_head->mpdt->ioctl_begin(mpath_device, cmd,
> +					&unlocked_ioctl_data);
> +	if (unlocked_ioctl_data)
> +		srcu_read_unlock(&mpath_head->srcu, srcu_idx);
> +	err = mpath_device->disk->fops->ioctl(
> +			mpath_device->disk->part0, mode, cmd, arg);

[Severity: High]
Does this code dereference the underlying device's ioctl function pointer
without checking if it's NULL?

If the active path's block device does not implement the optional .ioctl method
in its block_device_operations, won't this cause a NULL pointer dereference
when user-space issues an ioctl on the multipath device node?

> +	if (unlocked_ioctl_data) {
> +		mpath_head->mpdt->ioctl_finish(unlocked_ioctl_data);
> +		return err;
> +	}
> +out_unlock:
> +	srcu_read_unlock(&mpath_head->srcu, srcu_idx);
> +	return err;
> +}
> +

[ ... ]

> @@ -701,6 +733,8 @@ const struct block_device_operations mpath_ops = {
>  	.open		= mpath_bdev_open,
>  	.release	= mpath_bdev_release,
>  	.submit_bio	= mpath_bdev_submit_bio,
> +	.ioctl		= mpath_bdev_ioctl,
> +	.compat_ioctl	= blkdev_compat_ptr_ioctl,

[Severity: High]
Does using the generic blkdev_compat_ptr_ioctl wrapper here bypass the
underlying block device's custom .compat_ioctl handler?

If a 32-bit user space task invokes a compat ioctl, the core block layer will
delegate to this generic wrapper, which routes the call to the native
mpath_bdev_ioctl().

That native handler then unconditionally forwards the call to the underlying
path device's native .ioctl method, entirely skipping the underlying device's
.compat_ioctl handler.

Could this lead to 32-bit struct layouts being passed directly into a 64-bit
native .ioctl handler, potentially causing memory corruption or out-of-bounds
reads if the underlying driver requires custom struct repacking?

> +	.report_zones	= mpath_bdev_report_zones,
>  	.pr_ops		= &mpath_pr_ops,
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703102918.3723667-1-john.g.garry@oracle.com?part=11