| Submitter | Eric W. Biederman |
|---|---|
| Date | 2009-11-03 11:56:57 |
| Message ID | <1257249429-12384-1-git-send-email-ebiederm@xmission.com> |
| Download | mbox | patch |
| Permalink | /patch/57291/ |
| State | New |
| Headers | show |
Comments
Quoting Eric W. Biederman (ebiederm@xmission.com): > From: Eric W. Biederman <ebiederm@aristanetworks.com> > > 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 <ebiederm@aristanetworks.com> > --- > 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/
"Serge E. Hallyn" <serue@us.ibm.com> writes: > Quoting Eric W. Biederman (ebiederm@xmission.com): >> From: Eric W. Biederman <ebiederm@aristanetworks.com> >> >> 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 <ebiederm@aristanetworks.com> >> --- >> 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... No. It is more elegant than that. If sysfs_sd_setsecdata fails secdata holds the freshly allocated secdata value. If sysfs_sd_setsecdata succeeds secdata holds the old value of secdata. Eric -- 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/
Quoting Eric W. Biederman (ebiederm@xmission.com): > "Serge E. Hallyn" <serue@us.ibm.com> writes: > > > Quoting Eric W. Biederman (ebiederm@xmission.com): > >> From: Eric W. Biederman <ebiederm@aristanetworks.com> > >> > >> 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 <ebiederm@aristanetworks.com> > >> --- > >> 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... > > No. It is more elegant than that. > If sysfs_sd_setsecdata fails secdata holds the freshly allocated secdata value. > If sysfs_sd_setsecdata succeeds secdata holds the old value of secdata. Gah, sorry. Acked-by: Serge Hallyn <serue@us.ibm.com> (as an aside, I can't see any reason to not just return if strncmp(name, XATTR_SECURITY_PREFIX,) above to avoid a level of indent and needless gotos) thanks, -serge -- 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/
"Serge E. Hallyn" <serue@us.ibm.com> writes: > > Gah, sorry. No prob. > Acked-by: Serge Hallyn <serue@us.ibm.com> > > (as an aside, I can't see any reason to not just return if strncmp(name, > XATTR_SECURITY_PREFIX,) above to avoid a level of indent and needless gotos) Sounds like it is worth a patch. For now I'm concentrating on getting this patchset merged, and extraneous cleanups have a way of dragging the process out beyond what time I have to give. Eric -- 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/
Eric W. Biederman wrote: > From: Eric W. Biederman <ebiederm@aristanetworks.com> > > 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 <ebiederm@aristanetworks.com> Acked-by: Tejun Heo <tj@kernel.org>
Patch
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); + mutex_unlock(&sysfs_mutex); + + if (secdata) + security_release_secctx(secdata, secdata_len); } else return -EINVAL; out: