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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> Quoting David P. Quigley (dpquigl@xxxxxxxxxxxxx):
> > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > Quoting David P. Quigley (dpquigl@xxxxxxxxxxxxx):
> > > >  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.

-- 
Stephen Smalley
National Security Agency

-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux