Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

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

Quoting Stephen Smalley (sds@tycho.nsa.gov):
> On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > > >  static int task_alloc_security(struct task_struct *task)
> > > > > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > > > >   *
> > > > >   * Permission check is handled by selinux_inode_getxattr hook.
> > > > >   */
> > > > > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > > > > +static int selinux_inode_getsecurity(const struct inode *inode,
> > > > > +					const char *name,
> > > > > +					void **buffer)
> > > > >  {
> > > > > +	u32 size;
> > > > > +	int error;
> > > > >  	struct inode_security_struct *isec = inode->i_security;
> > > > >  
> > > > >  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > > > >  		return -EOPNOTSUPP;
> > > > >  
> > > > > -	return selinux_getsecurity(isec->sid, buffer, size);
> > > > > +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> > > > 
> > > > The only other downside I see here is that when the user just passes in
> > > > NULL for a buffer, security_sid_to_context() will still
> > > > kmalloc the buffer only to have it immediately freed by
> > > > xattr_getsecurity() through release_secctx().  I trust that isn't seen
> > > > as any major performance impact?
> > > 
> > > There is no way to avoid this in the SELinux case. SELinux doesn't store
> > > the sid to string mapping directly. Rather it takes the sid and then
> > > builds the string from fields in the related structure. So regardless
> > > this data is being allocated internally. The only issue I potentially
> > > see is that if someone passes in null expecting just to get the length
> > > we are actually returning a value. However we are changing the semantics
> > > of the function so the old semantics are no longer valid.
> > 
> > Hmm?  Which semantics are no longer valid?
> > 
> > You're changing the semantincs of the in-kernel API, but userspace can
> > still send in NULL to query the length of the buffer needed.  So if
> > userspace does two getxattrs, one to get the length, then another to get
> > the value, selinux will be kmallocing twice.
> > 
> > For a file manager doing a listing on a huge directory and wanting to
> > list the selinux type, i could see that being a performance issue.  Of
> > course they could get around that by sending in a 'reasonably large'
> > buffer for a first try.
> 
> That's what current userland does. libselinux always tries with an
> initial buffer first (and usually succeeds), thereby avoiding the second
> call to getxattr in the common case.

Ok - I figured for doing thousands of these in one directory listing
that could waste quite a bit of memory, but since (as i check) selinux
has always done a kmalloc for every getsecurity call, I guess it's a
fair tradeoff

thanks,
-serge