Bug 260176 - nfsrvd_verify() passes sfp=NULL to nfsv4_loadattr(), which can crash
Summary: nfsrvd_verify() passes sfp=NULL to nfsv4_loadattr(), which can crash
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Rick Macklem
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-12-02 19:16 UTC by Robert Morris
Modified: 2021-12-18 01:52 UTC (History)
2 users (show)

See Also:
rmacklem: mfc-stable13+
rmacklem: mfc-stable12+


Attachments
Crash an NFS v4 server with a VERIFY that mentions FILESAVAIL (30.63 KB, text/plain)
2021-12-02 19:16 UTC, Robert Morris
no flags Details
Just reply not same for verify of file system stats attributes (2.26 KB, patch)
2021-12-04 01:21 UTC, Rick Macklem
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Morris 2021-12-02 19:16:14 UTC
Created attachment 229846 [details]
Crash an NFS v4 server with a VERIFY that mentions FILESAVAIL

nfsrvd_verify() passes NULL for sfp to nfsv4_loadattr(), but if the
client includes an attribute like FILESAVAIL that uses sfp,
nfsv4_loadattr() will crash.

I've attached a demo:

# uname -a
FreeBSD  14.0-CURRENT FreeBSD 14.0-CURRENT #122 main-n250906-d95bc6b0bf4c-dirty: Thu Dec  2 05:26:06 EST 2021     rtm@xxx:/usr/obj/usr/rtm/symbsd/src/riscv.riscv64/sys/RTM  riscv
# cc fnfsd_9.c
# ./a.out
...
panic: Fatal page fault at 0xffffffc00020a678: 0x00000000000028
--- exception 13, tval = 0x28
nfsv4_loadattr() at nfsv4_loadattr+0x1a94
nfsrvd_verify() at nfsrvd_verify+0xb6
nfsrvd_dorpc() at nfsrvd_dorpc+0x147a
nfssvc_program() at nfssvc_program+0x5a8
svc_run_internal() at svc_run_internal+0x810
svc_run() at svc_run+0x1a2
nfsrvd_nfsd() at nfsrvd_nfsd+0x30c
nfssvc_nfsd() at nfssvc_nfsd+0x3ac
sys_nfssvc() at sys_nfssvc+0xd0
do_trap_user() at do_trap_user+0x220
cpu_exception_handler_user() at cpu_exception_handler_user+0x72
Comment 1 Rick Macklem freebsd_committer 2021-12-04 01:21:26 UTC
Created attachment 229867 [details]
Just reply not same for verify of file system stats attributes

To correctly verify file system stats like FILESFREE is
somewhat involved and, since no crashes are being reported,
not being done by extant NFSv4 clients.

Also, I do not see how file system stats that change constantly
could be effectively used by VERIFY/NVERIFY.

As such, this patch changes the code to just return NFSERR_NOTSAME
for them.  If a case where the VERIFY needs to work for any of
these attributes, a patch for that could be created.

This should avoid the crashes.

