[PATCH] security: fix the logic in security_inode_getsecctx()

[PATCH] security: fix the logic in security_inode_getsecctx()

[Ondrej Mosnacek]

The inode_getsecctx LSM hook has previously been corrected to have
-EOPNOTSUPP instead of 0 as the default return value to fix BPF LSM
behavior. However, the call_int_hook()-generated loop in
security_inode_getsecctx() was left treating 0 as the neutral value, so
after an LSM returns 0, the loop continues to try other LSMs, and if one
of them returns a non-zero value, the function immediately returns with
said value. So in a situation where SELinux and the BPF LSMs registered
this hook, -EOPNOTSUPP would be incorrectly returned whenever SELinux
returned 0.

Fix this by open-coding the call_int_hook() loop and making it use the
correct LSM_RET_DEFAULT() value as the neutral one, similar to what
other hooks do.

Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Link: https://lore.kernel.org/selinux/CAEjxPJ4ev-pasUwGx48fDhnmjBnq_Wh90jYPwRQRAqXxmOKD4Q@mail.gmail.com/
Fixes: b36995b8609a ("lsm: fix default return value for inode_getsecctx")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---

I ran 'tools/nfs.sh' on the patch and even though it fixes the most
serious issue that Stephen reported, some of the tests are still
failing under NFS (but I will presume that these are pre-existing issues
not caused by the patch).

I can also see an opportunity to clean up the hook implementations in
security/security.c - I plan to have a go at it and send it as a
separate patch later.

 security/security.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/security/security.c b/security/security.c
index 0144a98d3712..6196ccaba433 100644
--- a/security/security.c
+++ b/security/security.c
@@ -4255,7 +4255,19 @@ EXPORT_SYMBOL(security_inode_setsecctx);
  */
 int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
 {
-	return call_int_hook(inode_getsecctx, -EOPNOTSUPP, inode, ctx, ctxlen);
+	struct security_hook_list *hp;
+	int rc;
+
+	/*
+	 * Only one module will provide a security context.
+	 */
+	hlist_for_each_entry(hp, &security_hook_heads.inode_getsecctx, list) {
+		rc = hp->hook.inode_getsecctx(inode, ctx, ctxlen);
+		if (rc != LSM_RET_DEFAULT(inode_getsecctx))
+			return rc;
+	}
+
+	return LSM_RET_DEFAULT(inode_getsecctx);
 }
 EXPORT_SYMBOL(security_inode_getsecctx);
 
-- 
2.43.0

Re: [PATCH] security: fix the logic in security_inode_getsecctx()

[Ondrej Mosnacek]

On Fri, Jan 26, 2024 at 11:44 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> The inode_getsecctx LSM hook has previously been corrected to have
> -EOPNOTSUPP instead of 0 as the default return value to fix BPF LSM
> behavior. However, the call_int_hook()-generated loop in
> security_inode_getsecctx() was left treating 0 as the neutral value, so
> after an LSM returns 0, the loop continues to try other LSMs, and if one
> of them returns a non-zero value, the function immediately returns with
> said value. So in a situation where SELinux and the BPF LSMs registered
> this hook, -EOPNOTSUPP would be incorrectly returned whenever SELinux
> returned 0.
>
> Fix this by open-coding the call_int_hook() loop and making it use the
> correct LSM_RET_DEFAULT() value as the neutral one, similar to what
> other hooks do.
>
> Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Link: https://lore.kernel.org/selinux/CAEjxPJ4ev-pasUwGx48fDhnmjBnq_Wh90jYPwRQRAqXxmOKD4Q@mail.gmail.com/

Actually, I should have also added:

Link: https://bugzilla.redhat.com/show_bug.cgi?id=2257983

Hopefully it can be added when applying if there isn't going to be a respin.

