| Message ID | 20250204-work-pidfs-ioctl-v1-1-04987d239575@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pidfs: improve ioctl handling | expand |
On Tue, Feb 04, 2025 at 02:51:20PM +0100, Christian Brauner wrote: > Pidfs supports extensible and non-extensible ioctls. The extensible > ioctls need to check for the ioctl number itself not just the ioctl > command otherwise both backward- and forward compatibility are broken. > > The pidfs ioctl handler also needs to look at the type of the ioctl > command to guard against cases where "[...] a daemon receives some > random file descriptor from a (potentially less privileged) client and > expects the FD to be of some specific type, it might call ioctl() on > this FD with some type-specific command and expect the call to fail if > the FD is of the wrong type; but due to the missing type check, the > kernel instead performs some action that userspace didn't expect." > (cf. [1]] > > Reported-by: Jann Horn <jannh@google.com> > Cc: stable@vger.kernel.org # v6.13 > Fixes: https://lore.kernel.org/r/CAG48ez2K9A5GwtgqO31u9ZL292we8ZwAA=TJwwEv7wRuJ3j4Lw@mail.gmail.com [1] > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > Hey, > > Jann reported that the pidfs extensible ioctl checking has two issues: > > (1) We check for the ioctl command, not the number breaking forward and > backward compatibility. > > (2) We don't verify the type encoded in the ioctl to guard against > ioctl number collisions. > > This adresses both. > > Greg, when this patch (or a version thereof) lands upstream then it > needs to please be backported to v6.13 together with the already > upstream commit 8ce352818820 ("pidfs: check for valid ioctl commands"). Will do, thanks for the heads up. greg k-h
On Tue, Feb 4, 2025 at 2:51 PM Christian Brauner <brauner@kernel.org> wrote: > Pidfs supports extensible and non-extensible ioctls. The extensible > ioctls need to check for the ioctl number itself not just the ioctl > command otherwise both backward- and forward compatibility are broken. > > The pidfs ioctl handler also needs to look at the type of the ioctl > command to guard against cases where "[...] a daemon receives some > random file descriptor from a (potentially less privileged) client and > expects the FD to be of some specific type, it might call ioctl() on > this FD with some type-specific command and expect the call to fail if > the FD is of the wrong type; but due to the missing type check, the > kernel instead performs some action that userspace didn't expect." > (cf. [1]] Thanks, this looks good to me.
On Tue, 4 Feb 2025 at 13:51, Christian Brauner <brauner@kernel.org> wrote: > > Pidfs supports extensible and non-extensible ioctls. The extensible > ioctls need to check for the ioctl number itself not just the ioctl > command otherwise both backward- and forward compatibility are broken. > > The pidfs ioctl handler also needs to look at the type of the ioctl > command to guard against cases where "[...] a daemon receives some > random file descriptor from a (potentially less privileged) client and > expects the FD to be of some specific type, it might call ioctl() on > this FD with some type-specific command and expect the call to fail if > the FD is of the wrong type; but due to the missing type check, the > kernel instead performs some action that userspace didn't expect." > (cf. [1]] > > Reported-by: Jann Horn <jannh@google.com> > Cc: stable@vger.kernel.org # v6.13 > Fixes: https://lore.kernel.org/r/CAG48ez2K9A5GwtgqO31u9ZL292we8ZwAA=TJwwEv7wRuJ3j4Lw@mail.gmail.com [1] > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > Hey, > > Jann reported that the pidfs extensible ioctl checking has two issues: > > (1) We check for the ioctl command, not the number breaking forward and > backward compatibility. > > (2) We don't verify the type encoded in the ioctl to guard against > ioctl number collisions. > > This adresses both. > > Greg, when this patch (or a version thereof) lands upstream then it > needs to please be backported to v6.13 together with the already > upstream commit 8ce352818820 ("pidfs: check for valid ioctl commands"). > > Christian Thanks for the fixes, LGTM Acked-by: Luca Boccassi <luca.boccassi@gmail.com>
diff --git a/fs/pidfs.c b/fs/pidfs.c index 049352f973de..63f9699ebac3 100644 --- a/fs/pidfs.c +++ b/fs/pidfs.c @@ -287,7 +287,6 @@ static bool pidfs_ioctl_valid(unsigned int cmd) switch (cmd) { case FS_IOC_GETVERSION: case PIDFD_GET_CGROUP_NAMESPACE: - case PIDFD_GET_INFO: case PIDFD_GET_IPC_NAMESPACE: case PIDFD_GET_MNT_NAMESPACE: case PIDFD_GET_NET_NAMESPACE: @@ -300,6 +299,17 @@ static bool pidfs_ioctl_valid(unsigned int cmd) return true; } + /* Extensible ioctls require some more careful checks. */ + switch (_IOC_NR(cmd)) { + case _IOC_NR(PIDFD_GET_INFO): + /* + * Try to prevent performing a pidfd ioctl when someone + * 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 false; }
Pidfs supports extensible and non-extensible ioctls. The extensible ioctls need to check for the ioctl number itself not just the ioctl command otherwise both backward- and forward compatibility are broken. The pidfs ioctl handler also needs to look at the type of the ioctl command to guard against cases where "[...] a daemon receives some random file descriptor from a (potentially less privileged) client and expects the FD to be of some specific type, it might call ioctl() on this FD with some type-specific command and expect the call to fail if the FD is of the wrong type; but due to the missing type check, the kernel instead performs some action that userspace didn't expect." (cf. [1]] Reported-by: Jann Horn <jannh@google.com> Cc: stable@vger.kernel.org # v6.13 Fixes: https://lore.kernel.org/r/CAG48ez2K9A5GwtgqO31u9ZL292we8ZwAA=TJwwEv7wRuJ3j4Lw@mail.gmail.com [1] Signed-off-by: Christian Brauner <brauner@kernel.org> --- Hey, Jann reported that the pidfs extensible ioctl checking has two issues: (1) We check for the ioctl command, not the number breaking forward and backward compatibility. (2) We don't verify the type encoded in the ioctl to guard against ioctl number collisions. This adresses both. Greg, when this patch (or a version thereof) lands upstream then it needs to please be backported to v6.13 together with the already upstream commit 8ce352818820 ("pidfs: check for valid ioctl commands"). Christian --- fs/pidfs.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) --- base-commit: 6470d2c6d4233a781c67f842d3c066bf1cfa4fdc change-id: 20250204-work-pidfs-ioctl-6ef79a65e36b