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 <[email protected]>
---
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 <[email protected]> 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 <[email protected]>
> ---
> 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 */
--
paul-moore.com
On 7/17/2023 1:13 PM, Paul Moore wrote:
> On Mon, Jul 10, 2023 at 4:25 AM Leesoo Ahn <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]> wrote:
> On 7/17/2023 1:13 PM, Paul Moore wrote:
> > On Mon, Jul 10, 2023 at 4:25 AM Leesoo Ahn <[email protected]> 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 <[email protected]>
> >> ---
> >> 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.
--
paul-moore.com
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 <[email protected]>
> ---
> 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".