linux-kernel - Re: [PATCH 1/3] pidns: fix vfork() after unshare(CLONE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 22 Aug 2013 20:22:13 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Brad Spengler <spender@...ecurity.net>,
	Colin Walters <walters@...hat.com>,
	Pavel Emelyanov <xemul@...allels.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] pidns: fix vfork() after unshare(CLONE_NEWPID)

On 08/22, Andy Lutomirski wrote:
>
> On Thu, Aug 22, 2013 at 10:09 AM, Oleg Nesterov <oleg@...hat.com> wrote:
> > 8382fcac "pidns: Outlaw thread creation after unshare(CLONE_NEWPID)"
> > nacks CLONE_VM if the forking process unshared pid_ns, this obviously
> > breaks vfork:
> >
> >         int main(void)
> >         {
> >                 assert(unshare(CLONE_NEWUSER | CLONE_NEWPID) == 0);
> >                 assert(vfork() >= 0);
> >                 _exit(0);
> >                 return 0;
> >         }
> >
> > fails without this patch.
> >
> > Change this check to use CLONE_SIGHAND instead. This also forbids
> > CLONE_THREAD automatically, and this is what the comment implies.
> >
> > We could probably even drop CLONE_SIGHAND and use CLONE_THREAD,
> > but it would be safer to not do this. The current check denies
> > CLONE_SIGHAND implicitely and there is no reason to change this.
>
> Acked-by: Andy Lutomirski <luto@...capital.net>

Thanks Andy for taking a look.

> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -1173,10 +1173,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,
> >                 return ERR_PTR(-EINVAL);
> >
> >         /*
> > -        * If the new process will be in a different pid namespace
> > -        * don't allow the creation of threads.
> > +        * If the new process will be in a different pid namespace don't
> > +        * allow the creation of threads, or share the signal handlers.
>
> ...how about "If the new process will be in a different pid namespace,
> don't allow it to share a thread group or signal handlers with the
> parent"?

Agreed... But in this case I do not how 3/3 should explain CLONE_PARENT,
it adds "or share the parent" into this comment.

OK, I'll wait for other comments, then try to update this comment somehow
and send v2.

Perhaps "... with the forking task" would be fine?

Thanks.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/