Bug 260266 - NFS callback RPC with negative taglen can crash client
Summary: NFS callback RPC with negative taglen can crash client
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-07 18:52 UTC by Robert Morris
Modified: 2021-12-23 01:47 UTC (History)
2 users (show)

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


Attachments
Crash an NFS client with a callback with a bad taglen. (30.19 KB, text/plain)
2021-12-07 18:52 UTC, Robert Morris
no flags Details
properly sanity check the callback taglen (488 bytes, patch)
2021-12-08 21:58 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-07 18:52:37 UTC
Created attachment 229961 [details]
Crash an NFS client with a callback with a bad taglen.

nfscl_docb() has a few "goto nfsmout" before it initializes retopsp,
but the code after nfsmout dereferences retopsp if taglen != -1.

I've attached a demo:

# uname -a
FreeBSD  14.0-CURRENT FreeBSD 14.0-CURRENT #130 main-n250909-08e6880c1a5c-dirty: Tue Dec  7 13:30:08 EST 2021     rtm@xxx:/usr/obj/usr/rtm/symbsd/src/riscv.riscv64/sys/RTM  riscv
# cc fnfs_6.c
# ./a.out
...
panic: Fatal page fault at 0xffffffc00021c200: 0000000000000000
--- exception 15, tval = 0
nfscl_docb() at nfscl_docb+0x1ac
nfscb_program() at nfscb_program+0xee
svc_run_internal() at svc_run_internal+0x810
svc_run() at svc_run+0x1a2
nfscbd_nfsd() at nfscbd_nfsd+0xce
nfssvc_nfscl() at nfssvc_nfscl+0x204
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-08 21:58:11 UTC
Created attachment 229981 [details]
properly sanity check the callback taglen

The sanity check for taglen in the callback request
was incoorect in two ways.
1 - It did not check for an excessively large value.
2 - It did not set the taglen to -1 when the sanity check failed.
This patch fixes the above and should stop the crashes.

Maybe the reporter can confirm that the patch stops the
crashes for them?
Comment 2 Robert Morris 2021-12-09 09:56:11 UTC
(In reply to Rick Macklem from comment #1)
Yes -- the patch makes the crash go away for me.
Comment 3 commit-hook freebsd_committer 2021-12-09 22:19:14 UTC
A commit in branch main references this bug:

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

commit d9931c25617d6625e280fda19bd9c2878e49c091
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-09 22:15:48 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-09 22:15:48 +0000

    nfscl: Sanity check the callback tag length

    The sanity check for tag length in a callback request
    was broken in two ways:

    It checked for a negative value, but not a large positive
    value.

    It did not set taglen to -1, to indicate to the code that
    it should not be used.

    This patch fixes both of these issues.

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

 sys/fs/nfsclient/nfs_clstate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Comment 4 Rick Macklem freebsd_committer 2021-12-09 22:27:49 UTC
Patch has been committed and will be MFC'd.
Comment 5 commit-hook freebsd_committer 2021-12-23 01:40:06 UTC
A commit in branch stable/13 references this bug:

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

commit a41216dbe060032bb439d55220bd4a8e135ec80f
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-09 22:15:48 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-23 01:36:18 +0000

    nfscl: Sanity check the callback tag length

    The sanity check for tag length in a callback request
    was broken in two ways:

    It checked for a negative value, but not a large positive
    value.

    It did not set taglen to -1, to indicate to the code that
    it should not be used.

    This patch fixes both of these issues.

    PR:     260266

    (cherry picked from commit d9931c25617d6625e280fda19bd9c2878e49c091)

 sys/fs/nfsclient/nfs_clstate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Comment 6 commit-hook freebsd_committer 2021-12-23 01:46:08 UTC
A commit in branch stable/12 references this bug:

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

commit 055b230b85cde066eeedd1417e336d2acf6281b9
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-09 22:15:48 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-23 01:42:24 +0000

    nfscl: Sanity check the callback tag length

    The sanity check for tag length in a callback request
    was broken in two ways:

    It checked for a negative value, but not a large positive
    value.

    It did not set taglen to -1, to indicate to the code that
    it should not be used.

    This patch fixes both of these issues.

    PR:     260266

    (cherry picked from commit d9931c25617d6625e280fda19bd9c2878e49c091)

 sys/fs/nfsclient/nfs_clstate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Comment 7 Rick Macklem freebsd_committer 2021-12-23 01:47:49 UTC
Patch has been committed and MFC'd.