| 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
| ||
|
Message-Id: <d2e1d4a9-d475-43e3-824b-579e5084aaf3@app.fastmail.com>
Date: Thu, 10 Jul 2025 12:11:12 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Christian Brauner" <brauner@...nel.org>,
"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 List" <ltp@...ts.linux.it>,
"Dan Carpenter" <dan.carpenter@...aro.org>,
"Benjamin Copeland" <benjamin.copeland@...aro.org>, rbm@...e.com,
"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 Thu, Jul 10, 2025, at 10:00, Christian Brauner wrote:
> On Wed, Jul 09, 2025 at 08:10:14PM +0200, Arnd Bergmann wrote:
>> + 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().
I feel that is still an open question. As I understand it,
you want to make it slightly easier for userspace callers
to use future versions of an ioctl command by allowing them in
old kernels as well, by moving that complexity into the kernel.
Checking against _IOC_SIZE(FS_IOC_GETLBMD_CAP) would keep the
behavior consistent with the traditional model where userspace
needs to have a fallback to previous ABI versions.
> Arnd, objections to writing it as follows?:
> + /* 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);
The 'PAGE_SIZE' seems arbitrary here, especially since that is often
larger than the maximum size that can be encoded in an ioctl command
number (8KB or 16KB depending on the architecture). If we do need
an upper bound larger than _IOC_SIZE(FS_IOC_GETLBMD_CAP), I think it
should be a fixed number rather than a configuration dependent
one, and I would prefer a smaller one over a larger one. Anything
over a few dozen bytes is certainly suspicious, and once it gets
to thousands of bytes, you need a dynamic allocation to avoid stack
overflow in the kernel.
I had already updated my patch to move the checks into
blk_get_meta_cap() itself and keep the caller simpler:
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 9d9dc9c32083..2909ebf27dc2 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -62,10 +62,13 @@ int blk_get_meta_cap(struct block_device *bdev, unsigned int cmd,
struct logical_block_metadata_cap meta_cap = {};
size_t usize = _IOC_SIZE(cmd);
- if (!argp)
- return -EINVAL;
- if (usize < LBMD_SIZE_VER0)
- return -EINVAL;
+ 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))
+ return -ENOIOCTLCMD;
+
if (!bi)
goto out;
diff --git a/block/ioctl.c b/block/ioctl.c
index 9ad403733e19..af2e22e5533c 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -566,9 +566,11 @@ static int blkdev_common_ioctl(struct block_device *bdev, blk_mode_t mode,
void __user *argp)
{
unsigned int max_sectors;
+ int ret;
- if (_IOC_NR(cmd) == _IOC_NR(FS_IOC_GETLBMD_CAP))
- return blk_get_meta_cap(bdev, cmd, argp);
+ ret = blk_get_meta_cap(bdev, cmd, argp);
+ if (ret != -ENOIOCTLCMD)
+ return ret;
switch (cmd) {
case BLKFLSBUF:
Regardless of what upper bound we pick, that at least limits
the complexity to the one function that actually wants it.
> And can I ask you to please take a look at fs/pidfs.c:pidfd_ioctl() and
PIDFD_GET_INFO has part of the same problem, as it still fails to
check the _IOC_DIR() bits. I see you added a check for _IOC_TYPE()
in commit 091ee63e36e8 ("pidfs: improve ioctl handling"), but
the comment you added describes an unrelated issue and the fix
was incomplete.
> fs/nsfs.c:ns_ioctl()?
You tried to add a similar validation in commit 7fd511f8c911
("nsfs: validate ioctls"), but it seems you got that wrong
both by missing the _IOC_DIR check and by having a typo in the
'_IOC_TYPE(cmd) == _IOC_TYPE(cmd)' line that means this is
always true rather than comparing against 'NSIO'.
Overall my feeling is similar to Christoph's, that the added
complexity in any of these three cases was a mistake, as it's
too easy to mess it up.
Arnd
Powered by blists - more mailing lists