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

core: pass bpf_outer_map_fd to sd-executor only if RestrictFileSystems was set #30170

Merged
merged 3 commits into from
Nov 27, 2023

Conversation

bluca
Copy link
Member

@bluca bluca commented Nov 23, 2023

It causes SELinux denials to be raised, so restrict it only where needed

Follow-up for beb4ae8

There are no tests for this feature, so no idea if this works or not, completely speculative.

…s was set

It causes SELinux denials to be raised, so restrict it only where needed

Follow-up for beb4ae8
The helpers already skip if the FD is < 0
@bluca bluca added this to the v255 milestone Nov 23, 2023
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Nov 23, 2023
Copy link

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

@mrc0mmand
Copy link
Member

I'll look into adding some tests for the bpf stuff.

@mrc0mmand
Copy link
Member

I just accidentally noticed that the BPF stuff has one other side effect:

# systemd-run -q --service-type notify -p FileDescriptorStoreMax=7 --wait --pipe bash -xec 'ls -l /proc/self/fd/; systemd-notify --fd=4 4</dev/null; systemd-notify --ready'
+ ls -l /proc/self/fd/
total 0
lrwx------ 1 root root 64 Nov 25 23:58 0 -> /dev/pts/1
lrwx------ 1 root root 64 Nov 25 23:58 1 -> /dev/pts/1
lrwx------ 1 root root 64 Nov 25 23:58 145 -> anon_inode:bpf-map
lrwx------ 1 root root 64 Nov 25 23:58 2 -> /dev/pts/1
lr-x------ 1 root root 64 Nov 25 23:58 3 -> /proc/925/fd
+ systemd-notify --fd=4
Warning: 1 more file descriptors passed than referenced with --fd=.
+ systemd-notify --ready

(notice the extra bpf-map fd)

With this PR applied the issue seems to be gone:

# systemd-run -q --service-type notify -p FileDescriptorStoreMax=7 --wait --pipe bash -xec 'ls -l /proc/self/fd/; systemd-notify --fd=4 4</dev/null; systemd-notify --ready'
+ ls -l /proc/self/fd/
total 0
lrwx------ 1 root root 64 Nov 25 23:59 0 -> /dev/pts/1
lrwx------ 1 root root 64 Nov 25 23:59 1 -> /dev/pts/1
lrwx------ 1 root root 64 Nov 25 23:59 2 -> /dev/pts/1
lr-x------ 1 root root 64 Nov 25 23:59 3 -> /proc/1294/fd
+ systemd-notify --fd=4
+ systemd-notify --ready

@mrc0mmand
Copy link
Member

And just to add to the previous comment - the "issue" is still there if RestrictFileSystems= is used (as expected):

# systemd-run -q --service-type notify -p RestrictFileSystems=~ext4 -p FileDescriptorStoreMax=7 --wait --pipe bash -xec 'ls -l /proc/self/fd/; systemd-notify --fd=4 4</dev/null; systemd-notify --ready'
+ ls -l /proc/self/fd/
total 0
lrwx------ 1 root root 64 Nov 27 14:10 0 -> /dev/pts/1
lrwx------ 1 root root 64 Nov 27 14:10 1 -> /dev/pts/1
lrwx------ 1 root root 64 Nov 27 14:10 2 -> /dev/pts/1
lr-x------ 1 root root 64 Nov 27 14:10 3 -> /proc/1923/fd
lrwx------ 1 root root 64 Nov 27 14:10 82 -> anon_inode:bpf-map
+ systemd-notify --fd=4
Warning: 1 more file descriptors passed than referenced with --fd=.
+ systemd-notify --ready

Not sure if there are any "real" consequences of the extra FD, though.

@poettering
Copy link
Member

lgtm

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Nov 27, 2023
@bluca bluca merged commit 04fc5b6 into systemd:main Nov 27, 2023
45 of 49 checks passed
@bluca bluca deleted the exec_bpf_fd branch November 27, 2023 15:44
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants