Re: [PATCH] per-user signal pending and message queue limits

[Chris Wright <chrisw@osdl.org>]

* Marcelo Tosatti (marcelo.tosatti@cyclades.com) wrote:
> > This BUG() is too easy to trigger, e.g. user creates mqueue, logs out,
> > root comes by later and cleans up...BUG().  Simply caching user directly
> > eliminates this altogether.
> 
> And with user_struct->__count you deal with that, yes?

Yes, exactly.

> Caching the user_struct directly is indeed much nicer.
> 
> > Some inconsistent use of p and current below.
> > 
> > >  	struct msg_msg **msgs = NULL;
> > >  	struct mq_attr attr;
> > >  	int ret;
> > > @@ -553,15 +578,26 @@
> > >  					attr.mq_msgsize > msgsize_max)
> > >  				return ERR_PTR(-EINVAL);
> > >  		}
> > > +	  	if(p->user->msg_queues+ ((attr.mq_maxmsg * sizeof(struct msg_msg *)
> > > +				+ (attr.mq_maxmsg * attr.mq_msgsize)))
> > > +			  >= p->rlim[RLIMIT_MSGQUEUE].rlim_cur)
> > 
> > Hrm, this thing can overflow.  Seems like the hard maxes should be
> > smaller.  As it stands, looks like the hard max mq_msgsize that root
> > could setup is INT_MAX.
> 
> Eek. Just decreasing max mq_msgsize to something _much_ smaller is ok, isnt it? 
> Like, say, 64MB.

I think so.  I've no idea what a good max is, but last night I simply
added some overflow detection to handle this.

> > > +			return ERR_PTR(-ENOMEM);
> > > +
> > >  		msgs = kmalloc(attr.mq_maxmsg * sizeof(*msgs), GFP_KERNEL);
> > >  		if (!msgs)
> > >  			return ERR_PTR(-ENOMEM);
> > > +
> > > +		spin_lock(&mq_lock);
> > > +		current->user->msg_queues += (attr.mq_maxmsg * sizeof(*msgs) +
> > > +					(attr.mq_maxmsg * attr.mq_msgsize));
> > > +		spin_unlock(&mq_lock);
> > 
> > This path means the user is penalized for the mq_attr sized accounting,
> > plus the default sized accounting which happens later in mqueue_get_inode().
> > It is removed below, but as mentioned above, this could incorrectly
> > cause mq_open() to fail.
> 
> OK!
> 
> > >  	} else {
> > >  		msgs = NULL;
> > >  	}
> > >  
> > >  	ret = vfs_create(dir->d_inode, dentry, mode, NULL);
> > >  	if (ret) {
> > > +		/* kfree(msgs): msgs can be NULL -mt */
> > >  		kfree(msgs);
> > >  		return ERR_PTR(ret);
> > >  	}
> > > @@ -572,8 +608,17 @@
> > >  	if (msgs) {
> > >  		info->attr.mq_maxmsg = attr.mq_maxmsg;
> > >  		info->attr.mq_msgsize = attr.mq_msgsize;
> > > +		spin_lock(&mq_lock);
> > > +		current->user->msg_queues -= (info->attr.mq_maxmsg 
> > > +						* sizeof (struct msg_msg *) +
> > > +						(info->attr.mq_maxmsg * 
> > > +						info->attr.mq_msgsize));
> > > +		if (current->user->msg_queues < 0)
> > > +			current->user->msg_queues = 0;	
> > 
> > Oops, I think the subtraction is slightly wrong here.  Should be before
> > the info->attr is updated, else you are actually carrying accounting for
> > the default size (minus the actually allocated size).  Should be
> > subtracting off the recently added default size.
> > 
> > New patch below (based on 2.6.6-rc3-bk).  Couple known issues are the
> > possible mq_bytes caluclation overflow (not yet fixed in this patch),
> > and setuid issue on signal side.  All other known issues have been
> > addressed.
> 
> The setuid issue on signal side is pretty harmless though. It only affects the
> users which are setuid() capable, and which do so with signal pending.

Yes.  It's part of why I left it for now.

> I see no other way to fix it than cache the user struct in "struct sigqueue". 
> Thats not good news for performance (not sure how bad it would matter).

My thoughts exactly.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net