Validate extensible ioctls stricter than we do now.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/pidfs.c | 2 +-
include/linux/fs.h | 14 ++++++++++++++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/fs/pidfs.c b/fs/pidfs.c
index edc35522d75c..0a5083b9cce5 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -440,7 +440,7 @@ static bool pidfs_ioctl_valid(unsigned int cmd)
* erronously mistook the file descriptor for a pidfd.
* This is not perfect but will catch most cases.
*/
- return (_IOC_TYPE(cmd) == _IOC_TYPE(PIDFD_GET_INFO));
+ return extensible_ioctl_valid(cmd, PIDFD_GET_INFO, PIDFD_INFO_SIZE_VER0);
}
return false;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d7ab4f96d705..2f2edc53bf3c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -4023,4 +4023,18 @@ static inline bool vfs_empty_path(int dfd, const char __user *path)
int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter);
+static inline bool extensible_ioctl_valid(unsigned int cmd_a,
+ unsigned int cmd_b, size_t min_size)
+{
+ if (_IOC_DIR(cmd_a) != _IOC_DIR(cmd_b))
+ return false;
+ if (_IOC_TYPE(cmd_a) != _IOC_TYPE(cmd_b))
+ return false;
+ if (_IOC_NR(cmd_a) != _IOC_NR(cmd_b))
+ return false;
+ if (_IOC_SIZE(cmd_a) < min_size)
+ return false;
+ return true;
+}
+
#endif /* _LINUX_FS_H */
--
2.47.3On 10. 09. 25, 16:36, Christian Brauner wrote:
> Validate extensible ioctls stricter than we do now.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
> fs/pidfs.c | 2 +-
> include/linux/fs.h | 14 ++++++++++++++
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index edc35522d75c..0a5083b9cce5 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -440,7 +440,7 @@ static bool pidfs_ioctl_valid(unsigned int cmd)
> * erronously mistook the file descriptor for a pidfd.
> * This is not perfect but will catch most cases.
> */
> - return (_IOC_TYPE(cmd) == _IOC_TYPE(PIDFD_GET_INFO));
> + return extensible_ioctl_valid(cmd, PIDFD_GET_INFO, PIDFD_INFO_SIZE_VER0);
Hi,
this turned EINVAL (from pidfd_info()) into ENOTTY (from pidfd_ioctl())
for at least LTP's:
struct pidfd_info_invalid {
uint32_t dummy;
};
#define PIDFD_GET_INFO_SHORT _IOWR(PIDFS_IOCTL_MAGIC, 11, struct
pidfd_info_invalid)
ioctl(pidfd, PIDFD_GET_INFO_SHORT, info_invalid) == EINVAL
at:
https://github.com/linux-test-project/ltp/blob/9bb94efa39bb1b08f37e56c7437db5fa13ddcae2/testcases/kernel/syscalls/ioctl/ioctl_pidfd05.c#L46
Is this expected?
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -4023,4 +4023,18 @@ static inline bool vfs_empty_path(int dfd, const char __user *path)
>
> int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter);
>
> +static inline bool extensible_ioctl_valid(unsigned int cmd_a,
> + unsigned int cmd_b, size_t min_size)
> +{
> + if (_IOC_DIR(cmd_a) != _IOC_DIR(cmd_b))
> + return false;
> + if (_IOC_TYPE(cmd_a) != _IOC_TYPE(cmd_b))
> + return false;
> + if (_IOC_NR(cmd_a) != _IOC_NR(cmd_b))
> + return false;
> + if (_IOC_SIZE(cmd_a) < min_size)
> + return false;
> + return true;
> +}
> +
> #endif /* _LINUX_FS_H */
>
thanks,
--
js
suse labs
On Thu 23-10-25 12:46:45, Jiri Slaby wrote:
> On 10. 09. 25, 16:36, Christian Brauner wrote:
> > Validate extensible ioctls stricter than we do now.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> > fs/pidfs.c | 2 +-
> > include/linux/fs.h | 14 ++++++++++++++
> > 2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/pidfs.c b/fs/pidfs.c
> > index edc35522d75c..0a5083b9cce5 100644
> > --- a/fs/pidfs.c
> > +++ b/fs/pidfs.c
> > @@ -440,7 +440,7 @@ static bool pidfs_ioctl_valid(unsigned int cmd)
> > * erronously mistook the file descriptor for a pidfd.
> > * This is not perfect but will catch most cases.
> > */
> > - return (_IOC_TYPE(cmd) == _IOC_TYPE(PIDFD_GET_INFO));
> > + return extensible_ioctl_valid(cmd, PIDFD_GET_INFO, PIDFD_INFO_SIZE_VER0);
>
> Hi,
>
> this turned EINVAL (from pidfd_info()) into ENOTTY (from pidfd_ioctl()) for
> at least LTP's:
> struct pidfd_info_invalid {
> uint32_t dummy;
> };
>
> #define PIDFD_GET_INFO_SHORT _IOWR(PIDFS_IOCTL_MAGIC, 11, struct
> pidfd_info_invalid)
>
> ioctl(pidfd, PIDFD_GET_INFO_SHORT, info_invalid) == EINVAL
>
> at:
> https://github.com/linux-test-project/ltp/blob/9bb94efa39bb1b08f37e56c7437db5fa13ddcae2/testcases/kernel/syscalls/ioctl/ioctl_pidfd05.c#L46
>
> Is this expected?
We already discussed this internally but for others the problem was
discussed here [1] and we decided the new errno value is OK and LTP test is
being fixed up.
Honza
[1] https://lore.kernel.org/all/aPIPGeWo8gtxVxQX@yuki.lan/
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
On 2025-09-10, Christian Brauner <brauner@kernel.org> wrote:
> Validate extensible ioctls stricter than we do now.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
> fs/pidfs.c | 2 +-
> include/linux/fs.h | 14 ++++++++++++++
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index edc35522d75c..0a5083b9cce5 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -440,7 +440,7 @@ static bool pidfs_ioctl_valid(unsigned int cmd)
> * erronously mistook the file descriptor for a pidfd.
> * This is not perfect but will catch most cases.
> */
> - return (_IOC_TYPE(cmd) == _IOC_TYPE(PIDFD_GET_INFO));
> + return extensible_ioctl_valid(cmd, PIDFD_GET_INFO, PIDFD_INFO_SIZE_VER0);
> }
>
> return false;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index d7ab4f96d705..2f2edc53bf3c 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -4023,4 +4023,18 @@ static inline bool vfs_empty_path(int dfd, const char __user *path)
>
> int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter);
>
> +static inline bool extensible_ioctl_valid(unsigned int cmd_a,
> + unsigned int cmd_b, size_t min_size)
> +{
> + if (_IOC_DIR(cmd_a) != _IOC_DIR(cmd_b))
> + return false;
> + if (_IOC_TYPE(cmd_a) != _IOC_TYPE(cmd_b))
> + return false;
> + if (_IOC_NR(cmd_a) != _IOC_NR(cmd_b))
> + return false;
> + if (_IOC_SIZE(cmd_a) < min_size)
> + return false;
> + return true;
> +}
> +
nit: I know only we use them for now, but does this maybe belong in
ioctl.h (or even uaccess.h with the other extensible struct stuff)?
Otherwise,
Reviewed-by: Aleksa Sarai <cyphar@cyphar.com>
> #endif /* _LINUX_FS_H */
>
> --
> 2.47.3
>
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
On Wed 10-09-25 16:36:46, Christian Brauner wrote:
> Validate extensible ioctls stricter than we do now.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/pidfs.c | 2 +-
> include/linux/fs.h | 14 ++++++++++++++
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index edc35522d75c..0a5083b9cce5 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -440,7 +440,7 @@ static bool pidfs_ioctl_valid(unsigned int cmd)
> * erronously mistook the file descriptor for a pidfd.
> * This is not perfect but will catch most cases.
> */
> - return (_IOC_TYPE(cmd) == _IOC_TYPE(PIDFD_GET_INFO));
> + return extensible_ioctl_valid(cmd, PIDFD_GET_INFO, PIDFD_INFO_SIZE_VER0);
> }
>
> return false;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index d7ab4f96d705..2f2edc53bf3c 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -4023,4 +4023,18 @@ static inline bool vfs_empty_path(int dfd, const char __user *path)
>
> int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter);
>
> +static inline bool extensible_ioctl_valid(unsigned int cmd_a,
> + unsigned int cmd_b, size_t min_size)
> +{
> + if (_IOC_DIR(cmd_a) != _IOC_DIR(cmd_b))
> + return false;
> + if (_IOC_TYPE(cmd_a) != _IOC_TYPE(cmd_b))
> + return false;
> + if (_IOC_NR(cmd_a) != _IOC_NR(cmd_b))
> + return false;
> + if (_IOC_SIZE(cmd_a) < min_size)
> + return false;
> + return true;
> +}
> +
> #endif /* _LINUX_FS_H */
>
> --
> 2.47.3
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
* Christian Brauner:
> Validate extensible ioctls stricter than we do now.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
> fs/pidfs.c | 2 +-
> include/linux/fs.h | 14 ++++++++++++++
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index edc35522d75c..0a5083b9cce5 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -440,7 +440,7 @@ static bool pidfs_ioctl_valid(unsigned int cmd)
> * erronously mistook the file descriptor for a pidfd.
> * This is not perfect but will catch most cases.
> */
> - return (_IOC_TYPE(cmd) == _IOC_TYPE(PIDFD_GET_INFO));
> + return extensible_ioctl_valid(cmd, PIDFD_GET_INFO, PIDFD_INFO_SIZE_VER0);
> }
>
> return false;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index d7ab4f96d705..2f2edc53bf3c 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -4023,4 +4023,18 @@ static inline bool vfs_empty_path(int dfd, const char __user *path)
>
> int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter);
>
> +static inline bool extensible_ioctl_valid(unsigned int cmd_a,
> + unsigned int cmd_b, size_t min_size)
> +{
> + if (_IOC_DIR(cmd_a) != _IOC_DIR(cmd_b))
> + return false;
> + if (_IOC_TYPE(cmd_a) != _IOC_TYPE(cmd_b))
> + return false;
> + if (_IOC_NR(cmd_a) != _IOC_NR(cmd_b))
> + return false;
> + if (_IOC_SIZE(cmd_a) < min_size)
> + return false;
> + return true;
> +}
> +
> #endif /* _LINUX_FS_H */
Is this really the right direction? This implies that the ioctl
constants change as the structs get extended. At present, this impacts
struct pidfd_info and PIDFD_GET_INFO.
I think this is a deparature from the previous design, where (low-level)
userspace did not have not worry about the internal structure of ioctl
commands and could treat them as opaque bit patterns. With the new
approach, we have to dissect some of the commands in the same way
extensible_ioctl_valid does it above.
So far, this impacts glibc ABI tests. Looking at the strace sources, it
doesn't look to me as if the ioctl handler is prepared to deal with this
situation, either, because it uses the full ioctl command for lookups.
The sanitizers could implement generic ioctl checking with the embedded
size information in the ioctl command, but the current code structure is
not set up to handle this because it's indexed by the full ioctl
command, not the type. I think in some cases, the size is required to
disambiguate ioctl commands because the type field is not unique across
devices. In some cases, the sanitizers would have to know the exact
command (not just the size), to validate points embedded in the struct
passed to the ioctl. So I don't think changing ioctl constants when
extensible structs change is obviously beneficial to the sanitizers,
either.
I would prefer if the ioctl commands could be frozen and decoupled from
the structs. As far as I understand it, there is no requirement that
the embedded size matches what the kernel deals with.
Thanks,
Florian
Hi, On Wed, Nov 26, 2025 at 10:08:44AM +0100, Florian Weimer wrote: > Is this really the right direction? This implies that the ioctl > constants change as the structs get extended. At present, this impacts > struct pidfd_info and PIDFD_GET_INFO. > > I think this is a deparature from the previous design, where (low-level) > userspace did not have not worry about the internal structure of ioctl > commands and could treat them as opaque bit patterns. With the new > approach, we have to dissect some of the commands in the same way > extensible_ioctl_valid does it above. > > So far, this impacts glibc ABI tests. Looking at the strace sources, it > doesn't look to me as if the ioctl handler is prepared to deal with this > situation, either, because it uses the full ioctl command for lookups. > > The sanitizers could implement generic ioctl checking with the embedded > size information in the ioctl command, but the current code structure is > not set up to handle this because it's indexed by the full ioctl > command, not the type. I think in some cases, the size is required to > disambiguate ioctl commands because the type field is not unique across > devices. In some cases, the sanitizers would have to know the exact > command (not just the size), to validate points embedded in the struct > passed to the ioctl. So I don't think changing ioctl constants when > extensible structs change is obviously beneficial to the sanitizers, > either. Same for valgrind memcheck handling of ioctls. > I would prefer if the ioctl commands could be frozen and decoupled from > the structs. As far as I understand it, there is no requirement that > the embedded size matches what the kernel deals with. Yes please. Thanks, Mark
On Wed, Nov 26, 2025 at 10:19 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Christian Brauner:
>
> > Validate extensible ioctls stricter than we do now.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> > fs/pidfs.c | 2 +-
> > include/linux/fs.h | 14 ++++++++++++++
> > 2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/pidfs.c b/fs/pidfs.c
> > index edc35522d75c..0a5083b9cce5 100644
> > --- a/fs/pidfs.c
> > +++ b/fs/pidfs.c
> > @@ -440,7 +440,7 @@ static bool pidfs_ioctl_valid(unsigned int cmd)
> > * erronously mistook the file descriptor for a pidfd.
> > * This is not perfect but will catch most cases.
> > */
> > - return (_IOC_TYPE(cmd) == _IOC_TYPE(PIDFD_GET_INFO));
> > + return extensible_ioctl_valid(cmd, PIDFD_GET_INFO, PIDFD_INFO_SIZE_VER0);
> > }
> >
> > return false;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index d7ab4f96d705..2f2edc53bf3c 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -4023,4 +4023,18 @@ static inline bool vfs_empty_path(int dfd, const char __user *path)
> >
> > int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter);
> >
> > +static inline bool extensible_ioctl_valid(unsigned int cmd_a,
> > + unsigned int cmd_b, size_t min_size)
> > +{
> > + if (_IOC_DIR(cmd_a) != _IOC_DIR(cmd_b))
> > + return false;
> > + if (_IOC_TYPE(cmd_a) != _IOC_TYPE(cmd_b))
> > + return false;
> > + if (_IOC_NR(cmd_a) != _IOC_NR(cmd_b))
> > + return false;
> > + if (_IOC_SIZE(cmd_a) < min_size)
> > + return false;
> > + return true;
> > +}
> > +
> > #endif /* _LINUX_FS_H */
>
> Is this really the right direction? This implies that the ioctl
> constants change as the structs get extended. At present, this impacts
> struct pidfd_info and PIDFD_GET_INFO.
Well, some classes of ioctls are indeed designed like that (drm comes
to mind), but I agree that it is something that preferably should be
decided at the initial interface design stage (including the
constants, for example, evdev is probably a good example of handling
them), as otherwise there will be userspace code that assumes that the
size doesn't change.
> I think this is a deparature from the previous design, where (low-level)
> userspace did not have not worry about the internal structure of ioctl
> commands and could treat them as opaque bit patterns. With the new
> approach, we have to dissect some of the commands in the same way
> extensible_ioctl_valid does it above.
>
> So far, this impacts glibc ABI tests. Looking at the strace sources, it
> doesn't look to me as if the ioctl handler is prepared to deal with this
> situation, either, because it uses the full ioctl command for lookups.
strace usually handles this opportunistically, but indeed ioctls which
don't have fixed size across kernel version require additional care in
terms of handling, decoding, and printing.
> The sanitizers could implement generic ioctl checking with the embedded
> size information in the ioctl command, but the current code structure is
> not set up to handle this because it's indexed by the full ioctl
> command, not the type. I think in some cases, the size is required to
> disambiguate ioctl commands because the type field is not unique across
> devices.
In terms of disambiguation, one needs to look at the fd in question,
and even that is oftentimes not enough (again, in drm, one needs to
figure out what driver backs a specific fd in order to determine the
ioctl semantics properly).
> In some cases, the sanitizers would have to know the exact
> command (not just the size), to validate points embedded in the struct
> passed to the ioctl. So I don't think changing ioctl constants when
> extensible structs change is obviously beneficial to the sanitizers,
> either.
>
> I would prefer if the ioctl commands could be frozen and decoupled from
> the structs. As far as I understand it, there is no requirement that
> the embedded size matches what the kernel deals with.
>
> Thanks,
> Florian
>
> --
> Strace-devel mailing list
> Strace-devel@lists.strace.io
> https://lists.strace.io/mailman/listinfo/strace-devel
--
Eugene Syromyatnikov
mailto:evgsyr@gmail.com
xmpp:esyr@jabber.{ru|org}
© 2016 - 2026 Red Hat, Inc.