| Message ID | 87o8tacxl3.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Infrastructure to allow fixing exec deadlocks | expand |
On 3/5/20 10:15 PM, Eric W. Biederman wrote: > > Add a flag binfmt->unrecoverable to mark when execution has gotten to > the point where it is impossible to return to userspace with the > calling process unchanged. > > While techinically this state starts as soon as de_thread starts > killing threads, the only return path at that point is if there is a > fatal signal pending. I have choosen instead to set unrecoverable > when the killing stops, and there are possibilities of failures other > than fatal signals. In particular it is possible for the allocation > of a new sighand structure to fail. > > Setting unrecoverable at this point has the benefit that other actions > can be taken after the other threads are all dead, and the > unrecoverable flag can double as a flag that those actions have been > taken. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > fs/exec.c | 7 ++++--- > include/linux/binfmts.h | 7 ++++++- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index db17be51b112..c243f9660d46 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1061,7 +1061,7 @@ static int exec_mmap(struct mm_struct *mm) > * disturbing other processes. (Other processes might share the signal > * table via the CLONE_SIGHAND option to clone().) > */ > -static int de_thread(struct task_struct *tsk) > +static int de_thread(struct linux_binprm *bprm, struct task_struct *tsk) > { > struct signal_struct *sig = tsk->signal; > struct sighand_struct *oldsighand = tsk->sighand; > @@ -1182,6 +1182,7 @@ static int de_thread(struct task_struct *tsk) > release_task(leader); > } > > + bprm->unrecoverable = true; > sig->group_exit_task = NULL; > sig->notify_count = 0; > > @@ -1266,7 +1267,7 @@ int flush_old_exec(struct linux_binprm * bprm) > * Make sure we have a private signal table and that > * we are unassociated from the previous thread group. > */ > - retval = de_thread(current); > + retval = de_thread(bprm, current); can we get rid of passing current as parameter here? Thanks Bernd. > if (retval) > goto out; > > @@ -1664,7 +1665,7 @@ int search_binary_handler(struct linux_binprm *bprm) > > read_lock(&binfmt_lock); > put_binfmt(fmt); > - if (retval < 0 && !bprm->mm) { > + if (retval < 0 && bprm->unrecoverable) { > /* we got to flush_old_exec() and failed after it */ > read_unlock(&binfmt_lock); > force_sigsegv(SIGSEGV); > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h > index b40fc633f3be..12263115ce7a 100644 > --- a/include/linux/binfmts.h > +++ b/include/linux/binfmts.h > @@ -44,7 +44,12 @@ struct linux_binprm { > * exec has happened. Used to sanitize execution environment > * and to set AT_SECURE auxv for glibc. > */ > - secureexec:1; > + secureexec:1, > + /* > + * Set when changes have been made that prevent returning > + * to userspace. > + */ > + unrecoverable:1; > #ifdef __alpha__ > unsigned int taso:1; > #endif >
On 3/5/20 10:15 PM, Eric W. Biederman wrote: > > Add a flag binfmt->unrecoverable to mark when execution has gotten to > the point where it is impossible to return to userspace with the > calling process unchanged. > > While techinically this state starts as soon as de_thread starts > killing threads, the only return path at that point is if there is a > fatal signal pending. I have choosen instead to set unrecoverable > when the killing stops, and there are possibilities of failures other > than fatal signals. In particular it is possible for the allocation > of a new sighand structure to fail. > > Setting unrecoverable at this point has the benefit that other actions > can be taken after the other threads are all dead, and the > unrecoverable flag can double as a flag that those actions have been > taken. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > fs/exec.c | 7 ++++--- > include/linux/binfmts.h | 7 ++++++- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index db17be51b112..c243f9660d46 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1061,7 +1061,7 @@ static int exec_mmap(struct mm_struct *mm) > * disturbing other processes. (Other processes might share the signal > * table via the CLONE_SIGHAND option to clone().) > */ > -static int de_thread(struct task_struct *tsk) > +static int de_thread(struct linux_binprm *bprm, struct task_struct *tsk) > { > struct signal_struct *sig = tsk->signal; > struct sighand_struct *oldsighand = tsk->sighand; > @@ -1182,6 +1182,7 @@ static int de_thread(struct task_struct *tsk) > release_task(leader); > } > > + bprm->unrecoverable = true; > sig->group_exit_task = NULL; > sig->notify_count = 0; > ah, sorry, if (thread_group_empty(tsk)) goto no_thread_group; will skip this: sig->group_exit_task = NULL; sig->notify_count = 0; no_thread_group: /* we have changed execution domain */ tsk->exit_signal = SIGCHLD; so I think the bprm->unrecoverable = true; should be here? Bernd. > @@ -1266,7 +1267,7 @@ int flush_old_exec(struct linux_binprm * bprm) > * Make sure we have a private signal table and that > * we are unassociated from the previous thread group. > */ > - retval = de_thread(current); > + retval = de_thread(bprm, current); > if (retval) > goto out; > > @@ -1664,7 +1665,7 @@ int search_binary_handler(struct linux_binprm *bprm) > > read_lock(&binfmt_lock); > put_binfmt(fmt); > - if (retval < 0 && !bprm->mm) { > + if (retval < 0 && bprm->unrecoverable) { > /* we got to flush_old_exec() and failed after it */ > read_unlock(&binfmt_lock); > force_sigsegv(SIGSEGV); > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h > index b40fc633f3be..12263115ce7a 100644 > --- a/include/linux/binfmts.h > +++ b/include/linux/binfmts.h > @@ -44,7 +44,12 @@ struct linux_binprm { > * exec has happened. Used to sanitize execution environment > * and to set AT_SECURE auxv for glibc. > */ > - secureexec:1; > + secureexec:1, > + /* > + * Set when changes have been made that prevent returning > + * to userspace. > + */ > + unrecoverable:1; > #ifdef __alpha__ > unsigned int taso:1; > #endif >
Bernd Edlinger <bernd.edlinger@hotmail.de> writes: > On 3/5/20 10:15 PM, Eric W. Biederman wrote: >> >> Add a flag binfmt->unrecoverable to mark when execution has gotten to >> the point where it is impossible to return to userspace with the >> calling process unchanged. >> >> While techinically this state starts as soon as de_thread starts >> killing threads, the only return path at that point is if there is a >> fatal signal pending. I have choosen instead to set unrecoverable >> when the killing stops, and there are possibilities of failures other >> than fatal signals. In particular it is possible for the allocation >> of a new sighand structure to fail. >> >> Setting unrecoverable at this point has the benefit that other actions >> can be taken after the other threads are all dead, and the >> unrecoverable flag can double as a flag that those actions have been >> taken. >> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> --- >> fs/exec.c | 7 ++++--- >> include/linux/binfmts.h | 7 ++++++- >> 2 files changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/fs/exec.c b/fs/exec.c >> index db17be51b112..c243f9660d46 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -1061,7 +1061,7 @@ static int exec_mmap(struct mm_struct *mm) >> * disturbing other processes. (Other processes might share the signal >> * table via the CLONE_SIGHAND option to clone().) >> */ >> -static int de_thread(struct task_struct *tsk) >> +static int de_thread(struct linux_binprm *bprm, struct task_struct *tsk) >> { >> struct signal_struct *sig = tsk->signal; >> struct sighand_struct *oldsighand = tsk->sighand; >> @@ -1182,6 +1182,7 @@ static int de_thread(struct task_struct *tsk) >> release_task(leader); >> } >> >> + bprm->unrecoverable = true; >> sig->group_exit_task = NULL; >> sig->notify_count = 0; >> > > ah, sorry, > if (thread_group_empty(tsk)) > goto no_thread_group; > will skip this: > > sig->group_exit_task = NULL; > sig->notify_count = 0; > > no_thread_group: > /* we have changed execution domain */ > tsk->exit_signal = SIGCHLD; > > so I think the bprm->unrecoverable = true; should be here? Absolutely. Thank you very much. This is why I try and keep things to one clear simple thing per patch so silly thinkos like that can be caught. Eric
Bernd Edlinger <bernd.edlinger@hotmail.de> writes: > On 3/5/20 10:15 PM, Eric W. Biederman wrote: >> @@ -1266,7 +1267,7 @@ int flush_old_exec(struct linux_binprm * bprm) >> * Make sure we have a private signal table and that >> * we are unassociated from the previous thread group. >> */ >> - retval = de_thread(current); >> + retval = de_thread(bprm, current); > > can we get rid of passing current as parameter here? With a separate patch. It makes the patch less clear if I make that change in this one. Eric
On 3/6/20 6:09 AM, Eric W. Biederman wrote: > Bernd Edlinger <bernd.edlinger@hotmail.de> writes: > >> On 3/5/20 10:15 PM, Eric W. Biederman wrote: >>> >>> Add a flag binfmt->unrecoverable to mark when execution has gotten to >>> the point where it is impossible to return to userspace with the >>> calling process unchanged. >>> >>> While techinically this state starts as soon as de_thread starts typo: s/techinically/technically/ >>> killing threads, the only return path at that point is if there is a >>> fatal signal pending. I have choosen instead to set unrecoverable I'm not good at english, is this chosen ? Bernd.
Bernd Edlinger <bernd.edlinger@hotmail.de> writes: > On 3/6/20 6:09 AM, Eric W. Biederman wrote: >> Bernd Edlinger <bernd.edlinger@hotmail.de> writes: >> >>> On 3/5/20 10:15 PM, Eric W. Biederman wrote: >>>> >>>> Add a flag binfmt->unrecoverable to mark when execution has gotten to >>>> the point where it is impossible to return to userspace with the >>>> calling process unchanged. >>>> >>>> While techinically this state starts as soon as de_thread starts > > typo: s/techinically/technically/ >>>> killing threads, the only return path at that point is if there is a >>>> fatal signal pending. I have choosen instead to set unrecoverable > > I'm not good at english, is this chosen ? > Yes. Defintley worth fixing. Eric
diff --git a/fs/exec.c b/fs/exec.c index db17be51b112..c243f9660d46 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1061,7 +1061,7 @@ static int exec_mmap(struct mm_struct *mm) * disturbing other processes. (Other processes might share the signal * table via the CLONE_SIGHAND option to clone().) */ -static int de_thread(struct task_struct *tsk) +static int de_thread(struct linux_binprm *bprm, struct task_struct *tsk) { struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; @@ -1182,6 +1182,7 @@ static int de_thread(struct task_struct *tsk) release_task(leader); } + bprm->unrecoverable = true; sig->group_exit_task = NULL; sig->notify_count = 0; @@ -1266,7 +1267,7 @@ int flush_old_exec(struct linux_binprm * bprm) * Make sure we have a private signal table and that * we are unassociated from the previous thread group. */ - retval = de_thread(current); + retval = de_thread(bprm, current); if (retval) goto out; @@ -1664,7 +1665,7 @@ int search_binary_handler(struct linux_binprm *bprm) read_lock(&binfmt_lock); put_binfmt(fmt); - if (retval < 0 && !bprm->mm) { + if (retval < 0 && bprm->unrecoverable) { /* we got to flush_old_exec() and failed after it */ read_unlock(&binfmt_lock); force_sigsegv(SIGSEGV); diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index b40fc633f3be..12263115ce7a 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -44,7 +44,12 @@ struct linux_binprm { * exec has happened. Used to sanitize execution environment * and to set AT_SECURE auxv for glibc. */ - secureexec:1; + secureexec:1, + /* + * Set when changes have been made that prevent returning + * to userspace. + */ + unrecoverable:1; #ifdef __alpha__ unsigned int taso:1; #endif
Add a flag binfmt->unrecoverable to mark when execution has gotten to the point where it is impossible to return to userspace with the calling process unchanged. While techinically this state starts as soon as de_thread starts killing threads, the only return path at that point is if there is a fatal signal pending. I have choosen instead to set unrecoverable when the killing stops, and there are possibilities of failures other than fatal signals. In particular it is possible for the allocation of a new sighand structure to fail. Setting unrecoverable at this point has the benefit that other actions can be taken after the other threads are all dead, and the unrecoverable flag can double as a flag that those actions have been taken. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 7 ++++--- include/linux/binfmts.h | 7 ++++++- 2 files changed, 10 insertions(+), 4 deletions(-)