Support Landlock syscalls #2380

l0kod · 2021-01-07T15:54:42Z

Hi,

This is a WIP for Landlock (v26), a new LSM not upstream yet, but hopefully soon: https://lore.kernel.org/lkml/20201209192839.1396820-1-mic@digikod.net/
I'm working on a v27 patch series, but it should not change much the kernel part, mostly tests.

Landlock adds 3 new syscalls which are cover with this commit. I'm using syzkaller with it and it seems to work. :)

See the documentation: https://landlock.io/linux-doc/landlock-v26/userspace-api/landlock.html
and the syscall implementations: https://github.com/landlock-lsm/linux/blob/landlock-v26/security/landlock/syscall.c

Could you please give some feedback and advices to improve this patch?

I'll update this PR when Landlock will be merged upstream.

This is a team work with @vdagonneau-anssi.

google-cla · 2021-01-07T15:54:47Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

codecov · 2021-01-07T16:07:39Z

Codecov Report

Merging #2380 (aec00d2) into master (2af9d32) will increase coverage by 0.1%.
The diff coverage is n/a.

Impacted Files	Coverage Δ
prog/prog.go	`75.4% <0.0%> (-1.2%)`	⬇️
prog/any.go	`83.0% <0.0%> (-0.5%)`	⬇️
prog/mutation.go	`89.7% <0.0%> (+1.1%)`	⬆️
prog/rotation.go	`96.6% <0.0%> (+1.7%)`	⬆️
pkg/csource/csource.go	`79.6% <0.0%> (+4.3%)`	⬆️
prog/target.go	`65.9% <0.0%> (+5.7%)`	⬆️

google-cla · 2021-01-08T14:21:46Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

dvyukov · 2021-01-09T09:31:11Z

Hi Mickaël,

I am excited to see syzkaller descriptions being developed along with the new kernel subsystems!

I reviewed the descriptions themselves and they look good to me, I don't have any significant comments.
I have 2 style nits, though:

Suffix for the "landlock_add_rule$path_beneath" syscall:
https://github.com/google/syzkaller/blob/master/docs/syscall_descriptions.md#names
Order of declarations:
https://github.com/google/syzkaller/blob/master/docs/syscall_descriptions.md#order

When/if we are merging this, it would also be good to update syzbot configs:
https://github.com/google/syzkaller/tree/master/dashboard/config/linux
We will probably need something here:
https://github.com/google/syzkaller/blob/master/dashboard/config/linux/bits/lsm.yml
and update order of stacked LSMs for apparmour/seclinux/smack:
https://github.com/google/syzkaller/blob/master/dashboard/config/linux/bits/apparmor.yml

We can merge and start testing it once it's in linux-next. As far as I understand it's on trajectory of being merged, I think I've seen an LWN article about it or something.

And we will need signed CLA to do anything with this PR. As far as I know Microsoft has a corp CLA, you may ask me privately for details.

dvyukov · 2021-01-09T09:31:55Z

Const was initially generated with syz-extract and the "???" (because of bit shift operation) were manually fixed:

Oh, what's this? Any manual changes will be overwritten on the next regenerate.

l0kod · 2021-01-09T12:35:42Z

Hi Mickaël,

Hi Dmitry!

I am excited to see syzkaller descriptions being developed along with the new kernel subsystems!

I reviewed the descriptions themselves and they look good to me, I don't have any significant comments.
I have 2 style nits, though:

Suffix for the "landlock_add_rule$path_beneath" syscall:
https://github.com/google/syzkaller/blob/master/docs/syscall_descriptions.md#names

OK, I'll rename it to landlock_add_rule$LANDLOCK_RULE_PATH_BENEATH.

Order of declarations:
https://github.com/google/syzkaller/blob/master/docs/syscall_descriptions.md#order

OK

