Re: [PATCH 1/3] fork: assign refed credentials earlier
On Sat, Mar 21, 2015 at 09:29:04PM +0200, Konstantin Belousov wrote:
> On Sat, Mar 21, 2015 at 07:19:31PM +0100, Mateusz Guzik wrote:
> > On Sat, Mar 21, 2015 at 04:18:32PM +0200, Konstantin Belousov wrote:
> > > On Sat, Mar 21, 2015 at 02:57:22AM +0100, Mateusz Guzik wrote:
> > > > On Sat, Mar 21, 2015 at 03:51:51AM +0200, Konstantin Belousov wrote:
> > > > > On Sat, Mar 21, 2015 at 02:00:38AM +0100, Mateusz Guzik wrote:
> > > > > > From: Mateusz Guzik
> > > > > >
> > > > > > Prior to this change the kernel would take p1's credentials and
> > > > > > assign
> > > > > > them tempororarily to p2. But p1 could change credentials at that
> > > > > > time
> > > > > > and in effect give us a use-after-free.
> > > > > In which way could it change the credentials ? The assigned
> > > > > credentials
> > > > > are taken from td_ucred, which, I thought, are guaranteed to be stable
> > > > > for the duration of a syscall.
> > > > >
> > > >
> > > > It takes thread's credential in do_fork. But initial copy is taken
> > > > unlocked from struct proc.
> > > >
> > > > Relevant part of the diff:
> > > > > > @@ -870,7 +867,7 @@ fork1(struct thread *td, int flags, int pages,
> > > > > > struct proc **procp,
> > > > > > * XXX: This is ugly; when we copy resource usage, we need to
> > > > > > bump
> > > > > > * per-cred resource counters.
> > > > > > */
> > > > > > - proc_set_cred(newproc, p1->p_ucred);
> > > > > > + proc_set_cred(newproc, crhold(td->td_ucred));
> > > > > >
> > >
> > > I do not understand your note, nor I see the chunk above in the patches
> > > you send. Below is the citation from the patch 1:
> > >
> > > @@ -410,9 +410,6 @@ do_fork(struct thread *td, int flags, struct proc
> > > *p2,
> > > +struct thread *td2,
> > >
> > > bzero(&p2->p_startzero,
> > >
> > > __rangeof(struct proc, p_startzero, p_endzero));
> > >
> > >
> > >
> > > - crhold(td->td_ucred);
> > >
> > > - proc_set_cred(p2, td->td_ucred);
> > >
> > > -
> > >
> >
> > fork1 does:
> >
> > proc_set_cred(newproc, p1->p_ucred);
> >
> > p1 is unlocked, so whatever memory p1->p_ucred points to may already be
> > freed.
> >
> > /*
> > * Initialize resource accounting for the child process.
> > */
> > error = racct_proc_fork(p1, newproc);
> > if (error != 0) {
> > error = EAGAIN;
> > goto fail1;
> > }
> >
> > racct_proc_fork -> racct_add_locked results in accessing such
> > now-possibly-freed credentials.
> >
> > do_fork which properly assigns credentials (from a stable source
> > (td_ucred) + grabs a reference) is called later.
> >
> > The patch in question moves aforementioned assignent earlier to replace
> > unsafe one with p1->p_ucred.
>
> It seems that I understand now.
>
> If you instead assign td->td_ucred for the new process p_ucred temporary,
> would it allow to avoid introducing fail2 label ? I dislike even more
> contrived cleanup after the patch.
Yes but that seems like a hack awaiting for someone to trip over.
For instance I would say it would be desirable to move stuff like
freeing credentials into zone destructor handler.
A hack like this would leave us with an extra crfree.
Doing this work would require some care and making sure we have garbage
only where we can afford it and I'm not interested in dealing with this
right now.
So tl;dr I strongly prefer the patch as it is.
--
Mateusz Guzik
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: [PATCH 1/3] fork: assign refed credentials earlier
On Sat, Mar 21, 2015 at 07:19:31PM +0100, Mateusz Guzik wrote:
> On Sat, Mar 21, 2015 at 04:18:32PM +0200, Konstantin Belousov wrote:
> > On Sat, Mar 21, 2015 at 02:57:22AM +0100, Mateusz Guzik wrote:
> > > On Sat, Mar 21, 2015 at 03:51:51AM +0200, Konstantin Belousov wrote:
> > > > On Sat, Mar 21, 2015 at 02:00:38AM +0100, Mateusz Guzik wrote:
> > > > > From: Mateusz Guzik
> > > > >
> > > > > Prior to this change the kernel would take p1's credentials and assign
> > > > > them tempororarily to p2. But p1 could change credentials at that time
> > > > > and in effect give us a use-after-free.
> > > > In which way could it change the credentials ? The assigned credentials
> > > > are taken from td_ucred, which, I thought, are guaranteed to be stable
> > > > for the duration of a syscall.
> > > >
> > >
> > > It takes thread's credential in do_fork. But initial copy is taken
> > > unlocked from struct proc.
> > >
> > > Relevant part of the diff:
> > > > > @@ -870,7 +867,7 @@ fork1(struct thread *td, int flags, int pages,
> > > > > struct proc **procp,
> > > > >* XXX: This is ugly; when we copy resource usage, we need to
> > > > > bump
> > > > >* per-cred resource counters.
> > > > >*/
> > > > > - proc_set_cred(newproc, p1->p_ucred);
> > > > > + proc_set_cred(newproc, crhold(td->td_ucred));
> > > > >
> >
> > I do not understand your note, nor I see the chunk above in the patches
> > you send. Below is the citation from the patch 1:
> >
> > @@ -410,9 +410,6 @@ do_fork(struct thread *td, int flags, struct proc *p2,
> >
> > +struct thread *td2,
> >
> > bzero(&p2->p_startzero,
> >
> > __rangeof(struct proc, p_startzero, p_endzero));
> >
> >
> >
> > - crhold(td->td_ucred);
> >
> > - proc_set_cred(p2, td->td_ucred);
> >
> > -
> >
>
> fork1 does:
>
> proc_set_cred(newproc, p1->p_ucred);
>
> p1 is unlocked, so whatever memory p1->p_ucred points to may already be
> freed.
>
> /*
> * Initialize resource accounting for the child process.
> */
> error = racct_proc_fork(p1, newproc);
> if (error != 0) {
> error = EAGAIN;
> goto fail1;
> }
>
> racct_proc_fork -> racct_add_locked results in accessing such
> now-possibly-freed credentials.
>
> do_fork which properly assigns credentials (from a stable source
> (td_ucred) + grabs a reference) is called later.
>
> The patch in question moves aforementioned assignent earlier to replace
> unsafe one with p1->p_ucred.
It seems that I understand now.
If you instead assign td->td_ucred for the new process p_ucred temporary,
would it allow to avoid introducing fail2 label ? I dislike even more
contrived cleanup after the patch.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: [PATCH 1/3] fork: assign refed credentials earlier
On Sat, Mar 21, 2015 at 04:18:32PM +0200, Konstantin Belousov wrote:
> On Sat, Mar 21, 2015 at 02:57:22AM +0100, Mateusz Guzik wrote:
> > On Sat, Mar 21, 2015 at 03:51:51AM +0200, Konstantin Belousov wrote:
> > > On Sat, Mar 21, 2015 at 02:00:38AM +0100, Mateusz Guzik wrote:
> > > > From: Mateusz Guzik
> > > >
> > > > Prior to this change the kernel would take p1's credentials and assign
> > > > them tempororarily to p2. But p1 could change credentials at that time
> > > > and in effect give us a use-after-free.
> > > In which way could it change the credentials ? The assigned credentials
> > > are taken from td_ucred, which, I thought, are guaranteed to be stable
> > > for the duration of a syscall.
> > >
> >
> > It takes thread's credential in do_fork. But initial copy is taken
> > unlocked from struct proc.
> >
> > Relevant part of the diff:
> > > > @@ -870,7 +867,7 @@ fork1(struct thread *td, int flags, int pages,
> > > > struct proc **procp,
> > > > * XXX: This is ugly; when we copy resource usage, we need to
> > > > bump
> > > > * per-cred resource counters.
> > > > */
> > > > - proc_set_cred(newproc, p1->p_ucred);
> > > > + proc_set_cred(newproc, crhold(td->td_ucred));
> > > >
>
> I do not understand your note, nor I see the chunk above in the patches
> you send. Below is the citation from the patch 1:
>
> @@ -410,9 +410,6 @@ do_fork(struct thread *td, int flags, struct proc *p2,
>
> +struct thread *td2,
>
> bzero(&p2->p_startzero,
>
> __rangeof(struct proc, p_startzero, p_endzero));
>
>
>
> - crhold(td->td_ucred);
>
> - proc_set_cred(p2, td->td_ucred);
>
> -
>
fork1 does:
proc_set_cred(newproc, p1->p_ucred);
p1 is unlocked, so whatever memory p1->p_ucred points to may already be
freed.
/*
* Initialize resource accounting for the child process.
*/
error = racct_proc_fork(p1, newproc);
if (error != 0) {
error = EAGAIN;
goto fail1;
}
racct_proc_fork -> racct_add_locked results in accessing such
now-possibly-freed credentials.
do_fork which properly assigns credentials (from a stable source
(td_ucred) + grabs a reference) is called later.
The patch in question moves aforementioned assignent earlier to replace
unsafe one with p1->p_ucred.
--
Mateusz Guzik
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: [PATCH 1/3] fork: assign refed credentials earlier
On Sat, Mar 21, 2015 at 02:57:22AM +0100, Mateusz Guzik wrote: > On Sat, Mar 21, 2015 at 03:51:51AM +0200, Konstantin Belousov wrote: > > On Sat, Mar 21, 2015 at 02:00:38AM +0100, Mateusz Guzik wrote: > > > From: Mateusz Guzik > > > > > > Prior to this change the kernel would take p1's credentials and assign > > > them tempororarily to p2. But p1 could change credentials at that time > > > and in effect give us a use-after-free. > > In which way could it change the credentials ? The assigned credentials > > are taken from td_ucred, which, I thought, are guaranteed to be stable > > for the duration of a syscall. > > > > It takes thread's credential in do_fork. But initial copy is taken > unlocked from struct proc. > > Relevant part of the diff: > > > @@ -870,7 +867,7 @@ fork1(struct thread *td, int flags, int pages, struct > > > proc **procp, > > >* XXX: This is ugly; when we copy resource usage, we need to bump > > >* per-cred resource counters. > > >*/ > > > - proc_set_cred(newproc, p1->p_ucred); > > > + proc_set_cred(newproc, crhold(td->td_ucred)); > > > I do not understand your note, nor I see the chunk above in the patches you send. Below is the citation from the patch 1: @@ -410,9 +410,6 @@ do_fork(struct thread *td, int flags, struct proc *p2, +struct thread *td2, bzero(&p2->p_startzero, __rangeof(struct proc, p_startzero, p_endzero)); - crhold(td->td_ucred); - proc_set_cred(p2, td->td_ucred); - ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: [PATCH 1/3] fork: assign refed credentials earlier
On Sat, Mar 21, 2015 at 03:51:51AM +0200, Konstantin Belousov wrote: > On Sat, Mar 21, 2015 at 02:00:38AM +0100, Mateusz Guzik wrote: > > From: Mateusz Guzik > > > > Prior to this change the kernel would take p1's credentials and assign > > them tempororarily to p2. But p1 could change credentials at that time > > and in effect give us a use-after-free. > In which way could it change the credentials ? The assigned credentials > are taken from td_ucred, which, I thought, are guaranteed to be stable > for the duration of a syscall. > It takes thread's credential in do_fork. But initial copy is taken unlocked from struct proc. Relevant part of the diff: > > @@ -870,7 +867,7 @@ fork1(struct thread *td, int flags, int pages, struct > > proc **procp, > > * XXX: This is ugly; when we copy resource usage, we need to bump > > * per-cred resource counters. > > */ > > - proc_set_cred(newproc, p1->p_ucred); > > + proc_set_cred(newproc, crhold(td->td_ucred)); > > -- Mateusz Guzik ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: [PATCH 1/3] fork: assign refed credentials earlier
On Sat, Mar 21, 2015 at 02:00:38AM +0100, Mateusz Guzik wrote:
> From: Mateusz Guzik
>
> Prior to this change the kernel would take p1's credentials and assign
> them tempororarily to p2. But p1 could change credentials at that time
> and in effect give us a use-after-free.
In which way could it change the credentials ? The assigned credentials
are taken from td_ucred, which, I thought, are guaranteed to be stable
for the duration of a syscall.
Other two patches look fine.
> ---
> sys/kern/kern_fork.c | 15 +++
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
> index ae86fe1..15833fd 100644
> --- a/sys/kern/kern_fork.c
> +++ b/sys/kern/kern_fork.c
> @@ -410,9 +410,6 @@ do_fork(struct thread *td, int flags, struct proc *p2,
> struct thread *td2,
> bzero(&p2->p_startzero,
> __rangeof(struct proc, p_startzero, p_endzero));
>
> - crhold(td->td_ucred);
> - proc_set_cred(p2, td->td_ucred);
> -
> /* Tell the prison that we exist. */
> prison_proc_hold(p2->p_ucred->cr_prison);
>
> @@ -832,7 +829,7 @@ fork1(struct thread *td, int flags, int pages, struct
> proc **procp,
> td2 = thread_alloc(pages);
> if (td2 == NULL) {
> error = ENOMEM;
> - goto fail1;
> + goto fail2;
> }
> proc_linkup(newproc, td2);
> } else {
> @@ -841,7 +838,7 @@ fork1(struct thread *td, int flags, int pages, struct
> proc **procp,
> vm_thread_dispose(td2);
> if (!thread_alloc_stack(td2, pages)) {
> error = ENOMEM;
> - goto fail1;
> + goto fail2;
> }
> }
> }
> @@ -850,7 +847,7 @@ fork1(struct thread *td, int flags, int pages, struct
> proc **procp,
> vm2 = vmspace_fork(p1->p_vmspace, &mem_charged);
> if (vm2 == NULL) {
> error = ENOMEM;
> - goto fail1;
> + goto fail2;
> }
> if (!swap_reserve(mem_charged)) {
> /*
> @@ -861,7 +858,7 @@ fork1(struct thread *td, int flags, int pages, struct
> proc **procp,
>*/
> swap_reserve_force(mem_charged);
> error = ENOMEM;
> - goto fail1;
> + goto fail2;
> }
> } else
> vm2 = NULL;
> @@ -870,7 +867,7 @@ fork1(struct thread *td, int flags, int pages, struct
> proc **procp,
>* XXX: This is ugly; when we copy resource usage, we need to bump
>* per-cred resource counters.
>*/
> - proc_set_cred(newproc, p1->p_ucred);
> + proc_set_cred(newproc, crhold(td->td_ucred));
>
> /*
>* Initialize resource accounting for the child process.
> @@ -946,6 +943,8 @@ fail:
> #endif
> racct_proc_exit(newproc);
> fail1:
> + crfree(proc_set_cred(newproc, NULL));
> +fail2:
> if (vm2 != NULL)
> vmspace_free(vm2);
> uma_zfree(proc_zone, newproc);
> --
> 2.3.2
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
[PATCH 1/3] fork: assign refed credentials earlier
From: Mateusz Guzik
Prior to this change the kernel would take p1's credentials and assign
them tempororarily to p2. But p1 could change credentials at that time
and in effect give us a use-after-free.
---
sys/kern/kern_fork.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index ae86fe1..15833fd 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -410,9 +410,6 @@ do_fork(struct thread *td, int flags, struct proc *p2,
struct thread *td2,
bzero(&p2->p_startzero,
__rangeof(struct proc, p_startzero, p_endzero));
- crhold(td->td_ucred);
- proc_set_cred(p2, td->td_ucred);
-
/* Tell the prison that we exist. */
prison_proc_hold(p2->p_ucred->cr_prison);
@@ -832,7 +829,7 @@ fork1(struct thread *td, int flags, int pages, struct proc
**procp,
td2 = thread_alloc(pages);
if (td2 == NULL) {
error = ENOMEM;
- goto fail1;
+ goto fail2;
}
proc_linkup(newproc, td2);
} else {
@@ -841,7 +838,7 @@ fork1(struct thread *td, int flags, int pages, struct proc
**procp,
vm_thread_dispose(td2);
if (!thread_alloc_stack(td2, pages)) {
error = ENOMEM;
- goto fail1;
+ goto fail2;
}
}
}
@@ -850,7 +847,7 @@ fork1(struct thread *td, int flags, int pages, struct proc
**procp,
vm2 = vmspace_fork(p1->p_vmspace, &mem_charged);
if (vm2 == NULL) {
error = ENOMEM;
- goto fail1;
+ goto fail2;
}
if (!swap_reserve(mem_charged)) {
/*
@@ -861,7 +858,7 @@ fork1(struct thread *td, int flags, int pages, struct proc
**procp,
*/
swap_reserve_force(mem_charged);
error = ENOMEM;
- goto fail1;
+ goto fail2;
}
} else
vm2 = NULL;
@@ -870,7 +867,7 @@ fork1(struct thread *td, int flags, int pages, struct proc
**procp,
* XXX: This is ugly; when we copy resource usage, we need to bump
* per-cred resource counters.
*/
- proc_set_cred(newproc, p1->p_ucred);
+ proc_set_cred(newproc, crhold(td->td_ucred));
/*
* Initialize resource accounting for the child process.
@@ -946,6 +943,8 @@ fail:
#endif
racct_proc_exit(newproc);
fail1:
+ crfree(proc_set_cred(newproc, NULL));
+fail2:
if (vm2 != NULL)
vmspace_free(vm2);
uma_zfree(proc_zone, newproc);
--
2.3.2
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
