| Message ID | 20240827014108.222719-1-lihongbo22@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [RFC] fs: obtain the inode generation number from vfs directly | expand |
On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: > Many mainstream file systems already support the GETVERSION ioctl, > and their implementations are completely the same, essentially > just obtain the value of i_generation. We think this ioctl can be > implemented at the VFS layer, so the file systems do not need to > implement it individually. What if a filesystem never touches i_generation? Is it ok to advertise a generation number of zero when that's really meaningless? Or should we gate the generic ioctl on (say) whether or not the fs implements file handles and/or supports nfs? --D > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > --- > fs/ioctl.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 64776891120c..dff887ec52c4 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -878,6 +878,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd, > case FS_IOC_GETFSUUID: > return ioctl_getfsuuid(filp, argp); > > + case FS_IOC_GETVERSION: > + return put_user(inode->i_generation, (int __user *)argp); > + > case FS_IOC_GETFSSYSFSPATH: > return ioctl_get_fs_sysfs_path(filp, argp); > > @@ -992,6 +995,9 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, > cmd = (cmd == FS_IOC32_GETFLAGS) ? > FS_IOC_GETFLAGS : FS_IOC_SETFLAGS; > fallthrough; > + case FS_IOC32_GETVERSION: > + cmd = FS_IOC_GETVERSION; > + fallthrough; > /* > * everything else in do_vfs_ioctl() takes either a compatible > * pointer argument or no argument -- call it with a modified > -- > 2.34.1 > >
On 2024/8/27 10:13, Darrick J. Wong wrote: > On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: >> Many mainstream file systems already support the GETVERSION ioctl, >> and their implementations are completely the same, essentially >> just obtain the value of i_generation. We think this ioctl can be >> implemented at the VFS layer, so the file systems do not need to >> implement it individually. > > What if a filesystem never touches i_generation? Is it ok to advertise > a generation number of zero when that's really meaningless? Or should > we gate the generic ioctl on (say) whether or not the fs implements file > handles and/or supports nfs? This ioctl mainly returns the i_generation, and whether it has meaning is up to the specific file system. Some tools will invoke IOC_GETVERSION, such as `lsattr -v`(but if it's lattr, it won't), but users may not necessarily actually use this value. Thanks, Hongbo > > --D > >> Signed-off-by: Hongbo Li <lihongbo22@huawei.com> >> --- >> fs/ioctl.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/fs/ioctl.c b/fs/ioctl.c >> index 64776891120c..dff887ec52c4 100644 >> --- a/fs/ioctl.c >> +++ b/fs/ioctl.c >> @@ -878,6 +878,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd, >> case FS_IOC_GETFSUUID: >> return ioctl_getfsuuid(filp, argp); >> >> + case FS_IOC_GETVERSION: >> + return put_user(inode->i_generation, (int __user *)argp); >> + >> case FS_IOC_GETFSSYSFSPATH: >> return ioctl_get_fs_sysfs_path(filp, argp); >> >> @@ -992,6 +995,9 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, >> cmd = (cmd == FS_IOC32_GETFLAGS) ? >> FS_IOC_GETFLAGS : FS_IOC_SETFLAGS; >> fallthrough; >> + case FS_IOC32_GETVERSION: >> + cmd = FS_IOC_GETVERSION; >> + fallthrough; >> /* >> * everything else in do_vfs_ioctl() takes either a compatible >> * pointer argument or no argument -- call it with a modified >> -- >> 2.34.1 >> >> >
On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: > Many mainstream file systems already support the GETVERSION ioctl, > and their implementations are completely the same, essentially > just obtain the value of i_generation. We think this ioctl can be > implemented at the VFS layer, so the file systems do not need to > implement it individually. ... then you should also remove the implementation from every filesystem, not just add it to the VFS.
On 2024/8/27 10:53, Matthew Wilcox wrote: > On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: >> Many mainstream file systems already support the GETVERSION ioctl, >> and their implementations are completely the same, essentially >> just obtain the value of i_generation. We think this ioctl can be >> implemented at the VFS layer, so the file systems do not need to >> implement it individually. > > ... then you should also remove the implementation from every > filesystem, not just add it to the VFS. > Yeah, this is just an RFC submission, mainly to see what everyone's opinions are. If this is ok, I will send the v2 that includes the removal of all file systems' implementations on IOC_GETVERSION. Thanks, Hongbo >
On Tue, Aug 27, 2024 at 10:32:38AM +0800, Hongbo Li wrote: > > > On 2024/8/27 10:13, Darrick J. Wong wrote: > > On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: > > > Many mainstream file systems already support the GETVERSION ioctl, > > > and their implementations are completely the same, essentially > > > just obtain the value of i_generation. We think this ioctl can be > > > implemented at the VFS layer, so the file systems do not need to > > > implement it individually. > > > > What if a filesystem never touches i_generation? Is it ok to advertise > > a generation number of zero when that's really meaningless? Or should > > we gate the generic ioctl on (say) whether or not the fs implements file > > handles and/or supports nfs? > > This ioctl mainly returns the i_generation, and whether it has meaning is up > to the specific file system. Some tools will invoke IOC_GETVERSION, such as > `lsattr -v`(but if it's lattr, it won't), but users may not necessarily > actually use this value. That's not how that works. If the kernel starts exporting a datum, people will start using it, and then the expectation that it will *continue* to work becomes ingrained in the userspace ABI forever. Be careful about establishing new behaviors for vfat. --D > Thanks, > Hongbo > > > > > --D > > > > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > > > --- > > > fs/ioctl.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/fs/ioctl.c b/fs/ioctl.c > > > index 64776891120c..dff887ec52c4 100644 > > > --- a/fs/ioctl.c > > > +++ b/fs/ioctl.c > > > @@ -878,6 +878,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd, > > > case FS_IOC_GETFSUUID: > > > return ioctl_getfsuuid(filp, argp); > > > + case FS_IOC_GETVERSION: > > > + return put_user(inode->i_generation, (int __user *)argp); > > > + > > > case FS_IOC_GETFSSYSFSPATH: > > > return ioctl_get_fs_sysfs_path(filp, argp); > > > @@ -992,6 +995,9 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, > > > cmd = (cmd == FS_IOC32_GETFLAGS) ? > > > FS_IOC_GETFLAGS : FS_IOC_SETFLAGS; > > > fallthrough; > > > + case FS_IOC32_GETVERSION: > > > + cmd = FS_IOC_GETVERSION; > > > + fallthrough; > > > /* > > > * everything else in do_vfs_ioctl() takes either a compatible > > > * pointer argument or no argument -- call it with a modified > > > -- > > > 2.34.1 > > > > > > > > >
On Mon, Aug 26, 2024 at 10:37:12PM GMT, Darrick J. Wong wrote: > On Tue, Aug 27, 2024 at 10:32:38AM +0800, Hongbo Li wrote: > > > > > > On 2024/8/27 10:13, Darrick J. Wong wrote: > > > On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: > > > > Many mainstream file systems already support the GETVERSION ioctl, > > > > and their implementations are completely the same, essentially > > > > just obtain the value of i_generation. We think this ioctl can be > > > > implemented at the VFS layer, so the file systems do not need to > > > > implement it individually. > > > > > > What if a filesystem never touches i_generation? Is it ok to advertise > > > a generation number of zero when that's really meaningless? Or should > > > we gate the generic ioctl on (say) whether or not the fs implements file > > > handles and/or supports nfs? > > > > This ioctl mainly returns the i_generation, and whether it has meaning is up > > to the specific file system. Some tools will invoke IOC_GETVERSION, such as > > `lsattr -v`(but if it's lattr, it won't), but users may not necessarily > > actually use this value. > > That's not how that works. If the kernel starts exporting a datum, > people will start using it, and then the expectation that it will > *continue* to work becomes ingrained in the userspace ABI forever. > Be careful about establishing new behaviors for vfat. Is the meaning even the same across all filesystems? And what is the meaning of this anyway? Is this described/defined for userspace anywhere?
On Tue, Aug 27, 2024 at 11:22:17AM +0200, Christian Brauner wrote: > On Mon, Aug 26, 2024 at 10:37:12PM GMT, Darrick J. Wong wrote: > > On Tue, Aug 27, 2024 at 10:32:38AM +0800, Hongbo Li wrote: > > > > > > > > > On 2024/8/27 10:13, Darrick J. Wong wrote: > > > > On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: > > > > > Many mainstream file systems already support the GETVERSION ioctl, > > > > > and their implementations are completely the same, essentially > > > > > just obtain the value of i_generation. We think this ioctl can be > > > > > implemented at the VFS layer, so the file systems do not need to > > > > > implement it individually. > > > > > > > > What if a filesystem never touches i_generation? Is it ok to advertise > > > > a generation number of zero when that's really meaningless? Or should > > > > we gate the generic ioctl on (say) whether or not the fs implements file > > > > handles and/or supports nfs? > > > > > > This ioctl mainly returns the i_generation, and whether it has meaning is up > > > to the specific file system. Some tools will invoke IOC_GETVERSION, such as > > > `lsattr -v`(but if it's lattr, it won't), but users may not necessarily > > > actually use this value. > > > > That's not how that works. If the kernel starts exporting a datum, > > people will start using it, and then the expectation that it will > > *continue* to work becomes ingrained in the userspace ABI forever. > > Be careful about establishing new behaviors for vfat. > > Is the meaning even the same across all filesystems? And what is the > meaning of this anyway? Is this described/defined for userspace > anywhere? AFAICT there's no manpage so I guess we could return getrandom32() if we wanted to. ;) But in seriousness, the usual four filesystems return i_generation. That is changed every time an inumber gets reused so that anyone with an old file handle cannot accidentally open the wrong file. In theory one could use GETVERSION to construct file handles (if you do, UHLHAND!) instead of using name_to_handle_at, which is why it's dangerous to implement GETVERSION for everyone without checking if i_generation makes sense. --D
On 2024/8/28 1:11, Darrick J. Wong wrote: > On Tue, Aug 27, 2024 at 11:22:17AM +0200, Christian Brauner wrote: >> On Mon, Aug 26, 2024 at 10:37:12PM GMT, Darrick J. Wong wrote: >>> On Tue, Aug 27, 2024 at 10:32:38AM +0800, Hongbo Li wrote: >>>> >>>> >>>> On 2024/8/27 10:13, Darrick J. Wong wrote: >>>>> On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: >>>>>> Many mainstream file systems already support the GETVERSION ioctl, >>>>>> and their implementations are completely the same, essentially >>>>>> just obtain the value of i_generation. We think this ioctl can be >>>>>> implemented at the VFS layer, so the file systems do not need to >>>>>> implement it individually. >>>>> >>>>> What if a filesystem never touches i_generation? Is it ok to advertise >>>>> a generation number of zero when that's really meaningless? Or should >>>>> we gate the generic ioctl on (say) whether or not the fs implements file >>>>> handles and/or supports nfs? >>>> >>>> This ioctl mainly returns the i_generation, and whether it has meaning is up >>>> to the specific file system. Some tools will invoke IOC_GETVERSION, such as >>>> `lsattr -v`(but if it's lattr, it won't), but users may not necessarily >>>> actually use this value. >>> >>> That's not how that works. If the kernel starts exporting a datum, >>> people will start using it, and then the expectation that it will >>> *continue* to work becomes ingrained in the userspace ABI forever. >>> Be careful about establishing new behaviors for vfat. >> >> Is the meaning even the same across all filesystems? And what is the >> meaning of this anyway? Is this described/defined for userspace >> anywhere? > > AFAICT there's no manpage so I guess we could return getrandom32() if we > wanted to. ;) > > But in seriousness, the usual four filesystems return i_generation. > That is changed every time an inumber gets reused so that anyone with an > old file handle cannot accidentally open the wrong file. In theory one > could use GETVERSION to construct file handles (if you do, UHLHAND!) > instead of using name_to_handle_at, which is why it's dangerous to > implement GETVERSION for everyone without checking if i_generation makes > sense. I'm not sure if my understanding of you is correct. As my humble opinions, the ioctl is for returning information to the user, and it cannot rely on this information returned to the user to ensure the security. If the file system wants to be secure internally, it should decouple from the VFS layer interface, rather than just not implementing it. For NFS, constructing a file handle is easy, you can successfully construct a file handle by capturing the nfs protocol packet even thought the i_generation is not exposed. Thanks, Hongbo > > --D
On Tue, Aug 27, 2024 at 10:11:48AM -0700, Darrick J. Wong wrote: > > But in seriousness, the usual four filesystems return i_generation. > That is changed every time an inumber gets reused so that anyone with an > old file handle cannot accidentally open the wrong file. In theory one > could use GETVERSION to construct file handles (if you do, UHLHAND!) > instead of using name_to_handle_at, which is why it's dangerous to > implement GETVERSION for everyone without checking if i_generation makes > sense. I believe the primary use case for {FS,EXT4}_IOC_GETVERSION was for userspace NFS servers to construt file handles. For file systems that don't store persistent i_generation numbers, I think it would be absolutely wrong that FS_IOC_GETVERSION to return zero, or some nonsense random number. The right thing to do would be to have it return an ENOTTY error if somene tries to call FS_IOC_GETVERSION on a vfat file system. Otherwise this could lead to potential user data loss/corruption for users of userspace nfs servers. - Ted
On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: > Many mainstream file systems already support the GETVERSION ioctl, > and their implementations are completely the same, essentially > just obtain the value of i_generation. We think this ioctl can be > implemented at the VFS layer, so the file systems do not need to > implement it individually. > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> Nope. No. Definitely not. I definitely do not want unprivileged users to be able to construct arbitrary file handles in userspace..... -Dave.
On Tue, Aug 27, 2024 at 10:11:48AM -0700, Darrick J. Wong wrote: > On Tue, Aug 27, 2024 at 11:22:17AM +0200, Christian Brauner wrote: > > On Mon, Aug 26, 2024 at 10:37:12PM GMT, Darrick J. Wong wrote: > > > On Tue, Aug 27, 2024 at 10:32:38AM +0800, Hongbo Li wrote: > > > > > > > > > > > > On 2024/8/27 10:13, Darrick J. Wong wrote: > > > > > On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: > > > > > > Many mainstream file systems already support the GETVERSION ioctl, > > > > > > and their implementations are completely the same, essentially > > > > > > just obtain the value of i_generation. We think this ioctl can be > > > > > > implemented at the VFS layer, so the file systems do not need to > > > > > > implement it individually. > > > > > > > > > > What if a filesystem never touches i_generation? Is it ok to advertise > > > > > a generation number of zero when that's really meaningless? Or should > > > > > we gate the generic ioctl on (say) whether or not the fs implements file > > > > > handles and/or supports nfs? > > > > > > > > This ioctl mainly returns the i_generation, and whether it has meaning is up > > > > to the specific file system. Some tools will invoke IOC_GETVERSION, such as > > > > `lsattr -v`(but if it's lattr, it won't), but users may not necessarily > > > > actually use this value. > > > > > > That's not how that works. If the kernel starts exporting a datum, > > > people will start using it, and then the expectation that it will > > > *continue* to work becomes ingrained in the userspace ABI forever. > > > Be careful about establishing new behaviors for vfat. > > > > Is the meaning even the same across all filesystems? And what is the > > meaning of this anyway? Is this described/defined for userspace > > anywhere? > > AFAICT there's no manpage so I guess we could return getrandom32() if we > wanted to. ;) > > But in seriousness, the usual four filesystems return i_generation. We do? I thought we didn't expose it except via bulkstat (which requires CAP_SYS_ADMIN in the initns). /me goes looking Ugh. Well, there you go. I've been living a lie for 20 years. > That is changed every time an inumber gets reused so that anyone with an > old file handle cannot accidentally open the wrong file. In theory one > could use GETVERSION to construct file handles Not theory. We've been constructing XFS filehandles in -privileged- userspace applications since the late 90s. Both DMAPI applications (HSMs) and xfsdump do this in combination with bulkstat to retreive the generation to enable full filesystem access without directory traversal being necessary. I was completely unaware that FS_IOC_GETVERSION was implemented by XFS and so this information is available to unprivileged users... > (if you do, UHLHAND!) Not familiar with that acronym. > instead of using name_to_handle_at, which is why it's dangerous to > implement GETVERSION for everyone without checking if i_generation makes > sense. Yup. If you have predictable generation numbers then it's trivial to guess filehandles once you know the inode number. Exposing generation numbers to unprivileged users allows them to determine if the generation numbers are predictable. Determining patterns is often as simple as a loop doing open(create); get inode number + generation; unlink(). That's one of the reasons we moved to randomised base generation numbers in XFS back in 2008 - making NFS filehandles on XFS filesystems less predictable and less prone to collisions. Given that we are actually exposing them to unprivileged users, we probably also need to get rid of the monotonic increment when the inode is freed and randomise that as well. And now I see EXT4_IOC_SETVERSION, and I want to run screaming..... -Dave.
On Wed 28-08-24 15:38:49, Dave Chinner wrote: > On Tue, Aug 27, 2024 at 10:11:48AM -0700, Darrick J. Wong wrote: > > On Tue, Aug 27, 2024 at 11:22:17AM +0200, Christian Brauner wrote: > > > On Mon, Aug 26, 2024 at 10:37:12PM GMT, Darrick J. Wong wrote: > > > > On Tue, Aug 27, 2024 at 10:32:38AM +0800, Hongbo Li wrote: > > > > > > > > > > > > > > > On 2024/8/27 10:13, Darrick J. Wong wrote: > > > > > > On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: > > > > > > > Many mainstream file systems already support the GETVERSION ioctl, > > > > > > > and their implementations are completely the same, essentially > > > > > > > just obtain the value of i_generation. We think this ioctl can be > > > > > > > implemented at the VFS layer, so the file systems do not need to > > > > > > > implement it individually. > > > > > > > > > > > > What if a filesystem never touches i_generation? Is it ok to advertise > > > > > > a generation number of zero when that's really meaningless? Or should > > > > > > we gate the generic ioctl on (say) whether or not the fs implements file > > > > > > handles and/or supports nfs? > > > > > > > > > > This ioctl mainly returns the i_generation, and whether it has meaning is up > > > > > to the specific file system. Some tools will invoke IOC_GETVERSION, such as > > > > > `lsattr -v`(but if it's lattr, it won't), but users may not necessarily > > > > > actually use this value. > > > > > > > > That's not how that works. If the kernel starts exporting a datum, > > > > people will start using it, and then the expectation that it will > > > > *continue* to work becomes ingrained in the userspace ABI forever. > > > > Be careful about establishing new behaviors for vfat. > > > > > > Is the meaning even the same across all filesystems? And what is the > > > meaning of this anyway? Is this described/defined for userspace > > > anywhere? > > > > AFAICT there's no manpage so I guess we could return getrandom32() if we > > wanted to. ;) > > > > But in seriousness, the usual four filesystems return i_generation. > > We do? > > I thought we didn't expose it except via bulkstat (which requires > CAP_SYS_ADMIN in the initns). > > /me goes looking > > Ugh. Well, there you go. I've been living a lie for 20 years. > > > That is changed every time an inumber gets reused so that anyone with an > > old file handle cannot accidentally open the wrong file. In theory one > > could use GETVERSION to construct file handles > > Not theory. We've been constructing XFS filehandles in -privileged- > userspace applications since the late 90s. Both DMAPI applications > (HSMs) and xfsdump do this in combination with bulkstat to retreive > the generation to enable full filesystem access without directory > traversal being necessary. > > I was completely unaware that FS_IOC_GETVERSION was implemented by > XFS and so this information is available to unprivileged users... > > > (if you do, UHLHAND!) > > Not familiar with that acronym. > > > instead of using name_to_handle_at, which is why it's dangerous to > > implement GETVERSION for everyone without checking if i_generation makes > > sense. > > Yup. If you have predictable generation numbers then it's trivial to > guess filehandles once you know the inode number. Exposing > generation numbers to unprivileged users allows them to determine if > the generation numbers are predictable. Determining patterns is > often as simple as a loop doing open(create); get inode number + > generation; unlink(). As far as VFS goes, we have always assumed that a valid file handles can be easily forged by unpriviledged userspace and hence all syscalls taking file handle are gated by CAP_DAC_READ_SEARCH capability check. That means userspace can indeed create a valid file handle but unless the process has sufficient priviledges to crawl the whole filesystem, VFS will not allow it to do anything special with it. I don't know what XFS interfaces use file handles and what are the permission requirements there but effectively relying on a 32-bit cookie value for security seems like a rather weak security these days to me... Honza
On Tue, 27 Aug 2024 01:41:08 +0000, Hongbo Li wrote: > Many mainstream file systems already support the GETVERSION ioctl, > and their implementations are completely the same, essentially > just obtain the value of i_generation. We think this ioctl can be > implemented at the VFS layer, so the file systems do not need to > implement it individually. > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > --- > fs/ioctl.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 64776891120c..dff887ec52c4 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -878,6 +878,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd, > case FS_IOC_GETFSUUID: > return ioctl_getfsuuid(filp, argp); > > + case FS_IOC_GETVERSION: > + return put_user(inode->i_generation, (int __user *)argp); > + > case FS_IOC_GETFSSYSFSPATH: > return ioctl_get_fs_sysfs_path(filp, argp); > > @@ -992,6 +995,9 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, > cmd = (cmd == FS_IOC32_GETFLAGS) ? > FS_IOC_GETFLAGS : FS_IOC_SETFLAGS; > fallthrough; The above case falls through... > + case FS_IOC32_GETVERSION: > + cmd = FS_IOC_GETVERSION; ... to this new case, which clobbers cmd, breaking FS_IOC32_[GS]ETFLAGS. > + fallthrough; > /* > * everything else in do_vfs_ioctl() takes either a compatible > * pointer argument or no argument -- call it with a modified
On Wed, Aug 28, 2024 at 05:55:28PM +0200, Jan Kara wrote: > On Wed 28-08-24 15:38:49, Dave Chinner wrote: > > On Tue, Aug 27, 2024 at 10:11:48AM -0700, Darrick J. Wong wrote: > > > On Tue, Aug 27, 2024 at 11:22:17AM +0200, Christian Brauner wrote: > > > > On Mon, Aug 26, 2024 at 10:37:12PM GMT, Darrick J. Wong wrote: > > > > > On Tue, Aug 27, 2024 at 10:32:38AM +0800, Hongbo Li wrote: > > > > > > > > > > > > > > > > > > On 2024/8/27 10:13, Darrick J. Wong wrote: > > > > > > > On Tue, Aug 27, 2024 at 01:41:08AM +0000, Hongbo Li wrote: > > > > > > > > Many mainstream file systems already support the GETVERSION ioctl, > > > > > > > > and their implementations are completely the same, essentially > > > > > > > > just obtain the value of i_generation. We think this ioctl can be > > > > > > > > implemented at the VFS layer, so the file systems do not need to > > > > > > > > implement it individually. > > > > > > > > > > > > > > What if a filesystem never touches i_generation? Is it ok to advertise > > > > > > > a generation number of zero when that's really meaningless? Or should > > > > > > > we gate the generic ioctl on (say) whether or not the fs implements file > > > > > > > handles and/or supports nfs? > > > > > > > > > > > > This ioctl mainly returns the i_generation, and whether it has meaning is up > > > > > > to the specific file system. Some tools will invoke IOC_GETVERSION, such as > > > > > > `lsattr -v`(but if it's lattr, it won't), but users may not necessarily > > > > > > actually use this value. > > > > > > > > > > That's not how that works. If the kernel starts exporting a datum, > > > > > people will start using it, and then the expectation that it will > > > > > *continue* to work becomes ingrained in the userspace ABI forever. > > > > > Be careful about establishing new behaviors for vfat. > > > > > > > > Is the meaning even the same across all filesystems? And what is the > > > > meaning of this anyway? Is this described/defined for userspace > > > > anywhere? > > > > > > AFAICT there's no manpage so I guess we could return getrandom32() if we > > > wanted to. ;) > > > > > > But in seriousness, the usual four filesystems return i_generation. > > > > We do? > > > > I thought we didn't expose it except via bulkstat (which requires > > CAP_SYS_ADMIN in the initns). > > > > /me goes looking > > > > Ugh. Well, there you go. I've been living a lie for 20 years. > > > > > That is changed every time an inumber gets reused so that anyone with an > > > old file handle cannot accidentally open the wrong file. In theory one > > > could use GETVERSION to construct file handles > > > > Not theory. We've been constructing XFS filehandles in -privileged- > > userspace applications since the late 90s. Both DMAPI applications > > (HSMs) and xfsdump do this in combination with bulkstat to retreive > > the generation to enable full filesystem access without directory > > traversal being necessary. > > > > I was completely unaware that FS_IOC_GETVERSION was implemented by > > XFS and so this information is available to unprivileged users... > > > > > (if you do, UHLHAND!) > > Not familiar with that acronym. U Have Lost, Have A Nice Day! > > > > > instead of using name_to_handle_at, which is why it's dangerous to > > > implement GETVERSION for everyone without checking if i_generation makes > > > sense. > > > > Yup. If you have predictable generation numbers then it's trivial to > > guess filehandles once you know the inode number. Exposing > > generation numbers to unprivileged users allows them to determine if > > the generation numbers are predictable. Determining patterns is > > often as simple as a loop doing open(create); get inode number + > > generation; unlink(). > > As far as VFS goes, we have always assumed that a valid file handles can be > easily forged by unpriviledged userspace and hence all syscalls taking file > handle are gated by CAP_DAC_READ_SEARCH capability check. That means > userspace can indeed create a valid file handle but unless the process has > sufficient priviledges to crawl the whole filesystem, VFS will not allow it > to do anything special with it. > > I don't know what XFS interfaces use file handles and what are the > permission requirements there but effectively relying on a 32-bit cookie > value for security seems like a rather weak security these days to me... CAP_SYS_ADMIN. --D > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR >
On Wed, Aug 28, 2024 at 05:55:28PM +0200, Jan Kara wrote: > On Wed 28-08-24 15:38:49, Dave Chinner wrote: > > > instead of using name_to_handle_at, which is why it's dangerous to > > > implement GETVERSION for everyone without checking if i_generation makes > > > sense. > > > > Yup. If you have predictable generation numbers then it's trivial to > > guess filehandles once you know the inode number. Exposing > > generation numbers to unprivileged users allows them to determine if > > the generation numbers are predictable. Determining patterns is > > often as simple as a loop doing open(create); get inode number + > > generation; unlink(). > > As far as VFS goes, we have always assumed that a valid file handles can be > easily forged by unpriviledged userspace and hence all syscalls taking file > handle are gated by CAP_DAC_READ_SEARCH capability check. That means > userspace can indeed create a valid file handle but unless the process has > sufficient priviledges to crawl the whole filesystem, VFS will not allow it > to do anything special with it. Yup. The issue that was raised back in ~2008 was to do with rogue machines on the network being able to trivially spoof NFS filehandles to bypass directory access permission checks on the server side. Once the generation numbers are randomised, this sort of attack is easily detected as the ESTALE counter on the server side goes through the roof... > I don't know what XFS interfaces use file handles and what are the > permission requirements there but effectively relying on a 32-bit cookie > value for security seems like a rather weak security these days to me... It's always been CAP_SYS_ADMIN for local filehandle interfaces. -Dave.
diff --git a/fs/ioctl.c b/fs/ioctl.c index 64776891120c..dff887ec52c4 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -878,6 +878,9 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd, case FS_IOC_GETFSUUID: return ioctl_getfsuuid(filp, argp); + case FS_IOC_GETVERSION: + return put_user(inode->i_generation, (int __user *)argp); + case FS_IOC_GETFSSYSFSPATH: return ioctl_get_fs_sysfs_path(filp, argp); @@ -992,6 +995,9 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, cmd = (cmd == FS_IOC32_GETFLAGS) ? FS_IOC_GETFLAGS : FS_IOC_SETFLAGS; fallthrough; + case FS_IOC32_GETVERSION: + cmd = FS_IOC_GETVERSION; + fallthrough; /* * everything else in do_vfs_ioctl() takes either a compatible * pointer argument or no argument -- call it with a modified
Many mainstream file systems already support the GETVERSION ioctl, and their implementations are completely the same, essentially just obtain the value of i_generation. We think this ioctl can be implemented at the VFS layer, so the file systems do not need to implement it individually. Signed-off-by: Hongbo Li <lihongbo22@huawei.com> --- fs/ioctl.c | 6 ++++++ 1 file changed, 6 insertions(+)