> Fixes: b36995b8609a ("lsm: fix default return value for inode_getsecctx")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>
> I ran 'tools/nfs.sh' on the patch and even though it fixes the most
> serious issue that Stephen reported, some of the tests are still
> failing under NFS (but I will presume that these are pre-existing issues
> not caused by the patch).
>
> I can also see an opportunity to clean up the hook implementations in
> security/security.c - I plan to have a go at it and send it as a
> separate patch later.
>
>  security/security.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/security/security.c b/security/security.c
> index 0144a98d3712..6196ccaba433 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -4255,7 +4255,19 @@ EXPORT_SYMBOL(security_inode_setsecctx);
>   */
>  int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
>  {
> -       return call_int_hook(inode_getsecctx, -EOPNOTSUPP, inode, ctx, ctxlen);
> +       struct security_hook_list *hp;
> +       int rc;
> +
> +       /*
> +        * Only one module will provide a security context.
> +        */
> +       hlist_for_each_entry(hp, &security_hook_heads.inode_getsecctx, list) {
> +               rc = hp->hook.inode_getsecctx(inode, ctx, ctxlen);
> +               if (rc != LSM_RET_DEFAULT(inode_getsecctx))
> +                       return rc;
> +       }
> +
> +       return LSM_RET_DEFAULT(inode_getsecctx);
>  }
>  EXPORT_SYMBOL(security_inode_getsecctx);
>
> --
> 2.43.0
>


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

Re: [PATCH] security: fix the logic in security_inode_getsecctx()

[Stephen Smalley]

On Fri, Jan 26, 2024 at 5:44 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> The inode_getsecctx LSM hook has previously been corrected to have
> -EOPNOTSUPP instead of 0 as the default return value to fix BPF LSM
> behavior. However, the call_int_hook()-generated loop in
> security_inode_getsecctx() was left treating 0 as the neutral value, so
> after an LSM returns 0, the loop continues to try other LSMs, and if one
> of them returns a non-zero value, the function immediately returns with
> said value. So in a situation where SELinux and the BPF LSMs registered
> this hook, -EOPNOTSUPP would be incorrectly returned whenever SELinux
> returned 0.
>
> Fix this by open-coding the call_int_hook() loop and making it use the
> correct LSM_RET_DEFAULT() value as the neutral one, similar to what
> other hooks do.
>
> Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Link: https://lore.kernel.org/selinux/CAEjxPJ4ev-pasUwGx48fDhnmjBnq_Wh90jYPwRQRAqXxmOKD4Q@mail.gmail.com/
> Fixes: b36995b8609a ("lsm: fix default return value for inode_getsecctx")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>
> I ran 'tools/nfs.sh' on the patch and even though it fixes the most
> serious issue that Stephen reported, some of the tests are still
> failing under NFS (but I will presume that these are pre-existing issues
> not caused by the patch).

Do you have a list of the failing tests? For me, it was hanging on
unix_socket and thus not getting to many of the tests. I would like to
triage the still-failing ones to confirm that they are in fact
known/expected failures for NFS.

Re: [PATCH] security: fix the logic in security_inode_getsecctx()

[Stephen Smalley]

On Fri, Jan 26, 2024 at 10:03 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jan 26, 2024 at 5:44 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > The inode_getsecctx LSM hook has previously been corrected to have
> > -EOPNOTSUPP instead of 0 as the default return value to fix BPF LSM
> > behavior. However, the call_int_hook()-generated loop in
> > security_inode_getsecctx() was left treating 0 as the neutral value, so
> > after an LSM returns 0, the loop continues to try other LSMs, and if one
> > of them returns a non-zero value, the function immediately returns with
> > said value. So in a situation where SELinux and the BPF LSMs registered
> > this hook, -EOPNOTSUPP would be incorrectly returned whenever SELinux
> > returned 0.
> >
> > Fix this by open-coding the call_int_hook() loop and making it use the
> > correct LSM_RET_DEFAULT() value as the neutral one, similar to what
> > other hooks do.
> >
> > Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > Link: https://lore.kernel.org/selinux/CAEjxPJ4ev-pasUwGx48fDhnmjBnq_Wh90jYPwRQRAqXxmOKD4Q@mail.gmail.com/
> > Fixes: b36995b8609a ("lsm: fix default return value for inode_getsecctx")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >
> > I ran 'tools/nfs.sh' on the patch and even though it fixes the most
> > serious issue that Stephen reported, some of the tests are still
> > failing under NFS (but I will presume that these are pre-existing issues
> > not caused by the patch).
>
> Do you have a list of the failing tests? For me, it was hanging on
> unix_socket and thus not getting to many of the tests. I would like to
> triage the still-failing ones to confirm that they are in fact
> known/expected failures for NFS.

