Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/vfscore: Ensure we return symlink target on namei_resolve #1320

Merged

Conversation

mogasergiu
Copy link
Member

@mogasergiu mogasergiu commented Feb 10, 2024

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

Description of changes

The namei_resolve method is different from its *_no_follow variants as in it is supposed to follow the symbolic links and eventually return the dentry of the final symlink target. However, if the first dentry hit in the cache is a symlink, we no longer check if it is a symlink or not and simply return it. To fix this, simply add a check: if current dentry pointer is a symlink, jump to the symlink logic.

To reproduce this may be hard, as it requires parallelism (just concurrency on a single CPU system won't work):

  • thread 1 is following symlink/readlink or any other function that may involve holding a reference to the dentry of a symlink
  • at the same time, thread 2 does an openat() on the symlink and because a reference is still being held on the symlink it will be found as a dentry in the cache (lib/vfscore/lookup.c:143) and thus be returned immediately, without checking whether it is a symlink or not. The normal behavior of openat would have been to follow the symlink until the final target is reached, however this way the openat() ends up being done on the dentry of the symlink instead.

The `namei_resolve` method is different from its `*_no_follow` variants
as in it is supposed to follow the symbolic links and eventually return
the dentry of the final symlink target. However, if the first dentry
hit in the cache is a symlink, we no longer check if it is a symlink
or not and simply return it. To fix this, simply add a check: if current
dentry pointer is a symlink, jump to the symlink logic.

Signed-off-by: Sergiu Moga <sergiu@unikraft.io>
@mogasergiu mogasergiu requested a review from a team as a code owner February 10, 2024 13:20
@github-actions github-actions bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ lib/vfscore VFS Core Interface labels Feb 10, 2024
@mogasergiu mogasergiu requested review from andreittr and removed request for a team February 10, 2024 13:20
Copy link
Member

@michpappas michpappas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picking up this change has fixed the issue for me.

Reviewed-by: Michalis Pappas michalis@unikraft.io

@razvand razvand self-assigned this Mar 13, 2024
@razvand razvand added this to the v0.17.0 (Calypso) milestone Mar 13, 2024
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved-by: Razvan Deaconescu razvand@unikraft.io

@razvand razvand changed the base branch from staging to staging-1320 March 13, 2024 20:39
@razvand razvand merged commit 57c8a9f into unikraft:staging-1320 Mar 13, 2024
12 checks passed
razvand pushed a commit that referenced this pull request Mar 13, 2024
The `namei_resolve` method is different from its `*_no_follow` variants
as in it is supposed to follow the symbolic links and eventually return
the dentry of the final symlink target. However, if the first dentry
hit in the cache is a symlink, we no longer check if it is a symlink
or not and simply return it. To fix this, simply add a check: if current
dentry pointer is a symlink, jump to the symlink logic.

Signed-off-by: Sergiu Moga <sergiu@unikraft.io>
Reviewed-by: Michalis Pappas <michalis@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
GitHub-Closes: #1320
SerbanSo pushed a commit to SerbanSo/unikraft-ASLR that referenced this pull request Jun 16, 2024
The `namei_resolve` method is different from its `*_no_follow` variants
as in it is supposed to follow the symbolic links and eventually return
the dentry of the final symlink target. However, if the first dentry
hit in the cache is a symlink, we no longer check if it is a symlink
or not and simply return it. To fix this, simply add a check: if current
dentry pointer is a symlink, jump to the symlink logic.

Signed-off-by: Sergiu Moga <sergiu@unikraft.io>
Reviewed-by: Michalis Pappas <michalis@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
GitHub-Closes: unikraft#1320
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ lib/vfscore VFS Core Interface
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants