linux-kernel - Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for 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] [day] [month] [year] [list]

Message-ID: <20080414214417.GA1761@sergelap.austin.ibm.com>
Date:	Mon, 14 Apr 2008 16:44:17 -0500
From:	"Serge E. Hallyn" <serue@...ibm.com>
To:	"Serge E. Hallyn" <serue@...ibm.com>
Cc:	Manfred Spraul <manfred@...orfullife.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Pavel Emelyanov <xemul@...nvz.org>,
	Sukadev Bhattiprolu <sukadev@...ibm.com>,
	Pierre PEIFFER <pierre.peiffer@...l.net>,
	Michael Kerrisk <mtk.manpages@...glemail.com>
Subject: Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for
	CLONE_SYSVSEM

Quoting Serge E. Hallyn (serue@...ibm.com):
> Quoting Manfred Spraul (manfred@...orfullife.com):
> > Andrew Morton wrote:
> >> On Sun, 13 Apr 2008 10:04:17 +0200 Manfred Spraul 
> >> <manfred@...orfullife.com> wrote:
> >>
> >>   
> >>> sys_unshare(CLONE_NEWIPC) doesn't handle the undo lists properly, this 
> >>> can
> >>> cause a kernel memory corruption. CLONE_NEWIPC must detach from the 
> >>> existing
> >>> undo lists.
> >>> Fix, part 1: add support for sys_unshare(CLONE_SYSVSEM)
> >>>
> >>>     
> >>
> >> Is this a non-back-compatible change?
> >>
> >>   
> > It adds a new feature - previously sys_unshare(CLONE_SYSVSEM) returned 
> > -EINVAL.
> >
> >>> Signed-off-by: Manfred Spraul <manfred@...orfullife.com>
> >>> ---
> >>>  ipc/sem.c     |    1 +
> >>>  kernel/fork.c |   18 ++++++++++++++----
> >>>  2 files changed, 15 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/ipc/sem.c b/ipc/sem.c
> >>> index 0b45a4d..35841bd 100644
> >>> --- a/ipc/sem.c
> >>> +++ b/ipc/sem.c
> >>> @@ -1298,6 +1298,7 @@ void exit_sem(struct task_struct *tsk)
> >>>  	undo_list = tsk->sysvsem.undo_list;
> >>>  	if (!undo_list)
> >>>  		return;
> >>> +	tsk->sysvsem.undo_list = NULL;
> >>>   	if (!atomic_dec_and_test(&undo_list->refcnt))
> >>>  		return;
> >>> diff --git a/kernel/fork.c b/kernel/fork.c
> >>> index 9c042f9..7f242b0 100644
> >>> --- a/kernel/fork.c
> >>> +++ b/kernel/fork.c
> >>> @@ -1675,13 +1675,17 @@ static int unshare_fd(unsigned long 
> >>> unshare_flags, struct files_struct **new_fdp
> >>>  }
> >>>   /*
> >>> - * Unsharing of semundo for tasks created with CLONE_SYSVSEM is not
> >>> - * supported yet
> >>> + * Unsharing of semundo for tasks created with CLONE_SYSVSEM doesn't 
> >>> require
> >>> + * any allocations: it means that the task leaves the existing undo 
> >>> lists,
> >>> + * just like sys_exit(). The new undo lists are allocated on demand in 
> >>> the
> >>> + * ipc syscalls.
> >>> + * new_ulistp is set to a non-NULL value, the caller expects that on 
> >>> success.
> >>>   */
> >>>  static int unshare_semundo(unsigned long unshare_flags, struct 
> >>> sem_undo_list **new_ulistp)
> >>>  {
> >>> -	if (unshare_flags & CLONE_SYSVSEM)
> >>> -		return -EINVAL;
> >>> +	if (unshare_flags & CLONE_SYSVSEM) {
> >>> +		*new_ulistp = (void*)1;
> >>> +	}
> >>>     
> >>
> >> And can we do anything nicer than this?
> >>
> >>   
> > Attached is an alternative. If you prefer it, I'll send another patch set.
> 
> FWIW I definately far far prefer this version  :)
> 
> As for 'maintainers', in the end wrt this code I'd defer to any two of
> Pavel, Nadia, and Pierre, each of who I've seen do a great deal of
> digging around this code.
> 
> (I think I saw some set of these go into -mm so I guess I'll just grab
> mmotm and test with that a bit later today)
> 
> thanks,
> -serge

Of course if we do go this route, then the unshare(2) manpage will need
an update (so Michael Kerrisk cc:d).

thanks,
-serge
--
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/