Quoting Stephen Smalley ([EMAIL PROTECTED]):
> On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> > Quoting David P. Quigley ([EMAIL PROTECTED]):
> > > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > > Quoting David P. Quigley ([EMAIL PROTECTED]):
> > > > >  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
-
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to