linux-kernel - Re: sysfs: Do not return POSIX ACL xattrs via listxattr()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.WNT.2.20.1809101709260.3100@CLUIJ>
Date:   Mon, 10 Sep 2018 17:16:08 -0600 (Mountain Daylight Time)
From:   Marc Aurèle La France <tsi@...oix.net>
To:     Andreas Grünbacher <andreas.gruenbacher@...il.com>
cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Tejun Heo <tj@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>,
        Andreas Gruenbacher <agruenba@...hat.com>
Subject: Re: sysfs: Do not return POSIX ACL xattrs via listxattr()

On Mon, 10 Sep 2018, Andreas Grünbacher wrote:
> Am Mo., 10. Sep. 2018 schrieb Marc Aurèle La France:

>> Commit 786534b92f3ce68f4afc8a761c80b76887797b0a "tmpfs: listxattr
>> should include POSIX ACL xattrs", which first appeared in 4.5 kernels,
>> introduced a regression whereby listxattr() syscalls on anything in
>> sysfs, or its mountpoint, return the name of the two POSIX ACL xattrs,
>> but attempts to retrieve these values are denied with EOPNOTSUP.  For
>> example ...

>>         # getfattr -d --match=- /sys
>>         /sys: system.posix_acl_access: Operation not supported
>>         /sys: system.posix_acl_default: Operation not supported
>>         #

> I can confirm this regression.

>> This inconsistent behaviour confuses rsync(1) (among others) when it
>> runs into a sysfs mountpoint, even when told to not descend into it.
>> This issue occurs because simple_xattr_list() does not correctly deal
>> with cached ACLs.

>> The suggested fix below was developed with a 4.18.7 kernel, but should
>> apply, modulo patch fuzz, to any kernel >= 4.7.  A fix for 4.5 <=
>> kernels < 4.7 is trivially different, but I won't bother given such
>> kernels are no longer maintained.

>> Note that the only other simple_xattr_list() caller, shmem, avoids
>> this glitch by previously calling cache_no_acl() on all inodes it
>> creates, so perhaps sysfs/kernfs should do the same.

>> Signed-off-by: Marc Aurèle La France <tsi@...oix.net>

>> --- a/fs/xattr.c
>> +++ b/fs/xattr.c
>> @@ -949,13 +949,13 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
>>         int err = 0;
>>
>>  #ifdef CONFIG_FS_POSIX_ACL
>> -       if (inode->i_acl) {
>> +       if (inode->i_acl && !is_uncached_acl(inode->i_acl)) {
>>                 err = xattr_list_one(&buffer, &remaining_size,
>>                                      XATTR_NAME_POSIX_ACL_ACCESS);
>>                 if (err)
>>                         return err;
>>         }
>> -       if (inode->i_default_acl) {
>> +       if (inode->i_default_acl && !is_uncached_acl(inode->i_default_acl)) {
>>                 err = xattr_list_one(&buffer, &remaining_size,
>>                                      XATTR_NAME_POSIX_ACL_DEFAULT);
>>                 if (err)

> This seems to be a better fix, but I haven't fully verified it, yet:

> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -187,7 +187,8 @@ int inode_init_always(struct super_block *sb, struct
>     inode->i_mapping = mapping;
>     INIT_HLIST_HEAD(&inode->i_dentry);    /* buggered by rcu freeing */
>  #ifdef CONFIG_FS_POSIX_ACL
> -    inode->i_acl = inode->i_default_acl = ACL_NOT_CACHED;
> +    inode->i_acl = inode->i_default_acl =
> +           (sb->s_flags & SB_POSIXACL) ? ACL_NOT_CACHED : NULL;
>  #endif
>
>  #ifdef CONFIG_FSNOTIFY

Yes, that works too, and doesn't seem to cause other issues.

Tested-by: Marc Aurèle La France <tsi@...oix.net>

Thanks.

Marc.