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

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

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.

-serge