From: Jeff L. <jl...@re...> - 2007-08-06 13:54:18
|
Separate the handling of the local ia_valid bitmask from the one in attr->ia_valid. This allows us to hand off the actual handling of the ATTR_KILL_* flags to the .setattr i_op when one is defined. notify_change still needs to process those flags for the local ia_valid variable, since it uses that to decide whether to return early, and to pass a (hopefully) appropriate bitmask to fsnotify_change. Signed-off-by: Jeff Layton <jl...@re...> --- fs/attr.c | 54 +++++++++++++++++++++++++++++++++------------------ include/linux/fs.h | 1 + 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/fs/attr.c b/fs/attr.c index f8dfc22..47015e0 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -100,15 +100,39 @@ int inode_setattr(struct inode * inode, struct iattr * attr) } EXPORT_SYMBOL(inode_setattr); +void attr_kill_to_mode(struct inode *inode, struct iattr *attr) +{ + if (attr->ia_valid & ATTR_KILL_SUID) { + attr->ia_valid &= ~ATTR_KILL_SUID; + if (inode->i_mode & S_ISUID) { + if (!(attr->ia_valid & ATTR_MODE)) { + attr->ia_valid |= ATTR_MODE; + attr->ia_mode = inode->i_mode; + } + attr->ia_mode &= ~S_ISUID; + } + } + if (attr->ia_valid & ATTR_KILL_SGID) { + attr->ia_valid &= ~ATTR_KILL_SGID; + if ((inode->i_mode & (S_ISGID | S_IXGRP)) == + (S_ISGID | S_IXGRP)) { + if (!(attr->ia_valid & ATTR_MODE)) { + attr->ia_valid |= ATTR_MODE; + attr->ia_mode = inode->i_mode; + } + attr->ia_mode &= ~S_ISGID; + } + } +} +EXPORT_SYMBOL(attr_kill_to_mode); + int notify_change(struct dentry * dentry, struct iattr * attr) { struct inode *inode = dentry->d_inode; - mode_t mode; int error; struct timespec now; unsigned int ia_valid = attr->ia_valid; - mode = inode->i_mode; now = current_fs_time(inode->i_sb); attr->ia_ctime = now; @@ -117,26 +141,17 @@ int notify_change(struct dentry * dentry, struct iattr * attr) if (!(ia_valid & ATTR_MTIME_SET)) attr->ia_mtime = now; if (ia_valid & ATTR_KILL_SUID) { - attr->ia_valid &= ~ATTR_KILL_SUID; - if (mode & S_ISUID) { - if (!(ia_valid & ATTR_MODE)) { - ia_valid = attr->ia_valid |= ATTR_MODE; - attr->ia_mode = inode->i_mode; - } - attr->ia_mode &= ~S_ISUID; - } + ia_valid &= ~ATTR_KILL_SUID; + if (inode->i_mode & S_ISUID) + ia_valid |= ATTR_MODE; } if (ia_valid & ATTR_KILL_SGID) { - attr->ia_valid &= ~ ATTR_KILL_SGID; - if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) { - if (!(ia_valid & ATTR_MODE)) { - ia_valid = attr->ia_valid |= ATTR_MODE; - attr->ia_mode = inode->i_mode; - } - attr->ia_mode &= ~S_ISGID; - } + ia_valid &= ~ATTR_KILL_SGID; + if ((inode->i_mode & (S_ISGID | S_IXGRP)) == + (S_ISGID | S_IXGRP)) + ia_valid |= ATTR_MODE; } - if (!attr->ia_valid) + if (!ia_valid) return 0; if (ia_valid & ATTR_SIZE) @@ -147,6 +162,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr) if (!error) error = inode->i_op->setattr(dentry, attr); } else { + attr_kill_to_mode(inode, attr); error = inode_change_ok(inode, attr); if (!error) error = security_inode_setattr(dentry, attr); diff --git a/include/linux/fs.h b/include/linux/fs.h index d33bead..f617b23 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1561,6 +1561,7 @@ extern int do_remount_sb(struct super_block *sb, int flags, #ifdef CONFIG_BLOCK extern sector_t bmap(struct inode *, sector_t); #endif +extern void attr_kill_to_mode(struct inode *inode, struct iattr *attr); extern int notify_change(struct dentry *, struct iattr *); extern int permission(struct inode *, int, struct nameidata *); extern int generic_permission(struct inode *, int, -- 1.5.2.2 |
From: Miklos S. <mi...@sz...> - 2007-08-06 17:44:56
|
> Separate the handling of the local ia_valid bitmask from the one in > attr->ia_valid. This allows us to hand off the actual handling of the > ATTR_KILL_* flags to the .setattr i_op when one is defined. > > notify_change still needs to process those flags for the local ia_valid > variable, since it uses that to decide whether to return early, and to pass > a (hopefully) appropriate bitmask to fsnotify_change. I agree with this change and fuse will make use of it as well. Maybe instead of unconditionally moving attr_kill_to_mode() inside ->setattr() it could be made conditional based on an inode flag similarly to S_NOCMTIME. Advantages: - no need to modify a lot of in-tree filesystems - no silent breakage of out-of-tree fs Actually I think the new flag would be used by exacly the same filesystems as S_NOCMTIME, so maybe it would make sense to rename S_NOCMTIME to something more generic (S_NOATTRUPDATE or whatever) and use that. But that could still break out-of-tree fs, so a separate flag is probably better. Miklos |
From: Jeff L. <jl...@re...> - 2007-08-06 18:13:54
|
On Mon, 06 Aug 2007 19:43:46 +0200 Miklos Szeredi <mi...@sz...> wrote: > > Separate the handling of the local ia_valid bitmask from the one in > > attr->ia_valid. This allows us to hand off the actual handling of the > > ATTR_KILL_* flags to the .setattr i_op when one is defined. > > > > notify_change still needs to process those flags for the local ia_valid > > variable, since it uses that to decide whether to return early, and to pass > > a (hopefully) appropriate bitmask to fsnotify_change. > > I agree with this change and fuse will make use of it as well. > > Maybe instead of unconditionally moving attr_kill_to_mode() inside > ->setattr() it could be made conditional based on an inode flag > similarly to S_NOCMTIME. Advantages: > > - no need to modify a lot of in-tree filesystems > - no silent breakage of out-of-tree fs > > Actually I think the new flag would be used by exacly the same > filesystems as S_NOCMTIME, so maybe it would make sense to rename > S_NOCMTIME to something more generic (S_NOATTRUPDATE or whatever) and > use that. > > But that could still break out-of-tree fs, so a separate flag is > probably better. > In the past I've been told that adding new flags is something of a "last resort". Since it's not strictly necessary to fix this then it may be best to avoid that. That said, if the concensus is that we need a transition mechanism, then I'd be open to such a suggestion. -- Jeff Layton <jl...@re...> |
From: Miklos S. <mi...@sz...> - 2007-08-06 18:29:02
|
> > I agree with this change and fuse will make use of it as well. > > > > Maybe instead of unconditionally moving attr_kill_to_mode() inside > > ->setattr() it could be made conditional based on an inode flag > > similarly to S_NOCMTIME. Advantages: > > > > - no need to modify a lot of in-tree filesystems > > - no silent breakage of out-of-tree fs > > > > Actually I think the new flag would be used by exacly the same > > filesystems as S_NOCMTIME, so maybe it would make sense to rename > > S_NOCMTIME to something more generic (S_NOATTRUPDATE or whatever) and > > use that. > > > > But that could still break out-of-tree fs, so a separate flag is > > probably better. > > > > In the past I've been told that adding new flags is something of a > "last resort". Since it's not strictly necessary to fix this then > it may be best to avoid that. > > That said, if the concensus is that we need a transition mechanism, > then I'd be open to such a suggestion. I think there's really no other choice here. Your patch is changing the API in a very unsafe way, since there will be no error or warning on an unconverted fs. And that could lead to security holes. If we would rename the setattr method to setattr_new as well as changing it's behavior, that would be fine. But I guess we do not want to do that. Miklos |
From: Trond M. <tro...@fy...> - 2007-08-06 19:05:10
|
On Mon, 2007-08-06 at 20:28 +0200, Miklos Szeredi wrote: > Your patch is changing the API in a very unsafe way, since there will > be no error or warning on an unconverted fs. And that could lead to > security holes. > > If we would rename the setattr method to setattr_new as well as > changing it's behavior, that would be fine. But I guess we do not > want to do that. Which "unconverted fses"? If we're talking out of tree stuff, then too bad: it is _their_ responsibility to keep up with kernel changes. Trond |
From: Miklos S. <mi...@sz...> - 2007-08-06 19:37:46
|
> > Your patch is changing the API in a very unsafe way, since there will > > be no error or warning on an unconverted fs. And that could lead to > > security holes. > > > > If we would rename the setattr method to setattr_new as well as > > changing it's behavior, that would be fine. But I guess we do not > > want to do that. > > Which "unconverted fses"? If we're talking out of tree stuff, then too > bad: it is _their_ responsibility to keep up with kernel changes. It is usually a good idea to not change the semantics of an API in a backward incompatible way without changing the syntax as well. This is true regardless of whether we care about out-of-tree code or not (and we should care to some degree). And especially true if the change in question is security sensitive. Miklos |
From: Trond M. <tro...@fy...> - 2007-08-06 21:24:02
|
On Mon, 2007-08-06 at 21:37 +0200, Miklos Szeredi wrote: > > > Your patch is changing the API in a very unsafe way, since there will > > > be no error or warning on an unconverted fs. And that could lead to > > > security holes. > > > > > > If we would rename the setattr method to setattr_new as well as > > > changing it's behavior, that would be fine. But I guess we do not > > > want to do that. > > > > Which "unconverted fses"? If we're talking out of tree stuff, then too > > bad: it is _their_ responsibility to keep up with kernel changes. > > It is usually a good idea to not change the semantics of an API in a > backward incompatible way without changing the syntax as well. We're taking two setattr flags ATTR_KILL_SGID, and ATTR_KILL_SUID which have existed for several years in the VFS, and making them visible to the filesystems. Out-of-tree filesystems that care can check for them in a completely backward compatible way: you don't even need to add a #define. > This is true regardless of whether we care about out-of-tree code or > not (and we should care to some degree). And especially true if the > change in question is security sensitive. It is not true "regardless": the in-tree code is being converted. Out-of-tree code is the only "problem" here, and their only problem is that they may have to add support for the new flags if they also support suid/sgid mode bits. Are you advocating reserving a new filesystem bit every time we need to add an attribute flag? Trond |
From: Christoph H. <hc...@in...> - 2007-08-07 20:51:57
|
> +void attr_kill_to_mode(struct inode *inode, struct iattr *attr) This function badly needs a kerneldoc description. Also I can't say I like the name a lot, but without a clearly better idea I should probably not complain :) We should at least add a generic_ prefix to indicate it's a generic helper valid for most filesystem (and the kerneldoc comment can explain the details) |
From: Jeff L. <jl...@re...> - 2007-08-07 22:22:56
|
On Tue, 7 Aug 2007 21:51:49 +0100 Christoph Hellwig <hc...@in...> wrote: > > +void attr_kill_to_mode(struct inode *inode, struct iattr *attr) > > This function badly needs a kerneldoc description. Also I can't say > I like the name a lot, but without a clearly better idea I should > probably not complain :) > Thanks for the comments. I'm not thrilled with the name either, but kill_suid and *remove_suid were already taken, and I really didn't want to name this something too similar since there are already so many similarly named functions that don't do the same thing. I'm definitely open to suggestions for something different. > We should at least add a generic_ prefix to indicate it's a generic > helper valid for most filesystem (and the kerneldoc comment can explain > the details) > Both good suggestions. I'll plan to incorporate them in the next respin of the set. Thanks, -- Jeff Layton <jl...@re...> |