When/if we are merging this, it would also be good to update syzbot configs:
https://github.com/google/syzkaller/tree/master/dashboard/config/linux
We will probably need something here:
https://github.com/google/syzkaller/blob/master/dashboard/config/linux/bits/lsm.yml
and update order of stacked LSMs for apparmour/seclinux/smack:
https://github.com/google/syzkaller/blob/master/dashboard/config/linux/bits/apparmor.yml

OK, I'll add this with a dedicated commit.

We can merge and start testing it once it's in linux-next. As far as I understand it's on trajectory of being merged, I think I've seen an LWN article about it or something.

Yes, I think it is on a good trajectory.

And we will need signed CLA to do anything with this PR. As far as I know Microsoft has a corp CLA, you may ask me privately for details.

I'm dealing with that, it should be OK.

Thanks.

l0kod · 2021-01-09T12:38:19Z

Const was initially generated with syz-extract and the "???" (because of bit shift operation) were manually fixed:

Oh, what's this? Any manual changes will be overwritten on the next regenerate.

I guess it's because I use the (1ULL << 0) notation: https://github.com/landlock-lsm/linux/blob/landlock-v26/include/uapi/linux/landlock.h#L114

I can take a look to fix this in syz-extract.

(1ULL << 0) should work... I would expect.
syz-extract does not do any manual parsing of headers, it creates a C program that prints/extracts these values. So if C compiler can chew this, it should work.

l0kod · 2021-01-12T13:37:44Z

(1ULL << 0) should work... I would expect.
syz-extract does not do any manual parsing of headers, it creates a C program that prints/extracts these values. So if C compiler can chew this, it should work.

I didn't see your reply as an edit of my comment. Please add new comments to reply, it helps understand who wrote it and enables notifications.

I didn't investigate much yet, but there is already 342 matches for "???" in sys/linux/*.const, mostly for some architectures but not always:

syzkaller/sys/linux/dev_i915.txt.const

Line 3 in 2c1f251

AT_FDCWD = ???

Could this issue be caused by incorrect include path?

dvyukov · 2021-01-13T07:26:45Z

I didn't see your reply as an edit of my comment. Please add new comments to reply, it helps understand who wrote it and enables notifications.

Oh, sorry, I pressed a wrong menu item.

mostly for some architectures

Some constants are not present on some architectures, e.g. KVM is not supported on all arches.

Could this issue be caused by incorrect include path?

This is caused by a missing include file for this const.

l0kod · 2021-01-28T13:43:09Z

I fixed the ??? issue, it was because of #include instead of include. :)
I also took the styling suggestions into account.

I added two tests to help coverage, which is now 68% for security/landlock/ with syzkaller. I'm working on new ones.

One question though: do we need breaks_returns somewhere?

dvyukov · 2021-01-28T13:47:32Z

One question though: do we need breaks_returns somewhere?

It's only needed if the syscall makes this or subsequent syscalls return random errno values. syzkaller may use returned errno values as some coarse substitute of coverage, returning random garbage from syscalls interferes with this feature.

l0kod · 2021-01-28T14:07:53Z

It's only needed if the syscall makes this or subsequent syscalls return random errno values. syzkaller may use returned errno values as some coarse substitute of coverage, returning random garbage from syscalls interferes with this feature.

Because it's an access control system, Landlock may indirectly change the errno value of a subsequent syscall with EACCES. So I guess I should use breaks_returns for the (current) landlock_enforce_ruleset_self() (like a light seccomp$SET_MODE_FILTER) right?

dvyukov · 2021-01-28T14:10:08Z

Because it's an access control system, Landlock may indirectly change the errno value of a subsequent syscall with EACCES. So I guess I should use breaks_returns for the (current) landlock_enforce_ruleset_self() right?

If it's only EACCES, then, no, we don't need breaks_returns. It's only for completely random integers. 1 value is not a problem and these can potentially return EACCESS on their own.

l0kod · 2021-01-28T14:28:28Z

Does the errno in calls like syscall(x, y) # EACCES are taken into account? I didn't see any behavior change.

dvyukov · 2021-01-28T14:29:50Z