Maybe the reporter can confirm that it fixes the problem for him?
Comment 2 Robert Morris 2021-12-04 11:17:57 UTC
(In reply to Rick Macklem from comment #1)
The patch does fix the problem for me.
Comment 3 commit-hook freebsd_committer 2021-12-04 22:42:15 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=2d90ef47141d3ea0f88b43a1b6daf08d68ba8aba

commit 2d90ef47141d3ea0f88b43a1b6daf08d68ba8aba
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-04 22:38:55 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-04 22:38:55 +0000

    nfsd: Fix Verify for attributes like FilesAvail

    When the Verify operation calls nfsv4_loadattr(), it provides
    the "struct statfs" information that can be used for doing a
    compare for FilesAvail, FilesFree, FilesTotal, SpaceAvail,
    SpaceFree and SpaceTotal.  However, the code erroneously
    used the "struct nfsstatfs *" argument that is NULL.
    This patch fixes these cases to use the correct argument
    structure.  For the case of FilesAvail, the code in
    nfsv4_fillattr() was factored out into a separate function
    called nfsv4_filesavail(), so that it can be called from
    nfsv4_loadattr() as well as nfsv4_fillattr().

    In fact, most of the code in nfsv4_filesavail() is old
    OpenBSD code that does not build/run on FreeBSD, but I
    left it in place, in case it is of some use someday.

    I am not aware of any extant NFSv4 client that does Verify
    on these attributes.

    Reported by:    rtm@lcs.mit.edu
    Tested by:      rtm@lcs.mit.edu
    PR:     260176
    MFC after:      2 weeks

 sys/fs/nfs/nfs_commonsubs.c | 91 ++++++++++++++++++++++++++++++---------------
 1 file changed, 61 insertions(+), 30 deletions(-)
Comment 4 Rick Macklem freebsd_committer 2021-12-04 22:45:50 UTC
An alternate patch (which does do Verify for the cases) has
been committed and will be MFC'd.
Comment 5 commit-hook freebsd_committer 2021-12-18 01:40:38 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=f15f29c975f1374d67ae722df9f9bc7b55d4c42a

commit f15f29c975f1374d67ae722df9f9bc7b55d4c42a
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-04 22:38:55 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-18 01:36:41 +0000

    nfsd: Fix Verify for attributes like FilesAvail

    When the Verify operation calls nfsv4_loadattr(), it provides
    the "struct statfs" information that can be used for doing a
    compare for FilesAvail, FilesFree, FilesTotal, SpaceAvail,
    SpaceFree and SpaceTotal.  However, the code erroneously
    used the "struct nfsstatfs *" argument that is NULL.
    This patch fixes these cases to use the correct argument
    structure.  For the case of FilesAvail, the code in
    nfsv4_fillattr() was factored out into a separate function
    called nfsv4_filesavail(), so that it can be called from
    nfsv4_loadattr() as well as nfsv4_fillattr().

    In fact, most of the code in nfsv4_filesavail() is old
    OpenBSD code that does not build/run on FreeBSD, but I
    left it in place, in case it is of some use someday.

    I am not aware of any extant NFSv4 client that does Verify
    on these attributes.

    PR:     260176

    (cherry picked from commit 2d90ef47141d3ea0f88b43a1b6daf08d68ba8aba)

 sys/fs/nfs/nfs_commonsubs.c | 91 ++++++++++++++++++++++++++++++---------------
 1 file changed, 61 insertions(+), 30 deletions(-)
Comment 6 commit-hook freebsd_committer 2021-12-18 01:48:41 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=b386392ea909bff2d4b3c46d43ba9c1ee3217e42

commit b386392ea909bff2d4b3c46d43ba9c1ee3217e42
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-04 22:38:55 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-18 01:43:58 +0000

    nfsd: Fix Verify for attributes like FilesAvail

    When the Verify operation calls nfsv4_loadattr(), it provides
    the "struct statfs" information that can be used for doing a
    compare for FilesAvail, FilesFree, FilesTotal, SpaceAvail,
    SpaceFree and SpaceTotal.  However, the code erroneously
    used the "struct nfsstatfs *" argument that is NULL.
    This patch fixes these cases to use the correct argument
    structure.  For the case of FilesAvail, the code in
    nfsv4_fillattr() was factored out into a separate function
    called nfsv4_filesavail(), so that it can be called from
    nfsv4_loadattr() as well as nfsv4_fillattr().

    In fact, most of the code in nfsv4_filesavail() is old
    OpenBSD code that does not build/run on FreeBSD, but I
    left it in place, in case it is of some use someday.

    I am not aware of any extant NFSv4 client that does Verify
    on these attributes.

    PR:     260176

    (cherry picked from commit 2d90ef47141d3ea0f88b43a1b6daf08d68ba8aba)

 sys/fs/nfs/nfs_commonsubs.c | 91 ++++++++++++++++++++++++++++++---------------
 1 file changed, 61 insertions(+), 30 deletions(-)
Comment 7 Rick Macklem freebsd_committer 2021-12-18 01:52:28 UTC
Patch has been committed and MFC'd.