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