Applying your patch and removing unix_socket from the tests to be run
(since it hangs), I get the following failures:
mac_admin/test            (Wstat: 0 Tests: 8 Failed: 2)
  Failed tests:  5-6
filesystem/ext4/test      (Wstat: 512 (exited 2) Tests: 76 Failed: 2)
  Failed tests:  1, 64
  Non-zero exit status: 2
filesystem/xfs/test       (Wstat: 512 (exited 2) Tests: 76 Failed: 2)
  Failed tests:  1, 64
  Non-zero exit status: 2
filesystem/jfs/test       (Wstat: 512 (exited 2) Tests: 83 Failed: 2)
  Failed tests:  1, 71
  Non-zero exit status: 2
filesystem/vfat/test      (Wstat: 512 (exited 2) Tests: 52 Failed: 2)
  Failed tests:  1, 46
  Non-zero exit status: 2
fs_filesystem/ext4/test   (Wstat: 512 (exited 2) Tests: 75 Failed: 2)
  Failed tests:  1, 63
  Non-zero exit status: 2
fs_filesystem/xfs/test    (Wstat: 512 (exited 2) Tests: 75 Failed: 2)
  Failed tests:  1, 63
  Non-zero exit status: 2
fs_filesystem/jfs/test    (Wstat: 512 (exited 2) Tests: 82 Failed: 2)
  Failed tests:  1, 70
  Non-zero exit status: 2
fs_filesystem/vfat/test   (Wstat: 512 (exited 2) Tests: 51 Failed: 2)
  Failed tests:  1, 45
  Non-zero exit status: 2
Files=77, Tests=1256, 308 wallclock secs ( 0.30 usr  0.10 sys +  6.84
cusr 21.78 csys = 29.02 CPU)

mac_admin one is unsurprising for NFS. Will look at the others but
suspect those too are just quirks with performing tests of other
filesystems over NFS. Should be able to modify the test scripts to
skip all of these on NFS and thereby allow clean runs. unix_socket
hang though bears investigation, because I think that used to work.

Re: [PATCH] security: fix the logic in security_inode_getsecctx()

[Ondrej Mosnacek]

On Fri, Jan 26, 2024 at 5:04 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, Jan 26, 2024 at 10:03 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Jan 26, 2024 at 5:44 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > The inode_getsecctx LSM hook has previously been corrected to have
> > > -EOPNOTSUPP instead of 0 as the default return value to fix BPF LSM
> > > behavior. However, the call_int_hook()-generated loop in
> > > security_inode_getsecctx() was left treating 0 as the neutral value, so
> > > after an LSM returns 0, the loop continues to try other LSMs, and if one
> > > of them returns a non-zero value, the function immediately returns with
> > > said value. So in a situation where SELinux and the BPF LSMs registered
> > > this hook, -EOPNOTSUPP would be incorrectly returned whenever SELinux
> > > returned 0.
> > >
> > > Fix this by open-coding the call_int_hook() loop and making it use the
> > > correct LSM_RET_DEFAULT() value as the neutral one, similar to what
> > > other hooks do.
> > >
> > > Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > > Link: https://lore.kernel.org/selinux/CAEjxPJ4ev-pasUwGx48fDhnmjBnq_Wh90jYPwRQRAqXxmOKD4Q@mail.gmail.com/
> > > Fixes: b36995b8609a ("lsm: fix default return value for inode_getsecctx")
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >
> > > I ran 'tools/nfs.sh' on the patch and even though it fixes the most
> > > serious issue that Stephen reported, some of the tests are still
> > > failing under NFS (but I will presume that these are pre-existing issues
> > > not caused by the patch).
> >
> > Do you have a list of the failing tests? For me, it was hanging on
> > unix_socket and thus not getting to many of the tests. I would like to
> > triage the still-failing ones to confirm that they are in fact
> > known/expected failures for NFS.
>
> Applying your patch and removing unix_socket from the tests to be run
> (since it hangs), I get the following failures:
> mac_admin/test            (Wstat: 0 Tests: 8 Failed: 2)
>   Failed tests:  5-6
> filesystem/ext4/test      (Wstat: 512 (exited 2) Tests: 76 Failed: 2)
>   Failed tests:  1, 64
>   Non-zero exit status: 2
> filesystem/xfs/test       (Wstat: 512 (exited 2) Tests: 76 Failed: 2)
>   Failed tests:  1, 64
>   Non-zero exit status: 2
> filesystem/jfs/test       (Wstat: 512 (exited 2) Tests: 83 Failed: 2)
>   Failed tests:  1, 71
>   Non-zero exit status: 2
> filesystem/vfat/test      (Wstat: 512 (exited 2) Tests: 52 Failed: 2)
>   Failed tests:  1, 46
>   Non-zero exit status: 2
> fs_filesystem/ext4/test   (Wstat: 512 (exited 2) Tests: 75 Failed: 2)
>   Failed tests:  1, 63
>   Non-zero exit status: 2
> fs_filesystem/xfs/test    (Wstat: 512 (exited 2) Tests: 75 Failed: 2)
>   Failed tests:  1, 63
>   Non-zero exit status: 2
> fs_filesystem/jfs/test    (Wstat: 512 (exited 2) Tests: 82 Failed: 2)
>   Failed tests:  1, 70
>   Non-zero exit status: 2
> fs_filesystem/vfat/test   (Wstat: 512 (exited 2) Tests: 51 Failed: 2)
>   Failed tests:  1, 45
>   Non-zero exit status: 2
> Files=77, Tests=1256, 308 wallclock secs ( 0.30 usr  0.10 sys +  6.84
> cusr 21.78 csys = 29.02 CPU)

I got the same ones (I, too, removed unix_socket to allow the rest to run).

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

Re: [PATCH] security: fix the logic in security_inode_getsecctx()

[Stephen Smalley]

