Re: [PATCH 1/1] cr: lsm: restore LSM contexts for ipc objects

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting Stephen Smalley (sds@epoch.ncsc.mil):
> On Tue, 2009-06-23 at 14:57 -0500, Serge E. Hallyn wrote:
> > Quoting Serge E. Hallyn (serue@us.ibm.com):
> > > Quoting Stephen Smalley (sds@epoch.ncsc.mil):
> > > > On Fri, 2009-06-19 at 20:32 -0500, Serge E. Hallyn wrote:
> > > > > diff --git a/ipc/checkpoint_msg.c b/ipc/checkpoint_msg.c
> > > > > index 51385b0..ca55339 100644
> > > > > --- a/ipc/checkpoint_msg.c
> > > > > +++ b/ipc/checkpoint_msg.c
> > > > <snip>
> > > > > @@ -175,11 +183,26 @@ static int load_ipc_msg_hdr(struct ckpt_ctx *ctx,
> > > > >  			    struct msg_queue *msq)
> > > > >  {
> > > > >  	int ret = 0;
> > > > > +	int secid = 0;
> > > > >  
> > > > >  	ret = restore_load_ipc_perms(&h->perms, &msq->q_perm);
> > > > >  	if (ret < 0)
> > > > >  		return ret;
> > > > >  
> > > > > +	if (h->perms.secref) {
> > > > > +		struct sec_store *s;
> > > > > +		s = ckpt_obj_fetch(ctx, h->perms.secref, CKPT_OBJ_SECURITY);
> > > > > +		if (IS_ERR(s))
> > > > > +			return PTR_ERR(s);
> > > > > +		secid = s->secid;
> > > > > +	}
> > > > > +	ret = security_msg_queue_alloc(msq);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +	ret = security_msg_queue_restore(msq, secid);
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > 
> > > > I don't think you want to call security_msg_queue_alloc() here, as that
> > > > both allocates the security struct and performs the create check.  So I
> > > > would just call the _restore() function, and let it internally call
> > > > ipc_alloc_security() to allocate the struct but then apply its own
> > > > distinct restore check.  Likewise for the rest of them.
> > > 
> > > Ok, will change that.
> > 
> > Hmm, but that means that if there is some new LSM which allocates memory
> > in security_msg_queue_alloc(), but which does not define
> > security_msg_queue_restore() (for some stupid reason), it'll end up
> > causing a bug.
> > 
> > It's something we can certainly catch through code review, but do we
> > want to set such a scenario up at all?
> > 
> > Speaking just for SELinux, the security_msg_queue_alloc() hook would
> > return -EPERM only if the task calling sys_restart() wasn't allowed
> > to create a msg queue with its own type, right?  Is that something
> > which is often disallowed?
> 
> Certainly some program domains lack permission to create ipc objects.
> The ipc _alloc hooks are unusual in that they combine both allocation
> and create checking unlike the rest of the object alloc hooks.  I think
> that was discussed at the time, but people didn't want two different
> hook calls at the same call site.

Oh, no.  I wasn't thinking right.

The objects are actually restored through calls to do_shmget() etc,
so that security_xyz_alloc() already gets called.

So I think we'll just leave it as is right now, acknowledging that
it might become problematic if we want to confine a restart_t domain
to be able to restore but not alloc any ipcs.  The actual ramifications
of that still somewhat escape me, but I do prefer having the common
helpers used whenever possible to recreate objects.

thanks,
-serge