Page MenuHomeFreeBSD

[tmpfs]: Fix tmpfs issure reproduced by pjdfstest tests/rename/09.t
ClosedPublic

Authored by fsu on Jan 29 2023, 8:36 AM.
Tags
None
Referenced Files
Unknown Object (File)
Oct 1 2024, 4:05 AM
Unknown Object (File)
Sep 30 2024, 1:38 PM
Unknown Object (File)
Sep 23 2024, 12:37 PM
Unknown Object (File)
Sep 22 2024, 7:59 PM
Unknown Object (File)
Sep 21 2024, 5:24 AM
Unknown Object (File)
Sep 19 2024, 11:51 PM
Unknown Object (File)
Sep 18 2024, 12:11 PM
Unknown Object (File)
Sep 18 2024, 6:39 AM
Subscribers

Details

Summary

Fix rename when renamed directory not owned by user, but when user owns the sticky parent directory.

NOTE: On ufs side the test passed because of returning EACCESS from VNOP_ACCES on ufs_vnops.c:1485 from rename call.
Test Plan

tests/rename/09.t - should pass without errors

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

fsu requested review of this revision.Jan 29 2023, 8:36 AM

This change triggered me because explicit cr_uid checks mean that e.g. a future implementation of ACLs would be broken there.
VOP_ACCESS()/VOP_ACCESSX() should encapsulate such knowledge of permissions.

Also, isn't remove from sticky directory has similar problem?

[For UFS, the implementation denies improper renames and deletes at the lookup stage. The logic is in ufs_delete_denied(). I think tmpfs should copy this approach]

In D38245#869181, @kib wrote:

This change triggered me because explicit cr_uid checks mean that e.g. a future implementation of ACLs would be broken there.
VOP_ACCESS()/VOP_ACCESSX() should encapsulate such knowledge of permissions.

Also, isn't remove from sticky directory has similar problem?

The similar test for removing will pass (tests/rmdir/11.t).
Let's consider the test explanation from pjdfstest comment [https://github.com/pjd/pjdfstest/blob/master/tests/rename/09.t#L121]:

# User owns the source sticky directory, but doesn't own the source file.
# This fails when changing parent directory, because this will modify
# source directory inode (the .. link in it), but we can still rename it
# without changing its parent directory.

So, it mean, we cannot modify renamed inode, but can remove it.
Let's add some graphics:

sticky directory                child directory
owned by user (ino 100500)      not owned by user (ino 100501)
         |                              |
        \|/                            \|/
directory entries:                  directory entries: 
- "."     (ino 100500)              - "."     (ino 100501)
- ".."    (ino XXXXXX)    <-----    - ".."    (ino 100500)  <- cannot be modified
...                                 ...
- "child" (ino 100501)     ----->   ...

The error come when we will change child directory parent entry, mean entry with "cannot be modified" tag,
but we can rename and remove child directory without errors, because only parent entry will be modified
and child directory entries will not be touched.

[For UFS, the implementation denies improper renames and deletes at the lookup stage. The logic is in ufs_delete_denied(). I think tmpfs should copy this approach]

The equal code to ufs_delete_denied() is placed in tmpfs_vnops.c:194

The only thing is needed, add the check to tmpfs_rename(), that the user have write permissions to child directory, like it is done in ufs_vnops.c:1485

This revision is now accepted and ready to land.Feb 4 2023, 6:45 PM
This revision was automatically updated to reflect the committed changes.