On Fri, Jan 26, 2024 at 12:15 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Fri, Jan 26, 2024 at 5:04 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, Jan 26, 2024 at 10:03 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Fri, Jan 26, 2024 at 5:44 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > >
> > > > The inode_getsecctx LSM hook has previously been corrected to have
> > > > -EOPNOTSUPP instead of 0 as the default return value to fix BPF LSM
> > > > behavior. However, the call_int_hook()-generated loop in
> > > > security_inode_getsecctx() was left treating 0 as the neutral value, so
> > > > after an LSM returns 0, the loop continues to try other LSMs, and if one
> > > > of them returns a non-zero value, the function immediately returns with
> > > > said value. So in a situation where SELinux and the BPF LSMs registered
> > > > this hook, -EOPNOTSUPP would be incorrectly returned whenever SELinux
> > > > returned 0.
> > > >
> > > > Fix this by open-coding the call_int_hook() loop and making it use the
> > > > correct LSM_RET_DEFAULT() value as the neutral one, similar to what
> > > > other hooks do.
> > > >
> > > > Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> > > > Link: https://lore.kernel.org/selinux/CAEjxPJ4ev-pasUwGx48fDhnmjBnq_Wh90jYPwRQRAqXxmOKD4Q@mail.gmail.com/
> > > > Fixes: b36995b8609a ("lsm: fix default return value for inode_getsecctx")
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > ---
> > > >
> > > > I ran 'tools/nfs.sh' on the patch and even though it fixes the most
> > > > serious issue that Stephen reported, some of the tests are still
> > > > failing under NFS (but I will presume that these are pre-existing issues
> > > > not caused by the patch).
> > >
> > > Do you have a list of the failing tests? For me, it was hanging on
> > > unix_socket and thus not getting to many of the tests. I would like to
> > > triage the still-failing ones to confirm that they are in fact
> > > known/expected failures for NFS.
> >
> > Applying your patch and removing unix_socket from the tests to be run
> > (since it hangs), I get the following failures:
> > mac_admin/test            (Wstat: 0 Tests: 8 Failed: 2)
> >   Failed tests:  5-6
> > filesystem/ext4/test      (Wstat: 512 (exited 2) Tests: 76 Failed: 2)
> >   Failed tests:  1, 64
> >   Non-zero exit status: 2
> > filesystem/xfs/test       (Wstat: 512 (exited 2) Tests: 76 Failed: 2)
> >   Failed tests:  1, 64
> >   Non-zero exit status: 2
> > filesystem/jfs/test       (Wstat: 512 (exited 2) Tests: 83 Failed: 2)
> >   Failed tests:  1, 71
> >   Non-zero exit status: 2
> > filesystem/vfat/test      (Wstat: 512 (exited 2) Tests: 52 Failed: 2)
> >   Failed tests:  1, 46
> >   Non-zero exit status: 2
> > fs_filesystem/ext4/test   (Wstat: 512 (exited 2) Tests: 75 Failed: 2)
> >   Failed tests:  1, 63
> >   Non-zero exit status: 2
> > fs_filesystem/xfs/test    (Wstat: 512 (exited 2) Tests: 75 Failed: 2)
> >   Failed tests:  1, 63
> >   Non-zero exit status: 2
> > fs_filesystem/jfs/test    (Wstat: 512 (exited 2) Tests: 82 Failed: 2)
> >   Failed tests:  1, 70
> >   Non-zero exit status: 2
> > fs_filesystem/vfat/test   (Wstat: 512 (exited 2) Tests: 51 Failed: 2)
> >   Failed tests:  1, 45
> >   Non-zero exit status: 2
> > Files=77, Tests=1256, 308 wallclock secs ( 0.30 usr  0.10 sys +  6.84
> > cusr 21.78 csys = 29.02 CPU)
>
> I got the same ones (I, too, removed unix_socket to allow the rest to run).

unix_socket test is failing because type_transition rule is not being
applied to newly created server socket, leading to a denial when the
client tries to connect. I believe that once worked; will see if I can
find the last working kernel.

Re: [PATCH] security: fix the logic in security_inode_getsecctx()

[Paul Moore]

On Mon, Jan 29, 2024 at 2:49 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> unix_socket test is failing because type_transition rule is not being
> applied to newly created server socket, leading to a denial when the
> client tries to connect. I believe that once worked; will see if I can
> find the last working kernel.

If we had a socket type transition on new connections I think it would
have been a *long* time ago.  I don't recall us supporting that, but
it's possible I've simply forgotten.

That isn't to say I wouldn't support something like that, it could be
interesting, but we would want to make sure it applies to all
connection based sockets and not just AF_UNIX.  Although for the vast
majority of users it would probably only be useful for AF_UNIX as you
would need a valid peer label to do a meaningful transition.

