linux-kernel - Re: [PATCH] block: fix FS_IOC_GETLBMD_CAP parsing in blkdev_common

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250710-passen-petersilie-32f6f1e9a1fc@brauner>
Date: Thu, 10 Jul 2025 10:00:48 +0200
From: Christian Brauner <brauner@...nel.org>
To: Arnd Bergmann <arnd@...nel.org>
Cc: linux-fsdevel@...r.kernel.org, linux-block@...r.kernel.org, 
	Anuj Gupta <anuj20.g@...sung.com>, "Martin K . Petersen" <martin.petersen@...cle.com>, 
	Kanchan Joshi <joshi.k@...sung.com>, ltp@...ts.linux.it, dan.carpenter@...aro.org, 
	benjamin.copeland@...aro.org, rbm@...e.com, Arnd Bergmann <arnd@...db.de>, 
	Naresh Kamboju <naresh.kamboju@...aro.org>, Anders Roxell <anders.roxell@...aro.org>, 
	Jens Axboe <axboe@...nel.dk>, Pavel Begunkov <asml.silence@...il.com>, 
	Alexey Dobriyan <adobriyan@...il.com>, "Darrick J. Wong" <djwong@...nel.org>, 
	Eric Biggers <ebiggers@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] block: fix FS_IOC_GETLBMD_CAP parsing in
 blkdev_common_ioctl()

On Wed, Jul 09, 2025 at 08:10:14PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.de>
> 
> 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@mail.gmail.com/
> Reported-by: Naresh Kamboju <naresh.kamboju@...aro.org>
> Cc: Anders Roxell <anders.roxell@...aro.org>
> Cc: Naresh Kamboju <naresh.kamboju@...aro.org>
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---

Thanks!

> 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.
> ---

The current documentation in Documentation/dev-tools/checkuapi.rst needs
updating too then.

I want this to work. So as a start we should have a common static inline
helper that encapsulates the barrage of checks.

>  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) &&
> +	    _IOC_SIZE(cmd) >= LBMD_SIZE_VER0 &&
> +	    _IOC_SIZE(cmd) <= _IOC_SIZE(FS_IOC_GETLBMD_CAP))

This part is wrong as we handle larger sizes just fine via
copy_struct_{from,to}_user().

Arnd, objections to writing it as follows?:

diff --git a/block/ioctl.c b/block/ioctl.c
index 9ad403733e19..9887ec55f8ce 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,25 @@ 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;
        }
+
+       /* extensible ioctls */
+       switch (_IOC_NR(cmd)) {
+       case _IOC_NR(FS_IOC_GETLBMD_CAP):
+               if (_IOC_DIR(cmd) != _IOC_DIR(FS_IOC_GETLBMD_CAP))
+                       break;
+               if (_IOC_TYPE(cmd) != _IOC_TYPE(FS_IOC_GETLBMD_CAP))
+                       break;
+               if (_IOC_NR(cmd) != _IOC_NR(FS_IOC_GETLBMD_CAP))
+                       break;
+               if (_IOC_SIZE(cmd) < LBMD_SIZE_VER0)
+                       break;
+               if (_IOC_SIZE(cmd) > PAGE_SIZE)
+                       break;
+               return blk_get_meta_cap(bdev, cmd, argp);
+       }
+
+       return -ENOIOCTLCMD;
 }

 /*

And can I ask you to please take a look at fs/pidfs.c:pidfd_ioctl() and
fs/nsfs.c:ns_ioctl()?