[PATCH] security/landlock: use square brackets around "landlock-ruleset"

[PATCH] security/landlock: use square brackets around "landlock-ruleset"

[Christian Brauner]

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.

Cc: Mickaël Salaün <mic@digikod.net>
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 security/landlock/syscalls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);

base-commit: 9e1ff307c779ce1f0f810c7ecce3d95bbae40896
-- 
2.30.2

Re: [PATCH] security/landlock: use square brackets around "landlock-ruleset"

[Mickaël Salaün]

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).

Re: [PATCH] security/landlock: use square brackets around "landlock-ruleset"

[Christian Brauner]

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

Re: [PATCH] security/landlock: use square brackets around "landlock-ruleset"

[Paul Moore]

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

Re: [PATCH] security/landlock: use square brackets around "landlock-ruleset"

[Ondrej Mosnacek]

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.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH] security/landlock: use square brackets around "landlock-ruleset"

[Paul Moore]

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 :)

-- 
paul moore
www.paul-moore.com

Re: [PATCH] security/landlock: use square brackets around "landlock-ruleset"

[Mickaël Salaün]

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.

Re: [PATCH] security/landlock: use square brackets around "landlock-ruleset"

[Christian Brauner]

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

Re: [PATCH] security/landlock: use square brackets around "landlock-ruleset"

[Mickaël Salaün]

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
>