namei(9): allow NDINIT_ATVP() lookups in capability mode
ClosedPublic
Actions

Authored by kib on Sun, Nov 8, 4:27 AM.

Project Tags

None

Referenced Files

	F177415177: D61315.1794321897.diff
	Mon, Nov 9, 2:44 PM

	F177414992: D61315.1794320951.diff
	Mon, Nov 9, 2:29 PM

	F177414989: D61315.1794320945.diff
	Mon, Nov 9, 2:29 PM

Subscribers

imp

ivy

olce

Details

Reviewers

markj
olce

Commits

rG922eaa623076: namei(9): allow NDINIT_ATVP() lookups in capability mode
rG2f3d64f63677: namei(9): fix restart for NDINIT_ATVP

Summary

namei(9): fix restart for NDINIT_ATVP

namei() might restart for altroot or for ERESTART from
nameicap_tracker_add().

vfs_lookup() consumes the reference on ni_startdir and clears it, so
a restarted NDINIT_ATVP() lookup started from the current directory
instead of the vnode passed by the caller.

Save the starting vnode in a local variable, hold it while the lookup
can be restarted, and reference it before the restarted pass. Do not
assert NAMEI_DBG_HADSTARTDIR on the internal restart.

Noted by:       markj


namei(9): allow NDINIT_ATVP() lookups in capability mode

NDINIT_ATVP() initializes dirfd to AT_FDCWD, which is denied for a
process already in cap mode.  Since NDINIT_ATVP() is a special lookup
operation performed by the kernel, it should be allowed to proceed for
the paths beneath vp.

Fabricate a pseudo dirfd value that is an unused placeholder,
which fails the AT_FDCWD comparison and allows the lookup to proceed in
cap mode.

Note that failed cap-mode NDINIT_ATVP lookup returned ECAPMODE without
releasing the caller reference on ni_startdir, which is fixed by this
change.

Reported by:    ivy


fgetvp_lookup{,smr}(9): assert that nd_dirfd is valid

AT_FDCWD and AT_ATVP are special negative values that must not be passed
there.

Requested by:   markj

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Sun, Nov 8, 4:27 AM

Herald added a subscriber: imp. · View Herald TranscriptSun, Nov 8, 4:27 AM

kib requested review of this revision.Sun, Nov 8, 4:27 AM

Can't we just set NOCAPCHECK in NDINIT_ATVP?

In D61315#1405673, @markj wrote:

Can't we just set NOCAPCHECK in NDINIT_ATVP?

Then it would allow dotdot. Even if it is safe now, it feels like a landmine for some future use.

In D61315#1405674, @kib wrote:

In D61315#1405673, @markj wrote:

Can't we just set NOCAPCHECK in NDINIT_ATVP?

Then it would allow dotdot. Even if it is safe now, it feels like a landmine for some future use.

Ok.

I would assert that nd_dirfd >= 0 in fgetvp_lookup() and fgetvp_lookup_smr().

BTW, how does NDINIT_ATVP handle namei() restarts, e.g., for altroot? vfs_lookup() sets ndp->ni_startdir = NULL, so the dvp reference is consumed in the first attempt, but we might retry.

Handle restart for NDINIT_ATVP().

Herald added a reviewer: olce. · View Herald TranscriptSun, Nov 8, 8:47 PM

Herald added a subscriber: olce. · View Herald Transcript

Assert that nd_dirfd is not special.

markj accepted this revision.Mon, Nov 9, 2:57 PM

markj added inline comments.

sys/kern/kern_descrip.c
3402	Sorry, I think this is too strong actually. Probably all we can do is assert `fd != AT_ATVP && fd != AT_FDCWD`. Same below.

This revision is now accepted and ready to land.Mon, Nov 9, 2:57 PM

kib added inline comments.Mon, Nov 9, 4:22 PM

sys/kern/kern_descrip.c
3402	In fact, why would it be useful? If it mean that the specific fd values are filtered at the namei() level, then it really does not happen: for instance userspace can pass AT_ATVP. It just that it cannot set up other components of the ndp in a way that makes the control to reach there, I think. IMO it is enough to allow fget() to reject all really invalid fds, be it negative or missed in the filedesc table.

markj added inline comments.Mon, Nov 9, 8:02 PM

sys/kern/kern_descrip.c
3402	I was looking at cache_fplookup() and how it could handle AT_ATVP lookups. Then I saw that it doesn't: they are rejected by cache_can_fplookup(). So it seemed sensible to add an assertion in cases that ever changes. But I think you're right, it's probably fine to just drop the assertions. Sorry for the noise.