| Message ID | 20241011090023.655623-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | API for exporting connectable file handles to userspace | expand |
On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote: > Christian, > > These patches bring the NFS connectable file handles feature to > userspace servers. > > They rely on your and Aleksa's changes recently merged to v6.12. > > This v4 incorporates the review comments on Jeff and Jan (thanks!) > and there does not seem to be any objection for this new API, so > I think it is ready for staging. > > The API I chose for encoding conenctable file handles is pretty > conventional (AT_HANDLE_CONNECTABLE). > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > it more useful API that encoding a connectable file handle can mandate > the resolving of a connected fd, without having to opt-in for a > connected fd independently. > > I chose to implemnent this by using upper bits in the handle type field > It may be that out-of-tree filesystems return a handle type with upper > bits set, but AFAIK, no in-tree filesystem does that. > I added some warnings just in case we encouter that. > > I have written an fstest [4] and a man page draft [5] for the feature. > > Thanks, > Amir. > > Changes since v3 [3]: > - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff) > - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan) > - Add explicit check for negative type values (Jan) > - Added fstest and man-page draft > > Changes since v2 [2]: > - Use bit arithmetics instead of bitfileds (Jeff) > - Add assertions about use of high type bits > > Changes since v1 [1]: > - Assert on encode for disconnected path (Jeff) > - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH > - Drop the O_PATH mount_fd API hack (Jeff) > - Encode an explicit "connectable" flag in handle type > > [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/ > [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/ > [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/ > [4] https://github.com/amir73il/xfstests/commits/connectable-fh/ > [5] https://github.com/amir73il/man-pages/commits/connectable-fh/ > > Amir Goldstein (3): > fs: prepare for "explicit connectable" file handles > fs: name_to_handle_at() support for "explicit connectable" file > handles > fs: open_by_handle_at() support for decoding "explicit connectable" > file handles > > fs/exportfs/expfs.c | 17 ++++++++- > fs/fhandle.c | 75 +++++++++++++++++++++++++++++++++++--- > include/linux/exportfs.h | 13 +++++++ > include/uapi/linux/fcntl.h | 1 + > 4 files changed, 98 insertions(+), 8 deletions(-) > Aside from the relatively small stuff I noted in patch #2, this looks fine to me. Reviewed-by: Jeff Layton <jlayton@kernel.org>
> On Oct 11, 2024, at 5:00 AM, Amir Goldstein <amir73il@gmail.com> wrote: > > Christian, > > These patches bring the NFS connectable file handles feature to > userspace servers. > > They rely on your and Aleksa's changes recently merged to v6.12. > > This v4 incorporates the review comments on Jeff and Jan (thanks!) > and there does not seem to be any objection for this new API, so > I think it is ready for staging. > > The API I chose for encoding conenctable file handles is pretty > conventional (AT_HANDLE_CONNECTABLE). > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > it more useful API that encoding a connectable file handle can mandate > the resolving of a connected fd, without having to opt-in for a > connected fd independently. > > I chose to implemnent this by using upper bits in the handle type field > It may be that out-of-tree filesystems return a handle type with upper > bits set, but AFAIK, no in-tree filesystem does that. > I added some warnings just in case we encouter that. > > I have written an fstest [4] and a man page draft [5] for the feature. > > Thanks, > Amir. > > Changes since v3 [3]: > - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff) > - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan) > - Add explicit check for negative type values (Jan) > - Added fstest and man-page draft > > Changes since v2 [2]: > - Use bit arithmetics instead of bitfileds (Jeff) > - Add assertions about use of high type bits > > Changes since v1 [1]: > - Assert on encode for disconnected path (Jeff) > - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH > - Drop the O_PATH mount_fd API hack (Jeff) > - Encode an explicit "connectable" flag in handle type > > [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/ > [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/ > [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/ > [4] https://github.com/amir73il/xfstests/commits/connectable-fh/ > [5] https://github.com/amir73il/man-pages/commits/connectable-fh/ > > Amir Goldstein (3): > fs: prepare for "explicit connectable" file handles > fs: name_to_handle_at() support for "explicit connectable" file > handles > fs: open_by_handle_at() support for decoding "explicit connectable" > file handles > > fs/exportfs/expfs.c | 17 ++++++++- > fs/fhandle.c | 75 +++++++++++++++++++++++++++++++++++--- > include/linux/exportfs.h | 13 +++++++ > include/uapi/linux/fcntl.h | 1 + > 4 files changed, 98 insertions(+), 8 deletions(-) > > -- > 2.34.1 > Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>> Assuming this is going directly to Christian's tree. I'm a little concerned about how this new facility might be abused to get access to parts of the file system that a user is not authorized to access. But follow-up comments from Amir suggest that (with the current code) it is difficult or impossible to do. Are there self-tests or unit-tests for exportfs? -- Chuck Lever
On Fri, Oct 11, 2024 at 4:24 PM Chuck Lever III <chuck.lever@oracle.com> wrote: > > > > > On Oct 11, 2024, at 5:00 AM, Amir Goldstein <amir73il@gmail.com> wrote: > > > > Christian, > > > > These patches bring the NFS connectable file handles feature to > > userspace servers. > > > > They rely on your and Aleksa's changes recently merged to v6.12. > > > > This v4 incorporates the review comments on Jeff and Jan (thanks!) > > and there does not seem to be any objection for this new API, so > > I think it is ready for staging. > > > > The API I chose for encoding conenctable file handles is pretty > > conventional (AT_HANDLE_CONNECTABLE). > > > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > > it more useful API that encoding a connectable file handle can mandate > > the resolving of a connected fd, without having to opt-in for a > > connected fd independently. > > > > I chose to implemnent this by using upper bits in the handle type field > > It may be that out-of-tree filesystems return a handle type with upper > > bits set, but AFAIK, no in-tree filesystem does that. > > I added some warnings just in case we encouter that. > > > > I have written an fstest [4] and a man page draft [5] for the feature. > > > > Thanks, > > Amir. > > > > Changes since v3 [3]: > > - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff) > > - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan) > > - Add explicit check for negative type values (Jan) > > - Added fstest and man-page draft > > > > Changes since v2 [2]: > > - Use bit arithmetics instead of bitfileds (Jeff) > > - Add assertions about use of high type bits > > > > Changes since v1 [1]: > > - Assert on encode for disconnected path (Jeff) > > - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH > > - Drop the O_PATH mount_fd API hack (Jeff) > > - Encode an explicit "connectable" flag in handle type > > > > [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/ > > [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/ > > [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/ > > [4] https://github.com/amir73il/xfstests/commits/connectable-fh/ > > [5] https://github.com/amir73il/man-pages/commits/connectable-fh/ > > > > Amir Goldstein (3): > > fs: prepare for "explicit connectable" file handles > > fs: name_to_handle_at() support for "explicit connectable" file > > handles > > fs: open_by_handle_at() support for decoding "explicit connectable" > > file handles > > > > fs/exportfs/expfs.c | 17 ++++++++- > > fs/fhandle.c | 75 +++++++++++++++++++++++++++++++++++--- > > include/linux/exportfs.h | 13 +++++++ > > include/uapi/linux/fcntl.h | 1 + > > 4 files changed, 98 insertions(+), 8 deletions(-) > > > > -- > > 2.34.1 > > > > Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>> > > Assuming this is going directly to Christian's tree. > > I'm a little concerned about how this new facility might be > abused to get access to parts of the file system that a user > is not authorized to access. That's exactly the sort of thing I would like to be reviewed, but what makes you feel concerned? Are you concerned about handcrafted file handles? Correct me if I am wrong, but I think that any parts of the filesystem that could be accessed (by user with CAP_DAC_READ_SEARCH) using a handcrafted connectable file handle, could have also been accessed by the parent fid part before, so I do not see how connectable file handles create new ways to get access? > But follow-up comments from Amir > suggest that (with the current code) it is difficult or > impossible to do. > > Are there self-tests or unit-tests for exportfs? There are fstests, particularly, the "exportfs" test group and I added this one for connectable file handles: [4] https://github.com/amir73il/xfstests/commits/connectable-fh/ Did you mean another form of unit tests? Thanks, Amir.
> On Oct 11, 2024, at 2:22 PM, Amir Goldstein <amir73il@gmail.com> wrote: > > On Fri, Oct 11, 2024 at 4:24 PM Chuck Lever III <chuck.lever@oracle.com> wrote: >> >> >> >>> On Oct 11, 2024, at 5:00 AM, Amir Goldstein <amir73il@gmail.com> wrote: >>> >>> Christian, >>> >>> These patches bring the NFS connectable file handles feature to >>> userspace servers. >>> >>> They rely on your and Aleksa's changes recently merged to v6.12. >>> >>> This v4 incorporates the review comments on Jeff and Jan (thanks!) >>> and there does not seem to be any objection for this new API, so >>> I think it is ready for staging. >>> >>> The API I chose for encoding conenctable file handles is pretty >>> conventional (AT_HANDLE_CONNECTABLE). >>> >>> open_by_handle_at(2) does not have AT_ flags argument, but also, I find >>> it more useful API that encoding a connectable file handle can mandate >>> the resolving of a connected fd, without having to opt-in for a >>> connected fd independently. >>> >>> I chose to implemnent this by using upper bits in the handle type field >>> It may be that out-of-tree filesystems return a handle type with upper >>> bits set, but AFAIK, no in-tree filesystem does that. >>> I added some warnings just in case we encouter that. >>> >>> I have written an fstest [4] and a man page draft [5] for the feature. >>> >>> Thanks, >>> Amir. >>> >>> Changes since v3 [3]: >>> - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff) >>> - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan) >>> - Add explicit check for negative type values (Jan) >>> - Added fstest and man-page draft >>> >>> Changes since v2 [2]: >>> - Use bit arithmetics instead of bitfileds (Jeff) >>> - Add assertions about use of high type bits >>> >>> Changes since v1 [1]: >>> - Assert on encode for disconnected path (Jeff) >>> - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH >>> - Drop the O_PATH mount_fd API hack (Jeff) >>> - Encode an explicit "connectable" flag in handle type >>> >>> [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/ >>> [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/ >>> [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/ >>> [4] https://github.com/amir73il/xfstests/commits/connectable-fh/ >>> [5] https://github.com/amir73il/man-pages/commits/connectable-fh/ >>> >>> Amir Goldstein (3): >>> fs: prepare for "explicit connectable" file handles >>> fs: name_to_handle_at() support for "explicit connectable" file >>> handles >>> fs: open_by_handle_at() support for decoding "explicit connectable" >>> file handles >>> >>> fs/exportfs/expfs.c | 17 ++++++++- >>> fs/fhandle.c | 75 +++++++++++++++++++++++++++++++++++--- >>> include/linux/exportfs.h | 13 +++++++ >>> include/uapi/linux/fcntl.h | 1 + >>> 4 files changed, 98 insertions(+), 8 deletions(-) >>> >>> -- >>> 2.34.1 >>> >> >> Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>> >> >> Assuming this is going directly to Christian's tree. >> >> I'm a little concerned about how this new facility might be >> abused to get access to parts of the file system that a user >> is not authorized to access. > > That's exactly the sort of thing I would like to be reviewed, > but what makes you feel concerned? > > Are you concerned about handcrafted file handles? Yes; a user could construct a file handle that could bypass the usual authorization checks when it gets connected. It's a little hare-brained and hand-wavy because this is a new area for me. > Correct me if I am wrong, but I think that any parts of the filesystem > that could be accessed (by user with CAP_DAC_READ_SEARCH) > using a handcrafted connectable file handle, could have also been > accessed by the parent fid part before, so I do not see how connectable > file handles create new ways to get access? > >> But follow-up comments from Amir >> suggest that (with the current code) it is difficult or >> impossible to do. >> >> Are there self-tests or unit-tests for exportfs? > > There are fstests, particularly, the "exportfs" test group > and I added this one for connectable file handles: > > [4] https://github.com/amir73il/xfstests/commits/connectable-fh/ > > Did you mean another form of unit tests? No, actually; fstests wfm. That's something I could set up via kdevops and run on a regular basis. -- Chuck Lever
On Fri, Oct 11, 2024 at 8:40 PM Chuck Lever III <chuck.lever@oracle.com> wrote: > > > > > On Oct 11, 2024, at 2:22 PM, Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Fri, Oct 11, 2024 at 4:24 PM Chuck Lever III <chuck.lever@oracle.com> wrote: > >> > >> > >> > >>> On Oct 11, 2024, at 5:00 AM, Amir Goldstein <amir73il@gmail.com> wrote: > >>> > >>> Christian, > >>> > >>> These patches bring the NFS connectable file handles feature to > >>> userspace servers. > >>> > >>> They rely on your and Aleksa's changes recently merged to v6.12. > >>> > >>> This v4 incorporates the review comments on Jeff and Jan (thanks!) > >>> and there does not seem to be any objection for this new API, so > >>> I think it is ready for staging. > >>> > >>> The API I chose for encoding conenctable file handles is pretty > >>> conventional (AT_HANDLE_CONNECTABLE). > >>> > >>> open_by_handle_at(2) does not have AT_ flags argument, but also, I find > >>> it more useful API that encoding a connectable file handle can mandate > >>> the resolving of a connected fd, without having to opt-in for a > >>> connected fd independently. > >>> > >>> I chose to implemnent this by using upper bits in the handle type field > >>> It may be that out-of-tree filesystems return a handle type with upper > >>> bits set, but AFAIK, no in-tree filesystem does that. > >>> I added some warnings just in case we encouter that. > >>> > >>> I have written an fstest [4] and a man page draft [5] for the feature. > >>> > >>> Thanks, > >>> Amir. > >>> > >>> Changes since v3 [3]: > >>> - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff) > >>> - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan) > >>> - Add explicit check for negative type values (Jan) > >>> - Added fstest and man-page draft > >>> > >>> Changes since v2 [2]: > >>> - Use bit arithmetics instead of bitfileds (Jeff) > >>> - Add assertions about use of high type bits > >>> > >>> Changes since v1 [1]: > >>> - Assert on encode for disconnected path (Jeff) > >>> - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH > >>> - Drop the O_PATH mount_fd API hack (Jeff) > >>> - Encode an explicit "connectable" flag in handle type > >>> > >>> [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/ > >>> [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/ > >>> [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/ > >>> [4] https://github.com/amir73il/xfstests/commits/connectable-fh/ > >>> [5] https://github.com/amir73il/man-pages/commits/connectable-fh/ > >>> > >>> Amir Goldstein (3): > >>> fs: prepare for "explicit connectable" file handles > >>> fs: name_to_handle_at() support for "explicit connectable" file > >>> handles > >>> fs: open_by_handle_at() support for decoding "explicit connectable" > >>> file handles > >>> > >>> fs/exportfs/expfs.c | 17 ++++++++- > >>> fs/fhandle.c | 75 +++++++++++++++++++++++++++++++++++--- > >>> include/linux/exportfs.h | 13 +++++++ > >>> include/uapi/linux/fcntl.h | 1 + > >>> 4 files changed, 98 insertions(+), 8 deletions(-) > >>> > >>> -- > >>> 2.34.1 > >>> > >> > >> Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>> > >> > >> Assuming this is going directly to Christian's tree. > >> > >> I'm a little concerned about how this new facility might be > >> abused to get access to parts of the file system that a user > >> is not authorized to access. > > > > That's exactly the sort of thing I would like to be reviewed, > > but what makes you feel concerned? > > > > Are you concerned about handcrafted file handles? > > Yes; a user could construct a file handle that could bypass > the usual authorization checks when it gets connected. It's > a little hare-brained and hand-wavy because this is a new > area for me. > A malformed file handle is indeed a concern - it has always been, but in order to exploit one, an attacker would actually need to have a filesystem exported to nfs (to the attacking client machine). With commit 620c266f3949 ("fhandle: relax open_by_handle_at() permission checks"), attackers that have non-root access to a machine could also try to exploit filesystem bugs with malformed file handles. By adding support for connectable file handles, attackers could try to exploit bugs in ->fh_to_parent() implementations - bugs that would not have been exploitable so far unless filesystem is exported to nfs with subtree_check, which is quite rare IIUC. So I did an audit of the in-tree ->fh_to_{dentry,parent}() implementations. AFAICT all implementations properly check buffer length before trying to decode the handle... except for ceph. It looks to me like __snapfh_to_dentry() does not check buffer length before assuming that ceph_nfs_snapfh can be accessed. Ilya, Do you agree with my analysis? Please see the attached fix patch. Let me know if you want me to post it on ceph list or if that is sufficient. Thanks, Amir.
On Fri, 11 Oct 2024 11:00:20 +0200, Amir Goldstein wrote: > Christian, > > These patches bring the NFS connectable file handles feature to > userspace servers. > > They rely on your and Aleksa's changes recently merged to v6.12. > > [...] Applied to the vfs.exportfs branch of the vfs/vfs.git tree. Patches in the vfs.exportfs branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.exportfs [1/3] fs: prepare for "explicit connectable" file handles https://git.kernel.org/vfs/vfs/c/7f0e6b304c6c [2/3] fs: name_to_handle_at() support for "explicit connectable" file handles https://git.kernel.org/vfs/vfs/c/4142418cafc9 [3/3] fs: open_by_handle_at() support for decoding "explicit connectable" file handles https://git.kernel.org/vfs/vfs/c/81667fcf9b82
Christian, These patches bring the NFS connectable file handles feature to userspace servers. They rely on your and Aleksa's changes recently merged to v6.12. This v4 incorporates the review comments on Jeff and Jan (thanks!) and there does not seem to be any objection for this new API, so I think it is ready for staging. The API I chose for encoding conenctable file handles is pretty conventional (AT_HANDLE_CONNECTABLE). open_by_handle_at(2) does not have AT_ flags argument, but also, I find it more useful API that encoding a connectable file handle can mandate the resolving of a connected fd, without having to opt-in for a connected fd independently. I chose to implemnent this by using upper bits in the handle type field It may be that out-of-tree filesystems return a handle type with upper bits set, but AFAIK, no in-tree filesystem does that. I added some warnings just in case we encouter that. I have written an fstest [4] and a man page draft [5] for the feature. Thanks, Amir. Changes since v3 [3]: - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff) - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan) - Add explicit check for negative type values (Jan) - Added fstest and man-page draft Changes since v2 [2]: - Use bit arithmetics instead of bitfileds (Jeff) - Add assertions about use of high type bits Changes since v1 [1]: - Assert on encode for disconnected path (Jeff) - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH - Drop the O_PATH mount_fd API hack (Jeff) - Encode an explicit "connectable" flag in handle type [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/ [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/ [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/ [4] https://github.com/amir73il/xfstests/commits/connectable-fh/ [5] https://github.com/amir73il/man-pages/commits/connectable-fh/ Amir Goldstein (3): fs: prepare for "explicit connectable" file handles fs: name_to_handle_at() support for "explicit connectable" file handles fs: open_by_handle_at() support for decoding "explicit connectable" file handles fs/exportfs/expfs.c | 17 ++++++++- fs/fhandle.c | 75 +++++++++++++++++++++++++++++++++++--- include/linux/exportfs.h | 13 +++++++ include/uapi/linux/fcntl.h | 1 + 4 files changed, 98 insertions(+), 8 deletions(-)