* Re: [PATCH] integrity: Use current_uid() in integrity_audit_message()
From: Mimi Zohar @ 2020-08-24 20:33 UTC (permalink / raw)
To: Denis Efremov
Cc: James Morris, Serge E. Hallyn, Lakshmi Ramasubramanian,
linux-security-module, linux-kernel
In-Reply-To: <20200824125435.487194-1-efremov@linux.com>
On Mon, 2020-08-24 at 15:54 +0300, Denis Efremov wrote:
> Modify integrity_audit_message() to use current_uid().
>
> Signed-off-by: Denis Efremov <efremov@linux.com>
Thank you. This patch is now queued in next-integrity-testing.
Mimi
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Ondrej Mosnacek @ 2020-08-24 20:01 UTC (permalink / raw)
To: Stephen Smalley
Cc: Lakshmi Ramasubramanian, Mimi Zohar, Casey Schaufler, Tyler Hicks,
tusharsu, Sasha Levin, James Morris, linux-integrity,
SElinux list, LSM List, linux-kernel
In-Reply-To: <CAEjxPJ6-8WnZRJnADsn=RVakzJiESjEjK-f8nSkscpT7dnricQ@mail.gmail.com>
On Mon, Aug 24, 2020 at 9:30 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Mon, Aug 24, 2020 at 2:13 PM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
> >
> > On 8/24/20 7:00 AM, Stephen Smalley wrote:
> >
> > >> +int security_read_policy_kernel(struct selinux_state *state,
> > >> + void **data, size_t *len)
> > >> +{
> > >> + int rc;
> > >> +
> > >> + rc = security_read_policy_len(state, len);
> > >> + if (rc)
> > >> + return rc;
> > >> +
> > >> + *data = vmalloc(*len);
> > >> + if (!*data)
> > >> + return -ENOMEM;
> > >>
> > >> + return security_read_selinux_policy(state, data, len);
> > >> }
> > >
> > > See the discussion here:
> > > https://lore.kernel.org/selinux/20200824113015.1375857-1-omosnace@redhat.com/T/#t
> > >
> > > In order for this to be safe, you need to ensure that all callers of
> > > security_read_policy_kernel() have taken fsi->mutex in selinuxfs and
> > > any use of security_read_policy_len() occurs while holding the mutex.
> > > Otherwise, the length can change between security_read_policy_len()
> > > and security_read_selinux_policy() if policy is reloaded.
> > >
> >
> > "struct selinux_fs_info" is available when calling
> > security_read_policy_kernel() - currently in measure.c.
> > Only "struct selinux_state" is.
> >
> > Is Ondrej's re-try approach I need to use to workaround policy reload issue?
>
> No, I think perhaps we should move the mutex to selinux_state instead
> of selinux_fs_info. selinux_fs_info has a pointer to selinux_state so
> it can then use it indirectly. Note that your patches are going to
> conflict with other ongoing work in the selinux next branch that is
> refactoring policy load and converting the policy rwlock to RCU.
Yeah, and I'm experimenting with a patch on top of Stephen's RCU work
that would allow you to do this in a straightforward way without even
messing with the fsi->mutex. My patch may or may not be eventually
committed, but either way I'd recommend holding off on this for a
while until the dust settles around the RCU conversion.
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply
* Re: general protection fault in security_inode_getattr
From: syzbot @ 2020-08-24 19:37 UTC (permalink / raw)
To: andriin, ast, bpf, daniel, jmorris, john.fastabend, kafai,
kpsingh, linux-kernel, linux-security-module, netdev, serge,
songliubraving, syzkaller-bugs, yhs
In-Reply-To: <0000000000008caae305ab9a5318@google.com>
syzbot has found a reproducer for the following issue on:
HEAD commit: d012a719 Linux 5.9-rc2
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14aa130e900000
kernel config: https://syzkaller.appspot.com/x/.config?x=891ca5711a9f1650
dashboard link: https://syzkaller.appspot.com/bug?extid=f07cc9be8d1d226947ed
compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=104a650e900000
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+f07cc9be8d1d226947ed@syzkaller.appspotmail.com
general protection fault, probably for non-canonical address 0xdffffc000000000d: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000068-0x000000000000006f]
CPU: 1 PID: 32288 Comm: syz-executor.3 Not tainted 5.9.0-rc2-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:d_backing_inode include/linux/dcache.h:549 [inline]
RIP: 0010:security_inode_getattr+0x42/0x140 security/security.c:1276
Code: 1b fe 49 8d 5e 08 48 89 d8 48 c1 e8 03 42 80 3c 38 00 74 08 48 89 df e8 bc b4 5b fe 48 8b 1b 48 83 c3 68 48 89 d8 48 c1 e8 03 <42> 80 3c 38 00 74 08 48 89 df e8 9f b4 5b fe 48 8b 1b 48 83 c3 0c
RSP: 0018:ffffc9000a017750 EFLAGS: 00010202
RAX: 000000000000000d RBX: 0000000000000068 RCX: ffff888093ec6180
RDX: 0000000000000000 RSI: ffffc9000a017860 RDI: ffffc9000a017850
RBP: ffffc9000a017850 R08: dffffc0000000000 R09: ffffc9000a017850
R10: fffff52001402f0c R11: 0000000000000000 R12: ffffc9000a017860
R13: 0000000000008401 R14: ffffc9000a017850 R15: dffffc0000000000
FS: 00007f292d4ef700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b30920074 CR3: 00000000937fd000 CR4: 00000000001506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
vfs_getattr+0x21/0x60 fs/stat.c:121
ovl_copy_up_one fs/overlayfs/copy_up.c:850 [inline]
ovl_copy_up_flags+0x2ef/0x2a00 fs/overlayfs/copy_up.c:931
ovl_maybe_copy_up+0x154/0x180 fs/overlayfs/copy_up.c:963
ovl_open+0xa2/0x200 fs/overlayfs/file.c:147
do_dentry_open+0x7c8/0x1010 fs/open.c:817
do_open fs/namei.c:3251 [inline]
path_openat+0x2794/0x3840 fs/namei.c:3368
do_filp_open+0x191/0x3a0 fs/namei.c:3395
file_open_name+0x321/0x430 fs/open.c:1113
acct_on kernel/acct.c:207 [inline]
__do_sys_acct kernel/acct.c:286 [inline]
__se_sys_acct+0x122/0x6f0 kernel/acct.c:273
do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x45d579
Code: 5d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 2b b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f292d4eec78 EFLAGS: 00000246 ORIG_RAX: 00000000000000a3
RAX: ffffffffffffffda RBX: 0000000000000700 RCX: 000000000045d579
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000020000040
RBP: 000000000118cf70 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000118cf4c
R13: 00007ffc8e04bc4f R14: 00007f292d4ef9c0 R15: 000000000118cf4c
Modules linked in:
---[ end trace 7e4f1041b188e411 ]---
RIP: 0010:d_backing_inode include/linux/dcache.h:549 [inline]
RIP: 0010:security_inode_getattr+0x42/0x140 security/security.c:1276
Code: 1b fe 49 8d 5e 08 48 89 d8 48 c1 e8 03 42 80 3c 38 00 74 08 48 89 df e8 bc b4 5b fe 48 8b 1b 48 83 c3 68 48 89 d8 48 c1 e8 03 <42> 80 3c 38 00 74 08 48 89 df e8 9f b4 5b fe 48 8b 1b 48 83 c3 0c
RSP: 0018:ffffc9000a017750 EFLAGS: 00010202
RAX: 000000000000000d RBX: 0000000000000068 RCX: ffff888093ec6180
RDX: 0000000000000000 RSI: ffffc9000a017860 RDI: ffffc9000a017850
RBP: ffffc9000a017850 R08: dffffc0000000000 R09: ffffc9000a017850
R10: fffff52001402f0c R11: 0000000000000000 R12: ffffc9000a017860
R13: 0000000000008401 R14: ffffc9000a017850 R15: dffffc0000000000
FS: 00007f292d4ef700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fef820e7000 CR3: 00000000937fd000 CR4: 00000000001506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Stephen Smalley @ 2020-08-24 19:29 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Casey Schaufler, Tyler Hicks, tusharsu, sashal,
James Morris, linux-integrity, SElinux list, LSM List,
linux-kernel
In-Reply-To: <418618c4-a0c6-6b28-6718-2726a29b83c5@linux.microsoft.com>
On Mon, Aug 24, 2020 at 2:13 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> On 8/24/20 7:00 AM, Stephen Smalley wrote:
>
> >> +int security_read_policy_kernel(struct selinux_state *state,
> >> + void **data, size_t *len)
> >> +{
> >> + int rc;
> >> +
> >> + rc = security_read_policy_len(state, len);
> >> + if (rc)
> >> + return rc;
> >> +
> >> + *data = vmalloc(*len);
> >> + if (!*data)
> >> + return -ENOMEM;
> >>
> >> + return security_read_selinux_policy(state, data, len);
> >> }
> >
> > See the discussion here:
> > https://lore.kernel.org/selinux/20200824113015.1375857-1-omosnace@redhat.com/T/#t
> >
> > In order for this to be safe, you need to ensure that all callers of
> > security_read_policy_kernel() have taken fsi->mutex in selinuxfs and
> > any use of security_read_policy_len() occurs while holding the mutex.
> > Otherwise, the length can change between security_read_policy_len()
> > and security_read_selinux_policy() if policy is reloaded.
> >
>
> "struct selinux_fs_info" is available when calling
> security_read_policy_kernel() - currently in measure.c.
> Only "struct selinux_state" is.
>
> Is Ondrej's re-try approach I need to use to workaround policy reload issue?
No, I think perhaps we should move the mutex to selinux_state instead
of selinux_fs_info. selinux_fs_info has a pointer to selinux_state so
it can then use it indirectly. Note that your patches are going to
conflict with other ongoing work in the selinux next branch that is
refactoring policy load and converting the policy rwlock to RCU.
^ permalink raw reply
* Re: [PATCH v2 0/2] ima: Fix keyrings race condition and other key related bugs
From: Tyler Hicks @ 2020-08-24 18:53 UTC (permalink / raw)
To: Mimi Zohar
Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Tushar Sugandhi, Nayna Jain,
linux-kernel, linux-integrity, linux-security-module
In-Reply-To: <839d2b185ba482d664edd3fda7c03965543553fa.camel@linux.ibm.com>
On 2020-08-24 14:44:55, Mimi Zohar wrote:
> Hi Tyler,
>
> On Tue, 2020-08-11 at 14:26 -0500, Tyler Hicks wrote:
> > v2:
> > - Always return an ERR_PTR from ima_alloc_rule_opt_list() (Nayna)
> > - Add Lakshmi's Reviewed-by to both patches
> > - Rebased on commit 3db0d0c276a7 ("integrity: remove redundant
> > initialization of variable ret") of next-integrity
> > v1: https://lore.kernel.org/lkml/20200727140831.64251-1-tyhicks@linux.microsoft.com/
> >
> > Nayna pointed out that the "keyrings=" option in an IMA policy rule
> > should only be accepted when CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is
> > enabled:
> >
> > https://lore.kernel.org/linux-integrity/336cc947-1f70-0286-6506-6df3d1d23a1d@linux.vnet.ibm.com/
> >
> > While fixing this, the compiler warned me about the potential for the
> > ima_keyrings pointer to be NULL despite it being used, without a check
> > for NULL, as the destination address for the strcpy() in
> > ima_match_keyring().
> >
> > It also became apparent that there was not adequate locking around the
> > use of the pre-allocated buffer that ima_keyrings points to. The kernel
> > keyring has a lock (.sem member of struct key) that ensures only one key
> > can be added to a given keyring at a time but there's no protection
> > against adding multiple keys to different keyrings at the same time.
> >
> > The first patch in this series fixes both ima_keyrings related issues by
> > parsing the list of keyrings in a KEY_CHECK rule at policy load time
> > rather than deferring the parsing to policy check time. Once that fix is
> > in place, the second patch can enforce that
> > CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS must be enabled in order to use
> > "func=KEY_CHECK" or "keyrings=" options in IMA policy.
>
> Thank you for fixing and cleaning up the existing keyring policy
> support.
>
> >
> > The new "keyrings=" value handling is done in a generic manner that can
> > be reused by other options in the future. This seems to make sense as
> > "appraise_type=" has similar style (though it doesn't need to be fully
> > parsed at this time) and using "|" as an alternation delimiter is
> > becoming the norm in IMA policy.
>
> Yes, thank you. Better extending existing key value pairs than
> defining new boot command line options.
>
> This patch set is now queued in next-integrity-testing.
Thanks! I'm glad you found it useful.
Tyler
>
> Mimi
^ permalink raw reply
* Re: [PATCH v2 0/2] ima: Fix keyrings race condition and other key related bugs
From: Mimi Zohar @ 2020-08-24 18:44 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Tushar Sugandhi, Nayna Jain, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200811192621.281675-1-tyhicks@linux.microsoft.com>
Hi Tyler,
On Tue, 2020-08-11 at 14:26 -0500, Tyler Hicks wrote:
> v2:
> - Always return an ERR_PTR from ima_alloc_rule_opt_list() (Nayna)
> - Add Lakshmi's Reviewed-by to both patches
> - Rebased on commit 3db0d0c276a7 ("integrity: remove redundant
> initialization of variable ret") of next-integrity
> v1: https://lore.kernel.org/lkml/20200727140831.64251-1-tyhicks@linux.microsoft.com/
>
> Nayna pointed out that the "keyrings=" option in an IMA policy rule
> should only be accepted when CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is
> enabled:
>
> https://lore.kernel.org/linux-integrity/336cc947-1f70-0286-6506-6df3d1d23a1d@linux.vnet.ibm.com/
>
> While fixing this, the compiler warned me about the potential for the
> ima_keyrings pointer to be NULL despite it being used, without a check
> for NULL, as the destination address for the strcpy() in
> ima_match_keyring().
>
> It also became apparent that there was not adequate locking around the
> use of the pre-allocated buffer that ima_keyrings points to. The kernel
> keyring has a lock (.sem member of struct key) that ensures only one key
> can be added to a given keyring at a time but there's no protection
> against adding multiple keys to different keyrings at the same time.
>
> The first patch in this series fixes both ima_keyrings related issues by
> parsing the list of keyrings in a KEY_CHECK rule at policy load time
> rather than deferring the parsing to policy check time. Once that fix is
> in place, the second patch can enforce that
> CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS must be enabled in order to use
> "func=KEY_CHECK" or "keyrings=" options in IMA policy.
Thank you for fixing and cleaning up the existing keyring policy
support.
>
> The new "keyrings=" value handling is done in a generic manner that can
> be reused by other options in the future. This seems to make sense as
> "appraise_type=" has similar style (though it doesn't need to be fully
> parsed at this time) and using "|" as an alternation delimiter is
> becoming the norm in IMA policy.
Yes, thank you. Better extending existing key value pairs than
defining new boot command line options.
This patch set is now queued in next-integrity-testing.
Mimi
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Lakshmi Ramasubramanian @ 2020-08-24 18:13 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Casey Schaufler, Tyler Hicks, tusharsu, sashal,
James Morris, linux-integrity, SElinux list, LSM List,
linux-kernel
In-Reply-To: <CAEjxPJ5Kok-TBfS=XQ+NUC5tuaZRkyLBOawG4UDky51_bsMnGw@mail.gmail.com>
On 8/24/20 7:00 AM, Stephen Smalley wrote:
>> +int security_read_policy_kernel(struct selinux_state *state,
>> + void **data, size_t *len)
>> +{
>> + int rc;
>> +
>> + rc = security_read_policy_len(state, len);
>> + if (rc)
>> + return rc;
>> +
>> + *data = vmalloc(*len);
>> + if (!*data)
>> + return -ENOMEM;
>>
>> + return security_read_selinux_policy(state, data, len);
>> }
>
> See the discussion here:
> https://lore.kernel.org/selinux/20200824113015.1375857-1-omosnace@redhat.com/T/#t
>
> In order for this to be safe, you need to ensure that all callers of
> security_read_policy_kernel() have taken fsi->mutex in selinuxfs and
> any use of security_read_policy_len() occurs while holding the mutex.
> Otherwise, the length can change between security_read_policy_len()
> and security_read_selinux_policy() if policy is reloaded.
>
"struct selinux_fs_info" is available when calling
security_read_policy_kernel() - currently in measure.c.
Only "struct selinux_state" is.
Is Ondrej's re-try approach I need to use to workaround policy reload issue?
thanks,
-lakshmi
^ permalink raw reply
* Re: [RFC] security: replace indirect calls with static calls
From: Casey Schaufler @ 2020-08-24 17:54 UTC (permalink / raw)
To: Brendan Jackman
Cc: Brendan Jackman, linux-kernel, bpf, linux-security-module,
Paul Renauld, Alexei Starovoitov, Daniel Borkmann, James Morris,
Paul Turner, Jann Horn, Peter Zijlstra, rafael.j.wysocki,
Kees Cook, thgarnie, KP Singh, paul.renauld.epfl, Casey Schaufler
In-Reply-To: <CA+i-1C1GwgYJAfaUofzv47nyryQ15znE6OLWhAN-gsscm6mMoA@mail.gmail.com>
On 8/24/2020 10:04 AM, Brendan Jackman wrote:
> On Mon, 24 Aug 2020 at 18:43, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/24/2020 8:20 AM, Brendan Jackman wrote:
>>> On Fri, 21 Aug 2020 at 00:46, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 8/20/2020 9:47 AM, Brendan Jackman wrote:
>>> [...]
>>>> What does NOP really look like?
>>> The NOP is the same as a regular function call but the CALL
>>> instruction is replaced with a NOP instruction. The code that sets up
>>> the call parameters is unchanged, and so is the code that expects to
>>> get the return value in eax or whatever.
>> Right. Are you saying that NOP is in-line assembler in your switch?
> That's right - although it's behind the static_call API that the patch
> depends on ([5] in the original mail).
>
>>> That means we cannot actually
>>> call the static_calls for NULL slots, we'd get undefined behaviour
>>> (except for void hooks) - this is what Peter is talking about in the
>>> sibling thread.
>> Referring to the "sibling thread" is kinda confusing, and
>> assumes everyone is one all the right mailing lists, and knows
>> which other thread you're talking about.
> Sure, sorry - here's the Lore link for future reference:
>
> https://lore.kernel.org/lkml/20200820164753.3256899-1-jackmanb@chromium.org/T/#m5a6fb3f10141049ce43e18a41f154796090ae1d5
>
>>> For this reason, there are _no gaps_ in the callback table. For a
>>> given LSM hook, all the slots after base_slot_idx are filled,
>> Why go to all the trouble of maintaining the base_slot_idx
>> if NOP is so cheap? Why not fill all unused slots with NOP?
>> Worst case would be a hook with no users, in which case you
>> have 11 NOPS in the void hook case and 11 "if (ret != DEFAULT_RET)"
>> and 11 NOPS in the int case. No switch magic required. Even
>> better, in the int case you have two calls/slot, the first is the
>> module supplied function (or NOP) and the second is
>> int isit(int ret) { return (ret != DEFAULT_RET) ? ret : 0; }
>> (or NOP).
>>
>> The no security module case degenerates to 22 NOP instructions
>> and no if checks of any sort. I'm not the performance guy, but
>> that seems better than maintaining and checking base_slot_idx
>> to me.
> The switch trick is not really motivated by performance.
Then what is its motivation? It makes the code more complicated,
and is unnecessary.
> I think all the focus on the NOPs themselves is a bit misleading here
> - we _can't_ execute the NOPs for the int hooks, because there are
> instructions after them that expect a function to have just returned a
> value, which NOP doesn't do.
That's what I was hoping to address with the second call in the slot.
The first call in the slot would be either the module supplied code
or a NOP if the module isn't using the hook. The second would be
supplied by the LSM infrastructure and would be NOP if the module
didn't use the hook. The LSM supplied function would do the necessary
checking. Its more complicated than the void case, but not that much
more complicated than the existing list based scheme.
The concern about the non-existent return on a NOP can be dealt with
by setting up initial conditions correctly in most cases. Dealing with
multiple security modules providing information (e.g. secid_to_secctx)
is where it gets tricky.
> When there is a NOP in the slot instead
> of a CALL, it would appear to "return" whatever value is leftover in
> the return register. At the C level, this is why the static_call API
> doesn't allow static_call_cond to return a value (which is what PeterZ
> is referring to in the thread I linked above).
>
> So, we could drop the switch trick for void hooks and just use
> static_call_cond, but this doesn't work for int hooks. IMO that
> variation between the two hook types would just add confusion.
With the number of cases where the switch trick isn't going to
work in the long term I'm disinclined to think it makes things
less confusing.
>>>>> +#define __UNROLL_MACRO_LOOP_20(MACRO, ...) \
>>>>> + __UNROLL_MACRO_LOOP_19(MACRO, __VA_ARGS__) \
>>>>> + MACRO(19, __VA_ARGS__)
>>>>> +
>>>> Where does "20" come from? Why are you unrolling beyond 11?
>>> It's just an arbitrary limit on the unrolling macro implementation, we
>>> aren't actually unrolling beyond 11 where the macro is used (N is set
>>> to 11).
>> I'm not a fan of including macros you can't use, especially
>> when they're just obvious variants of other macros.
> Not sure what you mean here - is there already a macro that does what
> UNROLL_MACRO_LOOP does?
No, I'm saying that __UNROLL_MACRO_LOOP_20() will never be used on
a system that has at most 11 security modules. You've added a bunch of
text that serves no purpose. "Future expansion" is pretty silly here.
^ permalink raw reply
* Re: [PATCH 01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded
From: Mimi Zohar @ 2020-08-24 17:45 UTC (permalink / raw)
To: Roberto Sassu, mjg59
Cc: linux-integrity, linux-security-module, linux-kernel, stable
In-Reply-To: <2b204e31d21e93c0167d154c2397cd5d11be6e7f.camel@linux.ibm.com>
Hi Roberto,
On Fri, 2020-08-21 at 14:30 -0400, Mimi Zohar wrote:
> Sorry for the delay in reviewing these patches. Missing from this
> patch set is a cover letter with an explanation for grouping these
> patches into a patch set, other than for convenience. In this case, it
> would be along the lines that the original use case for EVM portable
> and immutable keys support was for a few critical files, not combined
> with an EVM encrypted key type. This patch set more fully integrates
> the initial EVM portable and immutable signature support.
Thank you for more fully integrating the EVM portable signatures into
IMA.
" [PATCH 08/11] ima: Allow imasig requirement to be satisfied by EVM
portable signatures" equates an IMA signature to having a portable and
immutable EVM signature. That is true in terms of signature
verification, but from an attestation perspective the "ima-sig"
template will not contain a signature. If not the EVM signature, then
at least some other indication should be included in the measurement
list.
Are you planning on posting the associated IMA/EVM regression tests?
Mimi
^ permalink raw reply
* Re: [RFC] security: replace indirect calls with static calls
From: Brendan Jackman @ 2020-08-24 17:04 UTC (permalink / raw)
To: Casey Schaufler
Cc: Brendan Jackman, linux-kernel, bpf, linux-security-module,
Paul Renauld, Alexei Starovoitov, Daniel Borkmann, James Morris,
Paul Turner, Jann Horn, Peter Zijlstra, rafael.j.wysocki,
Kees Cook, thgarnie, KP Singh, paul.renauld.epfl
In-Reply-To: <66a35f25-53be-17c3-8ab3-7cb32b0bc77a@schaufler-ca.com>
On Mon, 24 Aug 2020 at 18:43, Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 8/24/2020 8:20 AM, Brendan Jackman wrote:
> > On Fri, 21 Aug 2020 at 00:46, Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 8/20/2020 9:47 AM, Brendan Jackman wrote:
> > [...]
> >> What does NOP really look like?
> > The NOP is the same as a regular function call but the CALL
> > instruction is replaced with a NOP instruction. The code that sets up
> > the call parameters is unchanged, and so is the code that expects to
> > get the return value in eax or whatever.
>
> Right. Are you saying that NOP is in-line assembler in your switch?
That's right - although it's behind the static_call API that the patch
depends on ([5] in the original mail).
> > That means we cannot actually
> > call the static_calls for NULL slots, we'd get undefined behaviour
> > (except for void hooks) - this is what Peter is talking about in the
> > sibling thread.
>
> Referring to the "sibling thread" is kinda confusing, and
> assumes everyone is one all the right mailing lists, and knows
> which other thread you're talking about.
Sure, sorry - here's the Lore link for future reference:
https://lore.kernel.org/lkml/20200820164753.3256899-1-jackmanb@chromium.org/T/#m5a6fb3f10141049ce43e18a41f154796090ae1d5
> >
> > For this reason, there are _no gaps_ in the callback table. For a
> > given LSM hook, all the slots after base_slot_idx are filled,
>
> Why go to all the trouble of maintaining the base_slot_idx
> if NOP is so cheap? Why not fill all unused slots with NOP?
> Worst case would be a hook with no users, in which case you
> have 11 NOPS in the void hook case and 11 "if (ret != DEFAULT_RET)"
> and 11 NOPS in the int case. No switch magic required. Even
> better, in the int case you have two calls/slot, the first is the
> module supplied function (or NOP) and the second is
> int isit(int ret) { return (ret != DEFAULT_RET) ? ret : 0; }
> (or NOP).
>
> The no security module case degenerates to 22 NOP instructions
> and no if checks of any sort. I'm not the performance guy, but
> that seems better than maintaining and checking base_slot_idx
> to me.
The switch trick is not really motivated by performance.
I think all the focus on the NOPs themselves is a bit misleading here
- we _can't_ execute the NOPs for the int hooks, because there are
instructions after them that expect a function to have just returned a
value, which NOP doesn't do. When there is a NOP in the slot instead
of a CALL, it would appear to "return" whatever value is leftover in
the return register. At the C level, this is why the static_call API
doesn't allow static_call_cond to return a value (which is what PeterZ
is referring to in the thread I linked above).
So, we could drop the switch trick for void hooks and just use
static_call_cond, but this doesn't work for int hooks. IMO that
variation between the two hook types would just add confusion.
> >>> +#define __UNROLL_MACRO_LOOP_20(MACRO, ...) \
> >>> + __UNROLL_MACRO_LOOP_19(MACRO, __VA_ARGS__) \
> >>> + MACRO(19, __VA_ARGS__)
> >>> +
> >> Where does "20" come from? Why are you unrolling beyond 11?
> > It's just an arbitrary limit on the unrolling macro implementation, we
> > aren't actually unrolling beyond 11 where the macro is used (N is set
> > to 11).
>
> I'm not a fan of including macros you can't use, especially
> when they're just obvious variants of other macros.
Not sure what you mean here - is there already a macro that does what
UNROLL_MACRO_LOOP does?
^ permalink raw reply
* Re: [RFC] security: replace indirect calls with static calls
From: Casey Schaufler @ 2020-08-24 16:42 UTC (permalink / raw)
To: Brendan Jackman
Cc: Brendan Jackman, linux-kernel, bpf, linux-security-module,
Paul Renauld, Alexei Starovoitov, Daniel Borkmann, James Morris,
Paul Turner, Jann Horn, Peter Zijlstra, rafael.j.wysocki,
Kees Cook, thgarnie, KP Singh, paul.renauld.epfl, Casey Schaufler
In-Reply-To: <CA+i-1C09YZ8aCr6p5NOA2e3Ji5TKwdET=qAy=M328NK--L=0RA@mail.gmail.com>
On 8/24/2020 8:20 AM, Brendan Jackman wrote:
> On Fri, 21 Aug 2020 at 00:46, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/20/2020 9:47 AM, Brendan Jackman wrote:
> [...]
>> What does NOP really look like?
> The NOP is the same as a regular function call but the CALL
> instruction is replaced with a NOP instruction. The code that sets up
> the call parameters is unchanged, and so is the code that expects to
> get the return value in eax or whatever.
Right. Are you saying that NOP is in-line assembler in your switch?
> That means we cannot actually
> call the static_calls for NULL slots, we'd get undefined behaviour
> (except for void hooks) - this is what Peter is talking about in the
> sibling thread.
Referring to the "sibling thread" is kinda confusing, and
assumes everyone is one all the right mailing lists, and knows
which other thread you're talking about.
>
> For this reason, there are _no gaps_ in the callback table. For a
> given LSM hook, all the slots after base_slot_idx are filled,
Why go to all the trouble of maintaining the base_slot_idx
if NOP is so cheap? Why not fill all unused slots with NOP?
Worst case would be a hook with no users, in which case you
have 11 NOPS in the void hook case and 11 "if (ret != DEFAULT_RET)"
and 11 NOPS in the int case. No switch magic required. Even
better, in the int case you have two calls/slot, the first is the
module supplied function (or NOP) and the second is
int isit(int ret) { return (ret != DEFAULT_RET) ? ret : 0; }
(or NOP).
The no security module case degenerates to 22 NOP instructions
and no if checks of any sort. I'm not the performance guy, but
that seems better than maintaining and checking base_slot_idx
to me.
> and all
> before are empty, so jumping to base_slot_idx ensures that we don't
> reach an empty slot. That's what the switch trick is all about.
I hates tricks. They're so susceptible to clever attacks.
>>> if ret != 0:
>> I assume you'd want "ret != DEFAULT_RET" instead of "ret != 0".
> Yeah that's a good question - but the existing behaviour is to always
> check against 0 (DEFAULT_RET is called IRC in the real code),
> which does seem strange.
If you don't do this correctly you'll make a real mess of the security.
>> So what goes in for empty slots? What about gaps in the table?
> It's a NOP, but we never execute it (explained above). There are no gaps.
Right. Unused slots have NOP. NOP is (assumed to be) cheap.
>>> +#define __UNROLL_MACRO_LOOP_20(MACRO, ...) \
>>> + __UNROLL_MACRO_LOOP_19(MACRO, __VA_ARGS__) \
>>> + MACRO(19, __VA_ARGS__)
>>> +
>> Where does "20" come from? Why are you unrolling beyond 11?
> It's just an arbitrary limit on the unrolling macro implementation, we
> aren't actually unrolling beyond 11 where the macro is used (N is set
> to 11).
I'm not a fan of including macros you can't use, especially
when they're just obvious variants of other macros.
>>> With this use of the table and the
>>> switch, it is possible to jump directly to the first used slot and execute
>>> all of the slots after. This essentially makes the entry point of the table
>>> dynamic. Instead, it would also be possible to start from 0 and break after
>>> the final populated slot, but that would require an additional conditional
>>> after each slot.
>>>
>>> This macro is used to generate the code for each static slot, (e.g. each
>>> case statement in the previous example). This will expand into a call to
>>> MACRO for each static slot defined. For example, if with again 5 slots:
>>>
>>> SECURITY_FOREACH_STATIC_SLOT(MACRO, x, y) ->
>>>
>>> MACRO(0, x, y)
>>> MACRO(1, x, y)
>>> MACRO(2, x, y)
>>> MACRO(3, x, y)
>>> MACRO(4, x, y)
>>>
>>> This is used in conjunction with LSM_HOOK definitions in
>>> linux/lsm_hook_defs.h to execute a macro for each static slot of each LSM
>>> hook.
>>>
>>> The patches for static calls [6] are not upstreamed yet.
>>>
>>> The number of available slots for each LSM hook is currently fixed at
>>> 11 (the number of LSMs in the kernel). Ideally, it should automatically
>>> adapt to the number of LSMs compiled into the kernel.
>> #define SECURITY_STATIC_SLOT_COUNT ( \
>> 1 + /* Capability module is always there */ \
>> (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
>> (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
>> ... \
>> (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))
>>
> Yeah, that's exactly what we need but it needs to be expanded to an
> integer literal at preprocessor time, those +s won't work :(
???? Gosh. It works in my module stacking code.
>>> If there’s no practical way to implement such automatic adaptation, an
>>> option instead would be to remove the panic call by falling-back to the old
>>> linked-list mechanism, which is still present anyway (see below).
>>>
>>> A few special cases of LSM don't use the macro call_[int/void]_hook but
>>> have their own calling logic. The linked-lists are kept as a possible slow
>>> path fallback for them.
>> Unfortunately, the stacking effort is increasing the number of hooks
>> that will require special handling. security_secid_to_secctx() is one
>> example.
>>
>>> Before:
>>>
>>> https://gist.githubusercontent.com/PaulRenauld/fe3ee7b51121556e03c181432c8b3dd5/raw/62437b1416829ca0e8a0ed9101530bc90fd42d69/lsm-performance.png
>>>
>>> After:
>>>
>>> https://gist.githubusercontent.com/PaulRenauld/fe3ee7b51121556e03c181432c8b3dd5/raw/00e414b73e0c38c2eae8f05d5363a745179ba285/faster-lsm-results.png
>>>
>>> With this implementation, any overhead of the indirect call in the LSM
>>> framework is completely mitigated (performance results: [7]). This
>>> facilitates the adoption of "bpf" LSM on production machines and also
>>> benefits all other LSMs.
>> Your numbers for a system with BPF are encouraging. What do the numbers
>> look like for a system with SELinux, Smack or AppArmor?
> Yeah that's a good question. As I said in the sibling thread the
> motivating example is very lightweight LSM callbacks in very hot
> codepaths, but I'll get some broader data too and report back.
Even IoT systems are using security modules these days. You'll be
hard pressed to identify a class of systems that don't use an
LSM or two. My bet is that your fiendishly clever scheme is going
to make everyone's life better, but as with all things, it does
need to have hard evidence.
^ permalink raw reply
* Re: [RFC] security: replace indirect calls with static calls
From: Brendan Jackman @ 2020-08-24 15:20 UTC (permalink / raw)
To: Casey Schaufler
Cc: Brendan Jackman, linux-kernel, bpf, linux-security-module,
Paul Renauld, Alexei Starovoitov, Daniel Borkmann, James Morris,
Paul Turner, Jann Horn, Peter Zijlstra, rafael.j.wysocki,
Kees Cook, thgarnie, KP Singh, paul.renauld.epfl
In-Reply-To: <42fb4180-772c-5579-ef3e-b4003e2b784b@schaufler-ca.com>
On Fri, 21 Aug 2020 at 00:46, Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 8/20/2020 9:47 AM, Brendan Jackman wrote:
[...]
> What does NOP really look like?
The NOP is the same as a regular function call but the CALL
instruction is replaced with a NOP instruction. The code that sets up
the call parameters is unchanged, and so is the code that expects to
get the return value in eax or whatever. That means we cannot actually
call the static_calls for NULL slots, we'd get undefined behaviour
(except for void hooks) - this is what Peter is talking about in the
sibling thread.
For this reason, there are _no gaps_ in the callback table. For a
given LSM hook, all the slots after base_slot_idx are filled, and all
before are empty, so jumping to base_slot_idx ensures that we don't
reach an empty slot. That's what the switch trick is all about.
>
> > if ret != 0:
>
> I assume you'd want "ret != DEFAULT_RET" instead of "ret != 0".
Yeah that's a good question - but the existing behaviour is to always
check against 0 (DEFAULT_RET is called IRC in the real code),
which does seem strange.
> So what goes in for empty slots? What about gaps in the table?
It's a NOP, but we never execute it (explained above). There are no gaps.
>> +#define __UNROLL_MACRO_LOOP_20(MACRO, ...) \
>> + __UNROLL_MACRO_LOOP_19(MACRO, __VA_ARGS__) \
>> + MACRO(19, __VA_ARGS__)
>> +
> Where does "20" come from? Why are you unrolling beyond 11?
It's just an arbitrary limit on the unrolling macro implementation, we
aren't actually unrolling beyond 11 where the macro is used (N is set
to 11).
>
> > With this use of the table and the
> > switch, it is possible to jump directly to the first used slot and execute
> > all of the slots after. This essentially makes the entry point of the table
> > dynamic. Instead, it would also be possible to start from 0 and break after
> > the final populated slot, but that would require an additional conditional
> > after each slot.
> >
> > This macro is used to generate the code for each static slot, (e.g. each
> > case statement in the previous example). This will expand into a call to
> > MACRO for each static slot defined. For example, if with again 5 slots:
> >
> > SECURITY_FOREACH_STATIC_SLOT(MACRO, x, y) ->
> >
> > MACRO(0, x, y)
> > MACRO(1, x, y)
> > MACRO(2, x, y)
> > MACRO(3, x, y)
> > MACRO(4, x, y)
> >
> > This is used in conjunction with LSM_HOOK definitions in
> > linux/lsm_hook_defs.h to execute a macro for each static slot of each LSM
> > hook.
> >
> > The patches for static calls [6] are not upstreamed yet.
> >
> > The number of available slots for each LSM hook is currently fixed at
> > 11 (the number of LSMs in the kernel). Ideally, it should automatically
> > adapt to the number of LSMs compiled into the kernel.
>
> #define SECURITY_STATIC_SLOT_COUNT ( \
> 1 + /* Capability module is always there */ \
> (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> ... \
> (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))
>
Yeah, that's exactly what we need but it needs to be expanded to an
integer literal at preprocessor time, those +s won't work :(
> > If there’s no practical way to implement such automatic adaptation, an
> > option instead would be to remove the panic call by falling-back to the old
> > linked-list mechanism, which is still present anyway (see below).
> >
> > A few special cases of LSM don't use the macro call_[int/void]_hook but
> > have their own calling logic. The linked-lists are kept as a possible slow
> > path fallback for them.
>
> Unfortunately, the stacking effort is increasing the number of hooks
> that will require special handling. security_secid_to_secctx() is one
> example.
>
> >
> > Before:
> >
> > https://gist.githubusercontent.com/PaulRenauld/fe3ee7b51121556e03c181432c8b3dd5/raw/62437b1416829ca0e8a0ed9101530bc90fd42d69/lsm-performance.png
> >
> > After:
> >
> > https://gist.githubusercontent.com/PaulRenauld/fe3ee7b51121556e03c181432c8b3dd5/raw/00e414b73e0c38c2eae8f05d5363a745179ba285/faster-lsm-results.png
> >
> > With this implementation, any overhead of the indirect call in the LSM
> > framework is completely mitigated (performance results: [7]). This
> > facilitates the adoption of "bpf" LSM on production machines and also
> > benefits all other LSMs.
>
> Your numbers for a system with BPF are encouraging. What do the numbers
> look like for a system with SELinux, Smack or AppArmor?
Yeah that's a good question. As I said in the sibling thread the
motivating example is very lightweight LSM callbacks in very hot
codepaths, but I'll get some broader data too and report back.
^ permalink raw reply
* Re: [RFC] security: replace indirect calls with static calls
From: Brendan Jackman @ 2020-08-24 15:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Kees Cook, Brendan Jackman, linux-kernel, bpf,
linux-security-module, Alexei Starovoitov, Daniel Borkmann,
James Morris, pjt, Jann Horn, rafael.j.wysocki, thgarnie,
KP Singh, paul.renauld.epfl
In-Reply-To: <20200824143344.GB3982@worktop.programming.kicks-ass.net>
On Mon, Aug 24, 2020 at 04:33:44PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 24, 2020 at 04:09:09PM +0200, Brendan Jackman wrote:
>
> > > > Why this trick with a switch statement? The table of static call is defined
> > > > at compile time. The number of hook callbacks that will be defined is
> > > > unknown at that time, and the table cannot be resized at runtime. Static
> > > > calls do not define a conditional execution for a non-void function, so the
> > > > executed slots must be non-empty. With this use of the table and the
> > > > switch, it is possible to jump directly to the first used slot and execute
> > > > all of the slots after. This essentially makes the entry point of the table
> > > > dynamic. Instead, it would also be possible to start from 0 and break after
> > > > the final populated slot, but that would require an additional conditional
> > > > after each slot.
> > >
> > > Instead of just "NOP", having the static branches perform a jump would
> > > solve this pretty cleanly, yes? Something like:
> > >
> > > ret = DEFAULT_RET;
> > >
> > > ret = A(args); <--- direct call, no retpoline
> > > if ret != 0:
> > > goto out;
> > >
> > > ret = B(args); <--- direct call, no retpoline
> > > if ret != 0:
> > > goto out;
> > >
> > > goto out;
> > > if ret != 0:
> > > goto out;
> > >
> > > out:
> > > return ret;
> >
> > Hmm yeah that's a cool idea. This would either need to be implemented
> > with custom code-modification logic for the LSM hooks, or we'd need to
> > think of a way to express it in a sensible addition to the static_call
> > API. I do wonder if the latter could take the form of a generic system
> > for arrays of static calls.
>
> So you basically want something like:
>
> if (A[0] && (ret = static_call(A[0])(...)))
> return ret;
>
> if (A[1] && (ret = static_call(A[1])(...)))
> return ret;
>
> ....
>
> return ret;
>
> Right? The problem with static_call_cond() is that we don't know what to
> do with the return value when !func, which is why it's limited to void
> return type.
>
> You can however construct something like the above with a combination of
> static_branch() and static_call() though. It'll not be pretty, but it
> ought to work:
>
> if (static_branch_likely(A[0].key)) {
> ret = static_call(A[0].call)(...);
> if (ret)
> return ret;
> }
>
> ...
>
> return ret;
>
Right. That's actually exactly what Paul's first implementation
looked like for call_int_hook. But we thought the switch thing was
easier to understand.
>
> > It would also need to handle the fact that IIUC at the moment the last
> > static_call may be a tail call, so we'd be patching an existing jump
> > into a jump to a different target, I don't know if we can do that
> > atomically.
>
> Of course we can, the static_call() series supports tail-calls just
> fine. In fact, patching jumps is far easier, it was patching call that
> was the real problem because it mucks about with the stack.
>
OK great. I had a vague apprehension that we could only patch to or from
a NOP.
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Lakshmi Ramasubramanian @ 2020-08-24 14:35 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Casey Schaufler, Tyler Hicks, tusharsu, sashal,
James Morris, linux-integrity, SElinux list, LSM List,
linux-kernel
In-Reply-To: <CAEjxPJ5Kok-TBfS=XQ+NUC5tuaZRkyLBOawG4UDky51_bsMnGw@mail.gmail.com>
On 8/24/20 7:00 AM, Stephen Smalley wrote:
> On Fri, Aug 21, 2020 at 9:00 PM Lakshmi Ramasubramanian
>
>> +int security_read_policy_kernel(struct selinux_state *state,
>> + void **data, size_t *len)
>> +{
>> + int rc;
>> +
>> + rc = security_read_policy_len(state, len);
>> + if (rc)
>> + return rc;
>> +
>> + *data = vmalloc(*len);
>> + if (!*data)
>> + return -ENOMEM;
>>
>> + return security_read_selinux_policy(state, data, len);
>> }
>
> See the discussion here:
> https://lore.kernel.org/selinux/20200824113015.1375857-1-omosnace@redhat.com/T/#t
>
> In order for this to be safe, you need to ensure that all callers of
> security_read_policy_kernel() have taken fsi->mutex in selinuxfs and
> any use of security_read_policy_len() occurs while holding the mutex.
> Otherwise, the length can change between security_read_policy_len()
> and security_read_selinux_policy() if policy is reloaded.
>
Thanks. I'll make this change.
-lakshmi
^ permalink raw reply
* Re: [RFC] security: replace indirect calls with static calls
From: Peter Zijlstra @ 2020-08-24 14:33 UTC (permalink / raw)
To: Brendan Jackman
Cc: Kees Cook, Brendan Jackman, linux-kernel, bpf,
linux-security-module, Paul Renauld, Alexei Starovoitov,
Daniel Borkmann, James Morris, pjt, Jann Horn, rafael.j.wysocki,
thgarnie, KP Singh, paul.renauld.epfl
In-Reply-To: <CA+i-1C0XEuWWRm5nMPWCzEPUao7rp5346Eotpt1A_S3Za3Wysw@mail.gmail.com>
On Mon, Aug 24, 2020 at 04:09:09PM +0200, Brendan Jackman wrote:
> > > Why this trick with a switch statement? The table of static call is defined
> > > at compile time. The number of hook callbacks that will be defined is
> > > unknown at that time, and the table cannot be resized at runtime. Static
> > > calls do not define a conditional execution for a non-void function, so the
> > > executed slots must be non-empty. With this use of the table and the
> > > switch, it is possible to jump directly to the first used slot and execute
> > > all of the slots after. This essentially makes the entry point of the table
> > > dynamic. Instead, it would also be possible to start from 0 and break after
> > > the final populated slot, but that would require an additional conditional
> > > after each slot.
> >
> > Instead of just "NOP", having the static branches perform a jump would
> > solve this pretty cleanly, yes? Something like:
> >
> > ret = DEFAULT_RET;
> >
> > ret = A(args); <--- direct call, no retpoline
> > if ret != 0:
> > goto out;
> >
> > ret = B(args); <--- direct call, no retpoline
> > if ret != 0:
> > goto out;
> >
> > goto out;
> > if ret != 0:
> > goto out;
> >
> > out:
> > return ret;
>
> Hmm yeah that's a cool idea. This would either need to be implemented
> with custom code-modification logic for the LSM hooks, or we'd need to
> think of a way to express it in a sensible addition to the static_call
> API. I do wonder if the latter could take the form of a generic system
> for arrays of static calls.
So you basically want something like:
if (A[0] && (ret = static_call(A[0])(...)))
return ret;
if (A[1] && (ret = static_call(A[1])(...)))
return ret;
....
return ret;
Right? The problem with static_call_cond() is that we don't know what to
do with the return value when !func, which is why it's limited to void
return type.
You can however construct something like the above with a combination of
static_branch() and static_call() though. It'll not be pretty, but it
ought to work:
if (static_branch_likely(A[0].key)) {
ret = static_call(A[0].call)(...);
if (ret)
return ret;
}
...
return ret;
> It would also need to handle the fact that IIUC at the moment the last
> static_call may be a tail call, so we'd be patching an existing jump
> into a jump to a different target, I don't know if we can do that
> atomically.
Of course we can, the static_call() series supports tail-calls just
fine. In fact, patching jumps is far easier, it was patching call that
was the real problem because it mucks about with the stack.
^ permalink raw reply
* Re: [RFC] security: replace indirect calls with static calls
From: Brendan Jackman @ 2020-08-24 14:09 UTC (permalink / raw)
To: Kees Cook
Cc: Brendan Jackman, linux-kernel, bpf, linux-security-module,
Paul Renauld, Alexei Starovoitov, Daniel Borkmann, James Morris,
pjt, Jann Horn, peterz, rafael.j.wysocki, thgarnie, KP Singh,
paul.renauld.epfl
In-Reply-To: <202008201435.97CF8296@keescook>
On Thu, 20 Aug 2020 at 23:46, Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Aug 20, 2020 at 06:47:53PM +0200, Brendan Jackman wrote:
> > From: Paul Renauld <renauld@google.com>
> >
> > LSMs have high overhead due to indirect function calls through
> > retpolines. This RPC proposes to replace these with static calls [1]
>
> typo: RFC
Oops, thanks - I also meant to have the [RFC] subject prefix.
>
> > instead.
>
> Yay! :)
>
> > [...]
> > This overhead prevents the adoption of bpf LSM on performance critical
> > systems, and also, in general, slows down all LSMs.
>
> I'd be curious to see other workloads too. (Your measurements are a bit
> synthetic, mostly showing "worst case": one short syscall in a tight
> loop. I'm curious how much performance gain can be had -- we should
> still do it, it'll be a direct performance improvement, but I'm curious
> about "real world" impact too.)
>
Sounds good - I'll gather some more data and get back.
(I would also reiterate what KP said in response
to James: the "worst case" relative indirect call overhead (i.e. the case
where the hook callback does minimal work) is exactly the case we care
about here. If the callback is doing enough work that the indirect call overhead
becomes negligible, that callback is probably anyway too heavyweight for the
use cases that motivated this work).
> > [...]
> > Previously, the code for this hook would have looked like this:
> >
> > ret = DEFAULT_RET;
> >
> > for each cb in [A, B, C]:
> > ret = cb(args); <--- costly indirect call here
> > if ret != 0:
> > break;
> >
> > return ret;
> >
> > Static calls are defined at build time and are initially empty (NOP
> > instructions). When the LSMs are initialized, the slots are filled as
> > follows:
> >
> > slot idx content
> > |-----------|
> > 0 | |
> > |-----------|
> > 1 | |
> > |-----------|
> > 2 | call A | <-- base_slot_idx = 2
> > |-----------|
> > 3 | call B |
> > |-----------|
> > 4 | call C |
> > |-----------|
> >
> > The generated code will unroll the foreach loop to have a static call for
> > each possible LSM:
> >
> > ret = DEFAULT_RET;
> > switch(base_slot_idx):
> >
> > case 0:
> > NOP
> > if ret != 0:
> > break;
> > // fallthrough
> > case 1:
> > NOP
> > if ret != 0:
> > break;
> > // fallthrough
> > case 2:
> > ret = A(args); <--- direct call, no retpoline
> > if ret != 0:
> > break;
> > // fallthrough
> > case 3:
> > ret = B(args); <--- direct call, no retpoline
> > if ret != 0:
> > break;
> > // fallthrough
> >
> > [...]
> >
> > default:
> > break;
> >
> > return ret;
> >
> > A similar logic is applied for void hooks.
> >
> > Why this trick with a switch statement? The table of static call is defined
> > at compile time. The number of hook callbacks that will be defined is
> > unknown at that time, and the table cannot be resized at runtime. Static
> > calls do not define a conditional execution for a non-void function, so the
> > executed slots must be non-empty. With this use of the table and the
> > switch, it is possible to jump directly to the first used slot and execute
> > all of the slots after. This essentially makes the entry point of the table
> > dynamic. Instead, it would also be possible to start from 0 and break after
> > the final populated slot, but that would require an additional conditional
> > after each slot.
>
> Instead of just "NOP", having the static branches perform a jump would
> solve this pretty cleanly, yes? Something like:
>
> ret = DEFAULT_RET;
>
> ret = A(args); <--- direct call, no retpoline
> if ret != 0:
> goto out;
>
> ret = B(args); <--- direct call, no retpoline
> if ret != 0:
> goto out;
>
> goto out;
> if ret != 0:
> goto out;
>
> out:
> return ret;
Hmm yeah that's a cool idea. This would either need to be implemented
with custom
code-modification logic for the LSM hooks, or we'd need to think of a
way to express it
in a sensible addition to the static_call API. I do wonder if the
latter could take
the form of a generic system for arrays of static calls.
It would also need to handle the fact that IIUC at the moment the last
static_call may be a tail
call, so we'd be patching an existing jump into a jump to a different
target, I don't know if we
can do that atomically.
More research required on my side here, on both points.
> [...]
> > Signed-off-by: Paul Renauld <renauld@google.com>
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > Signed-off-by: Brendan Jackman <jackmanb@google.com>
>
> This implies a maintainership chain, with Paul as the sole author. If
> you mean all of you worked on the patch, include Co-developed-by: as
> needed[1].
Yep, this is intentional - Paul is the sole author so far (I suppose
KP's sign-off
is not technically required since he's also at the Google).
^ permalink raw reply
* Re: [PATCH] SELinux: Measure state and hash of policy using IMA
From: Stephen Smalley @ 2020-08-24 14:00 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Mimi Zohar, Casey Schaufler, Tyler Hicks, tusharsu, sashal,
James Morris, linux-integrity, SElinux list, LSM List,
linux-kernel
In-Reply-To: <20200822010018.19453-1-nramas@linux.microsoft.com>
On Fri, Aug 21, 2020 at 9:00 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> Critical data structures of security modules are currently not measured.
> Therefore an attestation service, for instance, would not be able to
> attest whether the security modules are always operating with the policies
> and configuration that the system administrator had setup. The policies
> and configuration for the security modules could be tampered with by
> rogue user mode agents or modified through some inadvertent actions on
> the system. Measuring such critical data would enable an attestation
> service to reliably assess the security configuration of the system.
>
> SELinux configuration and policy are some of the critical data for this
> security module that needs to be measured. This measurement can be used
> by an attestation service, for instance, to verify if the configuration
> and policies have been setup correctly and that they haven't been tampered
> with at runtime.
>
> Measure SELinux configuration, policy capabilities settings, and the hash
> of the loaded policy by calling the IMA hook ima_measure_critical_data().
> Since the size of the loaded policy can be quite large, hash of the policy
> is measured instead of the entire policy to avoid bloating the IMA log.
>
> Enable early boot measurement for SELinux in IMA since SELinux
> initializes its state and policy before custom IMA policy is loaded.
>
> Sample measurement of SELinux state and hash of the policy:
>
> 10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state-1595389364:287899386 696e697469616c697a65643d313b656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574776f726b5f706565725f636f6e74726f6c733d313b6f70656e5f7065726d733d313b657874656e6465645f736f636b65745f636c6173733d313b616c776179735f636865636b5f6e6574776f726b3d303b6367726f75705f7365636c6162656c3d313b6e6e705f6e6f737569645f7472616e736974696f6e3d313b67656e66735f7365636c6162656c5f73796d6c696e6b733d303
> 10 9e81...0857 ima-buf sha256:4941...68fc selinux-policy-hash-1597335667:462051628 8d1d...1834
>
> To verify the measurement check the following:
>
> Execute the following command to extract the measured data
> from the IMA log for SELinux configuration (selinux-state).
>
> grep -m 1 "selinux-state" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | cut -d' ' -f 6 | xxd -r -p
>
> The output should be the list of key-value pairs. For example,
> initialized=1;enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0;
>
> To verify the measured data with the current SELinux state:
>
> => enabled should be set to 1 if /sys/fs/selinux folder exists,
> 0 otherwise
>
> For other entries, compare the integer value in the files
> => /sys/fs/selinux/enforce
> => /sys/fs/selinux/checkreqprot
> And, each of the policy capabilities files under
> => /sys/fs/selinux/policy_capabilities
>
> For selinux-policy-hash, the hash of SELinux policy is included
> in the IMA log entry.
>
> To verify the measured data with the current SELinux policy run
> the following commands and verify the output hash values match.
>
> sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
>
> grep -m 1 "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | cut -d' ' -f 6
>
> This patch is dependent on the following patch series:
> https://patchwork.kernel.org/patch/11709527/
> https://patchwork.kernel.org/patch/11730193/
> https://patchwork.kernel.org/patch/11730757/
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> Reported-by: kernel test robot <lkp@intel.com> # error: implicit declaration of function 'vfree'
> Reported-by: kernel test robot <lkp@intel.com> # error: implicit declaration of function 'crypto_alloc_shash'
> Reported-by: kernel test robot <lkp@intel.com> # sparse: symbol 'security_read_selinux_policy' was not declared. Should it be static?
> ---
> +int security_read_policy_kernel(struct selinux_state *state,
> + void **data, size_t *len)
> +{
> + int rc;
> +
> + rc = security_read_policy_len(state, len);
> + if (rc)
> + return rc;
> +
> + *data = vmalloc(*len);
> + if (!*data)
> + return -ENOMEM;
>
> + return security_read_selinux_policy(state, data, len);
> }
See the discussion here:
https://lore.kernel.org/selinux/20200824113015.1375857-1-omosnace@redhat.com/T/#t
In order for this to be safe, you need to ensure that all callers of
security_read_policy_kernel() have taken fsi->mutex in selinuxfs and
any use of security_read_policy_len() occurs while holding the mutex.
Otherwise, the length can change between security_read_policy_len()
and security_read_selinux_policy() if policy is reloaded.
^ permalink raw reply
* Re: [PATCH] security: selinux: delete repeated words in comments
From: Paul Moore @ 2020-08-24 13:07 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-kernel, Stephen Smalley, Eric Paris, selinux, James Morris,
Serge E. Hallyn, linux-security-module
In-Reply-To: <CAHC9VhQW-8pem59QHQctx9UNhMNLEAjNwDiOU1ODz9wX5B_tdQ@mail.gmail.com>
On Fri, Aug 7, 2020 at 2:09 PM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Aug 7, 2020 at 12:51 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> >
> > Drop a repeated word in comments.
> > {open, is, then}
> >
> > Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
> > Cc: Eric Paris <eparis@parisplace.org>
> > Cc: selinux@vger.kernel.org
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: linux-security-module@vger.kernel.org
> > ---
> > security/selinux/hooks.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
>
> This obviously looks fine, but it will need to wait until after the
> merge window closes. I'll send another reply once it is merged.
Hi Randy, this got buried a bit due to vacations and other patches but
I just merged it into selinux/next. Thanks for your patience.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 08/11] ima: Allow imasig requirement to be satisfied by EVM portable signatures
From: Mimi Zohar @ 2020-08-24 13:02 UTC (permalink / raw)
To: Roberto Sassu, mjg59
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu
In-Reply-To: <20200618160458.1579-8-roberto.sassu@huawei.com>
On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> System administrators can require that all accessed files have a signature
> by specifying appraise_type=imasig in a policy rule.
>
> Currently, only IMA signatures satisfy this requirement.
Appended signatures may also satisfy this requirement, but are not
applicable as ...
> IMA signatures
> ensure data source authentication for file content and prevent any change.
> EVM signatures instead ensure data source authentication for file metadata.
> Given that the digest or signature of the file content must be included in
> the metadata, EVM signatures provide at least the same guarantees of IMA
> signatures.
^provide the same file data guarantees of IMA signatures, as well as
providing file metadata guarantees.
>
> This patch lets systems protected with EVM signatures pass appraisal
> verification if the appraise_type=imasig requirement is specified in the
> policy. This facilitates deployment in the scenarios where only EVM
> signatures are available.
>
> The patch makes the following changes:
>
> file xattr types:
> security.ima: IMA_XATTR_DIGEST/IMA_XATTR_DIGEST_NG
> security.evm: EVM_XATTR_PORTABLE_DIGSIG
>
> execve(), mmap(), open() behavior (with appraise_type=imasig):
> before: denied (file without IMA signature, imasig requirement not met)
> after: allowed (file with EVM portable signature, imasig requirement met)
>
> open(O_WRONLY) behavior (without appraise_type=imasig):
> before: allowed (file without IMA signature, not immutable)
> after: denied (file with EVM portable signature, immutable)
>
> In addition, similarly to IMA signatures, this patch temporarily allows
> new files without or with incomplete metadata to be opened so that content
> can be written.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
After addressing the comments above and below,
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
> security/integrity/ima/ima_appraise.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 21bda264fc30..9505bb390d90 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -219,12 +219,16 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
> hash_start = 1;
> /* fall through */
> case IMA_XATTR_DIGEST:
> - if (iint->flags & IMA_DIGSIG_REQUIRED) {
> - *cause = "IMA-signature-required";
> - *status = INTEGRITY_FAIL;
> - break;
> + if (*status != INTEGRITY_PASS_IMMUTABLE) {
> + if (iint->flags & IMA_DIGSIG_REQUIRED) {
> + *cause = "IMA-signature-required";
> + *status = INTEGRITY_FAIL;
> + break;
> + }
> + clear_bit(IMA_DIGSIG, &iint->atomic_flags);
> + } else {
> + set_bit(IMA_DIGSIG, &iint->atomic_flags);
> }
> - clear_bit(IMA_DIGSIG, &iint->atomic_flags);
> if (xattr_len - sizeof(xattr_value->type) - hash_start >=
> iint->ima_hash->length)
> /*
> @@ -394,6 +398,8 @@ int ima_appraise_measurement(enum ima_hooks func,
> cause = "missing-HMAC";
> goto out;
> case INTEGRITY_FAIL_IMMUTABLE:
> + set_bit(IMA_DIGSIG, &iint->atomic_flags);
> + fallthrough;
> case INTEGRITY_FAIL: /* Invalid HMAC/signature. */
> cause = "invalid-HMAC";
> goto out;
> @@ -437,9 +443,9 @@ int ima_appraise_measurement(enum ima_hooks func,
> status = INTEGRITY_PASS;
> }
>
> - /* Permit new files with file signatures, but without data. */
> + /* Permit new files marked as immutable, but without data. */
This comment isn't quite right.
> if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
> - xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
> + test_bit(IMA_DIGSIG, &iint->atomic_flags)) {
> status = INTEGRITY_PASS;
> }
>
^ permalink raw reply
* Re: [PATCH 09/11] ima: Don't remove security.ima if file must not be appraised
From: Mimi Zohar @ 2020-08-24 13:02 UTC (permalink / raw)
To: Roberto Sassu, mjg59
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu
In-Reply-To: <20200618160458.1579-9-roberto.sassu@huawei.com>
On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> Files might come from a remote source and might have xattrs, including
> security.ima. It should not be IMA task to decide whether security.ima
> should be kept or not. This patch removes the removexattr() system
> call in ima_inode_post_setattr().
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Yes, this has been previously discussed.
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH 10/11] ima: Don't ignore errors from crypto_shash_update()
From: Mimi Zohar @ 2020-08-24 13:02 UTC (permalink / raw)
To: Roberto Sassu, mjg59
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <20200618160458.1579-10-roberto.sassu@huawei.com>
On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> Errors returned by crypto_shash_update() are not checked in
> ima_calc_boot_aggregate_tfm() and thus can be overwritten at the next
> iteration of the loop. This patch adds a check after calling
> crypto_shash_update() and returns immediately if the result is not zero.
>
> Cc: stable@vger.kernel.org
> Fixes: 3323eec921efd ("integrity: IMA as an integrity service provider")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Verification of the boot_aggregate will fail, but yes this should be
fixed. This patch and the next should be moved up front to the
beginning of the patch set.
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
thanks,
Mimi
^ permalink raw reply
* [PATCH] integrity: Use current_uid() in integrity_audit_message()
From: Denis Efremov @ 2020-08-24 12:54 UTC (permalink / raw)
Cc: Denis Efremov, James Morris, Serge E. Hallyn,
Lakshmi Ramasubramanian, Mimi Zohar, linux-security-module,
linux-kernel
Modify integrity_audit_message() to use current_uid().
Signed-off-by: Denis Efremov <efremov@linux.com>
---
security/integrity/integrity_audit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index f25e7df099c8..29220056207f 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -47,7 +47,7 @@ void integrity_audit_message(int audit_msgno, struct inode *inode,
ab = audit_log_start(audit_context(), GFP_KERNEL, audit_msgno);
audit_log_format(ab, "pid=%d uid=%u auid=%u ses=%u",
task_pid_nr(current),
- from_kuid(&init_user_ns, current_cred()->uid),
+ from_kuid(&init_user_ns, current_uid()),
from_kuid(&init_user_ns, audit_get_loginuid(current)),
audit_get_sessionid(current));
audit_log_task_context(ab);
--
2.26.2
^ permalink raw reply related
* [PATCH] security/commoncap: Use current_user_ns()
From: Denis Efremov @ 2020-08-24 12:49 UTC (permalink / raw)
Cc: Denis Efremov, Serge Hallyn, James Morris, linux-security-module,
linux-kernel
Modify cap_inh_is_capped(), cap_task_prctl() to use current_user_ns().
Signed-off-by: Denis Efremov <efremov@linux.com>
---
security/commoncap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/commoncap.c b/security/commoncap.c
index 59bf3c1674c8..82a61f77c07c 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -220,7 +220,7 @@ static inline int cap_inh_is_capped(void)
/* they are so limited unless the current task has the CAP_SETPCAP
* capability
*/
- if (cap_capable(current_cred(), current_cred()->user_ns,
+ if (cap_capable(current_cred(), current_user_ns(),
CAP_SETPCAP, CAP_OPT_NONE) == 0)
return 0;
return 1;
@@ -1206,7 +1206,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
|| ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
|| (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
|| (cap_capable(current_cred(),
- current_cred()->user_ns,
+ current_user_ns(),
CAP_SETPCAP,
CAP_OPT_NONE) != 0) /*[4]*/
/*
--
2.26.2
^ permalink raw reply related
* Re: [PATCH 07/11] evm: Set IMA_CHANGE_XATTR/ATTR bit if EVM_ALLOW_METADATA_WRITES is set
From: Mimi Zohar @ 2020-08-24 12:17 UTC (permalink / raw)
To: Roberto Sassu, mjg59
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <20200618160458.1579-7-roberto.sassu@huawei.com>
On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
> metadata. Its main purpose is to allow users to freely set metadata when
> they are protected by a portable signature, until the HMAC key is loaded.
>
> However, IMA is not notified about metadata changes and, after the first
> appraisal, always allows access to the files without checking metadata
> again.
^after the first successful appraisal
>
> This patch checks in evm_reset_status() if EVM_ALLOW_METADATA WRITES is
> enabled and if it is, sets the IMA_CHANGE_XATTR/ATTR bits depending on the
> operation performed. At the next appraisal, metadata are revalidated.
EVM modifying IMA bits crosses the boundary between EVM and IMA. There
is already an IMA post_setattr hook. IMA could reset its own bit
there. If necessary EVM could export as a function it's status info.
Mimi
>
> This patch also adds a call to evm_reset_status() in
> evm_inode_post_setattr() so that EVM won't return the cached status the
> next time appraisal is performed.
>
> Cc: stable@vger.kernel.org # 4.16.x
> Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> security/integrity/evm/evm_main.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 41cc6a4aaaab..d4d918183094 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -478,13 +478,17 @@ int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name)
> return evm_protect_xattr(dentry, xattr_name, NULL, 0);
> }
>
> -static void evm_reset_status(struct inode *inode)
> +static void evm_reset_status(struct inode *inode, int bit)
> {
> struct integrity_iint_cache *iint;
>
> iint = integrity_iint_find(inode);
> - if (iint)
> + if (iint) {
> + if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> + set_bit(bit, &iint->atomic_flags);
> +
> iint->evm_status = INTEGRITY_UNKNOWN;
> + }
> }
>
> /**:q
> @@ -507,7 +511,7 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
> && !posix_xattr_acl(xattr_name)))
> return;
>
> - evm_reset_status(dentry->d_inode);
> + evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
>
> evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
> }
> @@ -527,7 +531,7 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
> if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
> return;
>
> - evm_reset_status(dentry->d_inode);
> + evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
>
> evm_update_evmxattr(dentry, xattr_name, NULL, 0);
> }
> @@ -600,6 +604,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
> if (!evm_key_loaded())
> return;
>
> + evm_reset_status(dentry->d_inode, IMA_CHANGE_ATTR);
> +
> if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> evm_update_evmxattr(dentry, NULL, NULL, 0);
> }
^ permalink raw reply
* Re: [PATCH 06/11] evm: Allow setxattr() and setattr() if metadata digest won't change
From: Mimi Zohar @ 2020-08-24 12:17 UTC (permalink / raw)
To: Roberto Sassu, mjg59
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu
In-Reply-To: <20200618160458.1579-6-roberto.sassu@huawei.com>
On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> If metadata are immutable, they cannot be changed. If metadata are already
> set to the final value before cp and tar restore the value from the source,
> those applications display an error even if the operation is legitimate
> (they don't change the value).
"metadata" is singular. The first sentence would be clearer by using
the specific metadata. What problem are you trying to solve? It
doesn't look like you're trying to solve the problem of writing the EVM
portable signatures without an exiting HMAC.
>
> This patch determines whether setxattr()/setattr() change metadata and, if
> not, allows the operations even if metadata are immutable.
Doesn't setxattr/setattr always change file metadata? Please describe
the real problem.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> security/integrity/evm/evm_main.c | 72 +++++++++++++++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 30072030f05d..41cc6a4aaaab 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -18,6 +18,7 @@
> #include <linux/integrity.h>
> #include <linux/evm.h>
> #include <linux/magic.h>
> +#include <linux/posix_acl_xattr.h>
>
> #include <crypto/hash.h>
> #include <crypto/hash_info.h>
> @@ -305,6 +306,56 @@ static enum integrity_status evm_verify_current_integrity(struct dentry *dentry)
> return evm_verify_hmac(dentry, NULL, NULL, 0, NULL);
> }
>
> +static int evm_xattr_acl_change(struct dentry *dentry, const char *xattr_name,
> + const void *xattr_value, size_t xattr_value_len)
> +{
> + umode_t mode;
> + struct posix_acl *acl = NULL, *acl_res;
> + struct inode *inode = d_backing_inode(dentry);
> + int rc;
> +
> + /* UID/GID in ACL have been already converted from user to init ns */
> + acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);
> + if (!acl)
> + return 1;
> +
> + acl_res = acl;
> + rc = posix_acl_update_mode(inode, &mode, &acl_res);
> +
> + posix_acl_release(acl);
> +
> + if (rc)
> + return 1;
> +
> + if (acl_res && inode->i_mode != mode)
> + return 1;
> +
> + return 0;
> +}
> +
> +static int evm_xattr_change(struct dentry *dentry, const char *xattr_name,
> + const void *xattr_value, size_t xattr_value_len)
> +{
> + char *xattr_data = NULL;
> + int rc = 0;
> +
> + if (posix_xattr_acl(xattr_name))
> + return evm_xattr_acl_change(dentry, xattr_name, xattr_value,
> + xattr_value_len);
> +
> + rc = vfs_getxattr_alloc(dentry, xattr_name, &xattr_data, 0, GFP_NOFS);
> + if (rc < 0)
> + return 1;
> +
> + if (rc == xattr_value_len)
> + rc = memcmp(xattr_value, xattr_data, rc);
> + else
> + rc = 1;
> +
> + kfree(xattr_data);
> + return rc;
> +}
> +
> /*
> * evm_protect_xattr - protect the EVM extended attribute
> *
> @@ -361,6 +412,10 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
> if (evm_status == INTEGRITY_FAIL_IMMUTABLE)
> return 0;
>
> + if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> + !evm_xattr_change(dentry, xattr_name, xattr_value, xattr_value_len))
> + return 0;
> +
> if (evm_status != INTEGRITY_PASS)
> integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
> dentry->d_name.name, "appraise_metadata",
> @@ -477,6 +532,19 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
> evm_update_evmxattr(dentry, xattr_name, NULL, 0);
> }
>
> +static int evm_attr_change(struct dentry *dentry, struct iattr *attr)
static functions don't normally require a function comment, but in this
case it wouldn't hurt to clarify why the uid, gid, mode bits are set,
but aren't being modified.
Similarly a function comment would help with the readability of
evm_xattr_acl_change().
Mimi
> +{
> + struct inode *inode = d_backing_inode(dentry);
> + unsigned int ia_valid = attr->ia_valid;
> +
> + if ((!(ia_valid & ATTR_UID) || uid_eq(attr->ia_uid, inode->i_uid)) &&
> + (!(ia_valid & ATTR_GID) || gid_eq(attr->ia_gid, inode->i_gid)) &&
> + (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode))
> + return 0;
> +
> + return 1;
> +}
> +
> /**
> * evm_inode_setattr - prevent updating an invalid EVM extended attribute
> * @dentry: pointer to the affected dentry
> @@ -506,6 +574,10 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
> (evm_status == INTEGRITY_FAIL_IMMUTABLE))
> return 0;
>
> + if (evm_status == INTEGRITY_PASS_IMMUTABLE &&
> + !evm_attr_change(dentry, attr))
> + return 0;
> +
> integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
> dentry->d_name.name, "appraise_metadata",
> integrity_status_msg[evm_status], -EPERM, 0);
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox