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.
>
Ok lets start this line of thought over again since it has been a while
since I wrote the patches and got almost no sleep last night.
Your concerns are that we are double allocating buffers one of which we
are just going to immediately free after a copy. So inside the SELinux
helper function there was what I saw as generic code for handling
xattrs. This can be seen in the new function xattr_getsecurity which use
to be internal to SELinux (selinux_getsecurity). What we are doing is
grabbing the string which internally is being allocated anyway and if
our buffer passed in for the copy is null we just goto out returning the
length and freeing the buffer. So here is our standard null handling
that we had before. In LSMs where there is no internal allocation to
handle the getsecurity call this should introduce almost no overhead.
For example in Casey's latest SMACK patch he has a table of the label
strings and he can pass a pointer directly into that table back via the
security_inode_getsecurity hook.
In addition to this completes the lifecycle management that
security_release_secctx attempts to perform. It doesn't seem right that
if we require security_release_secctx to free the data we obtained from
security_inode_getsecurity that the data buffer should be allocated
outside of security_inode_getsecurity.
I hope this clears up any questions/concerns.
> -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
-
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