Does the errno in calls like syscall(x, y) # EACCES are taken into account? I didn't see any behavior change.

I don't understand the question. Please elaborate.

l0kod · 2021-01-28T14:32:18Z

Comments like # EPERM are just cosmetic or are they used somehow by syzkaller?
e.g. https://github.com/google/syzkaller/blob/master/sys/linux/test/bpf_cgroup#L17

dvyukov · 2021-01-28T15:07:55Z

Comments like # EPERM are just cosmetic or are they used somehow by syzkaller?
e.g. https://github.com/google/syzkaller/blob/master/sys/linux/test/bpf_cgroup#L17

They are not cosmetic, they are checked by pkg/runtest:
https://github.com/google/syzkaller/blob/master/pkg/runtest/run.go#L303-L319
However, note that these tests are not run automatically by anybody. You need to use tools/syz-runtest to run them manually.

l0kod · 2021-02-04T14:59:22Z

The addition of fork() is redundant with #2412 but I will remove it from this branch if merged.

sys/linux/test/landlock_ptrace

l0kod · 2021-02-05T13:20:00Z

I think this should be good now, but still not ready to merge because not in -next.

l0kod · 2021-02-05T13:27:45Z

I reached 72% of coverage for security/landlock/ . The code not (yet) covered deals with race-conditions and internal errors (e.g. ENOMEM).

sys/linux/test/landlock_ptrace

l0kod · 2021-02-09T10:34:19Z

Is there a way to declare that a syscall changes the state of the calling thread? This could be relevant to force threaded=0 after a call to landlock_restrict_self() as well as seccomp().

dvyukov · 2021-02-09T10:38:50Z

Is there a way to declare that a syscall changes the state of the calling thread? This could be relevant to force threaded=0 after a call to landlock_restrict_self() as well as seccomp().

How would it be used by the fuzzer? We don't declare properties that we don't use (unnecessary/untested).

l0kod · 2021-02-10T16:31:24Z

Is there a way to declare that a syscall changes the state of the calling thread? This could be relevant to force threaded=0 after a call to landlock_restrict_self() as well as seccomp().

How would it be used by the fuzzer? We don't declare properties that we don't use (unnecessary/untested).

I may have misunderstood how syzkaller works, but it seems that it can call each syscall in a dedicated thread, hence the threaded=0 option for tests. In this case, if seccomp() or landlock_restrict_self() are called on a dedicated thread, successive sibling syscalls will not be restricted.

dvyukov · 2021-02-11T07:45:42Z

Is there a way to declare that a syscall changes the state of the calling thread? This could be relevant to force threaded=0 after a call to landlock_restrict_self() as well as seccomp().

How would it be used by the fuzzer? We don't declare properties that we don't use (unnecessary/untested).

I may have misunderstood how syzkaller works, but it seems that it can call each syscall in a dedicated thread, hence the threaded=0 option for tests. In this case, if seccomp() or landlock_restrict_self() are called on a dedicated thread, successive sibling syscalls will not be restricted.

It will call subsequent syscalls on separate threads only if a previous syscall has blocked. If syscalls don't block, it will call syscalls on the same thread.

l0kod · 2021-03-19T14:03:33Z

Landlock is now in linux-next: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/security/landlock?h=next-20210319&id=f00397ee41c79b6155b9b44abd0055b2c0621349

l0kod · 2021-03-19T14:16:45Z

I set the initial version to 5.13 though: 7dd0716#diff-708f4878ad38e69670500dbc92727f5c063a942ca2a458864a05a35657b7b907R17

dvyukov · 2021-03-19T14:31:32Z

