| Message ID | 20230710082500.1838896-1-lsahn@wewakecorp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: optimize major part with a kernel config in selinux_mmap_addr() | expand |
On Mon, Jul 10, 2023 at 4:25 AM Leesoo Ahn <lsahn@ooseel.net> wrote: > > The major part, the conditional branch in selinux_mmap_addr() is always to be > false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time. > > This usually happens in some linux distros, for instance Ubuntu, which > the config is set to zero in release version. Therefore it could be a bit > optimized with '#if <expr>' at compile time. If your distro is configuring LSM_MMAP_MIN_ADDR to 0, you might want to bug them about it, because that's not a great idea for security. And if you intend to use SELinux there, you'll want it set higher. Default value in the Kconfig is 65536. > > Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com> > --- > security/selinux/hooks.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index d06e350fedee..a049aab6524b 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr) > { > int rc = 0; > > +#if CONFIG_LSM_MMAP_MIN_ADDR > 0 > if (addr < CONFIG_LSM_MMAP_MIN_ADDR) { > u32 sid = current_sid(); > rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT, > MEMPROTECT__MMAP_ZERO, NULL); > } > +#endif > > return rc; > } > -- > 2.34.1 >
On Mon, Jul 10, 2023 at 4:25 AM Leesoo Ahn <lsahn@ooseel.net> wrote: > > The major part, the conditional branch in selinux_mmap_addr() is always to be > false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time. > > This usually happens in some linux distros, for instance Ubuntu, which > the config is set to zero in release version. Therefore it could be a bit > optimized with '#if <expr>' at compile time. > > Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com> > --- > security/selinux/hooks.c | 2 ++ > 1 file changed, 2 insertions(+) First, I agree with Stephen's comments that you should ask your distro (you mentioned Debian) to move MIN_ADDR higher. Beyond that, I have one request, see below ... > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index d06e350fedee..a049aab6524b 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr) > { > int rc = 0; > > +#if CONFIG_LSM_MMAP_MIN_ADDR > 0 > if (addr < CONFIG_LSM_MMAP_MIN_ADDR) { > u32 sid = current_sid(); > rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT, > MEMPROTECT__MMAP_ZERO, NULL); > } > +#endif > > return rc; > } Pre-processor conditionals inside a function are generally something we don't recommend. In this case I would suggest doing something like this: #if (MMAP_MIN_ADDR > 0) static int selinux_mmap_addr(...) { /* current func definition */ } #else /* MMAP_MIN_ADDR > 0 */ static int selinux_mmap_addr(...) { return 0; } #endif /* MMAP_MIN_ADDR > 0 */
On 7/17/2023 1:13 PM, Paul Moore wrote: > On Mon, Jul 10, 2023 at 4:25 AM Leesoo Ahn <lsahn@ooseel.net> wrote: >> The major part, the conditional branch in selinux_mmap_addr() is always to be >> false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time. >> >> This usually happens in some linux distros, for instance Ubuntu, which >> the config is set to zero in release version. Therefore it could be a bit >> optimized with '#if <expr>' at compile time. >> >> Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com> >> --- >> security/selinux/hooks.c | 2 ++ >> 1 file changed, 2 insertions(+) > First, I agree with Stephen's comments that you should ask your distro > (you mentioned Debian) to move MIN_ADDR higher. Beyond that, I have > one request, see below ... > >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index d06e350fedee..a049aab6524b 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr) >> { >> int rc = 0; >> >> +#if CONFIG_LSM_MMAP_MIN_ADDR > 0 >> if (addr < CONFIG_LSM_MMAP_MIN_ADDR) { >> u32 sid = current_sid(); >> rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT, >> MEMPROTECT__MMAP_ZERO, NULL); >> } >> +#endif >> >> return rc; >> } > Pre-processor conditionals inside a function are generally something > we don't recommend. In this case I would suggest doing something like > this: > > #if (MMAP_MIN_ADDR > 0) > static int selinux_mmap_addr(...) > { > /* current func definition */ > } > #else /* MMAP_MIN_ADDR > 0 */ > static int selinux_mmap_addr(...) > { > return 0; > } > #endif /* MMAP_MIN_ADDR > 0 */ Better yet, skip the #else here and #if out the LSM_HOOK_INIT(mmap_addr, ...). No hook at all is faster than a hook that does nothing.
On Mon, Jul 17, 2023 at 4:31 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 7/17/2023 1:13 PM, Paul Moore wrote: > > On Mon, Jul 10, 2023 at 4:25 AM Leesoo Ahn <lsahn@ooseel.net> wrote: > >> The major part, the conditional branch in selinux_mmap_addr() is always to be > >> false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time. > >> > >> This usually happens in some linux distros, for instance Ubuntu, which > >> the config is set to zero in release version. Therefore it could be a bit > >> optimized with '#if <expr>' at compile time. > >> > >> Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com> > >> --- > >> security/selinux/hooks.c | 2 ++ > >> 1 file changed, 2 insertions(+) > > First, I agree with Stephen's comments that you should ask your distro > > (you mentioned Debian) to move MIN_ADDR higher. Beyond that, I have > > one request, see below ... > > > >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >> index d06e350fedee..a049aab6524b 100644 > >> --- a/security/selinux/hooks.c > >> +++ b/security/selinux/hooks.c > >> @@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr) > >> { > >> int rc = 0; > >> > >> +#if CONFIG_LSM_MMAP_MIN_ADDR > 0 > >> if (addr < CONFIG_LSM_MMAP_MIN_ADDR) { > >> u32 sid = current_sid(); > >> rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT, > >> MEMPROTECT__MMAP_ZERO, NULL); > >> } > >> +#endif > >> > >> return rc; > >> } > > Pre-processor conditionals inside a function are generally something > > we don't recommend. In this case I would suggest doing something like > > this: > > > > #if (MMAP_MIN_ADDR > 0) > > static int selinux_mmap_addr(...) > > { > > /* current func definition */ > > } > > #else /* MMAP_MIN_ADDR > 0 */ > > static int selinux_mmap_addr(...) > > { > > return 0; > > } > > #endif /* MMAP_MIN_ADDR > 0 */ > > Better yet, skip the #else here and #if out the LSM_HOOK_INIT(mmap_addr, ...). > No hook at all is faster than a hook that does nothing. My only concern with that approach is the disconnected nature: one ifdef around the func definition, one around the LSM_HOOK_INIT() call. If we thought a zero MMAP_MIN_ADDR value was a good idea, or even common, I would be more inclined to pay the bad-code-practices-tax here, but seeing as we don't want to encourage a zero MMAP_MIN_ADDR value I'd rather lean towards the more maintainable code.
On 2023-07-10 17:25:00+0900, Leesoo Ahn wrote: > The major part, the conditional branch in selinux_mmap_addr() is always to be > false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time. > > This usually happens in some linux distros, for instance Ubuntu, which > the config is set to zero in release version. Therefore it could be a bit > optimized with '#if <expr>' at compile time. > > Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com> > --- > security/selinux/hooks.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index d06e350fedee..a049aab6524b 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr) > { > int rc = 0; > > +#if CONFIG_LSM_MMAP_MIN_ADDR > 0 > if (addr < CONFIG_LSM_MMAP_MIN_ADDR) { > u32 sid = current_sid(); > rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT, > MEMPROTECT__MMAP_ZERO, NULL); > } > +#endif Shouldn't the compiler figure out on its own that "0 < 0" is always false and optimize it all away? My gcc 13.1.1 does so. Without your change: $ ./scripts/bloat-o-meter security/selinux/hooks.o-min-addr-64k security/selinux/hooks.o-min-addr-0 add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-65 (-65) Function old new delta selinux_mmap_addr 81 16 -65 Total: Before=57673, After=57608, chg -0.11% The same with your patch and also with the proposal by Paul that redefines the whole function to "return 0".
2023-07-18 오전 4:21에 Stephen Smalley 이(가) 쓴 글: > On Mon, Jul 10, 2023 at 4:25 AM Leesoo Ahn <lsahn@ooseel.net> wrote: > > > > The major part, the conditional branch in selinux_mmap_addr() is > always to be > > false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time. > > > > This usually happens in some linux distros, for instance Ubuntu, which > > the config is set to zero in release version. Therefore it could be a bit > > optimized with '#if <expr>' at compile time. > > If your distro is configuring LSM_MMAP_MIN_ADDR to 0, you might want > to bug them about it, because that's not a great idea for security. > And if you intend to use SELinux there, you'll want it set higher. > Default value in the Kconfig is 65536. Thank you all for feedbacks! I'm closing the door of this topic with them. Best regards, Leesoo
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d06e350fedee..a049aab6524b 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3723,11 +3723,13 @@ static int selinux_mmap_addr(unsigned long addr) { int rc = 0; +#if CONFIG_LSM_MMAP_MIN_ADDR > 0 if (addr < CONFIG_LSM_MMAP_MIN_ADDR) { u32 sid = current_sid(); rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT, MEMPROTECT__MMAP_ZERO, NULL); } +#endif return rc; }
The major part, the conditional branch in selinux_mmap_addr() is always to be false so long as CONFIG_LSM_MMAP_MIN_ADDR is set to zero at compile time. This usually happens in some linux distros, for instance Ubuntu, which the config is set to zero in release version. Therefore it could be a bit optimized with '#if <expr>' at compile time. Signed-off-by: Leesoo Ahn <lsahn@wewakecorp.com> --- security/selinux/hooks.c | 2 ++ 1 file changed, 2 insertions(+)