On 12/9/2014 10:27 AM, Jan Kara wrote:
> On Fri 05-12-14 08:06:55, Casey Schaufler wrote:
>> On 12/4/2014 5:27 AM, Jan Kara wrote:
>>> Similarly as we remove suid bit on truncate, we also want to remove
>>> security extended attributes.
>> NAK
>>
>> Are you out of your mind?
>>
>> In Smack and SELinux the security attributes are associated with the
>> container, not the data.
> Is there some doc for this? It just seems strange to me that when a file
> is written we clear the attributes
This is not true for the LSM based attributes.
> but when the file is truncated we don't.
Have I miss-interpreted what you meant by "security extended attributes"?
Do you mean filesystem xattrs beginning with "security.", such as
"security.selinux" or "security.SMACK64", or something else?
> What's the rationale behind this? To me both operations modify content of
> the file and thus I'd expect them to behave identically with respect to
> security attributes...
>
> Honza
>
>>> After this patch there's only one user of should_remove_suid() - ocfs2 -
>>> and indeed it's buggy because it doesn't clear security attributes on
>>> write. However fixing it is difficult because of special locking
>>> constraints.
>>>
>>> Signed-off-by: Jan Kara <jack@xxxxxxx>
>>> ---
>>> fs/inode.c | 5 ++---
>>> fs/open.c | 6 ++++--
>>> include/linux/fs.h | 6 +++++-
>>> 3 files changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/inode.c b/fs/inode.c
>>> index 6807a2707828..8595c7b8841c 100644
>>> --- a/fs/inode.c
>>> +++ b/fs/inode.c
>>> @@ -1603,9 +1603,8 @@ EXPORT_SYMBOL(should_remove_suid);
>>> * response to write or truncate. Return 0 if nothing has to be changed.
>>> * Negative value on error (change should be denied).
>>> */
>>> -int file_needs_remove_privs(struct file *file)
>>> +int dentry_needs_remove_privs(struct dentry *dentry)
>>> {
>>> - struct dentry *dentry = file->f_path.dentry;
>>> struct inode *inode = dentry->d_inode;
>>> int mask = 0;
>>> int ret;
>>> @@ -1621,7 +1620,7 @@ int file_needs_remove_privs(struct file *file)
>>> mask |= ATTR_KILL_PRIV;
>>> return mask;
>>> }
>>> -EXPORT_SYMBOL(file_needs_remove_privs);
>>> +EXPORT_SYMBOL(dentry_needs_remove_privs);
>>>
>>> static int __remove_privs(struct dentry *dentry, int kill)
>>> {
>>> diff --git a/fs/open.c b/fs/open.c
>>> index de92c13b58be..e4e0863855d0 100644
>>> --- a/fs/open.c
>>> +++ b/fs/open.c
>>> @@ -51,8 +51,10 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>>> newattrs.ia_valid |= ATTR_FILE;
>>> }
>>>
>>> - /* Remove suid/sgid on truncate too */
>>> - ret = should_remove_suid(dentry);
>>> + /* Remove suid/sgid and security markings on truncate too */
>>> + ret = dentry_needs_remove_privs(dentry);
>>> + if (ret < 0)
>>> + return ret;
>>> if (ret)
>>> newattrs.ia_valid |= ret | ATTR_FORCE;
>>>
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index aac707cced66..c5ccc311e8fb 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -2429,7 +2429,11 @@ extern struct inode *new_inode(struct super_block *sb);
>>> extern void free_inode_nonrcu(struct inode *inode);
>>> extern int should_remove_suid(struct dentry *);
>>> extern int file_remove_privs(struct file *);
>>> -extern int file_needs_remove_privs(struct file *file);
>>> +extern int dentry_needs_remove_privs(struct dentry *dentry);
>>> +static inline int file_needs_remove_privs(struct file *file)
>>> +{
>>> + return dentry_needs_remove_privs(file->f_path.dentry);
>>> +}
>>>
>>> extern void __insert_inode_hash(struct inode *, unsigned long hashval);
>>> static inline void insert_inode_hash(struct inode *inode)
--
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
