Page MenuHomeFreeBSD
Differential D34667

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

Authored by mjg on Mar 24 2022, 9:03 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 18, 3:37 PM
Unknown Object (File)
Mon, Nov 18, 3:33 PM
Unknown Object (File)
Mon, Nov 18, 3:24 PM
Unknown Object (File)
Mon, Nov 18, 12:55 PM
Unknown Object (File)
Sat, Oct 26, 3:34 PM
Unknown Object (File)
Oct 4 2024, 4:29 PM
Unknown Object (File)
Sep 30 2024, 7:34 PM
Unknown Object (File)
Sep 30 2024, 7:32 PM
View All 74 Files
Subscribers
emaste
imp

Details

Reviewers
markj
Commits
rGfffe016c8155: vfs: fix memory leak on lookup with fds with ioctl caps
rG838d8e6fb60e: vfs: fix memory leak on lookup with fds with ioctl caps
rG0c805718cbd3: vfs: fix memory leak on lookup with fds with ioctl caps
Summary

Add WANTIOCTLCAPS so that the one caller which looks at them gets to keep the content. This is a minimal patch to fix the problem, subject to rework later.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mjg created this revision.Mar 24 2022, 9:03 PM
Herald added a subscriber: imp. · View Herald TranscriptMar 24 2022, 9:03 PM
mjg requested review of this revision.Mar 24 2022, 9:03 PM
emaste added a subscriber: emaste.Mar 24 2022, 9:08 PM
mjg planned changes to this revision.Mar 25 2022, 8:17 AM

All the extra checks for other lookups keep irking me. I'm going to do it better with forcing WANTFILECAPS consumer to free caps (if any) on its own on namei failure.

mjg updated this revision to Diff 104183.Mar 25 2022, 10:11 AM
mjg edited the summary of this revision. (Show Details)
  • make the ioctl caps-wanting caller responsible for always freeing them, even on namei failure

note rename also looks at caps, but happens to not look at ioctl.

mjg updated this revision to Diff 104185.Mar 25 2022, 10:44 AM
  • missing filecaps_free in NDREINIT
markj added a comment.Mar 25 2022, 3:01 PM

Thanks for working on this. I have a couple of comments:

  • I think WANTIOCTLCAPS should just be WANTCAPS. kern_openat() doesn't know or care about ioctls, so the need to specify "ioctl caps" is a layering violation.
  • Can we assert in NDFREE() that ni_filecaps is clear?
mjg added a comment.Mar 25 2022, 3:17 PM
In D34667#785494, @markj wrote:
  • I think WANTIOCTLCAPS should just be WANTCAPS. kern_openat() doesn't know or care about ioctls, so the need to specify "ioctl caps" is a layering violation.

well, rename looks at caps, but never takes a look at ioctl. with the proposed change it would also have to add the flag and make sure to free stuff later. I can't stress enough that explicit mention of ioctls, which is of no significance to almost anyone, allows to *ignore* presence of ioctls later on, not requiring non-kern-openat consumers to do anything extra.

  • Can we assert in NDFREE() that ni_filecaps is clear?

no, because currently NDFREE can be called at an arbitrary time and only to take care of part of the collected result. the current contract is incredibly loose.

markj added a comment.Mar 25 2022, 3:30 PM
In D34667#785496, @mjg wrote:
In D34667#785494, @markj wrote:
  • I think WANTIOCTLCAPS should just be WANTCAPS. kern_openat() doesn't know or care about ioctls, so the need to specify "ioctl caps" is a layering violation.

well, rename looks at caps, but never takes a look at ioctl. with the proposed change it would also have to add the flag and make sure to free stuff later. I can't stress enough that explicit mention of ioctls, which is of no significance to almost anyone, allows to *ignore* presence of ioctls later on, not requiring non-kern-openat consumers to do anything extra.

To be clear, I'm just suggesting a name change. If some other dynamically allocated capability limit is added to struct filecaps then the names WANTIOCTLCAPS and NDFREE_IOCTLCAPS() will be wrong and misleading. They should just be WANTCAPS and NDFREE_CAPS() with no other modification; consumers don't know or care about ioctl limits, so any such details should be hidden from them.