I would need to chase down the code paths for AF_UNIX, but for
AF_INET/AF_INET6 I expect you would need to augment
selinux_inet_conn_request() with the security_transition_sid() call.
Possibly something like this (completely untested, likely broken,
etc.) ...

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a6bf90ace84c..1c6a92173596 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5524,7 +5524,10 @@ static int selinux_inet_conn_request(const struct sock *s
k, struct sk_buff *skb,
       err = selinux_conn_sid(sksec->sid, peersid, &connsid);
       if (err)
               return err;
-       req->secid = connsid;
+       err = security_transition_sid(sksec->sid, connsid, sksec->sclass, NULL,
+                                     &req->secid);
+       if (err)
+               return err;
       req->peer_secid = peersid;

       return selinux_netlbl_inet_conn_request(req, family);

-- 
paul-moore.com

Re: [PATCH] security: fix the logic in security_inode_getsecctx()

[Stephen Smalley]

On Mon, Jan 29, 2024 at 4:56 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Jan 29, 2024 at 2:49 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > unix_socket test is failing because type_transition rule is not being
> > applied to newly created server socket, leading to a denial when the
> > client tries to connect. I believe that once worked; will see if I can
> > find the last working kernel.
>
> If we had a socket type transition on new connections I think it would
> have been a *long* time ago.  I don't recall us supporting that, but
> it's possible I've simply forgotten.
>
> That isn't to say I wouldn't support something like that, it could be
> interesting, but we would want to make sure it applies to all
> connection based sockets and not just AF_UNIX.  Although for the vast
> majority of users it would probably only be useful for AF_UNIX as you
> would need a valid peer label to do a meaningful transition.

Sorry, I probably wasn't clear. I mean that the Unix socket files are
NOT being labeled in accordance with the type_transition rules in
policy. Which does work on local file systems and used to work on NFS,
so this is a regression at some point (but not new to Ondrej's patch).

Re: [PATCH] security: fix the logic in security_inode_getsecctx()

[Paul Moore]

On Tue, Jan 30, 2024 at 10:44 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Mon, Jan 29, 2024 at 4:56 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Mon, Jan 29, 2024 at 2:49 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > > unix_socket test is failing because type_transition rule is not being
> > > applied to newly created server socket, leading to a denial when the
> > > client tries to connect. I believe that once worked; will see if I can
> > > find the last working kernel.
> >
> > If we had a socket type transition on new connections I think it would
> > have been a *long* time ago.  I don't recall us supporting that, but
> > it's possible I've simply forgotten.
> >
> > That isn't to say I wouldn't support something like that, it could be
> > interesting, but we would want to make sure it applies to all
> > connection based sockets and not just AF_UNIX.  Although for the vast
> > majority of users it would probably only be useful for AF_UNIX as you
> > would need a valid peer label to do a meaningful transition.
>
> Sorry, I probably wasn't clear. I mean that the Unix socket files are
> NOT being labeled in accordance with the type_transition rules in
> policy. Which does work on local file systems and used to work on NFS,
> so this is a regression at some point (but not new to Ondrej's patch).

Ah, gotcha.

I guess I'm not too surprised, the sock/socket/inode labeling and
duplication has always been very awkward and it wouldn't surprise me
if we inadvertently broke something over the years.  Tracking down the
source of the breakage is good, but if that is taking too long (I can
only imagine how long that might take), I would be happy with a fix
with a number of comment additions warning future devs against
changing the relevant code.

-- 
paul-moore.com

Re: [PATCH] security: fix the logic in security_inode_getsecctx()

[Casey Schaufler]

On 1/26/2024 2:44 AM, Ondrej Mosnacek wrote:
> The inode_getsecctx LSM hook has previously been corrected to have
> -EOPNOTSUPP instead of 0 as the default return value to fix BPF LSM
> behavior. However, the call_int_hook()-generated loop in
> security_inode_getsecctx() was left treating 0 as the neutral value, so
> after an LSM returns 0, the loop continues to try other LSMs, and if one
> of them returns a non-zero value, the function immediately returns with
> said value. So in a situation where SELinux and the BPF LSMs registered
> this hook, -EOPNOTSUPP would be incorrectly returned whenever SELinux
> returned 0.
>
> Fix this by open-coding the call_int_hook() loop and making it use the
> correct LSM_RET_DEFAULT() value as the neutral one, similar to what
> other hooks do.
>
> Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Link: https://lore.kernel.org/selinux/CAEjxPJ4ev-pasUwGx48fDhnmjBnq_Wh90jYPwRQRAqXxmOKD4Q@mail.gmail.com/
> Fixes: b36995b8609a ("lsm: fix default return value for inode_getsecctx")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>
> I ran 'tools/nfs.sh' on the patch and even though it fixes the most
> serious issue that Stephen reported, some of the tests are still
> failing under NFS (but I will presume that these are pre-existing issues
> not caused by the patch).
>
> I can also see an opportunity to clean up the hook implementations in
> security/security.c - I plan to have a go at it and send it as a
> separate patch later.
>
>  security/security.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/security/security.c b/security/security.c
> index 0144a98d3712..6196ccaba433 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -4255,7 +4255,19 @@ EXPORT_SYMBOL(security_inode_setsecctx);
>   */
>  int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
>  {
> -	return call_int_hook(inode_getsecctx, -EOPNOTSUPP, inode, ctx, ctxlen);
> +	struct security_hook_list *hp;
> +	int rc;
> +
> +	/*
> +	 * Only one module will provide a security context.
> +	 */
> +	hlist_for_each_entry(hp, &security_hook_heads.inode_getsecctx, list) {
> +		rc = hp->hook.inode_getsecctx(inode, ctx, ctxlen);
> +		if (rc != LSM_RET_DEFAULT(inode_getsecctx))
> +			return rc;
> +	}
> +
> +	return LSM_RET_DEFAULT(inode_getsecctx);
>  }
>  EXPORT_SYMBOL(security_inode_getsecctx);
>  

Re: [PATCH] security: fix the logic in security_inode_getsecctx()

[Paul Moore]

On Jan 26, 2024 Ondrej Mosnacek <omosnace@redhat.com> wrote:
> 
> The inode_getsecctx LSM hook has previously been corrected to have
> -EOPNOTSUPP instead of 0 as the default return value to fix BPF LSM
> behavior. However, the call_int_hook()-generated loop in
> security_inode_getsecctx() was left treating 0 as the neutral value, so
> after an LSM returns 0, the loop continues to try other LSMs, and if one
> of them returns a non-zero value, the function immediately returns with
> said value. So in a situation where SELinux and the BPF LSMs registered
> this hook, -EOPNOTSUPP would be incorrectly returned whenever SELinux
> returned 0.
> 
> Fix this by open-coding the call_int_hook() loop and making it use the
> correct LSM_RET_DEFAULT() value as the neutral one, similar to what
> other hooks do.
> 
> Reported-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Link: https://lore.kernel.org/selinux/CAEjxPJ4ev-pasUwGx48fDhnmjBnq_Wh90jYPwRQRAqXxmOKD4Q@mail.gmail.com/
> Fixes: b36995b8609a ("lsm: fix default return value for inode_getsecctx")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2257983
> ---
> 
> I ran 'tools/nfs.sh' on the patch and even though it fixes the most
> serious issue that Stephen reported, some of the tests are still
> failing under NFS (but I will presume that these are pre-existing issues
> not caused by the patch).
> 
> I can also see an opportunity to clean up the hook implementations in
> security/security.c - I plan to have a go at it and send it as a
> separate patch later.
> 
>  security/security.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

Merged, with the RHBZ link tag, into lsm/stable-6.8.  I've also added a
stable tag/Cc should this should get picked up by the stable folks to
fix the breakage in the recent stable kernel releases.

Assuming no problems are uncovered over the weekend and early next week,
I'll send this to Linus next week.

Thanks everyone!

--
paul-moore.com