| Message ID | 20211011133704.1704369-1-brauner@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security/landlock: use square brackets around "landlock-ruleset" | expand |
On 11/10/2021 15:37, Christian Brauner wrote: > From: Christian Brauner <christian.brauner@ubuntu.com> > > Make the name of the anon inode fd "[landlock-ruleset]" instead of > "landlock-ruleset". This is minor but most anon inode fds already > carry square brackets around their name: > > [eventfd] > [eventpoll] > [fanotify] > [fscontext] > [io_uring] > [pidfd] > [signalfd] > [timerfd] > [userfaultfd] > > For the sake of consistency lets do the same for the landlock-ruleset anon > inode fd that comes with landlock. We did the same in > 1cdc415f1083 ("uapi, fsopen: use square brackets around "fscontext" [ver #2]") > for the new mount api. Before creating "landlock-ruleset" FD, I looked at other anonymous FD and saw this kind of inconsistency. I don't get why we need to add extra characters to names, those brackets seem useless. If it should be part of the interface, why is it not enforced by anon_inode_getfd()? There is a lot of other names that come without brackets (e.g. inotify, bpf-*, btf, kvm-*, iio*). Do you plan to send patches for those too? Changing such FD names could break user space because they may already be exposed and used (e.g. through SELinux).
On Mon, Oct 11, 2021 at 04:38:55PM +0200, Mickaël Salaün wrote: > > On 11/10/2021 15:37, Christian Brauner wrote: > > From: Christian Brauner <christian.brauner@ubuntu.com> > > > > Make the name of the anon inode fd "[landlock-ruleset]" instead of > > "landlock-ruleset". This is minor but most anon inode fds already > > carry square brackets around their name: > > > > [eventfd] > > [eventpoll] > > [fanotify] > > [fscontext] > > [io_uring] > > [pidfd] > > [signalfd] > > [timerfd] > > [userfaultfd] > > > > For the sake of consistency lets do the same for the landlock-ruleset anon > > inode fd that comes with landlock. We did the same in > > 1cdc415f1083 ("uapi, fsopen: use square brackets around "fscontext" [ver #2]") > > for the new mount api. > > Before creating "landlock-ruleset" FD, I looked at other anonymous FD > and saw this kind of inconsistency. I don't get why we need to add extra > characters to names, those brackets seem useless. If it should be part Past inconsistency shouldn't justify future inconsistency. If you have a strong opinion about this for landlock I'm not going to push for it. Exchanging more than 2-3 email about something like this seems too much. > of the interface, why is it not enforced by anon_inode_getfd()? Sure, we can add that too. > > There is a lot of other names that come without brackets (e.g. inotify, > bpf-*, btf, kvm-*, iio*). Do you plan to send patches for those too? > Changing such FD names could break user space because they may already > be exposed and used (e.g. through SELinux). We didn't do it for bpf and kvm stuff because it has been that way for a long time. We try to do it for all new ones. Christian
On Tue, Oct 12, 2021 at 6:38 AM Christian Brauner <christian.brauner@ubuntu.com> wrote: > On Mon, Oct 11, 2021 at 04:38:55PM +0200, Mickaël Salaün wrote: > > On 11/10/2021 15:37, Christian Brauner wrote: > > > From: Christian Brauner <christian.brauner@ubuntu.com> > > > > > > Make the name of the anon inode fd "[landlock-ruleset]" instead of > > > "landlock-ruleset". This is minor but most anon inode fds already > > > carry square brackets around their name: > > > > > > [eventfd] > > > [eventpoll] > > > [fanotify] > > > [fscontext] > > > [io_uring] > > > [pidfd] > > > [signalfd] > > > [timerfd] > > > [userfaultfd] > > > > > > For the sake of consistency lets do the same for the landlock-ruleset anon > > > inode fd that comes with landlock. We did the same in > > > 1cdc415f1083 ("uapi, fsopen: use square brackets around "fscontext" [ver #2]") > > > for the new mount api. > > > > Before creating "landlock-ruleset" FD, I looked at other anonymous FD > > and saw this kind of inconsistency. I don't get why we need to add extra > > characters to names, those brackets seem useless. If it should be part > > Past inconsistency shouldn't justify future inconsistency. If you have a > strong opinion about this for landlock I'm not going to push for it. > Exchanging more than 2-3 email about something like this seems too much. [NOTE: adding the SELinux list as well as Chris (SELinux refrence policy maintainer) and Petr (Fedora/RHEL SELinux)] Chris and Petr, do either of you currently have any policy that references the "landlock-ruleset" anonymous inode? In other words, would adding the brackets around the name cause you any problems? -- paul moore www.paul-moore.com
On Tue, Oct 12, 2021 at 8:12 PM Paul Moore <paul@paul-moore.com> wrote: > On Tue, Oct 12, 2021 at 6:38 AM Christian Brauner > <christian.brauner@ubuntu.com> wrote: > > On Mon, Oct 11, 2021 at 04:38:55PM +0200, Mickaël Salaün wrote: > > > On 11/10/2021 15:37, Christian Brauner wrote: > > > > From: Christian Brauner <christian.brauner@ubuntu.com> > > > > > > > > Make the name of the anon inode fd "[landlock-ruleset]" instead of > > > > "landlock-ruleset". This is minor but most anon inode fds already > > > > carry square brackets around their name: > > > > > > > > [eventfd] > > > > [eventpoll] > > > > [fanotify] > > > > [fscontext] > > > > [io_uring] > > > > [pidfd] > > > > [signalfd] > > > > [timerfd] > > > > [userfaultfd] > > > > > > > > For the sake of consistency lets do the same for the landlock-ruleset anon > > > > inode fd that comes with landlock. We did the same in > > > > 1cdc415f1083 ("uapi, fsopen: use square brackets around "fscontext" [ver #2]") > > > > for the new mount api. > > > > > > Before creating "landlock-ruleset" FD, I looked at other anonymous FD > > > and saw this kind of inconsistency. I don't get why we need to add extra > > > characters to names, those brackets seem useless. If it should be part > > > > Past inconsistency shouldn't justify future inconsistency. If you have a > > strong opinion about this for landlock I'm not going to push for it. > > Exchanging more than 2-3 email about something like this seems too much. > > [NOTE: adding the SELinux list as well as Chris (SELinux refrence > policy maintainer) and Petr (Fedora/RHEL SELinux)] > > Chris and Petr, do either of you currently have any policy that > references the "landlock-ruleset" anonymous inode? In other words, > would adding the brackets around the name cause you any problems? AFAIU, the anon_inode transitions (the only mechanism where the "file name" would be exposed to the policy) are done only for inodes created by anon_inode_getfd_secure(), which is currently only used by userfaultfd. So you don't even need to ask that question; at this point it should be safe to change any of the names except "[userfaultfd]" as far as SELinux policy is concerned.
On Tue, Oct 12, 2021 at 4:38 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Tue, Oct 12, 2021 at 8:12 PM Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Oct 12, 2021 at 6:38 AM Christian Brauner > > <christian.brauner@ubuntu.com> wrote: > > > On Mon, Oct 11, 2021 at 04:38:55PM +0200, Mickaël Salaün wrote: > > > > On 11/10/2021 15:37, Christian Brauner wrote: > > > > > From: Christian Brauner <christian.brauner@ubuntu.com> > > > > > > > > > > Make the name of the anon inode fd "[landlock-ruleset]" instead of > > > > > "landlock-ruleset". This is minor but most anon inode fds already > > > > > carry square brackets around their name: > > > > > > > > > > [eventfd] > > > > > [eventpoll] > > > > > [fanotify] > > > > > [fscontext] > > > > > [io_uring] > > > > > [pidfd] > > > > > [signalfd] > > > > > [timerfd] > > > > > [userfaultfd] > > > > > > > > > > For the sake of consistency lets do the same for the landlock-ruleset anon > > > > > inode fd that comes with landlock. We did the same in > > > > > 1cdc415f1083 ("uapi, fsopen: use square brackets around "fscontext" [ver #2]") > > > > > for the new mount api. > > > > > > > > Before creating "landlock-ruleset" FD, I looked at other anonymous FD > > > > and saw this kind of inconsistency. I don't get why we need to add extra > > > > characters to names, those brackets seem useless. If it should be part > > > > > > Past inconsistency shouldn't justify future inconsistency. If you have a > > > strong opinion about this for landlock I'm not going to push for it. > > > Exchanging more than 2-3 email about something like this seems too much. > > > > [NOTE: adding the SELinux list as well as Chris (SELinux refrence > > policy maintainer) and Petr (Fedora/RHEL SELinux)] > > > > Chris and Petr, do either of you currently have any policy that > > references the "landlock-ruleset" anonymous inode? In other words, > > would adding the brackets around the name cause you any problems? > > AFAIU, the anon_inode transitions (the only mechanism where the "file > name" would be exposed to the policy) are done only for inodes created > by anon_inode_getfd_secure(), which is currently only used by > userfaultfd. So you don't even need to ask that question; at this > point it should be safe to change any of the names except > "[userfaultfd]" as far as SELinux policy is concerned. There is also io_uring if you look at selinux/next. Regardless, thanks, I didn't check to see if landlock was using the new anon inode interface, since both Mickaël and Christian were concerned about breaking SELinux I had assumed they were using it :)
On 12/10/2021 23:09, Paul Moore wrote: > On Tue, Oct 12, 2021 at 4:38 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: >> >> On Tue, Oct 12, 2021 at 8:12 PM Paul Moore <paul@paul-moore.com> wrote: >>> On Tue, Oct 12, 2021 at 6:38 AM Christian Brauner >>> <christian.brauner@ubuntu.com> wrote: >>>> On Mon, Oct 11, 2021 at 04:38:55PM +0200, Mickaël Salaün wrote: >>>>> On 11/10/2021 15:37, Christian Brauner wrote: >>>>>> From: Christian Brauner <christian.brauner@ubuntu.com> >>>>>> >>>>>> Make the name of the anon inode fd "[landlock-ruleset]" instead of >>>>>> "landlock-ruleset". This is minor but most anon inode fds already >>>>>> carry square brackets around their name: >>>>>> >>>>>> [eventfd] >>>>>> [eventpoll] >>>>>> [fanotify] >>>>>> [fscontext] >>>>>> [io_uring] >>>>>> [pidfd] >>>>>> [signalfd] >>>>>> [timerfd] >>>>>> [userfaultfd] >>>>>> >>>>>> For the sake of consistency lets do the same for the landlock-ruleset anon >>>>>> inode fd that comes with landlock. We did the same in >>>>>> 1cdc415f1083 ("uapi, fsopen: use square brackets around "fscontext" [ver #2]") >>>>>> for the new mount api. >>>>> >>>>> Before creating "landlock-ruleset" FD, I looked at other anonymous FD >>>>> and saw this kind of inconsistency. I don't get why we need to add extra >>>>> characters to names, those brackets seem useless. If it should be part >>>> >>>> Past inconsistency shouldn't justify future inconsistency. If you have a >>>> strong opinion about this for landlock I'm not going to push for it. >>>> Exchanging more than 2-3 email about something like this seems too much. >>> >>> [NOTE: adding the SELinux list as well as Chris (SELinux refrence >>> policy maintainer) and Petr (Fedora/RHEL SELinux)] >>> >>> Chris and Petr, do either of you currently have any policy that >>> references the "landlock-ruleset" anonymous inode? In other words, >>> would adding the brackets around the name cause you any problems? >> >> AFAIU, the anon_inode transitions (the only mechanism where the "file >> name" would be exposed to the policy) are done only for inodes created >> by anon_inode_getfd_secure(), which is currently only used by >> userfaultfd. So you don't even need to ask that question; at this >> point it should be safe to change any of the names except >> "[userfaultfd]" as far as SELinux policy is concerned. > > There is also io_uring if you look at selinux/next. > > Regardless, thanks, I didn't check to see if landlock was using the > new anon inode interface, since both Mickaël and Christian were > concerned about breaking SELinux I had assumed they were using it :) > Ok, thanks Paul and Ondrej. Such anonymous inode names seem to be only exposed to proc for now. Let's change this name then. I think it make sense to backport this patch down to 5.13 to fix all the inconsistencies.
On Wed, Oct 13, 2021 at 05:47:53PM +0200, Mickaël Salaün wrote: > > On 12/10/2021 23:09, Paul Moore wrote: > > On Tue, Oct 12, 2021 at 4:38 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > >> > >> On Tue, Oct 12, 2021 at 8:12 PM Paul Moore <paul@paul-moore.com> wrote: > >>> On Tue, Oct 12, 2021 at 6:38 AM Christian Brauner > >>> <christian.brauner@ubuntu.com> wrote: > >>>> On Mon, Oct 11, 2021 at 04:38:55PM +0200, Mickaël Salaün wrote: > >>>>> On 11/10/2021 15:37, Christian Brauner wrote: > >>>>>> From: Christian Brauner <christian.brauner@ubuntu.com> > >>>>>> > >>>>>> Make the name of the anon inode fd "[landlock-ruleset]" instead of > >>>>>> "landlock-ruleset". This is minor but most anon inode fds already > >>>>>> carry square brackets around their name: > >>>>>> > >>>>>> [eventfd] > >>>>>> [eventpoll] > >>>>>> [fanotify] > >>>>>> [fscontext] > >>>>>> [io_uring] > >>>>>> [pidfd] > >>>>>> [signalfd] > >>>>>> [timerfd] > >>>>>> [userfaultfd] > >>>>>> > >>>>>> For the sake of consistency lets do the same for the landlock-ruleset anon > >>>>>> inode fd that comes with landlock. We did the same in > >>>>>> 1cdc415f1083 ("uapi, fsopen: use square brackets around "fscontext" [ver #2]") > >>>>>> for the new mount api. > >>>>> > >>>>> Before creating "landlock-ruleset" FD, I looked at other anonymous FD > >>>>> and saw this kind of inconsistency. I don't get why we need to add extra > >>>>> characters to names, those brackets seem useless. If it should be part > >>>> > >>>> Past inconsistency shouldn't justify future inconsistency. If you have a > >>>> strong opinion about this for landlock I'm not going to push for it. > >>>> Exchanging more than 2-3 email about something like this seems too much. > >>> > >>> [NOTE: adding the SELinux list as well as Chris (SELinux refrence > >>> policy maintainer) and Petr (Fedora/RHEL SELinux)] > >>> > >>> Chris and Petr, do either of you currently have any policy that > >>> references the "landlock-ruleset" anonymous inode? In other words, > >>> would adding the brackets around the name cause you any problems? > >> > >> AFAIU, the anon_inode transitions (the only mechanism where the "file > >> name" would be exposed to the policy) are done only for inodes created > >> by anon_inode_getfd_secure(), which is currently only used by > >> userfaultfd. So you don't even need to ask that question; at this > >> point it should be safe to change any of the names except > >> "[userfaultfd]" as far as SELinux policy is concerned. > > > > There is also io_uring if you look at selinux/next. > > > > Regardless, thanks, I didn't check to see if landlock was using the > > new anon inode interface, since both Mickaël and Christian were > > concerned about breaking SELinux I had assumed they were using it :) > > > > Ok, thanks Paul and Ondrej. > > Such anonymous inode names seem to be only exposed to proc for now. > Let's change this name then. I think it make sense to backport this > patch down to 5.13 to fix all the inconsistencies. Thank you. I do appreciate the point about this being annoying that we have this inconsistency and it has bothered me too. Christian
CCing linux-api and stable to give them a chance to confirm that changing proc symlink content is OK. On 15/10/2021 11:10, Christian Brauner wrote: > On Wed, Oct 13, 2021 at 05:47:53PM +0200, Mickaël Salaün wrote: >> >> On 12/10/2021 23:09, Paul Moore wrote: >>> On Tue, Oct 12, 2021 at 4:38 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: >>>> >>>> On Tue, Oct 12, 2021 at 8:12 PM Paul Moore <paul@paul-moore.com> wrote: >>>>> On Tue, Oct 12, 2021 at 6:38 AM Christian Brauner >>>>> <christian.brauner@ubuntu.com> wrote: >>>>>> On Mon, Oct 11, 2021 at 04:38:55PM +0200, Mickaël Salaün wrote: >>>>>>> On 11/10/2021 15:37, Christian Brauner wrote: >>>>>>>> From: Christian Brauner <christian.brauner@ubuntu.com> >>>>>>>> >>>>>>>> Make the name of the anon inode fd "[landlock-ruleset]" instead of >>>>>>>> "landlock-ruleset". This is minor but most anon inode fds already >>>>>>>> carry square brackets around their name: >>>>>>>> >>>>>>>> [eventfd] >>>>>>>> [eventpoll] >>>>>>>> [fanotify] >>>>>>>> [fscontext] >>>>>>>> [io_uring] >>>>>>>> [pidfd] >>>>>>>> [signalfd] >>>>>>>> [timerfd] >>>>>>>> [userfaultfd] >>>>>>>> >>>>>>>> For the sake of consistency lets do the same for the landlock-ruleset anon >>>>>>>> inode fd that comes with landlock. We did the same in >>>>>>>> 1cdc415f1083 ("uapi, fsopen: use square brackets around "fscontext" [ver #2]") >>>>>>>> for the new mount api. >>>>>>> >>>>>>> Before creating "landlock-ruleset" FD, I looked at other anonymous FD >>>>>>> and saw this kind of inconsistency. I don't get why we need to add extra >>>>>>> characters to names, those brackets seem useless. If it should be part >>>>>> >>>>>> Past inconsistency shouldn't justify future inconsistency. If you have a >>>>>> strong opinion about this for landlock I'm not going to push for it. >>>>>> Exchanging more than 2-3 email about something like this seems too much. >>>>> >>>>> [NOTE: adding the SELinux list as well as Chris (SELinux refrence >>>>> policy maintainer) and Petr (Fedora/RHEL SELinux)] >>>>> >>>>> Chris and Petr, do either of you currently have any policy that >>>>> references the "landlock-ruleset" anonymous inode? In other words, >>>>> would adding the brackets around the name cause you any problems? >>>> >>>> AFAIU, the anon_inode transitions (the only mechanism where the "file >>>> name" would be exposed to the policy) are done only for inodes created >>>> by anon_inode_getfd_secure(), which is currently only used by >>>> userfaultfd. So you don't even need to ask that question; at this >>>> point it should be safe to change any of the names except >>>> "[userfaultfd]" as far as SELinux policy is concerned. >>> >>> There is also io_uring if you look at selinux/next. >>> >>> Regardless, thanks, I didn't check to see if landlock was using the >>> new anon inode interface, since both Mickaël and Christian were >>> concerned about breaking SELinux I had assumed they were using it :) >>> >> >> Ok, thanks Paul and Ondrej. >> >> Such anonymous inode names seem to be only exposed to proc for now. >> Let's change this name then. I think it make sense to backport this >> patch down to 5.13 to fix all the inconsistencies. > > Thank you. I do appreciate the point about this being annoying that we > have this inconsistency and it has bothered me too. > > Christian >
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c index 32396962f04d..7e27ce394020 100644 --- a/security/landlock/syscalls.c +++ b/security/landlock/syscalls.c @@ -192,7 +192,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset, return PTR_ERR(ruleset); /* Creates anonymous FD referring to the ruleset. */ - ruleset_fd = anon_inode_getfd("landlock-ruleset", &ruleset_fops, + ruleset_fd = anon_inode_getfd("[landlock-ruleset]", &ruleset_fops, ruleset, O_RDWR | O_CLOEXEC); if (ruleset_fd < 0) landlock_put_ruleset(ruleset);