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: <14865b4a-dfad-4336-9113-b70d65c9ad52@app.fastmail.com>
Date: Thu, 10 Jul 2025 12:50:44 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Christoph Hellwig" <hch@...radead.org>,
 "Christian Brauner" <brauner@...nel.org>
Cc: "Arnd Bergmann" <arnd@...nel.org>, 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:14, Christoph Hellwig wrote:
> On Thu, Jul 10, 2025 at 10:00:48AM +0200, Christian Brauner wrote:
>> +       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);
>> +       }
>
> Yikes.  I really don't get why we're trying change the way how ioctls
> worked forever.  We can and usually do use the size based macro already.
> And when we introduce a new size (which should happen rarely), we add a
> new entry to the switch using the normal _IO* macros, and either
> rename the struct, or use offsetofend in the _IO* entry for the old one.
>
> Just in XFS which I remember in detail we've done that to extend
> structures in backwards compatible ways multiple times.

There are multiple methods we've used to do this in the past,
but I don't think any of them are great, including the version
that Christian is trying to push now:

The most common variant is to leave extra room at the end of
a structure and use that as in your 1fd8159e7ca4 ("xfs: export zoned
geometry via XFS_FSOP_GEOM") and many other examples.
This is probably the easiest and it only fails once you run out of
spare room and have to pick a new command number. A common mistake
here is to forget checking the padding in the input data against
zero, so old kernels just ignore whatever new userspace tried
to pass.

I think the variant from commit 1b6d968de22b ("xfs: bump
XFS_IOC_FSGEOMETRY to v5 structures") where the old structure
gets renamed and the existing macro refers to a different
command code is more problematic. We used to always require
userspace to be built against the oldest kernel headers it could run
on. This worked fine in the past but it appears that userspace
(in particular glibc) has increasingly expected to also work
on older kernels when building against new headers.

Adding a new command code along with the new structure as in
cc68a8a5a433 ("btrfs: new ioctl TREE_SEARCH_V2") is probably
better here: While this does require userspace to have code
for calling either version, building an old program against
the new header still does the right thing and works on both
old and new kernels.

Christian's version using the copy_struct_{from,to}_user()
aims to avoid most of the problems. The main downside I see
here is the extra complexity in the kernel. As far as I can
tell, this has mainly led to extra kernel bugs but has not
actually resulted in any structure getting seamlessly
extended.

      Arnd