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@xxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx # v6.13 > Fixes: https://lore.kernel.org/r/CAG48ez2K9A5GwtgqO31u9ZL292we8ZwAA=TJwwEv7wRuJ3j4Lw@xxxxxxxxxxxxxx [1] This is not a proper Fixes: tag. > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx> > --- > 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(-) > > 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)); > + } > + This double-checks _IOC_TYPE(cmd) against _IOC_TYPE(PIDFD_GET_INFO)) due to the switch() and case statements. A simple return _IOC_TYPE(cmd) == _IOC_TYPE(PIDFD_GET_INFO); for the entire block would have had the same result. Or at least you might have used a simple return true; in the case statement if more are expected in the future. [ reported by coverity ] Guenter
