Bug 262515

Summary: Missing filecaps_free() in many places
Product: Base System Reporter: firk
Component: kernAssignee: Mateusz Guzik <mjg>
Status: Closed FIXED    
Severity: Affects Some People CC: emaste, markj, mjg, oshogbo
Priority: ---    
Version: CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
reproducer none

Description firk 2022-03-13 01:20:42 UTC 
When called with ndp->ni_startdir==NULL && ndp->ni_dirfd!=AT_FDCWD,

namei() -> namei_setup() fills ndp->ni_filecaps via fget_cap() with possibly (but seems not on most systems) dynamically allocated data.

filecaps_free() is needed somewhere after that, but there is no.


ndp->ni_startdir==NULL && ndp->ni_dirfd!=AT_FDCWD is usually a result of NDINIT_AT() or NDINIT_ATRIGHTS() macros with externally specified fd.

Found places (I think there is mre):
uipc_usrreq.c uipc_bindat(), unp_connectat()
vfs_cache.c kern___realpathat()
vfs_syscalls() a lot of kern_*at(); kern_openat() is an exception: it has proper filecaps_free() at least on success branch (but not on errors)
Comment 1 Mark Johnston freebsd_committer freebsd_triage 2022-03-24 19:40:37 UTC 
Good find.  I'll attach a program which demonstrates the leak.

I'm not sure how to fix it.  Certainly it's not practical to patch all namei() users.  But most of them don't inspect caps in the first place, so perhaps they should be copied on demand...?
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2022-03-24 19:41:11 UTC 
Created attachment 232680 [details]
reproducer

Reproducer using renameat(), though there are lots of ways to trigger it.
Comment 3 Mark Johnston freebsd_committer freebsd_triage 2022-03-24 19:45:50 UTC 
(In reply to Mark Johnston from comment #2)
This is rather slow, but can be tracked easily:

# vmstat -m | grep filecaps
     filecaps 34596888 540577K 34596961  16,32,64
Comment 4 Mateusz Guzik freebsd_committer freebsd_triage 2022-03-24 19:51:27 UTC 
I agree it should be on demand, but I have further notes.

1. fd-related code in namei_setup needs to be factored away to kern_descrip.c.

2. if the caller does not care to look at caps, we can make a copy without allocating anything (and subsequently without having to free anything). as shown in the reproducer, the only part which requires malloc is ioctl handling, but ioctls are not inspected in the lookup itself.

3. instead of direct filecaps_free perhaps NDFREE_CAPS or similar should be provided

I'll sleep on it and probably hack it up over the weekend. It will be mfcable.
Comment 5 Mateusz Guzik freebsd_committer freebsd_triage 2022-03-24 21:04:22 UTC 
This is the almost minimal patch for the immediate fix: https://reviews.freebsd.org/D34667
Comment 6 commit-hook freebsd_committer freebsd_triage 2022-04-02 12:09:17 UTC 
A commit in branch main references this bug:

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

commit 0c805718cbd3709e3ffc1a0d41612168c8242360
Author:     Mateusz Guzik <mjg@FreeBSD.org>
AuthorDate: 2022-03-24 20:51:03 +0000
Commit:     Mateusz Guzik <mjg@FreeBSD.org>
CommitDate: 2022-04-02 12:09:07 +0000

    vfs: fix memory leak on lookup with fds with ioctl caps

    Reviewed by:    markj
    PR:             262515
    Noted by:       firk@cantconnect.ru
    Differential Revision:  https://reviews.freebsd.org/D34667

 sys/kern/kern_descrip.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++-
 sys/kern/vfs_cache.c    |  2 +-
 sys/kern/vfs_lookup.c   | 50 ++-------------------------------
 sys/kern/vfs_syscalls.c |  8 +++---
 sys/sys/file.h          |  1 +
 sys/sys/namei.h         |  7 ++++-
 6 files changed, 88 insertions(+), 55 deletions(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2022-04-04 19:20:13 UTC 
A commit in branch stable/13 references this bug:

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

commit 838d8e6fb60e12e610701ae10be717309f3ea935
Author:     Mateusz Guzik <mjg@FreeBSD.org>
AuthorDate: 2022-03-24 20:51:03 +0000
Commit:     Mateusz Guzik <mjg@FreeBSD.org>
CommitDate: 2022-04-04 19:16:18 +0000

    vfs: fix memory leak on lookup with fds with ioctl caps

    Reviewed by:    markj
    PR:             262515
    Noted by:       firk@cantconnect.ru
    Differential Revision:  https://reviews.freebsd.org/D34667

    (cherry picked from commit 0c805718cbd3709e3ffc1a0d41612168c8242360)

 sys/kern/kern_descrip.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++-
 sys/kern/vfs_cache.c    |  3 +-
 sys/kern/vfs_lookup.c   | 50 ++-------------------------------
 sys/kern/vfs_syscalls.c |  8 +++---
 sys/sys/file.h          |  1 +
 sys/sys/namei.h         |  7 ++++-
 6 files changed, 89 insertions(+), 55 deletions(-)
Comment 8 commit-hook freebsd_committer freebsd_triage 2022-04-06 23:27:00 UTC 
A commit in branch releng/13.1 references this bug:

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

commit fffe016c8155805d1bd957a73a612f2c5e6ceac0
Author:     Mateusz Guzik <mjg@FreeBSD.org>
AuthorDate: 2022-03-24 20:51:03 +0000
Commit:     Mateusz Guzik <mjg@FreeBSD.org>
CommitDate: 2022-04-06 23:25:50 +0000

    vfs: fix memory leak on lookup with fds with ioctl caps

    Reviewed by:    markj
    PR:             262515
    Noted by:       firk@cantconnect.ru
    Differential Revision:  https://reviews.freebsd.org/D34667
    Approved by:    re (gjb)

    (cherry picked from commit 0c805718cbd3709e3ffc1a0d41612168c8242360)
    (cherry picked from commit 838d8e6fb60e12e610701ae10be717309f3ea935)

 sys/kern/kern_descrip.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++-
 sys/kern/vfs_cache.c    |  3 +-
 sys/kern/vfs_lookup.c   | 50 ++-------------------------------
 sys/kern/vfs_syscalls.c |  8 +++---
 sys/sys/file.h          |  1 +
 sys/sys/namei.h         |  7 ++++-
 6 files changed, 89 insertions(+), 55 deletions(-)