Quoting David P. Quigley (dpquigl@xxxxxxxxxxxxx):
> 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.
> >
>
> 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.
Ah, thanks, you reminded me of what I was trying to point out.
SMACK won't do allocations so it's ok. SELinux will do allocations
in any case so it's ok. So in terms of current users it's fine, so I
don't want to complaint too loudly.
But the now-generic xattr_getsecurity() call passes in 'buffer' from its
stack, with no indication to the LSM of whether userspace passed in NULL
or a buffer. So if there *were* an lsm which had to allocate space to
return data, but didn't want to do so when the user just asked for the
length of the data, then that LSM would be out of luck.
So would you object to passing in a boolean telling the LSM whether the
user had passed in a buffer in which to return data or not?
thanks,
-serge
> 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 majordomo@xxxxxxxxxxxxxxx
> > 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 majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
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
