Re: [PATCH v2 1/1] xattr: Allow user.* xattr on symlink and special files

[Casey Schaufler]

On 7/9/2021 8:27 AM, Vivek Goyal wrote:
> On Fri, Jul 09, 2021 at 11:19:15AM +0200, Christian Brauner wrote:
>> On Thu, Jul 08, 2021 at 01:57:38PM -0400, Vivek Goyal wrote:
>>> Currently user.* xattr are not allowed on symlink and special files.
>>>
>>> man xattr and recent discussion suggested that primary reason for this
>>> restriction is how file permissions for symlinks and special files
>>> are little different from regular files and directories.
>>>
>>> For symlinks, they are world readable/writable and if user xattr were
>>> to be permitted, it will allow unpriviliged users to dump a huge amount
>>> of user.* xattrs on symlinks without any control.
>>>
>>> For special files, permissions typically control capability to read/write
>>> from devices (and not necessarily from filesystem). So if a user can
>>> write to device (/dev/null), does not necessarily mean it should be allowed
>>> to write large number of user.* xattrs on the filesystem device node is
>>> residing in.
>>>
>>> This patch proposes to relax the restrictions a bit and allow file owner
>>> or priviliged user (CAP_FOWNER), to be able to read/write user.* xattrs
>>> on symlink and special files.
>>>
>>> virtiofs daemon has a need to store user.* xatrrs on all the files
>>> (including symlinks and special files), and currently that fails. This
>>> patch should help.
>>>
>>> Link: https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@xxxxxxxxxx/
>>> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
>>> ---
>> Seems reasonable and useful.
>> Acked-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
>>
>> One question, do all filesystem supporting xattrs deal with setting them
>> on symlinks/device files correctly?
> Wrote a simple bash script to do setfattr/getfattr user.foo xattr on
> symlink and device node on ext4, xfs and btrfs and it works fine.

How about nfs, tmpfs, overlayfs and/or some of the other less conventional
filesystems?

>
> https://github.com/rhvgoyal/misc/blob/master/generic-programs/user-xattr-special-files.sh
>
> I probably can add some more filesystems to test.
>
> Thanks
> Vivek
>
>>>  fs/xattr.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/xattr.c b/fs/xattr.c
>>> index 5c8c5175b385..2f1855c8b620 100644
>>> --- a/fs/xattr.c
>>> +++ b/fs/xattr.c
>>> @@ -120,12 +120,14 @@ xattr_permission(struct user_namespace *mnt_userns, struct inode *inode,
>>>  	}
>>>  
>>>  	/*
>>> -	 * In the user.* namespace, only regular files and directories can have
>>> -	 * extended attributes. For sticky directories, only the owner and
>>> -	 * privileged users can write attributes.
>>> +	 * In the user.* namespace, for symlinks and special files, only
>>> +	 * the owner and priviliged users can read/write attributes.
>>> +	 * For sticky directories, only the owner and privileged users can
>>> +	 * write attributes.
>>>  	 */
>>>  	if (!strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN)) {
>>> -		if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
>>> +		if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode) &&
>>> +		    !inode_owner_or_capable(mnt_userns, inode))
>>>  			return (mask & MAY_WRITE) ? -EPERM : -ENODATA;
>>>  		if (S_ISDIR(inode->i_mode) && (inode->i_mode & S_ISVTX) &&
>>>  		    (mask & MAY_WRITE) &&
>>> -- 
>>> 2.25.4
>>>