On Wed, Jul 09, 2025 at 08:10:14PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> Anders and Naresh found that the addition of the FS_IOC_GETLBMD_CAP
> handling in the blockdev ioctl handler breaks all ioctls with
> _IOC_NR==2, as the new command is not added to the switch but only
> a few of the command bits are check.
>
> Refine the check to also validate the direction/type/length bits,
> but still allow all supported sizes for future extensions.
>
> Move the new command to the end of the function to avoid slowing
> down normal ioctl commands with the added branches.
>
> Fixes: 9eb22f7fedfc ("fs: add ioctl to query metadata and protection info capabilities")
> Link: https://lore.kernel.org/all/CA+G9fYvk9HHE5UJ7cdJHTcY6P5JKnp+_e+sdC5U-ZQFTP9_hqQ@xxxxxxxxxxxxxx/
> Reported-by: Naresh Kamboju <naresh.kamboju@xxxxxxxxxx>
> Cc: Anders Roxell <anders.roxell@xxxxxxxxxx>
> Cc: Naresh Kamboju <naresh.kamboju@xxxxxxxxxx>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
> It seems that we have a lot of drivers with the same bug, as the
> large majority of all _IOC_NR() users in the kernel fail to also
> check the other bits of the ioctl command code. There are currently
> 55 files referencing _IOC_NR, and they all need to be manually
> checked for this problem.
> ---
> block/ioctl.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 9ad403733e19..5e5a422bd09f 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -567,9 +567,6 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
> {
> unsigned int max_sectors;
>
> - if (_IOC_NR(cmd) == _IOC_NR(FS_IOC_GETLBMD_CAP))
> - return blk_get_meta_cap(bdev, cmd, argp);
> -
> switch (cmd) {
> case BLKFLSBUF:
> return blkdev_flushbuf(bdev, cmd, arg);
> @@ -647,9 +644,16 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
> return blkdev_pr_preempt(bdev, mode, argp, true);
> case IOC_PR_CLEAR:
> return blkdev_pr_clear(bdev, mode, argp);
> - default:
> - return -ENOIOCTLCMD;
> }
> +
> + if (_IOC_DIR(cmd) == _IOC_DIR(FS_IOC_GETLBMD_CAP) &&
> + _IOC_TYPE(cmd) == _IOC_TYPE(FS_IOC_GETLBMD_CAP) &&
> + _IOC_NR(cmd) == _IOC_NR(FS_IOC_GETLBMD_CAP) &&
I think this problem was introduced by brauner trying to persuade people
to perform size independent dispatch of ioctls:
switch (_IOC_NR(cmd)) {
case _IOC_NR(FS_IOC_FSGETXATTR):
if (WARN_ON_ONCE(_IOC_TYPE(cmd) != _IOC_TYPE(FS_IOC_FSGETXATTR)))
return SOMETHING_SOMETHING;
/* Only handle original size. */
return ioctl_fsgetxattr(filp, argp);
https://lore.kernel.org/linux-xfs/20250515-bedarf-absagen-464773be3e72@brauner/
though we probably want a helper or something to encapsulate those three
comparisons to avoid the SOMETHING_SOMETHING part:
#define IOC_DISPATCH(c) \
((c) & ~(_IOC(0, 0, 0, _IOC_SIZE(_IOC_SIZEMASK))))
switch (IOC_DISPATCH(cmd)) {
case IOC_DISPATCH(FS_IOC_FSGETXATTR):
return ioctl_fsgetxattr(filp, cmd, argp);
Assuming that ioctl_fsgetxattr derives size from @cmd and rejects values
that it doesn't like. Hrm?
> + _IOC_SIZE(cmd) >= LBMD_SIZE_VER0 &&
> + _IOC_SIZE(cmd) <= _IOC_SIZE(FS_IOC_GETLBMD_CAP))
blk_get_meta_cap already checks this.
--D
> + return blk_get_meta_cap(bdev, cmd, argp);
> +
> + return -ENOIOCTLCMD;
> }
>
> /*
> --
> 2.39.5
>