sys/kern/kern_descrip.c
1817

This can and should be local to kern_descrip.c.

mjg updated this revision to Diff 104193.Mar 25 2022, 3:36 PM
  • make filecaps_free_ioctl static
mjg added a comment.EditedMar 25 2022, 3:39 PM
In D34667#785509, @markj wrote:
In D34667#785496, @mjg wrote:
In D34667#785494, @markj wrote:
  • I think WANTIOCTLCAPS should just be WANTCAPS. kern_openat() doesn't know or care about ioctls, so the need to specify "ioctl caps" is a layering violation.

well, rename looks at caps, but never takes a look at ioctl. with the proposed change it would also have to add the flag and make sure to free stuff later. I can't stress enough that explicit mention of ioctls, which is of no significance to almost anyone, allows to *ignore* presence of ioctls later on, not requiring non-kern-openat consumers to do anything extra.

To be clear, I'm just suggesting a name change. If some other dynamically allocated capability limit is added to struct filecaps then the names WANTIOCTLCAPS and NDFREE_IOCTLCAPS() will be wrong and misleading. They should just be WANTCAPS and NDFREE_CAPS() with no other modification; consumers don't know or care about ioctl limits, so any such details should be hidden from them.

But with your proposal rename has to add the flag, otherwise it has no "right" to look at the caps stored in nameidata itself -- it did not ask for caps to be there. How about WANTFULLCAPS instead?

markj accepted this revision.Mar 31 2022, 6:44 PM
In D34667#785518, @mjg wrote:
In D34667#785509, @markj wrote:
In D34667#785496, @mjg wrote:
In D34667#785494, @markj wrote:
  • I think WANTIOCTLCAPS should just be WANTCAPS. kern_openat() doesn't know or care about ioctls, so the need to specify "ioctl caps" is a layering violation.

well, rename looks at caps, but never takes a look at ioctl. with the proposed change it would also have to add the flag and make sure to free stuff later. I can't stress enough that explicit mention of ioctls, which is of no significance to almost anyone, allows to *ignore* presence of ioctls later on, not requiring non-kern-openat consumers to do anything extra.

To be clear, I'm just suggesting a name change. If some other dynamically allocated capability limit is added to struct filecaps then the names WANTIOCTLCAPS and NDFREE_IOCTLCAPS() will be wrong and misleading. They should just be WANTCAPS and NDFREE_CAPS() with no other modification; consumers don't know or care about ioctl limits, so any such details should be hidden from them.

But with your proposal rename has to add the flag, otherwise it has no "right" to look at the caps stored in nameidata itself -- it did not ask for caps to be there. How about WANTFULLCAPS instead?

I see, ugh. Well, that's ok, we are unconditionally copying the ioctl limits today anyway so there is no performance impact. We should really have a filecaps_check() function to wrap the cap_check() call as well.

But ok, I do not object to the current patch then. Better to fix the leak minimally I suppose.

This revision is now accepted and ready to land.Mar 31 2022, 6:44 PM
Closed by commit rG0c805718cbd3: vfs: fix memory leak on lookup with fds with ioctl caps (authored by mjg). · Explain WhyApr 2 2022, 12:09 PM
This revision was automatically updated to reflect the committed changes.
mjg added a commit: rG0c805718cbd3: vfs: fix memory leak on lookup with fds with ioctl caps.
mjg added a commit: rG838d8e6fb60e: vfs: fix memory leak on lookup with fds with ioctl caps.Apr 4 2022, 7:20 PM
mjg added a commit: rGfffe016c8155: vfs: fix memory leak on lookup with fds with ioctl caps.Apr 6 2022, 11:26 PM

Revision Contents
Changeset List

PathSize
sys/
kern/
75 lines
2 lines
50 lines
8 lines
sys/
1 line
7 lines

Diff 104193

View Options

sys/kern/kern_descrip.c

Loading...
View Options

sys/kern/vfs_cache.c

Loading...
View Options

sys/kern/vfs_lookup.c

Loading...
View Options

sys/kern/vfs_syscalls.c

Loading...
View Options

sys/sys/file.h

Loading...
View Options

sys/sys/namei.h

Loading...