Bug 216834 - fchmodat changes permission bits of symlink
Summary: fchmodat changes permission bits of symlink
Status: NEW
Alias: None
Product: File System
Classification: Unclassified
Component: Other (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: fs_other
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-12-22 22:06 UTC by Ryan Prichard
Modified: 2022-12-23 10:43 UTC (History)
1 user (show)

See Also:
Kernel Version: 5.19.11-1rodete1-amd64, 5.15.41-android13-8-00205-gf1bf82c3dacd-ab8747247, "latest" as of 2022-12-22
Subsystem:
Regression: No
Bisected commit-id:


Attachments
modify-symlink.c (771 bytes, text/x-csrc)
2022-12-22 22:06 UTC, Ryan Prichard
Details

Description Ryan Prichard 2022-12-22 22:06:13 UTC
Created attachment 303456 [details]
modify-symlink.c

I've noticed that I'm able to change the permission bits of a symlink by invoking __NR_fchmodat on a /proc/self/fd path for a symlink opened with O_PATH. On some filesystems (ext4, btrfs, and xfs), the syscall fails with ENOTSUP, but the permission bits are still changed. On f2fs, on the other hand, the syscall fails with ENOTSUP and the bits are unchanged.

Using the attached modify-symlink.c, I see this output:

-1 [Operation not supported]
lrw---x-w- 1 rprichard primarygroup 1 Dec 22 13:58 A -> B
-rw-r--r-- 1 rprichard primarygroup 0 Dec 22 13:58 B

I see this behavior with the kernel on my gLinux (Debian) machine, as well as various Android kernels. I showed the problem to jaegeuk@google.com, who also tested it on a recent kernel.
Comment 1 Christian Brauner 2022-12-23 10:43:48 UTC
On Thu, Dec 22, 2022 at 10:06:13PM +0000, bugzilla-daemon@kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=216834
> 
>             Bug ID: 216834
>            Summary: fchmodat changes permission bits of symlink
>            Product: File System
>            Version: 2.5
>     Kernel Version: 5.19.11-1rodete1-amd64,
>                     5.15.41-android13-8-00205-gf1bf82c3dacd-ab8747247,
>                     "latest" as of 2022-12-22
>           Hardware: All
>                 OS: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: Other
>           Assignee: fs_other@kernel-bugs.osdl.org
>           Reporter: rprichard@google.com
>         Regression: No
> 
> Created attachment 303456 [details]
>   --> https://bugzilla.kernel.org/attachment.cgi?id=303456&action=edit
> modify-symlink.c
> 
> I've noticed that I'm able to change the permission bits of a symlink by
> invoking __NR_fchmodat on a /proc/self/fd path for a symlink opened with
> O_PATH. On some filesystems (ext4, btrfs, and xfs), the syscall fails with
> ENOTSUP, but the permission bits are still changed. On f2fs, on the other
> hand,
> the syscall fails with ENOTSUP and the bits are unchanged.
> 
> Using the attached modify-symlink.c, I see this output:
> 
> -1 [Operation not supported]
> lrw---x-w- 1 rprichard primarygroup 1 Dec 22 13:58 A -> B
> -rw-r--r-- 1 rprichard primarygroup 0 Dec 22 13:58 B
> 
> I see this behavior with the kernel on my gLinux (Debian) machine, as well as
> various Android kernels. I showed the problem to jaegeuk@google.com, who also
> tested it on a recent kernel.

Yeah, this is a known issue. Or at least I'm aware of it and it's a bit
wonky as this is at the intersection of two issues. Wouldn't be fun if
it wasn't:

(1) /proc/self/fd/<nr> doesn't honor permission bits. For example, you
    can easily upgrade permission bits by reopening an fd via
    /proc/self/fd<nr>. This is well-known and next year we'll hopefully
    be able to further restrict this with upgrade masks specifiable in
    openat2().
    Consequently, /proc/self/fd/<nr> easily let's you change permission
    bits and other stuff. It's not really like an fd.
(2) Symlinks don't support xattrs and thus don't support POSIX ACLs.
    If you change the mode of a file and the file has POSIX ACLs then
    the permission mask stored in the POSIX ACLs needs to be updated.
    Since symlinks don't support POSIX ACLs this is where it fails and
    this is where the EOPNOTSUPP comes from.

    For example, xfs keeps changing POSIX ACLs out of the transaction
    path. IOW, it calls posix_acl_chmod() after it has already updated
    the inode and completed the transaction. The same behavior exists
    for ext4 and btrfs.

    In a way, your chmod() is successful it's just that updating POSIX
    ACLs fails which is ok since symlinks don't support them.

One way to improve this situation is to just make posix_acl_chmod() not
bother reporting errors for acls on symlinks instead of reporting a
failure. IOW,

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 74dc0f571dc9..a9246934a4ca 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -594,7 +594,7 @@ int
        struct posix_acl *acl;
        int ret = 0;

-       if (!IS_POSIXACL(inode))
+       if (S_ISLNK(mode) || !IS_POSIXACL(dir))
                return 0;
        if (!inode->i_op->set_acl)
                return -EOPNOTSUPP;

Note You need to log in before you can comment on or make changes to this bug.