Page MenuHomeFreeBSD

exec: provide right hardlink name in AT_EXECPATH
ClosedPublic

Authored by kib on Oct 23 2021, 1:06 AM.

Details

Summary
For this, use vn_fullpath_hardlink() to resolve executable name for
execve(2).

This should provide the right hardlink name, used for execution, instead
of random hardlink pointing to this binary.  Also this should make the
AT_EXECNAME reliable for execve(2), since kernel only needs to resolve
parent directory path, which should always succeed (except pathological
cases like unlinking a directory).

PR:     248184
exec: store parent directory and hardlink name of the binary in struct proc
sysctl kern.proc.procname: report right hardlink name

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Oct 23 2021, 1:06 AM

Can this only add WANTPARENT if vn_fullpath_hardlink is going to be used to begin with? The extra ref/unref on containing directory would preferably be avoided if it can be helped.

In D32611#736134, @mjg wrote:

Can this only add WANTPARENT if vn_fullpath_hardlink is going to be used to begin with? The extra ref/unref on containing directory would preferably be avoided if it can be helped.

Isn't several jmps on modern CPUs cost same as atomic? I do not want to go with saving WANTPARENT in some var, it would be too ugly and really not save any jumps.

Anyway, below is update as you asked for. I also converted imgparam to bool as well. Per-commit view is available at https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/execpath

Avoid WANTPARENT if not needed, i.e. fexecve(2) or path is absolute.
Convert imgparam to use bools, and group them together.

kib planned changes to this revision.Oct 23 2021, 6:05 PM

Restore unconditional WANTPARENT: the binary' dvp is now saved in the struct proc to allow for kern.proc.pathname to return right name as well.

sys/kern/kern_exec.c
438

Initializing oldtextdvp twice. I think it is technically safe to leave newtextvp uninitialized.

693

Don't we unconditionally set nd.ni_dvp = NULL above? i.e. the dvp reference is unconditionally transferred to newtextdvp after the namei() call.

kib marked 2 inline comments as done.Oct 25 2021, 2:20 PM
kib added inline comments.
sys/kern/kern_exec.c
438

Perhaps yes, but following the mix of success/failure paths on the function exit is risky.

693

So yes this is true there, I changed the vrele to assert that ni_dvp == NULL.

I want the vrele(ni_dvp) to be left as is on exec_fail_dealloc: label. I think that technically ni_dvp cannot be non-null there, but it is too fragile to assume on the recovery path.

kib marked 2 inline comments as done.

Fix typo in NULL initialization.
Assert that ni_dvp is NULL, instead of releasing it, in the switch to interpreter' binary.

sys/kern/kern_exec.c
438

I prefer the explicit initialization as well. I just wasn't sure if it was intentional.

693

Actually I do not understand how this works. Doesn't the use of vn_fullpath_hardlink() assume that ni_dvp != NULL?

sys/kern/kern_exit.c
437 ↗(On Diff #97401)

Consider setting p->p_binname = NULL here, like we're doing for other fields.

kib marked 3 inline comments as done.Oct 25 2021, 5:50 PM
kib added inline comments.
sys/kern/kern_exec.c
693

Right, there were too many moves around.

So I did one more, lets calculate the execpath right after namei(). I like the move on its own, because it naturally fit into fexecve(2) path of calculating execpath, also removing one vnode relock. Then ni_dvp is not used anywhere except this args->fname != NULL path.

Also I think it is possible to eliminate second NDINIT() by moving first one under the same if() branch.

kib marked an inline comment as done.

Coalesce most of args->fname != NULL processing, including interpreter NDINIT() reset of nd.
As consequence, audit interpreter path and remove XXX comment.
Use nd.ni_dvp locally and only care about reference on newtextdvp.
Set p_binname to NULL on exit.

sys/kern/kern_exec.c
692

It took me a while to convince myself that we do not need to set imgp->vp = NULL here, but maybe it would be worthwhile anyway.

864
sys/kern/kern_proc.c
2280 ↗(On Diff #97418)

There is some layering violation here I think, we are assuming that vn_fullpath_hardlink() only accesses these fields. I'm not sure how best to resolve it.

2291 ↗(On Diff #97418)

If namei() fails we leak freebuf. It'll be overwritten by vn_fullpath().

kib marked 4 inline comments as done.Oct 26 2021, 1:59 AM
kib added inline comments.
sys/kern/kern_exec.c
692

imgp is fully reset on interpreter recycle, I will add the reset.

kib marked an inline comment as done.

Change interface for vn_fullpath_hardlink: take vp/dvp/name instead of struct nameidata *
Leaks fixes
More reshuffling

Fix vp/dvp juggling in vn_fullpath_hardlink()

This revision is now accepted and ready to land.Oct 26 2021, 1:08 PM