[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 6 Jul 2011 20:57:33 +0400
From: Vasiliy Kulikov <segoon@...nwall.com>
To: kernel-hardening@...ts.openwall.com
Cc: akpm@...ux-foundation.org, daniel.lezcano@...e.fr,
ebiederm@...ssion.com, mingo@...e.hu, oleg@...hat.com,
rdunlap@...otime.net, tj@...nel.org
Subject: Re: Re: [PATCH] shm: handle separate PID
namespaces case
Hi Serge,
On Wed, Jul 06, 2011 at 11:31 -0500, Serge Hallyn wrote:
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index ab3385a..bf46636 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -74,6 +74,7 @@ void shm_init_ns(struct ipc_namespace *ns)
> > ns->shm_ctlmax = SHMMAX;
> > ns->shm_ctlall = SHMALL;
> > ns->shm_ctlmni = SHMMNI;
> > + ns->shm_rmid_forced = 1;
>
> Given the description in Documentation/sysctl/kernel.txt, shouldn't
> this default to 0?
This is a change for testing purposes only, by Andrew:
http://www.openwall.com/lists/kernel-hardening/2011/06/29/7
> > /*
> > + * shm_may_destroy - identifies whether shm segment should be destroyed now
> > + *
> > + * Returns true if and only if there are no active users of the segment and
> > + * one of the following is true:
> > + *
> > + * 1) shmctl(id, IPC_RMID, NULL) was called for this shp
> > + *
> > + * 2) sysctl kernel.shm_rmid_forced is set to 1.
> > + */
> > +static bool shm_may_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
>
> 'may' usually implies a permission check. Would this be better named
> 'shm_should_destroy()'?
Looks right.
> > +/* Called with ns->shm_ids(ns).rw_mutex locked */
> > +static int shm_try_destroy_current(int id, void *p, void *data)
> > +{
> > + struct ipc_namespace *ns = data;
> > + struct kern_ipc_perm *ipcp = p;
> > + struct shmid_kernel *shp = container_of(ipcp, struct shmid_kernel, shm_perm);
> > +
> > + if (shp->shm_creator != current)
> > + return 0;
> > +
> > + /*
> > + * Mark it as orphaned to destroy the segment when
> > + * kernel.shm_rmid_forced is changed.
> > + * It is noop if the following shm_may_destroy() returns true.
> > + */
> > + shp->shm_creator = NULL;
> > +
> > + /*
> > + * Don't even try to destroy it. If shm_rmid_forced=0 and IPC_RMID
> > + * is not set, it shouldn't be deleted here.
> > + */
> > + if (!ns->shm_rmid_forced)
> > + return 0;
> > +
> > + if (shm_may_destroy(ns, shp)) {
>
> This seems redundant. Would it be better to just make this
>
> if (shp->shm_nattch == 0) {
>
> here as we already know ns->shm_rmid_forced == 1?
As this check doesn't cost much (shm_may_destroy() even may be inlined),
I want to leave the code here more readable.
> > + shm_lock_by_ptr(shp);
> > + shm_destroy(ns, shp);
>
> Wish there were a clean way to document that the locks will be
> released by shm_destroy().
Isn't the current comment sufficient?
/*
* shm_destroy - free the struct shmid_kernel
*
* @ns: namespace
* @shp: struct to free
*
* It has to be called with shp and shm_ids.rw_mutex (writer) locked,
* but returns with shp unlocked and freed.
*/
> > +void shm_destroy_orphaned(struct ipc_namespace *ns)
> > +{
> > + down_write(&shm_ids(ns).rw_mutex);
> > + if (&shm_ids(ns).in_use)
> > + idr_for_each(&shm_ids(ns).ipcs_idr, &shm_try_destroy_orphaned, ns);
> > + up_write(&shm_ids(ns).rw_mutex);
>
> Hm, is this going to cause contention when killing a lot of tasks?
The default limit is 4096 segments, IMO too few to cause something
nasty.
> > +}
> > +
> > +
> > +void exit_shm(struct task_struct *task)
> > +{
> > + struct ipc_namespace *ns = task->nsproxy->ipc_ns;
> > +
> > + /* Destroy all already created segments, but not mapped yet */
> > + down_write(&shm_ids(ns).rw_mutex);
> > + if (&shm_ids(ns).in_use)
> > + idr_for_each(&shm_ids(ns).ipcs_idr, &shm_try_destroy_current, ns);
> > + up_write(&shm_ids(ns).rw_mutex);
>
> Having exit_shm() call shm_destroy_orphaned(task->nsproxy->ipc_ns) seems
> more future-proof?
shm_destroy_orphaned() doesn't clear ->shm_creator. Logically it sovles
another problem - it is used ONLY to be consistent while changing
kernel.shm_rmid_forced (having orphans with shm_rmid_forced=1 is not
honest).
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
