Amerigo Wang <amwang@xxxxxxxxxx> writes: > OGAWA Hirofumi wrote: >> akpm@xxxxxxxxxxxxxxxxxxxx writes: >> >> >>> diff -puN fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set fs/open.c >>> --- a/fs/open.c~vfs-allow-file-truncations-when-both-suid-and-write-permissions-set >>> +++ a/fs/open.c >>> @@ -213,11 +213,15 @@ int do_truncate(struct dentry *dentry, l >>> newattrs.ia_valid |= ATTR_FILE; >>> } >>> >>> + mutex_lock(&dentry->d_inode->i_mutex); >>> /* Remove suid/sgid on truncate too */ >>> - newattrs.ia_valid |= should_remove_suid(dentry); >>> + err = dentry_remove_suid(dentry); >>> + if (err) >>> + goto unlock; >>> >> >> Can't we use ATTR_FORCE for this? Because this calls notify_change() >> twice, and I guess this removes s[ug]id even if vmtruncate() (or in >> future ->truncate() may return error) or something returned error. >> >> I think it would not be good behavior. >> > > Hi, please check: > http://lkml.org/lkml/2009/7/1/459 Sorry for same argument. I see. However, um... I found this piece in security/selinux/hooks.c static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) { const struct cred *cred = current_cred(); if (iattr->ia_valid & ATTR_FORCE) return 0; if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_ATIME_SET | ATTR_MTIME_SET)) return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR); return dentry_has_perm(cred, NULL, dentry, FILE__WRITE); } I guess it's assuming the ia_valid doesn't have (ATTR_MODE | ATTR_SIZE), but truncate() already does it, I don't know whether it's ok. The definition of ATTR_FORCE is unclear at all, it would be problem. But, I'm not sure though, I suspect the above code also has problem... Thanks. -- OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> -- 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
