On Fri, 2007-10-26 at 11:36 -0500, Serge E. Hallyn wrote:
> 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?
I don't see why we couldn't add a bool. I am just wondering are there
really use-cases where the developer is going to need this though.
Unfortunately I'm not all knowing so I can't foresee what us "Insane"
security people will do so I think it's a reasonable addition. I'll
rebase the patch and make these changes and hopefully have a new
revision to the list before the end of the day.
Dave
>
> 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
