Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753989AbZKCNsn (ORCPT ); Tue, 3 Nov 2009 08:48:43 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750975AbZKCNsm (ORCPT ); Tue, 3 Nov 2009 08:48:42 -0500 Received: from e36.co.us.ibm.com ([32.97.110.154]:44466 "EHLO e36.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750751AbZKCNsl (ORCPT ); Tue, 3 Nov 2009 08:48:41 -0500 Date: Tue, 3 Nov 2009 07:48:23 -0600 From: "Serge E. Hallyn" To: "Eric W. Biederman" Cc: Greg Kroah-Hartman , Kay Sievers , Greg KH , linux-kernel@vger.kernel.org, Tejun Heo , Cornelia Huck , linux-fsdevel@vger.kernel.org, Eric Dumazet , Benjamin LaHaise , "Eric W. Biederman" Subject: Re: [PATCH 01/13] sysfs: Update sysfs_setxattr so it updates secdata under the sysfs_mutex Message-ID: <20091103134823.GB31981@us.ibm.com> References: <1257249429-12384-1-git-send-email-ebiederm@xmission.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1257249429-12384-1-git-send-email-ebiederm@xmission.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3144 Lines: 100 Quoting Eric W. Biederman (ebiederm@xmission.com): > From: Eric W. Biederman > > The sysfs_mutex is required to ensure updates are and will remain > atomic with respect to other inode iattr updates, that do not happen > through the filesystem. > > Signed-off-by: Eric W. Biederman > --- > fs/sysfs/inode.c | 41 +++++++++++++++++++++++++++++------------ > 1 files changed, 29 insertions(+), 12 deletions(-) > > diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c > index e28cecf..8a08250 100644 > --- a/fs/sysfs/inode.c > +++ b/fs/sysfs/inode.c > @@ -122,23 +122,39 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr) > return error; > } > > +static int sysfs_sd_setsecdata(struct sysfs_dirent *sd, void **secdata, u32 *secdata_len) > +{ > + struct sysfs_inode_attrs *iattrs; > + void *old_secdata; > + size_t old_secdata_len; > + > + iattrs = sd->s_iattr; > + if (!iattrs) > + iattrs = sysfs_init_inode_attrs(sd); > + if (!iattrs) > + return -ENOMEM; > + > + old_secdata = iattrs->ia_secdata; > + old_secdata_len = iattrs->ia_secdata_len; > + > + iattrs->ia_secdata = *secdata; > + iattrs->ia_secdata_len = *secdata_len; > + > + *secdata = old_secdata; > + *secdata_len = old_secdata_len; > + return 0; > +} > + > int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value, > size_t size, int flags) > { > struct sysfs_dirent *sd = dentry->d_fsdata; > - struct sysfs_inode_attrs *iattrs; > void *secdata; > int error; > u32 secdata_len = 0; > > if (!sd) > return -EINVAL; > - if (!sd->s_iattr) > - sd->s_iattr = sysfs_init_inode_attrs(sd); > - if (!sd->s_iattr) > - return -ENOMEM; > - > - iattrs = sd->s_iattr; > > if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) { > const char *suffix = name + XATTR_SECURITY_PREFIX_LEN; > @@ -150,12 +166,13 @@ int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value, > &secdata, &secdata_len); > if (error) > goto out; > - if (iattrs->ia_secdata) > - security_release_secctx(iattrs->ia_secdata, > - iattrs->ia_secdata_len); > - iattrs->ia_secdata = secdata; > - iattrs->ia_secdata_len = secdata_len; > > + mutex_lock(&sysfs_mutex); > + error = sysfs_sd_setsecdata(sd, &secdata, &secdata_len); You're ignoring the potential -ENOMEM return value here? Worse, if -ENOMEM, then secdata was never set, so you call security_release_secctx() on a random value left on the stack... > + mutex_unlock(&sysfs_mutex); > + > + if (secdata) > + security_release_secctx(secdata, secdata_len); > } else > return -EINVAL; > out: > -- > 1.6.5.2.143.g8cc62 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/