We could start testing it now, syzbot has instances on linux-next. You can set the version to linux-next (we later replace these with real version, once it's merged).

Add config fragments for Landlock LSM. Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

Based on Linux next-20210319: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=f00397ee41c79b6155b9b44abd0055b2c0621349 Co-developed-by: Vincent Dagonneau <vincent.dagonneau@ssi.gouv.fr> Signed-off-by: Vincent Dagonneau <vincent.dagonneau@ssi.gouv.fr> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

This test helps cover security/landlock/fs.c:hook_sb_delete() Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

This test helps cover security/landlock/fs.c:check_access_path() Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

This test helps cover security/landlock/ptrace.c Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

This test helps cover most types of access checks in security/landlock/fs.c Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

This test covers mount namespace manipulation forbidden in security/landlock/fs.c Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

dvyukov · 2021-03-19T14:54:29Z

We also need to regenerate kernel configs with make configs:
https://github.com/google/syzkaller/blob/master/dashboard/config/linux/README.md
but it requires a bunch of compilers installed, so I regenerated them to save a round trip:
#2511

The change looks good to me. Waiting for the CI to finish to merge.
Thanks for your contribution. I especially liked the added sys/linux/test/* tests.

l0kod · 2021-03-19T15:15:59Z

Thanks Dmitry!

l0kod · 2021-03-20T18:54:48Z

We could start testing it now, syzbot has instances on linux-next. You can set the version to linux-next (we later replace these with real version, once it's merged).

I guess something is missing, this instance doesn't have Landlock yet: https://storage.googleapis.com/syzkaller/cover/ci-upstream-linux-next-kasan-gce-root.html

dvyukov · 2021-03-20T18:59:06Z

linux-next is boot broken:
https://syzkaller.appspot.com/upstream
The instance is still on commit f00397ee. But there is a fixing commit for that kernel bug already, so when linux-next rebases on upstream, it should be fixed.

l0kod force-pushed the landlock branch from ae5a179 to 0262d36 Compare January 8, 2021 14:21

l0kod force-pushed the landlock branch from 0262d36 to 337f84e Compare January 28, 2021 13:38

l0kod mentioned this pull request Jan 28, 2021

Add Linux syscall variants #2412

Merged

l0kod force-pushed the landlock branch 2 times, most recently from e77df45 to 8e6110b Compare February 4, 2021 14:57

l0kod commented Feb 4, 2021

View reviewed changes

sys/linux/test/landlock_ptrace Show resolved Hide resolved

l0kod force-pushed the landlock branch from 8e6110b to eaa6ac3 Compare February 5, 2021 13:17

l0kod commented Feb 5, 2021

View reviewed changes

sys/linux/test/landlock_ptrace Outdated Show resolved Hide resolved

l0kod force-pushed the landlock branch from eaa6ac3 to bb9fd7e Compare February 5, 2021 14:30

l0kod commented Feb 7, 2021

View reviewed changes

sys/linux/test/landlock_ptrace Outdated Show resolved Hide resolved

l0kod force-pushed the landlock branch from bb9fd7e to 8228cd9 Compare February 9, 2021 09:40

l0kod changed the title ~~sys/linux: add landlock syscalls~~ Support Landlock syscalls Feb 10, 2021

l0kod mentioned this pull request Feb 11, 2021

Mknod variants #2439

Merged

l0kod force-pushed the landlock branch from 5c982dd to 1bdd2be Compare February 18, 2021 11:31

l0kod mentioned this pull request Mar 5, 2021

Fix prctl variants #2474

Merged

l0kod force-pushed the landlock branch 2 times, most recently from 46c058f to 406040b Compare March 19, 2021 14:01

l0kod marked this pull request as ready for review March 19, 2021 14:01

l0kod force-pushed the landlock branch from 406040b to 8ea83dd Compare March 19, 2021 14:13

l0kod added 7 commits March 19, 2021 15:33

dashboard/config/linux: enable Landlock

7541101

Add config fragments for Landlock LSM. Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

sys/linux/test: add landlock_sb_delete

56eb06f

This test helps cover security/landlock/fs.c:hook_sb_delete() Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

sys/linux/test: add landlock_layers

0d9f62e

This test helps cover security/landlock/fs.c:check_access_path() Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

sys/linux/test: add landlock_ptrace

80e7cc4

This test helps cover security/landlock/ptrace.c Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

sys/linux/test: add landlock_fs_accesses

b2e3f62

This test helps cover most types of access checks in security/landlock/fs.c Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

sys/linux/test: add landlock_fs_forbidden

aec00d2

This test covers mount namespace manipulation forbidden in security/landlock/fs.c Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>

l0kod force-pushed the landlock branch from 8ea83dd to aec00d2 Compare March 19, 2021 14:34

dvyukov merged commit e208126 into google:master Mar 19, 2021

l0kod deleted the landlock branch March 19, 2021 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Landlock syscalls #2380

Support Landlock syscalls #2380

l0kod commented Jan 7, 2021

google-cla bot commented Jan 7, 2021

codecov bot commented Jan 7, 2021 •

edited

Loading

google-cla bot commented Jan 8, 2021

dvyukov commented Jan 9, 2021

dvyukov commented Jan 9, 2021

l0kod commented Jan 9, 2021

l0kod commented Jan 9, 2021 •

edited by dvyukov

Loading

l0kod commented Jan 12, 2021 •

edited

Loading

dvyukov commented Jan 13, 2021

l0kod commented Jan 28, 2021

dvyukov commented Jan 28, 2021

l0kod commented Jan 28, 2021 •

edited

Loading

dvyukov commented Jan 28, 2021

l0kod commented Jan 28, 2021

dvyukov commented Jan 28, 2021

l0kod commented Jan 28, 2021 •

edited

Loading

dvyukov commented Jan 28, 2021

l0kod commented Feb 4, 2021

l0kod commented Feb 5, 2021

l0kod commented Feb 5, 2021

l0kod commented Feb 9, 2021 •

edited

Loading

dvyukov commented Feb 9, 2021

l0kod commented Feb 10, 2021

dvyukov commented Feb 11, 2021

l0kod commented Mar 19, 2021

l0kod commented Mar 19, 2021

dvyukov commented Mar 19, 2021

dvyukov commented Mar 19, 2021

l0kod commented Mar 19, 2021

l0kod commented Mar 20, 2021

dvyukov commented Mar 20, 2021

Support Landlock syscalls #2380

Support Landlock syscalls #2380

Conversation

l0kod commented Jan 7, 2021

google-cla bot commented Jan 7, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

codecov bot commented Jan 7, 2021 • edited Loading

Codecov Report

google-cla bot commented Jan 8, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

dvyukov commented Jan 9, 2021

dvyukov commented Jan 9, 2021

l0kod commented Jan 9, 2021

l0kod commented Jan 9, 2021 • edited by dvyukov Loading

l0kod commented Jan 12, 2021 • edited Loading

dvyukov commented Jan 13, 2021

l0kod commented Jan 28, 2021

dvyukov commented Jan 28, 2021

l0kod commented Jan 28, 2021 • edited Loading

dvyukov commented Jan 28, 2021

l0kod commented Jan 28, 2021

dvyukov commented Jan 28, 2021

l0kod commented Jan 28, 2021 • edited Loading

dvyukov commented Jan 28, 2021

l0kod commented Feb 4, 2021

l0kod commented Feb 5, 2021

l0kod commented Feb 5, 2021

l0kod commented Feb 9, 2021 • edited Loading

dvyukov commented Feb 9, 2021

l0kod commented Feb 10, 2021

dvyukov commented Feb 11, 2021

l0kod commented Mar 19, 2021

l0kod commented Mar 19, 2021

dvyukov commented Mar 19, 2021

dvyukov commented Mar 19, 2021

l0kod commented Mar 19, 2021

l0kod commented Mar 20, 2021

dvyukov commented Mar 20, 2021

codecov bot commented Jan 7, 2021 •

edited

Loading

l0kod commented Jan 9, 2021 •

edited by dvyukov

Loading

l0kod commented Jan 12, 2021 •

edited

Loading

l0kod commented Jan 28, 2021 •

edited

Loading

l0kod commented Jan 28, 2021 •

edited

Loading

l0kod commented Feb 9, 2021 •

edited

Loading