From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Serge E. Hallyn" Subject: Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer Date: Fri, 26 Oct 2007 10:35:30 -0500 Message-ID: <20071026153530.GD539@sergelap.austin.ibm.com> References: <1193079974.30930.2.camel@moss-terrapins.epoch.ncsc.mil> <1193080250.30930.6.camel@moss-terrapins.epoch.ncsc.mil> <20071026000210.GB2795@vino.hallyn.com> <1193410245.27034.29.camel@moss-terrapins.epoch.ncsc.mil> <20071026150225.GA539@sergelap.austin.ibm.com> <1193411065.11953.124.camel@moss-spartans.epoch.ncsc.mil> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "Serge E. Hallyn" , "David P. Quigley" , linux-security-module@vger.kernel.org, linux-fsdevel@vger.kernel.org, jmorris@namei.org To: Stephen Smalley Return-path: Content-Disposition: inline In-Reply-To: <1193411065.11953.124.camel@moss-spartans.epoch.ncsc.mil> Sender: linux-security-module-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org Quoting Stephen Smalley (sds@tycho.nsa.gov): > On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote: > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov): > > > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote: > > > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov): > > > > > 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