On Fri, May 29, 2020 at 11:46:40AM -0500, Eric W. Biederman wrote: > > There is a small bug in the code that recomputes parts of bprm->cred > for every bprm->file. The code never recomputes the part of > clear_dangerous_personality_flags it is responsible for. > > Which means that in practice if someone creates a sgid script > the interpreter will not be able to use any of: > READ_IMPLIES_EXEC > ADDR_NO_RANDOMIZE > ADDR_COMPAT_LAYOUT > MMAP_PAGE_ZERO. > > This accentially clearing of personality flags probably does > not matter in practice because no one has complained > but it does make the code more difficult to understand. > > Further remaining bug compatible prevents the recomputation from being > removed and replaced by simply computing bprm->cred once from the > final bprm->file. > > Making this change removes the last behavior difference between > computing bprm->creds from the final file and recomputing > bprm->cred several times. Which allows this behavior change > to be justified for it's own reasons, and for any but hunts > looking into why the behavior changed to wind up here instead > of in the code that will follow that computes bprm->cred > from the final bprm->file. > > This small logic bug appears to have existed since the code > started clearing dangerous personality bits. > > History Tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git > Fixes: 1bb0fa189c6a ("[PATCH] NX: clean up legacy binary support") > Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> Yup, this looks good. Pointless nit because it's removed in the next patch, but pf_per_clear is following the same behavioral pattern as active_secureexec, it could be named active_per_clear, but since this already been bikeshed in v1, it's fine! :) Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx> I wish we had more robust execve tests. :( -- Kees Cook