* Re: [PATCH v11 02/25] LSM: Create and manage the lsmblob data structure.
From: Tetsuo Handa @ 2019-11-23 5:21 UTC (permalink / raw)
To: Casey Schaufler; +Cc: linux-security-module
In-Reply-To: <20191123011454.3292-1-casey@schaufler-ca.com>
On 2019/11/23 10:14, Casey Schaufler wrote:
> When more than one security module is exporting data to
> audit and networking sub-systems a single 32 bit integer
> is no longer sufficient to represent the data. Add a
> structure to be used instead.
>
> The lsmblob structure is currently an array of
> u32 "secids". There is an entry for each of the
> security modules built into the system that would
> use secids if active. The system assigns the module
> a "slot" when it registers hooks. If modules are
> compiled in but not registered there will be unused
> slots.
>
> A new lsm_id structure, which contains the name
> of the LSM and its slot number, is created. There
> is an instance for each LSM, which assigns the name
> and passes it to the infrastructure to set the slot.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/lsm_hooks.h | 12 ++++++--
> include/linux/security.h | 58 ++++++++++++++++++++++++++++++++++++++
> security/apparmor/lsm.c | 7 ++++-
> security/commoncap.c | 7 ++++-
> security/loadpin/loadpin.c | 8 +++++-
> security/safesetid/lsm.c | 8 +++++-
> security/security.c | 28 ++++++++++++++----
> security/selinux/hooks.c | 8 +++++-
> security/smack/smack_lsm.c | 7 ++++-
> security/tomoyo/tomoyo.c | 8 +++++-
> security/yama/yama_lsm.c | 7 ++++-
> 11 files changed, 142 insertions(+), 16 deletions(-)
>
For TOMOYO part,
Acked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
^ permalink raw reply
* Re: general protection fault in __schedule (2)
From: Dmitry Vyukov @ 2019-11-23 5:15 UTC (permalink / raw)
To: Sean Christopherson
Cc: syzbot, Casey Schaufler, Frederic Weisbecker, Greg Kroah-Hartman,
H. Peter Anvin, Jim Mattson, James Morris, Raslan, KarimAllah,
Kate Stewart, KVM list, LKML, linux-security-module, Ingo Molnar,
Ingo Molnar, Pavel Tatashin, Paolo Bonzini, Philippe Ombredanne,
Radim Krčmář, Serge E. Hallyn, syzkaller-bugs,
Thomas Gleixner, the arch/x86 maintainers
In-Reply-To: <20191122205453.GE31235@linux.intel.com>
On Fri, Nov 22, 2019 at 9:54 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Thu, Nov 21, 2019 at 11:19:00PM -0800, syzbot wrote:
> > syzbot has bisected this bug to:
> >
> > commit 8fcc4b5923af5de58b80b53a069453b135693304
> > Author: Jim Mattson <jmattson@google.com>
> > Date: Tue Jul 10 09:27:20 2018 +0000
> >
> > kvm: nVMX: Introduce KVM_CAP_NESTED_STATE
> >
> > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=124cdbace00000
> > start commit: 234b69e3 ocfs2: fix ocfs2 read block panic
> > git tree: upstream
> > final crash: https://syzkaller.appspot.com/x/report.txt?x=114cdbace00000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=164cdbace00000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=5fa12be50bca08d8
> > dashboard link: https://syzkaller.appspot.com/bug?extid=7e2ab84953e4084a638d
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=150f0a4e400000
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17f67111400000
> >
> > Reported-by: syzbot+7e2ab84953e4084a638d@syzkaller.appspotmail.com
> > Fixes: 8fcc4b5923af ("kvm: nVMX: Introduce KVM_CAP_NESTED_STATE")
> >
> > For information about bisection process see: https://goo.gl/tpsmEJ#bisection
>
> Is there a way to have syzbot stop processing/bisecting these things
> after a reasonable amount of time? The original crash is from August of
> last year...
>
> Note, the original crash is actually due to KVM's put_kvm() fd race, but
> whatever we want to blame, it's a duplicate.
>
> #syz dup: general protection fault in kvm_lapic_hv_timer_in_use
Hi Sean,
syzbot only sends bisection results to open bugs with no known fixes.
So what you did (marking the bug as invalid/dup, or attaching a fix)
would stop it from doing/sending bisection.
"Original crash happened a long time ago" is not necessary a good
signal. On the syzbot dashboard
(https://syzkaller.appspot.com/upstream), you can see bugs with the
original crash 2+ years ago, but they are still pretty much relevant.
The default kernel development process strategy for invalidating bug
reports by burying them in oblivion has advantages, but also
downsides. FWIW syzbot prefers explicit status tracking.
Besides implications on the mainline development, consider the
following. We regularly discover the same bugs (missed backports) on
LTS kernels:
https://syzkaller.appspot.com/linux-4.14
https://syzkaller.appspot.com/linux-4.19
The dashboard also shows similar crash signatures in other tested
kernels. So say you see a crash in your product kernel, and you notice
that a similar crash happened on mainline some time ago, but
presumably it was fixed, but then you look at the bug report thread
and there is no info whatsoever as to what happened.
Now this bug report:
https://syzkaller.appspot.com/bug?extid=7e2ab84953e4084a638d
is linked to "general protection fault in kvm_lapic_hv_timer_in_use":
https://syzkaller.appspot.com/bug?id=0c330c4e475223a40d95f1d94c761357dd0f011f
which has a recorded fix "KVM: nVMX: Fix bad cleanup on error of
get/set nested state IOCTLs":
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=26b471c7e2f7befd0f59c35b257749ca57e0ed70
^ permalink raw reply
* Re: general protection fault in __schedule (2)
From: Sean Christopherson @ 2019-11-22 20:54 UTC (permalink / raw)
To: syzbot
Cc: casey, frederic, gregkh, hpa, jmattson, jmorris, karahmed,
kstewart, kvm, linux-kernel, linux-security-module, mingo, mingo,
pasha.tatashin, pbonzini, pombredanne, rkrcmar, serge,
syzkaller-bugs, tglx, x86
In-Reply-To: <0000000000005eb1070597ea3a1f@google.com>
On Thu, Nov 21, 2019 at 11:19:00PM -0800, syzbot wrote:
> syzbot has bisected this bug to:
>
> commit 8fcc4b5923af5de58b80b53a069453b135693304
> Author: Jim Mattson <jmattson@google.com>
> Date: Tue Jul 10 09:27:20 2018 +0000
>
> kvm: nVMX: Introduce KVM_CAP_NESTED_STATE
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=124cdbace00000
> start commit: 234b69e3 ocfs2: fix ocfs2 read block panic
> git tree: upstream
> final crash: https://syzkaller.appspot.com/x/report.txt?x=114cdbace00000
> console output: https://syzkaller.appspot.com/x/log.txt?x=164cdbace00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=5fa12be50bca08d8
> dashboard link: https://syzkaller.appspot.com/bug?extid=7e2ab84953e4084a638d
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=150f0a4e400000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17f67111400000
>
> Reported-by: syzbot+7e2ab84953e4084a638d@syzkaller.appspotmail.com
> Fixes: 8fcc4b5923af ("kvm: nVMX: Introduce KVM_CAP_NESTED_STATE")
>
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
Is there a way to have syzbot stop processing/bisecting these things
after a reasonable amount of time? The original crash is from August of
last year...
Note, the original crash is actually due to KVM's put_kvm() fd race, but
whatever we want to blame, it's a duplicate.
#syz dup: general protection fault in kvm_lapic_hv_timer_in_use
^ permalink raw reply
* [PATCH 2/2] selinux: fall back to ref-walk if audit is required
From: Stephen Smalley @ 2019-11-22 17:22 UTC (permalink / raw)
To: selinux
Cc: paul, will, viro, neilb, linux-fsdevel, linux-security-module,
Stephen Smalley
In-Reply-To: <20191122172245.7875-1-sds@tycho.nsa.gov>
commit bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
passed down the rcu flag to the SELinux AVC, but failed to adjust the
test in slow_avc_audit() to also return -ECHILD on LSM_AUDIT_DATA_DENTRY.
Previously, we only returned -ECHILD if generating an audit record with
LSM_AUDIT_DATA_INODE since this was only relevant from inode_permission.
Move the handling of MAY_NOT_BLOCK to avc_audit() and its inlined
equivalent in selinux_inode_permission() immediately after we determine
that audit is required, and always fall back to ref-walk in this case.
Fixes: bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
Reported-by: Will Deacon <will@kernel.org>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
security/selinux/avc.c | 24 +++++-------------------
security/selinux/hooks.c | 11 +++++++----
security/selinux/include/avc.h | 8 +++++---
3 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 74c43ebe34bb..23dc888ae305 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -424,7 +424,7 @@ static inline int avc_xperms_audit(struct selinux_state *state,
if (likely(!audited))
return 0;
return slow_avc_audit(state, ssid, tsid, tclass, requested,
- audited, denied, result, ad, 0);
+ audited, denied, result, ad);
}
static void avc_node_free(struct rcu_head *rhead)
@@ -758,8 +758,7 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
noinline int slow_avc_audit(struct selinux_state *state,
u32 ssid, u32 tsid, u16 tclass,
u32 requested, u32 audited, u32 denied, int result,
- struct common_audit_data *a,
- unsigned int flags)
+ struct common_audit_data *a)
{
struct common_audit_data stack_data;
struct selinux_audit_data sad;
@@ -772,17 +771,6 @@ noinline int slow_avc_audit(struct selinux_state *state,
a->type = LSM_AUDIT_DATA_NONE;
}
- /*
- * When in a RCU walk do the audit on the RCU retry. This is because
- * the collection of the dname in an inode audit message is not RCU
- * safe. Note this may drop some audits when the situation changes
- * during retry. However this is logically just as if the operation
- * happened a little later.
- */
- if ((a->type == LSM_AUDIT_DATA_INODE) &&
- (flags & MAY_NOT_BLOCK))
- return -ECHILD;
-
sad.tclass = tclass;
sad.requested = requested;
sad.ssid = ssid;
@@ -855,16 +843,14 @@ static int avc_update_node(struct selinux_avc *avc,
/*
* If we are in a non-blocking code path, e.g. VFS RCU walk,
* then we must not add permissions to a cache entry
- * because we cannot safely audit the denial. Otherwise,
+ * because we will not audit the denial. Otherwise,
* during the subsequent blocking retry (e.g. VFS ref walk), we
* will find the permissions already granted in the cache entry
* and won't audit anything at all, leading to silent denials in
* permissive mode that only appear when in enforcing mode.
*
- * See the corresponding handling in slow_avc_audit(), and the
- * logic in selinux_inode_follow_link and selinux_inode_permission
- * for the VFS MAY_NOT_BLOCK flag, which is transliterated into
- * AVC_NONBLOCKING for avc_has_perm_noaudit().
+ * See the corresponding handling of MAY_NOT_BLOCK in avc_audit()
+ * and selinux_inode_permission().
*/
if (flags & AVC_NONBLOCKING)
return 0;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3eaa3b419463..fd34e25c016f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3015,8 +3015,7 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct inode *inode,
static noinline int audit_inode_permission(struct inode *inode,
u32 perms, u32 audited, u32 denied,
- int result,
- unsigned flags)
+ int result)
{
struct common_audit_data ad;
struct inode_security_struct *isec = selinux_inode(inode);
@@ -3027,7 +3026,7 @@ static noinline int audit_inode_permission(struct inode *inode,
rc = slow_avc_audit(&selinux_state,
current_sid(), isec->sid, isec->sclass, perms,
- audited, denied, result, &ad, flags);
+ audited, denied, result, &ad);
if (rc)
return rc;
return 0;
@@ -3074,7 +3073,11 @@ static int selinux_inode_permission(struct inode *inode, int mask)
if (likely(!audited))
return rc;
- rc2 = audit_inode_permission(inode, perms, audited, denied, rc, flags);
+ /* fall back to ref-walk if we have to generate audit */
+ if (flags & MAY_NOT_BLOCK)
+ return -ECHILD;
+
+ rc2 = audit_inode_permission(inode, perms, audited, denied, rc);
if (rc2)
return rc2;
return rc;
diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
index 74ea50977c20..cf4cc3ef959b 100644
--- a/security/selinux/include/avc.h
+++ b/security/selinux/include/avc.h
@@ -100,8 +100,7 @@ static inline u32 avc_audit_required(u32 requested,
int slow_avc_audit(struct selinux_state *state,
u32 ssid, u32 tsid, u16 tclass,
u32 requested, u32 audited, u32 denied, int result,
- struct common_audit_data *a,
- unsigned flags);
+ struct common_audit_data *a);
/**
* avc_audit - Audit the granting or denial of permissions.
@@ -135,9 +134,12 @@ static inline int avc_audit(struct selinux_state *state,
audited = avc_audit_required(requested, avd, result, 0, &denied);
if (likely(!audited))
return 0;
+ /* fall back to ref-walk if we have to generate audit */
+ if (flags & MAY_NOT_BLOCK)
+ return -ECHILD;
return slow_avc_audit(state, ssid, tsid, tclass,
requested, audited, denied, result,
- a, flags);
+ a);
}
#define AVC_STRICT 1 /* Ignore permissive mode. */
--
2.23.0
^ permalink raw reply related
* [PATCH 1/2] selinux: revert "stop passing MAY_NOT_BLOCK to the AVC upon follow_link"
From: Stephen Smalley @ 2019-11-22 17:22 UTC (permalink / raw)
To: selinux
Cc: paul, will, viro, neilb, linux-fsdevel, linux-security-module,
Stephen Smalley
This reverts commit e46e01eebbbc ("selinux: stop passing MAY_NOT_BLOCK
to the AVC upon follow_link"). The correct fix is to instead fall
back to ref-walk if audit is required irrespective of the specific
audit data type. This is done in the next commit.
Fixes: e46e01eebbbc ("selinux: stop passing MAY_NOT_BLOCK to the AVC upon follow_link")
Reported-by: Will Deacon <will@kernel.org>
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
security/selinux/avc.c | 24 ++++++++++++++++++++++--
security/selinux/hooks.c | 5 +++--
security/selinux/include/avc.h | 5 +++++
3 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index ecd3829996aa..74c43ebe34bb 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -862,8 +862,9 @@ static int avc_update_node(struct selinux_avc *avc,
* permissive mode that only appear when in enforcing mode.
*
* See the corresponding handling in slow_avc_audit(), and the
- * logic in selinux_inode_permission for the MAY_NOT_BLOCK flag,
- * which is transliterated into AVC_NONBLOCKING.
+ * logic in selinux_inode_follow_link and selinux_inode_permission
+ * for the VFS MAY_NOT_BLOCK flag, which is transliterated into
+ * AVC_NONBLOCKING for avc_has_perm_noaudit().
*/
if (flags & AVC_NONBLOCKING)
return 0;
@@ -1205,6 +1206,25 @@ int avc_has_perm(struct selinux_state *state, u32 ssid, u32 tsid, u16 tclass,
return rc;
}
+int avc_has_perm_flags(struct selinux_state *state,
+ u32 ssid, u32 tsid, u16 tclass, u32 requested,
+ struct common_audit_data *auditdata,
+ int flags)
+{
+ struct av_decision avd;
+ int rc, rc2;
+
+ rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested,
+ (flags & MAY_NOT_BLOCK) ? AVC_NONBLOCKING : 0,
+ &avd);
+
+ rc2 = avc_audit(state, ssid, tsid, tclass, requested, &avd, rc,
+ auditdata, flags);
+ if (rc2)
+ return rc2;
+ return rc;
+}
+
u32 avc_policy_seqno(struct selinux_state *state)
{
return state->avc->avc_cache.latest_notif;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 36e531b91df2..3eaa3b419463 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3008,8 +3008,9 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct inode *inode,
if (IS_ERR(isec))
return PTR_ERR(isec);
- return avc_has_perm(&selinux_state,
- sid, isec->sid, isec->sclass, FILE__READ, &ad);
+ return avc_has_perm_flags(&selinux_state,
+ sid, isec->sid, isec->sclass, FILE__READ, &ad,
+ rcu ? MAY_NOT_BLOCK : 0);
}
static noinline int audit_inode_permission(struct inode *inode,
diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h
index 7be0e1e90e8b..74ea50977c20 100644
--- a/security/selinux/include/avc.h
+++ b/security/selinux/include/avc.h
@@ -153,6 +153,11 @@ int avc_has_perm(struct selinux_state *state,
u32 ssid, u32 tsid,
u16 tclass, u32 requested,
struct common_audit_data *auditdata);
+int avc_has_perm_flags(struct selinux_state *state,
+ u32 ssid, u32 tsid,
+ u16 tclass, u32 requested,
+ struct common_audit_data *auditdata,
+ int flags);
int avc_has_extended_perms(struct selinux_state *state,
u32 ssid, u32 tsid, u16 tclass, u32 requested,
--
2.23.0
^ permalink raw reply related
* Re: [RFC PATCH 2/2] selinux: fall back to ref-walk upon LSM_AUDIT_DATA_DENTRY too
From: Stephen Smalley @ 2019-11-22 17:04 UTC (permalink / raw)
To: Paul Moore
Cc: selinux, will, viro, neilb, linux-fsdevel, linux-security-module,
Linus Torvalds
In-Reply-To: <6fcbe2ee-bdb2-7365-5c3d-46424b7bfc23@tycho.nsa.gov>
On 11/22/19 10:09 AM, Stephen Smalley wrote:
> On 11/22/19 9:49 AM, Paul Moore wrote:
>> On Fri, Nov 22, 2019 at 8:37 AM Stephen Smalley <sds@tycho.nsa.gov>
>> wrote:
>>> On 11/21/19 7:30 PM, Paul Moore wrote:
>>>> On Thu, Nov 21, 2019 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
>>>>> On Thu, Nov 21, 2019 at 9:52 AM Stephen Smalley <sds@tycho.nsa.gov>
>>>>> wrote:
>>>>>> commit bda0be7ad994 ("security: make inode_follow_link RCU-walk
>>>>>> aware")
>>>>>> passed down the rcu flag to the SELinux AVC, but failed to adjust the
>>>>>> test in slow_avc_audit() to also return -ECHILD on
>>>>>> LSM_AUDIT_DATA_DENTRY.
>>>>>> Previously, we only returned -ECHILD if generating an audit record
>>>>>> with
>>>>>> LSM_AUDIT_DATA_INODE since this was only relevant from
>>>>>> inode_permission.
>>>>>> Return -ECHILD on either LSM_AUDIT_DATA_INODE or
>>>>>> LSM_AUDIT_DATA_DENTRY.
>>>>>> LSM_AUDIT_DATA_INODE only requires this handling due to the fact
>>>>>> that dump_common_audit_data() calls d_find_alias() and collects the
>>>>>> dname from the result if any.
>>>>>> Other cases that might require similar treatment in the future are
>>>>>> LSM_AUDIT_DATA_PATH and LSM_AUDIT_DATA_FILE if any hook that takes
>>>>>> a path or file is called under RCU-walk.
>>>>>>
>>>>>> Fixes: bda0be7ad994 ("security: make inode_follow_link RCU-walk
>>>>>> aware")
>>>>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>>>>>> ---
>>>>>> security/selinux/avc.c | 3 ++-
>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
>>>>>> index 74c43ebe34bb..f1fa1072230c 100644
>>>>>> --- a/security/selinux/avc.c
>>>>>> +++ b/security/selinux/avc.c
>>>>>> @@ -779,7 +779,8 @@ noinline int slow_avc_audit(struct
>>>>>> selinux_state *state,
>>>>>> * during retry. However this is logically just as if
>>>>>> the operation
>>>>>> * happened a little later.
>>>>>> */
>>>>>> - if ((a->type == LSM_AUDIT_DATA_INODE) &&
>>>>>> + if ((a->type == LSM_AUDIT_DATA_INODE ||
>>>>>> + a->type == LSM_AUDIT_DATA_DENTRY) &&
>>>>>> (flags & MAY_NOT_BLOCK))
>>>>>> return -ECHILD;
>>>>
>>>> With LSM_AUDIT_DATA_INODE we eventually end up calling d_find_alias()
>>>> in dump_common_audit_data() which could block, which is bad, that I
>>>> understand. However, looking at LSM_AUDIT_DATA_DENTRY I'm less clear
>>>> on why that is bad? It makes a few audit_log*() calls and one call to
>>>> d_backing_inode() which is non-blocking and trivial.
>>>>
>>>> What am I missing?
>>>
>>> For those who haven't, you may wish to also read the earlier thread here
>>> that led to this one:
>>> https://lore.kernel.org/selinux/20191119184057.14961-1-will@kernel.org/T/#t
>>>
>>>
>>> AFAIK, neither the LSM_AUDIT_DATA_INODE nor the LSM_AUDIT_DATA_DENTRY
>>> case truly block (d_find_alias does not block AFAICT, nor should
>>> audit_log* as long as we use audit_log_start with GFP_ATOMIC or
>>> GFP_NOWAIT).
>>
>> Yes, the audit_log*() functions should be safe, if not I would
>> consider that a bug; I thought d_find_alias() might block, but it's
>> very likely I'm wrong in that regard.
>
> No, it doesn't appear to block. However, it does take d_lock and
> increment d_lockref.count, which IIUC aren't permitted during rcu-walk.
>
>>> My impression from the comment in slow_avc_audit() is that
>>> the issue is not really about blocking but rather about the inability to
>>> safely dereference the dentry->d_name during RCU walk, which is
>>> something that can occur under LSM_AUDIT_DATA_INODE or _DENTRY (or _PATH
>>> or _FILE, but neither of the latter two are currently used from the two
>>> hooks that are called during RCU walk, inode_permission and
>>> inode_follow_link). Originally _PATH, _DENTRY, and _INODE were all
>>> under a single _FS type and the original test in slow_avc_audit() was
>>> against LSM_AUDIT_DATA_FS before the split.
>>
>> Thanks, I think that is the part I was missing. I was focused too
>> much on the VFS stuff that I didn't pay enough attention to
>> slow_avc_audit().
>>
>> If that is the case, the comment and code in dentry_cmp() would seem
>> to indicate that it would be safe to fetch the dentry name string as
>> long as we use READ_ONCE(). The length field in the qstr might be
>> off, but the audit_log_untrustedstring() function doesn't use the
>> qstr's length information. I suppose if we don't mind the extra
>> spinlock we could use take_dentry_name_snapshot(); that should be safe
>> and we are already in the "slow" path. I didn't check the _PATH or
>> _FILE cases.
>>
>> Once again, let me know if I'm missing something.
>
> We can't take any spinlocks on the dentry during rcu-walk IIUC; that
> would defeat the purpose. In looking for a parallel with filesystem
> implementations, I noted that fs/namei.c:get_link() doesn't even pass
> the dentry to the filesystem get_link() method in the rcu-walk case,
> only doing so under ref-walk. So they won't permit the filesystem
> implementations to ever dereference the dentry for get_link() under
> rcu-walk. Not sure why it gets passed to security_inode_follow_link()
> then, or if it is ever safe for a security module to dereference its
> fields.
>
> I was hoping to get fsdevel folks to comment since I feel like we're
> guessing about exactly what guarantees we have in this area.
>
>>
>> As an aside, if we somehow can guarantee (e.g. via a name_snapshot)
>> that qstr length information is valid, we might want to consider
>> moving from audit_log_unstrustedstring() to
>> audit_log_n_untrustedstring() to save us a call to strlen().
>>
>>>>> Added the LSM list as I'm beginning to wonder if we should push this
>>>>> logic down into common_lsm_audit(), this problem around blocking
>>>>> shouldn't be SELinux specific.
>>>
>>> That would require passing down the MAY_NOT_BLOCK flag or a rcu bool to
>>> common_lsm_audit() just so that it could immediately return if that is
>>> set and a->type is _INODE or _DENTRY (or _PATH or _FILE). And the
>>> individual security module still needs to have its own handling of
>>> MAY_NOT_BLOCK/rcu for its own processing, so it won't free the security
>>> module authors from thinking about it.
>>
>> Looking at the current SELinux code, all we do is bail out early with
>> -ECHILD. If we didn't have that check it looks like the only impact
>> would be some extra assignments into a struct living on the stack and
>> a call into common_lsm_audit(). That doesn't seem terrible for a slow
>> path, especially if it pushes this code into a LSM common function.
>
> Not terrible, just not sure if it ends up being a worthwhile change. If
> the LSM maintainers would like it that way, I can do that.
I think this rendered moot by viro's suggestion, since we are taking the
handling of MAY_NOT_BLOCK up even earlier in the processing and the
flags don't need to be passed down to slow_avc_audit() anymore. Sure,
we could still pass them down and defer the handling to
common_lsm_audit(), but that's just extra wasted work before we bail
out, and we are no longer even testing the a->type field with the new
logic so there is no longer anything related to the lsm_audit
implementation.
>
>>
>>>>> For the LSM folks just joining, the full patchset can be found here:
>>>>> *
>>>>> https://lore.kernel.org/selinux/20191121145245.8637-1-sds@tycho.nsa.gov/T/#t
>>>>>
>>
>
^ permalink raw reply
* Re: [RFC PATCH 2/2] selinux: fall back to ref-walk upon LSM_AUDIT_DATA_DENTRY too
From: Stephen Smalley @ 2019-11-22 15:09 UTC (permalink / raw)
To: Paul Moore
Cc: selinux, will, viro, neilb, linux-fsdevel, linux-security-module,
Linus Torvalds
In-Reply-To: <CAHC9VhRiRdWfqP8sp8YKRdc4D9r9u1QYP5o2sRh7QwvgCRYYbg@mail.gmail.com>
On 11/22/19 9:49 AM, Paul Moore wrote:
> On Fri, Nov 22, 2019 at 8:37 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 11/21/19 7:30 PM, Paul Moore wrote:
>>> On Thu, Nov 21, 2019 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
>>>> On Thu, Nov 21, 2019 at 9:52 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>> commit bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
>>>>> passed down the rcu flag to the SELinux AVC, but failed to adjust the
>>>>> test in slow_avc_audit() to also return -ECHILD on LSM_AUDIT_DATA_DENTRY.
>>>>> Previously, we only returned -ECHILD if generating an audit record with
>>>>> LSM_AUDIT_DATA_INODE since this was only relevant from inode_permission.
>>>>> Return -ECHILD on either LSM_AUDIT_DATA_INODE or LSM_AUDIT_DATA_DENTRY.
>>>>> LSM_AUDIT_DATA_INODE only requires this handling due to the fact
>>>>> that dump_common_audit_data() calls d_find_alias() and collects the
>>>>> dname from the result if any.
>>>>> Other cases that might require similar treatment in the future are
>>>>> LSM_AUDIT_DATA_PATH and LSM_AUDIT_DATA_FILE if any hook that takes
>>>>> a path or file is called under RCU-walk.
>>>>>
>>>>> Fixes: bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
>>>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>>>>> ---
>>>>> security/selinux/avc.c | 3 ++-
>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
>>>>> index 74c43ebe34bb..f1fa1072230c 100644
>>>>> --- a/security/selinux/avc.c
>>>>> +++ b/security/selinux/avc.c
>>>>> @@ -779,7 +779,8 @@ noinline int slow_avc_audit(struct selinux_state *state,
>>>>> * during retry. However this is logically just as if the operation
>>>>> * happened a little later.
>>>>> */
>>>>> - if ((a->type == LSM_AUDIT_DATA_INODE) &&
>>>>> + if ((a->type == LSM_AUDIT_DATA_INODE ||
>>>>> + a->type == LSM_AUDIT_DATA_DENTRY) &&
>>>>> (flags & MAY_NOT_BLOCK))
>>>>> return -ECHILD;
>>>
>>> With LSM_AUDIT_DATA_INODE we eventually end up calling d_find_alias()
>>> in dump_common_audit_data() which could block, which is bad, that I
>>> understand. However, looking at LSM_AUDIT_DATA_DENTRY I'm less clear
>>> on why that is bad? It makes a few audit_log*() calls and one call to
>>> d_backing_inode() which is non-blocking and trivial.
>>>
>>> What am I missing?
>>
>> For those who haven't, you may wish to also read the earlier thread here
>> that led to this one:
>> https://lore.kernel.org/selinux/20191119184057.14961-1-will@kernel.org/T/#t
>>
>> AFAIK, neither the LSM_AUDIT_DATA_INODE nor the LSM_AUDIT_DATA_DENTRY
>> case truly block (d_find_alias does not block AFAICT, nor should
>> audit_log* as long as we use audit_log_start with GFP_ATOMIC or
>> GFP_NOWAIT).
>
> Yes, the audit_log*() functions should be safe, if not I would
> consider that a bug; I thought d_find_alias() might block, but it's
> very likely I'm wrong in that regard.
No, it doesn't appear to block. However, it does take d_lock and
increment d_lockref.count, which IIUC aren't permitted during rcu-walk.
>> My impression from the comment in slow_avc_audit() is that
>> the issue is not really about blocking but rather about the inability to
>> safely dereference the dentry->d_name during RCU walk, which is
>> something that can occur under LSM_AUDIT_DATA_INODE or _DENTRY (or _PATH
>> or _FILE, but neither of the latter two are currently used from the two
>> hooks that are called during RCU walk, inode_permission and
>> inode_follow_link). Originally _PATH, _DENTRY, and _INODE were all
>> under a single _FS type and the original test in slow_avc_audit() was
>> against LSM_AUDIT_DATA_FS before the split.
>
> Thanks, I think that is the part I was missing. I was focused too
> much on the VFS stuff that I didn't pay enough attention to
> slow_avc_audit().
>
> If that is the case, the comment and code in dentry_cmp() would seem
> to indicate that it would be safe to fetch the dentry name string as
> long as we use READ_ONCE(). The length field in the qstr might be
> off, but the audit_log_untrustedstring() function doesn't use the
> qstr's length information. I suppose if we don't mind the extra
> spinlock we could use take_dentry_name_snapshot(); that should be safe
> and we are already in the "slow" path. I didn't check the _PATH or
> _FILE cases.
>
> Once again, let me know if I'm missing something.
We can't take any spinlocks on the dentry during rcu-walk IIUC; that
would defeat the purpose. In looking for a parallel with filesystem
implementations, I noted that fs/namei.c:get_link() doesn't even pass
the dentry to the filesystem get_link() method in the rcu-walk case,
only doing so under ref-walk. So they won't permit the filesystem
implementations to ever dereference the dentry for get_link() under
rcu-walk. Not sure why it gets passed to security_inode_follow_link()
then, or if it is ever safe for a security module to dereference its fields.
I was hoping to get fsdevel folks to comment since I feel like we're
guessing about exactly what guarantees we have in this area.
>
> As an aside, if we somehow can guarantee (e.g. via a name_snapshot)
> that qstr length information is valid, we might want to consider
> moving from audit_log_unstrustedstring() to
> audit_log_n_untrustedstring() to save us a call to strlen().
>
>>>> Added the LSM list as I'm beginning to wonder if we should push this
>>>> logic down into common_lsm_audit(), this problem around blocking
>>>> shouldn't be SELinux specific.
>>
>> That would require passing down the MAY_NOT_BLOCK flag or a rcu bool to
>> common_lsm_audit() just so that it could immediately return if that is
>> set and a->type is _INODE or _DENTRY (or _PATH or _FILE). And the
>> individual security module still needs to have its own handling of
>> MAY_NOT_BLOCK/rcu for its own processing, so it won't free the security
>> module authors from thinking about it.
>
> Looking at the current SELinux code, all we do is bail out early with
> -ECHILD. If we didn't have that check it looks like the only impact
> would be some extra assignments into a struct living on the stack and
> a call into common_lsm_audit(). That doesn't seem terrible for a slow
> path, especially if it pushes this code into a LSM common function.
Not terrible, just not sure if it ends up being a worthwhile change. If
the LSM maintainers would like it that way, I can do that.
>
>>>> For the LSM folks just joining, the full patchset can be found here:
>>>> * https://lore.kernel.org/selinux/20191121145245.8637-1-sds@tycho.nsa.gov/T/#t
>
^ permalink raw reply
* Re: [RFC PATCH 2/2] selinux: fall back to ref-walk upon LSM_AUDIT_DATA_DENTRY too
From: Paul Moore @ 2019-11-22 14:49 UTC (permalink / raw)
To: Stephen Smalley
Cc: selinux, will, viro, neilb, linux-fsdevel, linux-security-module
In-Reply-To: <9d825be2-c3ae-f4ad-9f82-adce7e2059d7@tycho.nsa.gov>
On Fri, Nov 22, 2019 at 8:37 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 11/21/19 7:30 PM, Paul Moore wrote:
> > On Thu, Nov 21, 2019 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
> >> On Thu, Nov 21, 2019 at 9:52 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>> commit bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
> >>> passed down the rcu flag to the SELinux AVC, but failed to adjust the
> >>> test in slow_avc_audit() to also return -ECHILD on LSM_AUDIT_DATA_DENTRY.
> >>> Previously, we only returned -ECHILD if generating an audit record with
> >>> LSM_AUDIT_DATA_INODE since this was only relevant from inode_permission.
> >>> Return -ECHILD on either LSM_AUDIT_DATA_INODE or LSM_AUDIT_DATA_DENTRY.
> >>> LSM_AUDIT_DATA_INODE only requires this handling due to the fact
> >>> that dump_common_audit_data() calls d_find_alias() and collects the
> >>> dname from the result if any.
> >>> Other cases that might require similar treatment in the future are
> >>> LSM_AUDIT_DATA_PATH and LSM_AUDIT_DATA_FILE if any hook that takes
> >>> a path or file is called under RCU-walk.
> >>>
> >>> Fixes: bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
> >>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> >>> ---
> >>> security/selinux/avc.c | 3 ++-
> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> >>> index 74c43ebe34bb..f1fa1072230c 100644
> >>> --- a/security/selinux/avc.c
> >>> +++ b/security/selinux/avc.c
> >>> @@ -779,7 +779,8 @@ noinline int slow_avc_audit(struct selinux_state *state,
> >>> * during retry. However this is logically just as if the operation
> >>> * happened a little later.
> >>> */
> >>> - if ((a->type == LSM_AUDIT_DATA_INODE) &&
> >>> + if ((a->type == LSM_AUDIT_DATA_INODE ||
> >>> + a->type == LSM_AUDIT_DATA_DENTRY) &&
> >>> (flags & MAY_NOT_BLOCK))
> >>> return -ECHILD;
> >
> > With LSM_AUDIT_DATA_INODE we eventually end up calling d_find_alias()
> > in dump_common_audit_data() which could block, which is bad, that I
> > understand. However, looking at LSM_AUDIT_DATA_DENTRY I'm less clear
> > on why that is bad? It makes a few audit_log*() calls and one call to
> > d_backing_inode() which is non-blocking and trivial.
> >
> > What am I missing?
>
> For those who haven't, you may wish to also read the earlier thread here
> that led to this one:
> https://lore.kernel.org/selinux/20191119184057.14961-1-will@kernel.org/T/#t
>
> AFAIK, neither the LSM_AUDIT_DATA_INODE nor the LSM_AUDIT_DATA_DENTRY
> case truly block (d_find_alias does not block AFAICT, nor should
> audit_log* as long as we use audit_log_start with GFP_ATOMIC or
> GFP_NOWAIT).
Yes, the audit_log*() functions should be safe, if not I would
consider that a bug; I thought d_find_alias() might block, but it's
very likely I'm wrong in that regard.
> My impression from the comment in slow_avc_audit() is that
> the issue is not really about blocking but rather about the inability to
> safely dereference the dentry->d_name during RCU walk, which is
> something that can occur under LSM_AUDIT_DATA_INODE or _DENTRY (or _PATH
> or _FILE, but neither of the latter two are currently used from the two
> hooks that are called during RCU walk, inode_permission and
> inode_follow_link). Originally _PATH, _DENTRY, and _INODE were all
> under a single _FS type and the original test in slow_avc_audit() was
> against LSM_AUDIT_DATA_FS before the split.
Thanks, I think that is the part I was missing. I was focused too
much on the VFS stuff that I didn't pay enough attention to
slow_avc_audit().
If that is the case, the comment and code in dentry_cmp() would seem
to indicate that it would be safe to fetch the dentry name string as
long as we use READ_ONCE(). The length field in the qstr might be
off, but the audit_log_untrustedstring() function doesn't use the
qstr's length information. I suppose if we don't mind the extra
spinlock we could use take_dentry_name_snapshot(); that should be safe
and we are already in the "slow" path. I didn't check the _PATH or
_FILE cases.
Once again, let me know if I'm missing something.
As an aside, if we somehow can guarantee (e.g. via a name_snapshot)
that qstr length information is valid, we might want to consider
moving from audit_log_unstrustedstring() to
audit_log_n_untrustedstring() to save us a call to strlen().
> >> Added the LSM list as I'm beginning to wonder if we should push this
> >> logic down into common_lsm_audit(), this problem around blocking
> >> shouldn't be SELinux specific.
>
> That would require passing down the MAY_NOT_BLOCK flag or a rcu bool to
> common_lsm_audit() just so that it could immediately return if that is
> set and a->type is _INODE or _DENTRY (or _PATH or _FILE). And the
> individual security module still needs to have its own handling of
> MAY_NOT_BLOCK/rcu for its own processing, so it won't free the security
> module authors from thinking about it.
Looking at the current SELinux code, all we do is bail out early with
-ECHILD. If we didn't have that check it looks like the only impact
would be some extra assignments into a struct living on the stack and
a call into common_lsm_audit(). That doesn't seem terrible for a slow
path, especially if it pushes this code into a LSM common function.
> >> For the LSM folks just joining, the full patchset can be found here:
> >> * https://lore.kernel.org/selinux/20191121145245.8637-1-sds@tycho.nsa.gov/T/#t
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH 2/2] selinux: fall back to ref-walk upon LSM_AUDIT_DATA_DENTRY too
From: Stephen Smalley @ 2019-11-22 13:50 UTC (permalink / raw)
To: Paul Moore
Cc: selinux, will, viro, neilb, linux-fsdevel, linux-security-module
In-Reply-To: <9d825be2-c3ae-f4ad-9f82-adce7e2059d7@tycho.nsa.gov>
On 11/22/19 8:37 AM, Stephen Smalley wrote:
> On 11/21/19 7:30 PM, Paul Moore wrote:
>> On Thu, Nov 21, 2019 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
>>> On Thu, Nov 21, 2019 at 9:52 AM Stephen Smalley <sds@tycho.nsa.gov>
>>> wrote:
>>>> commit bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
>>>> passed down the rcu flag to the SELinux AVC, but failed to adjust the
>>>> test in slow_avc_audit() to also return -ECHILD on
>>>> LSM_AUDIT_DATA_DENTRY.
>>>> Previously, we only returned -ECHILD if generating an audit record with
>>>> LSM_AUDIT_DATA_INODE since this was only relevant from
>>>> inode_permission.
>>>> Return -ECHILD on either LSM_AUDIT_DATA_INODE or LSM_AUDIT_DATA_DENTRY.
>>>> LSM_AUDIT_DATA_INODE only requires this handling due to the fact
>>>> that dump_common_audit_data() calls d_find_alias() and collects the
>>>> dname from the result if any.
>>>> Other cases that might require similar treatment in the future are
>>>> LSM_AUDIT_DATA_PATH and LSM_AUDIT_DATA_FILE if any hook that takes
>>>> a path or file is called under RCU-walk.
>>>>
>>>> Fixes: bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
>>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>>>> ---
>>>> security/selinux/avc.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
>>>> index 74c43ebe34bb..f1fa1072230c 100644
>>>> --- a/security/selinux/avc.c
>>>> +++ b/security/selinux/avc.c
>>>> @@ -779,7 +779,8 @@ noinline int slow_avc_audit(struct selinux_state
>>>> *state,
>>>> * during retry. However this is logically just as if the
>>>> operation
>>>> * happened a little later.
>>>> */
>>>> - if ((a->type == LSM_AUDIT_DATA_INODE) &&
>>>> + if ((a->type == LSM_AUDIT_DATA_INODE ||
>>>> + a->type == LSM_AUDIT_DATA_DENTRY) &&
>>>> (flags & MAY_NOT_BLOCK))
>>>> return -ECHILD;
>>
>> With LSM_AUDIT_DATA_INODE we eventually end up calling d_find_alias()
>> in dump_common_audit_data() which could block, which is bad, that I
>> understand. However, looking at LSM_AUDIT_DATA_DENTRY I'm less clear
>> on why that is bad? It makes a few audit_log*() calls and one call to
>> d_backing_inode() which is non-blocking and trivial.
>>
>> What am I missing?
>
> For those who haven't, you may wish to also read the earlier thread here
> that led to this one:
> https://lore.kernel.org/selinux/20191119184057.14961-1-will@kernel.org/T/#t
>
> AFAIK, neither the LSM_AUDIT_DATA_INODE nor the LSM_AUDIT_DATA_DENTRY
> case truly block (d_find_alias does not block AFAICT, nor should
> audit_log* as long as we use audit_log_start with GFP_ATOMIC or
> GFP_NOWAIT). My impression from the comment in slow_avc_audit() is that
> the issue is not really about blocking but rather about the inability to
> safely dereference the dentry->d_name during RCU walk, which is
> something that can occur under LSM_AUDIT_DATA_INODE or _DENTRY (or _PATH
> or _FILE, but neither of the latter two are currently used from the two
> hooks that are called during RCU walk, inode_permission and
> inode_follow_link). Originally _PATH, _DENTRY, and _INODE were all
> under a single _FS type and the original test in slow_avc_audit() was
> against LSM_AUDIT_DATA_FS before the split.
>
>>
>>> Added the LSM list as I'm beginning to wonder if we should push this
>>> logic down into common_lsm_audit(), this problem around blocking
>>> shouldn't be SELinux specific.
>
> That would require passing down the MAY_NOT_BLOCK flag or a rcu bool to
> common_lsm_audit() just so that it could immediately return if that is
> set and a->type is _INODE or _DENTRY (or _PATH or _FILE). And the
> individual security module still needs to have its own handling of
> MAY_NOT_BLOCK/rcu for its own processing, so it won't free the security
> module authors from thinking about it. This is only relevant for
> modules implementing the inode_permission and/or inode_follow_link
> hooks, so it only currently affects SELinux and Smack, and Smack only
> presently implements inode_permission and always returns -ECHILD if
> MAY_NOT_BLOCK (aside from a couple trivial cases), so it will never
> reach common_lsm_audit() in that case.
This would also require changing common_lsm_audit() to be able to return
errors so that it can return -ECHILD and updating all callers to handle
that.
>
>
>>>
>>> For the LSM folks just joining, the full patchset can be found here:
>>> *
>>> https://lore.kernel.org/selinux/20191121145245.8637-1-sds@tycho.nsa.gov/T/#t
>>>
^ permalink raw reply
* Re: [RFC PATCH 2/2] selinux: fall back to ref-walk upon LSM_AUDIT_DATA_DENTRY too
From: Stephen Smalley @ 2019-11-22 13:37 UTC (permalink / raw)
To: Paul Moore
Cc: selinux, will, viro, neilb, linux-fsdevel, linux-security-module
In-Reply-To: <CAHC9VhRURZMtEDagtSKEuuOLEJen=4PQZig3iGNomzXC1HTNSA@mail.gmail.com>
On 11/21/19 7:30 PM, Paul Moore wrote:
> On Thu, Nov 21, 2019 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
>> On Thu, Nov 21, 2019 at 9:52 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>> commit bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
>>> passed down the rcu flag to the SELinux AVC, but failed to adjust the
>>> test in slow_avc_audit() to also return -ECHILD on LSM_AUDIT_DATA_DENTRY.
>>> Previously, we only returned -ECHILD if generating an audit record with
>>> LSM_AUDIT_DATA_INODE since this was only relevant from inode_permission.
>>> Return -ECHILD on either LSM_AUDIT_DATA_INODE or LSM_AUDIT_DATA_DENTRY.
>>> LSM_AUDIT_DATA_INODE only requires this handling due to the fact
>>> that dump_common_audit_data() calls d_find_alias() and collects the
>>> dname from the result if any.
>>> Other cases that might require similar treatment in the future are
>>> LSM_AUDIT_DATA_PATH and LSM_AUDIT_DATA_FILE if any hook that takes
>>> a path or file is called under RCU-walk.
>>>
>>> Fixes: bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>>> ---
>>> security/selinux/avc.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
>>> index 74c43ebe34bb..f1fa1072230c 100644
>>> --- a/security/selinux/avc.c
>>> +++ b/security/selinux/avc.c
>>> @@ -779,7 +779,8 @@ noinline int slow_avc_audit(struct selinux_state *state,
>>> * during retry. However this is logically just as if the operation
>>> * happened a little later.
>>> */
>>> - if ((a->type == LSM_AUDIT_DATA_INODE) &&
>>> + if ((a->type == LSM_AUDIT_DATA_INODE ||
>>> + a->type == LSM_AUDIT_DATA_DENTRY) &&
>>> (flags & MAY_NOT_BLOCK))
>>> return -ECHILD;
>
> With LSM_AUDIT_DATA_INODE we eventually end up calling d_find_alias()
> in dump_common_audit_data() which could block, which is bad, that I
> understand. However, looking at LSM_AUDIT_DATA_DENTRY I'm less clear
> on why that is bad? It makes a few audit_log*() calls and one call to
> d_backing_inode() which is non-blocking and trivial.
>
> What am I missing?
For those who haven't, you may wish to also read the earlier thread here
that led to this one:
https://lore.kernel.org/selinux/20191119184057.14961-1-will@kernel.org/T/#t
AFAIK, neither the LSM_AUDIT_DATA_INODE nor the LSM_AUDIT_DATA_DENTRY
case truly block (d_find_alias does not block AFAICT, nor should
audit_log* as long as we use audit_log_start with GFP_ATOMIC or
GFP_NOWAIT). My impression from the comment in slow_avc_audit() is that
the issue is not really about blocking but rather about the inability to
safely dereference the dentry->d_name during RCU walk, which is
something that can occur under LSM_AUDIT_DATA_INODE or _DENTRY (or _PATH
or _FILE, but neither of the latter two are currently used from the two
hooks that are called during RCU walk, inode_permission and
inode_follow_link). Originally _PATH, _DENTRY, and _INODE were all
under a single _FS type and the original test in slow_avc_audit() was
against LSM_AUDIT_DATA_FS before the split.
>
>> Added the LSM list as I'm beginning to wonder if we should push this
>> logic down into common_lsm_audit(), this problem around blocking
>> shouldn't be SELinux specific.
That would require passing down the MAY_NOT_BLOCK flag or a rcu bool to
common_lsm_audit() just so that it could immediately return if that is
set and a->type is _INODE or _DENTRY (or _PATH or _FILE). And the
individual security module still needs to have its own handling of
MAY_NOT_BLOCK/rcu for its own processing, so it won't free the security
module authors from thinking about it. This is only relevant for
modules implementing the inode_permission and/or inode_follow_link
hooks, so it only currently affects SELinux and Smack, and Smack only
presently implements inode_permission and always returns -ECHILD if
MAY_NOT_BLOCK (aside from a couple trivial cases), so it will never
reach common_lsm_audit() in that case.
>>
>> For the LSM folks just joining, the full patchset can be found here:
>> * https://lore.kernel.org/selinux/20191121145245.8637-1-sds@tycho.nsa.gov/T/#t
^ permalink raw reply
* Re: general protection fault in __schedule (2)
From: syzbot @ 2019-11-22 7:19 UTC (permalink / raw)
To: casey, frederic, gregkh, hpa, jmattson, jmorris, karahmed,
kstewart, kvm, linux-kernel, linux-security-module, mingo, mingo,
pasha.tatashin, pbonzini, pombredanne, rkrcmar, serge,
syzkaller-bugs, tglx, x86
In-Reply-To: <000000000000e67a05057314ddf6@google.com>
syzbot has bisected this bug to:
commit 8fcc4b5923af5de58b80b53a069453b135693304
Author: Jim Mattson <jmattson@google.com>
Date: Tue Jul 10 09:27:20 2018 +0000
kvm: nVMX: Introduce KVM_CAP_NESTED_STATE
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=124cdbace00000
start commit: 234b69e3 ocfs2: fix ocfs2 read block panic
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x=114cdbace00000
console output: https://syzkaller.appspot.com/x/log.txt?x=164cdbace00000
kernel config: https://syzkaller.appspot.com/x/.config?x=5fa12be50bca08d8
dashboard link: https://syzkaller.appspot.com/bug?extid=7e2ab84953e4084a638d
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=150f0a4e400000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17f67111400000
Reported-by: syzbot+7e2ab84953e4084a638d@syzkaller.appspotmail.com
Fixes: 8fcc4b5923af ("kvm: nVMX: Introduce KVM_CAP_NESTED_STATE")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* [PATCH AUTOSEL 4.14 120/127] apparmor: delete the dentry in aafs_remove() to avoid a leak
From: Sasha Levin @ 2019-11-22 5:55 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Chris Coulson, John Johansen, Sasha Levin, linux-security-module
In-Reply-To: <20191122055544.3299-1-sashal@kernel.org>
From: Chris Coulson <chris.coulson@canonical.com>
[ Upstream commit 201218e4d3dfa1346e30997f48725acce3f26d01 ]
Although the apparmorfs dentries are always dropped from the dentry cache
when the usage count drops to zero, there is no guarantee that this will
happen in aafs_remove(), as another thread might still be using it. In
this scenario, this means that the dentry will temporarily continue to
appear in the results of lookups, even after the call to aafs_remove().
In the case of removal of a profile - it also causes simple_rmdir()
on the profile directory to fail, as the directory won't be empty until
the usage counts of all child dentries have decreased to zero. This
results in the dentry for the profile directory leaking and appearing
empty in the file system tree forever.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/apparmorfs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index dd746bd69a9b2..c106988c1b254 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -363,6 +363,7 @@ static void aafs_remove(struct dentry *dentry)
simple_rmdir(dir, dentry);
else
simple_unlink(dir, dentry);
+ d_delete(dentry);
dput(dentry);
}
inode_unlock(dir);
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 205/219] apparmor: delete the dentry in aafs_remove() to avoid a leak
From: Sasha Levin @ 2019-11-22 5:48 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Chris Coulson, John Johansen, Sasha Levin, linux-security-module
In-Reply-To: <20191122054911.1750-1-sashal@kernel.org>
From: Chris Coulson <chris.coulson@canonical.com>
[ Upstream commit 201218e4d3dfa1346e30997f48725acce3f26d01 ]
Although the apparmorfs dentries are always dropped from the dentry cache
when the usage count drops to zero, there is no guarantee that this will
happen in aafs_remove(), as another thread might still be using it. In
this scenario, this means that the dentry will temporarily continue to
appear in the results of lookups, even after the call to aafs_remove().
In the case of removal of a profile - it also causes simple_rmdir()
on the profile directory to fail, as the directory won't be empty until
the usage counts of all child dentries have decreased to zero. This
results in the dentry for the profile directory leaking and appearing
empty in the file system tree forever.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/apparmorfs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 40e3a098f6fb5..d95a7e41a29d4 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -361,6 +361,7 @@ static void aafs_remove(struct dentry *dentry)
simple_rmdir(dir, dentry);
else
simple_unlink(dir, dentry);
+ d_delete(dentry);
dput(dentry);
}
inode_unlock(dir);
--
2.20.1
^ permalink raw reply related
* Re: [RFC PATCH 2/2] selinux: fall back to ref-walk upon LSM_AUDIT_DATA_DENTRY too
From: Paul Moore @ 2019-11-22 0:30 UTC (permalink / raw)
To: Stephen Smalley
Cc: selinux, will, viro, neilb, linux-fsdevel, linux-security-module
In-Reply-To: <CAHC9VhTAq7CgcRRcvZCYis7ELAo+bo2q8pCUXfHUP9YAcUhwsQ@mail.gmail.com>
On Thu, Nov 21, 2019 at 7:12 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Nov 21, 2019 at 9:52 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > commit bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
> > passed down the rcu flag to the SELinux AVC, but failed to adjust the
> > test in slow_avc_audit() to also return -ECHILD on LSM_AUDIT_DATA_DENTRY.
> > Previously, we only returned -ECHILD if generating an audit record with
> > LSM_AUDIT_DATA_INODE since this was only relevant from inode_permission.
> > Return -ECHILD on either LSM_AUDIT_DATA_INODE or LSM_AUDIT_DATA_DENTRY.
> > LSM_AUDIT_DATA_INODE only requires this handling due to the fact
> > that dump_common_audit_data() calls d_find_alias() and collects the
> > dname from the result if any.
> > Other cases that might require similar treatment in the future are
> > LSM_AUDIT_DATA_PATH and LSM_AUDIT_DATA_FILE if any hook that takes
> > a path or file is called under RCU-walk.
> >
> > Fixes: bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
> > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> > ---
> > security/selinux/avc.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > index 74c43ebe34bb..f1fa1072230c 100644
> > --- a/security/selinux/avc.c
> > +++ b/security/selinux/avc.c
> > @@ -779,7 +779,8 @@ noinline int slow_avc_audit(struct selinux_state *state,
> > * during retry. However this is logically just as if the operation
> > * happened a little later.
> > */
> > - if ((a->type == LSM_AUDIT_DATA_INODE) &&
> > + if ((a->type == LSM_AUDIT_DATA_INODE ||
> > + a->type == LSM_AUDIT_DATA_DENTRY) &&
> > (flags & MAY_NOT_BLOCK))
> > return -ECHILD;
With LSM_AUDIT_DATA_INODE we eventually end up calling d_find_alias()
in dump_common_audit_data() which could block, which is bad, that I
understand. However, looking at LSM_AUDIT_DATA_DENTRY I'm less clear
on why that is bad? It makes a few audit_log*() calls and one call to
d_backing_inode() which is non-blocking and trivial.
What am I missing?
> Added the LSM list as I'm beginning to wonder if we should push this
> logic down into common_lsm_audit(), this problem around blocking
> shouldn't be SELinux specific.
>
> For the LSM folks just joining, the full patchset can be found here:
> * https://lore.kernel.org/selinux/20191121145245.8637-1-sds@tycho.nsa.gov/T/#t
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH 2/2] selinux: fall back to ref-walk upon LSM_AUDIT_DATA_DENTRY too
From: Paul Moore @ 2019-11-22 0:12 UTC (permalink / raw)
To: Stephen Smalley
Cc: selinux, will, viro, neilb, linux-fsdevel, linux-security-module
In-Reply-To: <20191121145245.8637-2-sds@tycho.nsa.gov>
On Thu, Nov 21, 2019 at 9:52 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> commit bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
> passed down the rcu flag to the SELinux AVC, but failed to adjust the
> test in slow_avc_audit() to also return -ECHILD on LSM_AUDIT_DATA_DENTRY.
> Previously, we only returned -ECHILD if generating an audit record with
> LSM_AUDIT_DATA_INODE since this was only relevant from inode_permission.
> Return -ECHILD on either LSM_AUDIT_DATA_INODE or LSM_AUDIT_DATA_DENTRY.
> LSM_AUDIT_DATA_INODE only requires this handling due to the fact
> that dump_common_audit_data() calls d_find_alias() and collects the
> dname from the result if any.
> Other cases that might require similar treatment in the future are
> LSM_AUDIT_DATA_PATH and LSM_AUDIT_DATA_FILE if any hook that takes
> a path or file is called under RCU-walk.
>
> Fixes: bda0be7ad994 ("security: make inode_follow_link RCU-walk aware")
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/avc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 74c43ebe34bb..f1fa1072230c 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -779,7 +779,8 @@ noinline int slow_avc_audit(struct selinux_state *state,
> * during retry. However this is logically just as if the operation
> * happened a little later.
> */
> - if ((a->type == LSM_AUDIT_DATA_INODE) &&
> + if ((a->type == LSM_AUDIT_DATA_INODE ||
> + a->type == LSM_AUDIT_DATA_DENTRY) &&
> (flags & MAY_NOT_BLOCK))
> return -ECHILD;
Added the LSM list as I'm beginning to wonder if we should push this
logic down into common_lsm_audit(), this problem around blocking
shouldn't be SELinux specific.
For the LSM folks just joining, the full patchset can be found here:
* https://lore.kernel.org/selinux/20191121145245.8637-1-sds@tycho.nsa.gov/T/#t
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v11 18/25] LSM: security_secid_to_secctx in netlink netfilter
From: James Morris @ 2019-11-21 20:36 UTC (permalink / raw)
To: John Johansen
Cc: Casey Schaufler, casey.schaufler, linux-security-module, selinux,
keescook, penguin-kernel, paul, sds
In-Reply-To: <d2b3065d-c8b7-4d77-c447-5ae81ba9b27c@canonical.com>
On Thu, 21 Nov 2019, John Johansen wrote:
> On 11/13/19 10:24 AM, Casey Schaufler wrote:
> > Change netlink netfilter interfaces to use lsmcontext
> > pointers, and remove scaffolding.
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Reviewed-by: John Johansen <john.johansen@canonical.com>
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > cc: netdev@vger.kernel.org
>
> Acked-by: John Johansen <john.johansen@canonical.com>
Just to clarify, you don't need to ack if you've already added a
reviewed-by, I just want to make sure all of the patches have some kind of
signoff.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH v11 25/25] AppArmor: Remove the exclusive flag
From: John Johansen @ 2019-11-21 19:03 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20191113182506.2580-9-casey@schaufler-ca.com>
On 11/13/19 10:25 AM, Casey Schaufler wrote:
> With the inclusion of the "display" process attribute
> mechanism AppArmor no longer needs to be treated as an
> "exclusive" security module. Remove the flag that indicates
> it is exclusive. Remove the stub getpeersec_dgram AppArmor
> hook as it has no effect in the single LSM case and
> interferes in the multiple LSM case.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> security/apparmor/lsm.c | 20 +-------------------
> 1 file changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index b4c964fdc2f9..81bc4f773429 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1099,22 +1099,6 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock, char **optval,
> return error;
> }
>
> -/**
> - * apparmor_socket_getpeersec_dgram - get security label of packet
> - * @sock: the peer socket
> - * @skb: packet data
> - * @secid: pointer to where to put the secid of the packet
> - *
> - * Sets the netlabel socket state on sk from parent
> - */
> -static int apparmor_socket_getpeersec_dgram(struct socket *sock,
> - struct sk_buff *skb, u32 *secid)
> -
> -{
> - /* TODO: requires secid support */
> - return -ENOPROTOOPT;
> -}
> -
> /**
> * apparmor_sock_graft - Initialize newly created socket
> * @sk: child sock
> @@ -1218,8 +1202,6 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> #endif
> LSM_HOOK_INIT(socket_getpeersec_stream,
> apparmor_socket_getpeersec_stream),
> - LSM_HOOK_INIT(socket_getpeersec_dgram,
> - apparmor_socket_getpeersec_dgram),
> LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
> #ifdef CONFIG_NETWORK_SECMARK
> LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
> @@ -1777,7 +1759,7 @@ static int __init apparmor_init(void)
>
> DEFINE_LSM(apparmor) = {
> .name = "apparmor",
> - .flags = LSM_FLAG_LEGACY_MAJOR | LSM_FLAG_EXCLUSIVE,
> + .flags = LSM_FLAG_LEGACY_MAJOR,
> .enabled = &apparmor_enabled,
> .blobs = &apparmor_blob_sizes,
> .init = apparmor_init,
>
^ permalink raw reply
* Re: [PATCH v11 19/25] NET: Store LSM netlabel data in a lsmblob
From: John Johansen @ 2019-11-21 19:02 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20191113182506.2580-3-casey@schaufler-ca.com>
On 11/13/19 10:25 AM, Casey Schaufler wrote:
> Netlabel uses LSM interfaces requiring an lsmblob and
> the internal storage is used to pass information between
> these interfaces, so change the internal data from a secid
> to a lsmblob. Update the netlabel interfaces and their
> callers to accommodate the change. This requires that the
> modules using netlabel use the lsm_id.slot to access the
> correct secid when using netlabel.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.com
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> include/net/netlabel.h | 8 ++--
> net/ipv4/cipso_ipv4.c | 6 ++-
> net/netlabel/netlabel_kapi.c | 6 +--
> net/netlabel/netlabel_unlabeled.c | 57 +++++++++++------------------
> net/netlabel/netlabel_unlabeled.h | 2 +-
> security/selinux/hooks.c | 2 +-
> security/selinux/include/security.h | 1 +
> security/selinux/netlabel.c | 2 +-
> security/selinux/ss/services.c | 4 +-
> security/smack/smack.h | 1 +
> security/smack/smack_lsm.c | 5 ++-
> security/smack/smackfs.c | 10 +++--
> 12 files changed, 50 insertions(+), 54 deletions(-)
>
> diff --git a/include/net/netlabel.h b/include/net/netlabel.h
> index 43ae50337685..73fc25b4042b 100644
> --- a/include/net/netlabel.h
> +++ b/include/net/netlabel.h
> @@ -166,7 +166,7 @@ struct netlbl_lsm_catmap {
> * @attr.mls: MLS sensitivity label
> * @attr.mls.cat: MLS category bitmap
> * @attr.mls.lvl: MLS sensitivity level
> - * @attr.secid: LSM specific secid token
> + * @attr.lsmblob: LSM specific data
> *
> * Description:
> * This structure is used to pass security attributes between NetLabel and the
> @@ -201,7 +201,7 @@ struct netlbl_lsm_secattr {
> struct netlbl_lsm_catmap *cat;
> u32 lvl;
> } mls;
> - u32 secid;
> + struct lsmblob lsmblob;
> } attr;
> };
>
> @@ -415,7 +415,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info);
> int netlbl_cfg_unlbl_static_del(struct net *net,
> const char *dev_name,
> @@ -523,7 +523,7 @@ static inline int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> return -ENOSYS;
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 376882215919..8ee7a804423e 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1467,7 +1467,8 @@ static int cipso_v4_gentag_loc(const struct cipso_v4_doi *doi_def,
>
> buffer[0] = CIPSO_V4_TAG_LOCAL;
> buffer[1] = CIPSO_V4_TAG_LOC_BLEN;
> - *(u32 *)&buffer[2] = secattr->attr.secid;
> + /* only one netlabel user - the first */
> + *(u32 *)&buffer[2] = secattr->attr.lsmblob.secid[0];
>
> return CIPSO_V4_TAG_LOC_BLEN;
> }
> @@ -1487,7 +1488,8 @@ static int cipso_v4_parsetag_loc(const struct cipso_v4_doi *doi_def,
> const unsigned char *tag,
> struct netlbl_lsm_secattr *secattr)
> {
> - secattr->attr.secid = *(u32 *)&tag[2];
> + /* only one netlabel user - the first */
> + secattr->attr.lsmblob.secid[0] = *(u32 *)&tag[2];
> secattr->flags |= NETLBL_SECATTR_SECID;
>
> return 0;
> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
> index 409a3ae47ce2..f2ebd43a7992 100644
> --- a/net/netlabel/netlabel_kapi.c
> +++ b/net/netlabel/netlabel_kapi.c
> @@ -196,7 +196,7 @@ int netlbl_cfg_unlbl_map_add(const char *domain,
> * @addr: IP address in network byte order (struct in[6]_addr)
> * @mask: address mask in network byte order (struct in[6]_addr)
> * @family: address family
> - * @secid: LSM secid value for the entry
> + * @lsmblob: LSM data value for the entry
> * @audit_info: NetLabel audit information
> *
> * Description:
> @@ -210,7 +210,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> u32 addr_len;
> @@ -230,7 +230,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
>
> return netlbl_unlhsh_add(net,
> dev_name, addr, mask, addr_len,
> - secid, audit_info);
> + lsmblob, audit_info);
> }
>
> /**
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index c03fe9a4f7b9..3b0f07b59436 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -66,7 +66,7 @@ struct netlbl_unlhsh_tbl {
> #define netlbl_unlhsh_addr4_entry(iter) \
> container_of(iter, struct netlbl_unlhsh_addr4, list)
> struct netlbl_unlhsh_addr4 {
> - u32 secid;
> + struct lsmblob lsmblob;
>
> struct netlbl_af4list list;
> struct rcu_head rcu;
> @@ -74,7 +74,7 @@ struct netlbl_unlhsh_addr4 {
> #define netlbl_unlhsh_addr6_entry(iter) \
> container_of(iter, struct netlbl_unlhsh_addr6, list)
> struct netlbl_unlhsh_addr6 {
> - u32 secid;
> + struct lsmblob lsmblob;
>
> struct netlbl_af6list list;
> struct rcu_head rcu;
> @@ -219,7 +219,7 @@ static struct netlbl_unlhsh_iface *netlbl_unlhsh_search_iface(int ifindex)
> * @iface: the associated interface entry
> * @addr: IPv4 address in network byte order
> * @mask: IPv4 address mask in network byte order
> - * @secid: LSM secid value for entry
> + * @lsmblob: LSM data value for entry
> *
> * Description:
> * Add a new address entry into the unlabeled connection hash table using the
> @@ -230,7 +230,7 @@ static struct netlbl_unlhsh_iface *netlbl_unlhsh_search_iface(int ifindex)
> static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> const struct in_addr *addr,
> const struct in_addr *mask,
> - u32 secid)
> + struct lsmblob *lsmblob)
> {
> int ret_val;
> struct netlbl_unlhsh_addr4 *entry;
> @@ -242,7 +242,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> entry->list.addr = addr->s_addr & mask->s_addr;
> entry->list.mask = mask->s_addr;
> entry->list.valid = 1;
> - entry->secid = secid;
> + entry->lsmblob = *lsmblob;
>
> spin_lock(&netlbl_unlhsh_lock);
> ret_val = netlbl_af4list_add(&entry->list, &iface->addr4_list);
> @@ -259,7 +259,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> * @iface: the associated interface entry
> * @addr: IPv6 address in network byte order
> * @mask: IPv6 address mask in network byte order
> - * @secid: LSM secid value for entry
> + * @lsmblob: LSM data value for entry
> *
> * Description:
> * Add a new address entry into the unlabeled connection hash table using the
> @@ -270,7 +270,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> static int netlbl_unlhsh_add_addr6(struct netlbl_unlhsh_iface *iface,
> const struct in6_addr *addr,
> const struct in6_addr *mask,
> - u32 secid)
> + struct lsmblob *lsmblob)
> {
> int ret_val;
> struct netlbl_unlhsh_addr6 *entry;
> @@ -286,7 +286,7 @@ static int netlbl_unlhsh_add_addr6(struct netlbl_unlhsh_iface *iface,
> entry->list.addr.s6_addr32[3] &= mask->s6_addr32[3];
> entry->list.mask = *mask;
> entry->list.valid = 1;
> - entry->secid = secid;
> + entry->lsmblob = *lsmblob;
>
> spin_lock(&netlbl_unlhsh_lock);
> ret_val = netlbl_af6list_add(&entry->list, &iface->addr6_list);
> @@ -365,7 +365,7 @@ int netlbl_unlhsh_add(struct net *net,
> const void *addr,
> const void *mask,
> u32 addr_len,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> int ret_val;
> @@ -374,7 +374,6 @@ int netlbl_unlhsh_add(struct net *net,
> struct netlbl_unlhsh_iface *iface;
> struct audit_buffer *audit_buf = NULL;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> if (addr_len != sizeof(struct in_addr) &&
> addr_len != sizeof(struct in6_addr))
> @@ -407,7 +406,7 @@ int netlbl_unlhsh_add(struct net *net,
> const struct in_addr *addr4 = addr;
> const struct in_addr *mask4 = mask;
>
> - ret_val = netlbl_unlhsh_add_addr4(iface, addr4, mask4, secid);
> + ret_val = netlbl_unlhsh_add_addr4(iface, addr4, mask4, lsmblob);
> if (audit_buf != NULL)
> netlbl_af4list_audit_addr(audit_buf, 1,
> dev_name,
> @@ -420,7 +419,7 @@ int netlbl_unlhsh_add(struct net *net,
> const struct in6_addr *addr6 = addr;
> const struct in6_addr *mask6 = mask;
>
> - ret_val = netlbl_unlhsh_add_addr6(iface, addr6, mask6, secid);
> + ret_val = netlbl_unlhsh_add_addr6(iface, addr6, mask6, lsmblob);
> if (audit_buf != NULL)
> netlbl_af6list_audit_addr(audit_buf, 1,
> dev_name,
> @@ -437,8 +436,7 @@ int netlbl_unlhsh_add(struct net *net,
> unlhsh_add_return:
> rcu_read_unlock();
> if (audit_buf != NULL) {
> - lsmblob_init(&blob, secid);
> - if (security_secid_to_secctx(&blob, &context) == 0) {
> + if (security_secid_to_secctx(lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -473,7 +471,6 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> list_entry = netlbl_af4list_remove(addr->s_addr, mask->s_addr,
> @@ -493,10 +490,8 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> addr->s_addr, mask->s_addr);
> if (dev != NULL)
> dev_put(dev);
> - if (entry != NULL)
> - lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob, &context) == 0) {
> + security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -537,7 +532,6 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> list_entry = netlbl_af6list_remove(addr, mask, &iface->addr6_list);
> @@ -556,10 +550,8 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> addr, mask);
> if (dev != NULL)
> dev_put(dev);
> - if (entry != NULL)
> - lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob, &context) == 0) {
> + security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -913,9 +905,8 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
> if (ret_val != 0)
> return ret_val;
>
> - /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - dev_name, addr, mask, addr_len, blob.secid[0],
> + dev_name, addr, mask, addr_len, &blob,
> &audit_info);
> }
>
> @@ -963,10 +954,8 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
> if (ret_val != 0)
> return ret_val;
>
> - /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - NULL, addr, mask, addr_len, blob.secid[0],
> - &audit_info);
> + NULL, addr, mask, addr_len, &blob, &audit_info);
> }
>
> /**
> @@ -1078,8 +1067,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> struct net_device *dev;
> struct lsmcontext context;
> void *data;
> - u32 secid;
> - struct lsmblob blob;
> + struct lsmblob *lsmb;
>
> data = genlmsg_put(cb_arg->skb, NETLINK_CB(cb_arg->nl_cb->skb).portid,
> cb_arg->seq, &netlbl_unlabel_gnl_family,
> @@ -1117,7 +1105,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> if (ret_val != 0)
> goto list_cb_failure;
>
> - secid = addr4->secid;
> + lsmb = (struct lsmblob *)&addr4->lsmblob;
> } else {
> ret_val = nla_put_in6_addr(cb_arg->skb,
> NLBL_UNLABEL_A_IPV6ADDR,
> @@ -1131,11 +1119,10 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> if (ret_val != 0)
> goto list_cb_failure;
>
> - secid = addr6->secid;
> + lsmb = (struct lsmblob *)&addr6->lsmblob;
> }
>
> - lsmblob_init(&blob, secid);
> - ret_val = security_secid_to_secctx(&blob, &context);
> + ret_val = security_secid_to_secctx(lsmb, &context);
> if (ret_val != 0)
> goto list_cb_failure;
> ret_val = nla_put(cb_arg->skb,
> @@ -1487,7 +1474,7 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
> &iface->addr4_list);
> if (addr4 == NULL)
> goto unlabel_getattr_nolabel;
> - secattr->attr.secid = netlbl_unlhsh_addr4_entry(addr4)->secid;
> + secattr->attr.lsmblob = netlbl_unlhsh_addr4_entry(addr4)->lsmblob;
> break;
> }
> #if IS_ENABLED(CONFIG_IPV6)
> @@ -1500,7 +1487,7 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
> &iface->addr6_list);
> if (addr6 == NULL)
> goto unlabel_getattr_nolabel;
> - secattr->attr.secid = netlbl_unlhsh_addr6_entry(addr6)->secid;
> + secattr->attr.lsmblob = netlbl_unlhsh_addr6_entry(addr6)->lsmblob;
> break;
> }
> #endif /* IPv6 */
> diff --git a/net/netlabel/netlabel_unlabeled.h b/net/netlabel/netlabel_unlabeled.h
> index 058e3a285d56..168920780994 100644
> --- a/net/netlabel/netlabel_unlabeled.h
> +++ b/net/netlabel/netlabel_unlabeled.h
> @@ -211,7 +211,7 @@ int netlbl_unlhsh_add(struct net *net,
> const void *addr,
> const void *mask,
> u32 addr_len,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info);
> int netlbl_unlhsh_remove(struct net *net,
> const char *dev_name,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5f50dae7c107..16348270b98e 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6802,7 +6802,7 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> .lbs_sock = sizeof(struct sk_security_struct),
> };
>
> -static struct lsm_id selinux_lsmid __lsm_ro_after_init = {
> +struct lsm_id selinux_lsmid __lsm_ro_after_init = {
> .lsm = "selinux",
> .slot = LSMBLOB_NEEDED
> };
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 111121281c47..25ca805d74a2 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -69,6 +69,7 @@
> struct netlbl_lsm_secattr;
>
> extern int selinux_enabled;
> +extern struct lsm_id selinux_lsmid;
>
> /* Policy capabilities */
> enum {
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 6a94b31b5472..d8d7603ab14e 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -108,7 +108,7 @@ static struct netlbl_lsm_secattr *selinux_netlbl_sock_getattr(
> return NULL;
>
> if ((secattr->flags & NETLBL_SECATTR_SECID) &&
> - (secattr->attr.secid == sid))
> + (secattr->attr.lsmblob.secid[selinux_lsmid.slot] == sid))
> return secattr;
>
> return NULL;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index a5813c7629c1..2b7680903b6b 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3599,7 +3599,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
> if (secattr->flags & NETLBL_SECATTR_CACHE)
> *sid = *(u32 *)secattr->cache->data;
> else if (secattr->flags & NETLBL_SECATTR_SECID)
> - *sid = secattr->attr.secid;
> + *sid = secattr->attr.lsmblob.secid[selinux_lsmid.slot];
> else if (secattr->flags & NETLBL_SECATTR_MLS_LVL) {
> rc = -EIDRM;
> ctx = sidtab_search(sidtab, SECINITSID_NETMSG);
> @@ -3672,7 +3672,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
> if (secattr->domain == NULL)
> goto out;
>
> - secattr->attr.secid = sid;
> + secattr->attr.lsmblob.secid[selinux_lsmid.slot] = sid;
> secattr->flags |= NETLBL_SECATTR_DOMAIN_CPY | NETLBL_SECATTR_SECID;
> mls_export_netlbl_lvl(policydb, ctx, secattr);
> rc = mls_export_netlbl_cat(policydb, ctx, secattr);
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 2836540f9577..6e76b6b33063 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -316,6 +316,7 @@ void smk_destroy_label_list(struct list_head *list);
> * Shared data.
> */
> extern int smack_enabled;
> +extern struct lsm_id smack_lsmid;
> extern int smack_cipso_direct;
> extern int smack_cipso_mapped;
> extern struct smack_known *smack_net_ambient;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 4e464e5e942e..e23792dae35c 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3776,7 +3776,8 @@ static struct smack_known *smack_from_secattr(struct netlbl_lsm_secattr *sap,
> /*
> * Looks like a fallback, which gives us a secid.
> */
> - return smack_from_secid(sap->attr.secid);
> + return smack_from_secid(
> + sap->attr.lsmblob.secid[smack_lsmid.slot]);
> /*
> * Without guidance regarding the smack value
> * for the packet fall back on the network
> @@ -4598,7 +4599,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> .lbs_sock = sizeof(struct socket_smack),
> };
>
> -static struct lsm_id smack_lsmid __lsm_ro_after_init = {
> +struct lsm_id smack_lsmid __lsm_ro_after_init = {
> .lsm = "smack",
> .slot = LSMBLOB_NEEDED
> };
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index e3e05c04dbd1..d10e9c96717e 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -1122,6 +1122,7 @@ static void smk_net4addr_insert(struct smk_net4addr *new)
> static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> + struct lsmblob lsmblob;
> struct smk_net4addr *snp;
> struct sockaddr_in newname;
> char *smack;
> @@ -1253,10 +1254,13 @@ static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
> * this host so that incoming packets get labeled.
> * but only if we didn't get the special CIPSO option
> */
> - if (rc == 0 && skp != NULL)
> + if (rc == 0 && skp != NULL) {
> + lsmblob_init(&lsmblob, 0);
> + lsmblob.secid[smack_lsmid.slot] = snp->smk_label->smk_secid;
> rc = netlbl_cfg_unlbl_static_add(&init_net, NULL,
> - &snp->smk_host, &snp->smk_mask, PF_INET,
> - snp->smk_label->smk_secid, &audit_info);
> + &snp->smk_host, &snp->smk_mask, PF_INET, &lsmblob,
> + &audit_info);
> + }
>
> if (rc == 0)
> rc = count;
>
^ permalink raw reply
* Re: [PATCH v11 18/25] LSM: security_secid_to_secctx in netlink netfilter
From: John Johansen @ 2019-11-21 19:01 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20191113182506.2580-2-casey@schaufler-ca.com>
On 11/13/19 10:24 AM, Casey Schaufler wrote:
> Change netlink netfilter interfaces to use lsmcontext
> pointers, and remove scaffolding.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> net/netfilter/nfnetlink_queue.c | 32 +++++++++++++-------------------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 2d6668fd026c..a1296453d8f2 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -301,12 +301,10 @@ static int nfqnl_put_sk_uidgid(struct sk_buff *skb, struct sock *sk)
> return -1;
> }
>
> -static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> +static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsmcontext *context)
> {
> - u32 seclen = 0;
> #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
> struct lsmblob blob;
> - struct lsmcontext context = { };
>
> if (!skb || !sk_fullsock(skb->sk))
> return 0;
> @@ -314,15 +312,16 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> read_lock_bh(&skb->sk->sk_callback_lock);
>
> if (skb->secmark) {
> + /* Any LSM might be looking for the secmark */
> lsmblob_init(&blob, skb->secmark);
> - security_secid_to_secctx(&blob, &context);
> - *secdata = context.context;
> + security_secid_to_secctx(&blob, context);
> }
>
> read_unlock_bh(&skb->sk->sk_callback_lock);
> - seclen = context.len;
> + return context->len;
> +#else
> + return 0;
> #endif
> - return seclen;
> }
>
> static u32 nfqnl_get_bridge_size(struct nf_queue_entry *entry)
> @@ -398,8 +397,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> enum ip_conntrack_info uninitialized_var(ctinfo);
> struct nfnl_ct_hook *nfnl_ct;
> bool csum_verify;
> - struct lsmcontext scaff; /* scaffolding */
> - char *secdata = NULL;
> + struct lsmcontext context = { };
> u32 seclen = 0;
>
> size = nlmsg_total_size(sizeof(struct nfgenmsg))
> @@ -466,7 +464,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> }
>
> if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
> - seclen = nfqnl_get_sk_secctx(entskb, &secdata);
> + seclen = nfqnl_get_sk_secctx(entskb, &context);
> if (seclen)
> size += nla_total_size(seclen);
> }
> @@ -601,7 +599,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
> goto nla_put_failure;
>
> - if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
> + if (seclen && nla_put(skb, NFQA_SECCTX, context.len, context.context))
> goto nla_put_failure;
>
> if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
> @@ -629,10 +627,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> }
>
> nlh->nlmsg_len = skb->len;
> - if (seclen) {
> - lsmcontext_init(&scaff, secdata, seclen, 0);
> - security_release_secctx(&scaff);
> - }
> + if (seclen)
> + security_release_secctx(&context);
> return skb;
>
> nla_put_failure:
> @@ -640,10 +636,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> kfree_skb(skb);
> net_err_ratelimited("nf_queue: error creating packet message\n");
> nlmsg_failure:
> - if (seclen) {
> - lsmcontext_init(&scaff, secdata, seclen, 0);
> - security_release_secctx(&scaff);
> - }
> + if (seclen)
> + security_release_secctx(&context);
> return NULL;
> }
>
>
^ permalink raw reply
* Re: [PATCH v11 16/25] LSM: Use lsmcontext in security_dentry_init_security
From: John Johansen @ 2019-11-21 19:00 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20191113181925.2437-17-casey@schaufler-ca.com>
On 11/13/19 10:19 AM, Casey Schaufler wrote:
> Change the security_dentry_init_security() interface to
> fill an lsmcontext structure instead of a void * data area
> and a length. The lone caller of this interface is NFS4,
> which may make copies of the data using its own mechanisms.
> A rework of the nfs4 code to use the lsmcontext properly
> is a significant project, so the coward's way out is taken,
> and the lsmcontext data from security_dentry_init_security()
> is copied, then released directly.
>
> This interface does not use the "display". There is currently
> not case where that is useful or reasonable.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> fs/nfs/nfs4proc.c | 26 ++++++++++++++++----------
> include/linux/security.h | 7 +++----
> security/security.c | 29 +++++++++++++++++++++++++----
> 3 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 74e9f4b7cc07..2f76741ee528 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -113,6 +113,7 @@ static inline struct nfs4_label *
> nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> struct iattr *sattr, struct nfs4_label *label)
> {
> + struct lsmcontext context;
> int err;
>
> if (label == NULL)
> @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> return NULL;
>
> err = security_dentry_init_security(dentry, sattr->ia_mode,
> - &dentry->d_name, (void **)&label->label, &label->len);
> - if (err == 0)
> - return label;
> + &dentry->d_name, &context);
> +
> + if (err)
> + return NULL;
> +
> + label->label = kmemdup(context.context, context.len, GFP_KERNEL);
> + if (label->label == NULL)
> + label = NULL;
> + else
> + label->len = context.len;
> +
> + security_release_secctx(&context);
> +
> + return label;
>
> - return NULL;
> }
> static inline void
> nfs4_label_release_security(struct nfs4_label *label)
> {
> - struct lsmcontext scaff; /* scaffolding */
> -
> - if (label) {
> - lsmcontext_init(&scaff, label->label, label->len, 0);
> - security_release_secctx(&scaff);
> - }
> + kfree(label->label);
> }
> static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
> {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index e47cef3d62f0..3e333104720d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -396,8 +396,8 @@ int security_add_mnt_opt(const char *option, const char *val,
> int len, void **mnt_opts);
> int security_move_mount(const struct path *from_path, const struct path *to_path);
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen);
> + const struct qstr *name,
> + struct lsmcontext *ctx);
> int security_dentry_create_files_as(struct dentry *dentry, int mode,
> struct qstr *name,
> const struct cred *old,
> @@ -788,8 +788,7 @@ static inline void security_inode_free(struct inode *inode)
> static inline int security_dentry_init_security(struct dentry *dentry,
> int mode,
> const struct qstr *name,
> - void **ctx,
> - u32 *ctxlen)
> + struct lsmcontext *ctx)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/security/security.c b/security/security.c
> index 618d4f90936b..6f43dafe1249 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1011,12 +1011,33 @@ void security_inode_free(struct inode *inode)
> inode_free_by_rcu);
> }
>
> +/*
> + * security_dentry_init_security - initial context for a dentry
> + * @dentry: directory entry
> + * @mode: access mode
> + * @name: path name
> + * @context: resulting security context
> + *
> + * Use at most one security module to get the initial
> + * security context. Do not use the "display".
> + *
> + * Returns -EOPNOTSUPP if not supplied by any module or the module result.
> + */
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen)
> + const struct qstr *name,
> + struct lsmcontext *cp)
> {
> - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> - name, ctx, ctxlen);
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security,
> + list) {
> + cp->slot = hp->lsmid->slot;
> + return hp->hook.dentry_init_security(dentry, mode, name,
> + (void **)&cp->context,
> + &cp->len);
> + }
> +
> + return -EOPNOTSUPP;
> }
> EXPORT_SYMBOL(security_dentry_init_security);
>
>
^ permalink raw reply
* Re: [PATCH v11 14/25] LSM: Ensure the correct LSM context releaser
From: John Johansen @ 2019-11-21 19:00 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20191113181925.2437-15-casey@schaufler-ca.com>
On 11/13/19 10:19 AM, Casey Schaufler wrote:
> Add a new lsmcontext data structure to hold all the information
> about a "security context", including the string, its size and
> which LSM allocated the string. The allocation information is
> necessary because LSMs have different policies regarding the
> lifecycle of these strings. SELinux allocates and destroys
> them on each use, whereas Smack provides a pointer to an entry
> in a list that never goes away.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: linux-integrity@vger.kernel.org
> cc: netdev@vger.kernel.org
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> drivers/android/binder.c | 10 +++++--
> fs/ceph/xattr.c | 6 +++-
> fs/nfs/nfs4proc.c | 8 +++--
> fs/nfsd/nfs4xdr.c | 7 +++--
> include/linux/security.h | 39 +++++++++++++++++++++++--
> include/net/scm.h | 5 +++-
> kernel/audit.c | 14 ++++++---
> kernel/auditsc.c | 12 ++++++--
> net/ipv4/ip_sockglue.c | 4 ++-
> net/netfilter/nf_conntrack_netlink.c | 4 ++-
> net/netfilter/nf_conntrack_standalone.c | 4 ++-
> net/netfilter/nfnetlink_queue.c | 13 ++++++---
> net/netlabel/netlabel_unlabeled.c | 19 +++++++++---
> net/netlabel/netlabel_user.c | 4 ++-
> security/security.c | 18 ++++++++----
> security/smack/smack_lsm.c | 14 ++++++---
> 16 files changed, 141 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 3a7fcdc8dbe2..49b84b6fafd9 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2865,6 +2865,7 @@ static void binder_transaction(struct binder_proc *proc,
> int t_debug_id = atomic_inc_return(&binder_last_id);
> char *secctx = NULL;
> u32 secctx_sz = 0;
> + struct lsmcontext scaff; /* scaffolding */
>
> e = binder_transaction_log_add(&binder_transaction_log);
> e->debug_id = t_debug_id;
> @@ -3161,7 +3162,8 @@ static void binder_transaction(struct binder_proc *proc,
> t->security_ctx = 0;
> WARN_ON(1);
> }
> - security_release_secctx(secctx, secctx_sz);
> + lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> + security_release_secctx(&scaff);
> secctx = NULL;
> }
> t->buffer->debug_id = t->debug_id;
> @@ -3494,8 +3496,10 @@ static void binder_transaction(struct binder_proc *proc,
> binder_alloc_free_buf(&target_proc->alloc, t->buffer);
> err_binder_alloc_buf_failed:
> err_bad_extra_size:
> - if (secctx)
> - security_release_secctx(secctx, secctx_sz);
> + if (secctx) {
> + lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> + security_release_secctx(&scaff);
> + }
> err_get_secctx_failed:
> kfree(tcomplete);
> binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index cb18ee637cb7..ad501b5cad2c 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -1271,12 +1271,16 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
>
> void ceph_release_acl_sec_ctx(struct ceph_acl_sec_ctx *as_ctx)
> {
> +#ifdef CONFIG_CEPH_FS_SECURITY_LABEL
> + struct lsmcontext scaff; /* scaffolding */
> +#endif
> #ifdef CONFIG_CEPH_FS_POSIX_ACL
> posix_acl_release(as_ctx->acl);
> posix_acl_release(as_ctx->default_acl);
> #endif
> #ifdef CONFIG_CEPH_FS_SECURITY_LABEL
> - security_release_secctx(as_ctx->sec_ctx, as_ctx->sec_ctxlen);
> + lsmcontext_init(&scaff, as_ctx->sec_ctx, as_ctx->sec_ctxlen, 0);
> + security_release_secctx(&scaff);
> #endif
> if (as_ctx->pagelist)
> ceph_pagelist_release(as_ctx->pagelist);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index caacf5e7f5e1..74e9f4b7cc07 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -131,8 +131,12 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> static inline void
> nfs4_label_release_security(struct nfs4_label *label)
> {
> - if (label)
> - security_release_secctx(label->label, label->len);
> + struct lsmcontext scaff; /* scaffolding */
> +
> + if (label) {
> + lsmcontext_init(&scaff, label->label, label->len, 0);
> + security_release_secctx(&scaff);
> + }
> }
> static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
> {
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 533d0fc3c96b..b17aad082bde 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2421,6 +2421,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> int err;
> struct nfs4_acl *acl = NULL;
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> + struct lsmcontext scaff; /* scaffolding */
> void *context = NULL;
> int contextlen;
> #endif
> @@ -2923,8 +2924,10 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>
> out:
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> - if (context)
> - security_release_secctx(context, contextlen);
> + if (context) {
> + lsmcontext_init(&scaff, context, contextlen, 0); /*scaffolding*/
> + security_release_secctx(&scaff);
> + }
> #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
> kfree(acl);
> if (tempfh) {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index f7bc7aef95cb..9bb11d9f1348 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -126,6 +126,41 @@ enum lockdown_reason {
> LOCKDOWN_CONFIDENTIALITY_MAX,
> };
>
> +/*
> + * A "security context" is the text representation of
> + * the information used by LSMs.
> + * This structure contains the string, its length, and which LSM
> + * it is useful for.
> + */
> +struct lsmcontext {
> + char *context; /* Provided by the module */
> + u32 len;
> + int slot; /* Identifies the module */
> +};
> +
> +/**
> + * lsmcontext_init - initialize an lsmcontext structure.
> + * @cp: Pointer to the context to initialize
> + * @context: Initial context, or NULL
> + * @size: Size of context, or 0
> + * @slot: Which LSM provided the context
> + *
> + * Fill in the lsmcontext from the provided information.
> + * This is a scaffolding function that will be removed when
> + * lsmcontext integration is complete.
> + */
> +static inline void lsmcontext_init(struct lsmcontext *cp, char *context,
> + u32 size, int slot)
> +{
> + cp->slot = slot;
> + cp->context = context;
> +
> + if (context == NULL || size == 0)
> + cp->len = 0;
> + else
> + cp->len = strlen(context);
> +}
> +
> /*
> * Data exported by the security modules
> *
> @@ -496,7 +531,7 @@ int security_ismaclabel(const char *name);
> int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen);
> int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *blob);
> -void security_release_secctx(char *secdata, u32 seclen);
> +void security_release_secctx(struct lsmcontext *cp);
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> @@ -1310,7 +1345,7 @@ static inline int security_secctx_to_secid(const char *secdata,
> return -EOPNOTSUPP;
> }
>
> -static inline void security_release_secctx(char *secdata, u32 seclen)
> +static inline void security_release_secctx(struct lsmcontext *cp)
> {
> }
>
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 31ae605fcc0a..30ba801c91bd 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -92,6 +92,7 @@ static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
> #ifdef CONFIG_SECURITY_NETWORK
> static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
> {
> + struct lsmcontext context;
> char *secdata;
> u32 seclen;
> int err;
> @@ -102,7 +103,9 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
>
> if (!err) {
> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
> - security_release_secctx(secdata, seclen);
> + /*scaffolding*/
> + lsmcontext_init(&context, secdata, seclen, 0);
> + security_release_secctx(&context);
> }
> }
> }
> diff --git a/kernel/audit.c b/kernel/audit.c
> index ba9f78e36d1e..35970e7191b6 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1180,6 +1180,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> struct audit_sig_info *sig_data;
> char *ctx = NULL;
> u32 len;
> + struct lsmcontext scaff; /* scaffolding */
>
> err = audit_netlink_ok(skb, msg_type);
> if (err)
> @@ -1424,15 +1425,18 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> }
> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
> if (!sig_data) {
> - if (lsmblob_is_set(&audit_sig_lsm))
> - security_release_secctx(ctx, len);
> + if (lsmblob_is_set(&audit_sig_lsm)) {
> + lsmcontext_init(&scaff, ctx, len, 0);
> + security_release_secctx(&scaff);
> + }
> return -ENOMEM;
> }
> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
> sig_data->pid = audit_sig_pid;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> memcpy(sig_data->ctx, ctx, len);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&scaff, ctx, len, 0);
> + security_release_secctx(&scaff);
> }
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
> @@ -2061,6 +2065,7 @@ int audit_log_task_context(struct audit_buffer *ab)
> unsigned len;
> int error;
> struct lsmblob blob;
> + struct lsmcontext scaff; /* scaffolding */
>
> security_task_getsecid(current, &blob);
> if (!lsmblob_is_set(&blob))
> @@ -2074,7 +2079,8 @@ int audit_log_task_context(struct audit_buffer *ab)
> }
>
> audit_log_format(ab, " subj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&scaff, ctx, len, 0);
> + security_release_secctx(&scaff);
> return 0;
>
> error_path:
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index c1e3ac8eb1ad..8790e7aafa7d 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -962,6 +962,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> struct lsmblob *blob, char *comm)
> {
> struct audit_buffer *ab;
> + struct lsmcontext lsmcxt;
> char *ctx = NULL;
> u32 len;
> int rc = 0;
> @@ -979,7 +980,8 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> rc = 1;
> } else {
> audit_log_format(ab, " obj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&lsmcxt, ctx, len, 0); /*scaffolding*/
> + security_release_secctx(&lsmcxt);
> }
> }
> audit_log_format(ab, " ocomm=");
> @@ -1192,6 +1194,7 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
>
> static void show_special(struct audit_context *context, int *call_panic)
> {
> + struct lsmcontext lsmcxt;
> struct audit_buffer *ab;
> int i;
>
> @@ -1225,7 +1228,8 @@ static void show_special(struct audit_context *context, int *call_panic)
> *call_panic = 1;
> } else {
> audit_log_format(ab, " obj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&lsmcxt, ctx, len, 0);
> + security_release_secctx(&lsmcxt);
> }
> }
> if (context->ipc.has_perm) {
> @@ -1371,6 +1375,7 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> char *ctx = NULL;
> u32 len;
> struct lsmblob blob;
> + struct lsmcontext lsmcxt;
>
> lsmblob_init(&blob, n->osid);
> if (security_secid_to_secctx(&blob, &ctx, &len)) {
> @@ -1379,7 +1384,8 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> *call_panic = 2;
> } else {
> audit_log_format(ab, " obj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&lsmcxt, ctx, len, 0); /* scaffolding */
> + security_release_secctx(&lsmcxt);
> }
> }
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 1ca97d0cb4a9..96d56a30ecca 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -130,6 +130,7 @@ static void ip_cmsg_recv_checksum(struct msghdr *msg, struct sk_buff *skb,
>
> static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> {
> + struct lsmcontext context;
> struct lsmblob lb;
> char *secdata;
> u32 seclen;
> @@ -144,7 +145,8 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> return;
>
> put_cmsg(msg, SOL_IP, SCM_SECURITY, seclen, secdata);
> - security_release_secctx(secdata, seclen);
> + lsmcontext_init(&context, secdata, seclen, 0); /* scaffolding */
> + security_release_secctx(&context);
> }
>
> static void ip_cmsg_recv_dstaddr(struct msghdr *msg, struct sk_buff *skb)
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 0412f6744185..78791e015d8b 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -332,6 +332,7 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> int len, ret;
> char *secctx;
> struct lsmblob blob;
> + struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> ret = security_secid_to_secctx(&blob, &secctx, &len);
> @@ -349,7 +350,8 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
>
> ret = 0;
> nla_put_failure:
> - security_release_secctx(secctx, len);
> + lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> + security_release_secctx(&context);
> return ret;
> }
> #else
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index 183a85412155..8601fcb99f7a 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -176,6 +176,7 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> u32 len;
> char *secctx;
> struct lsmblob blob;
> + struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> ret = security_secid_to_secctx(&blob, &secctx, &len);
> @@ -184,7 +185,8 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
>
> seq_printf(s, "secctx=%s ", secctx);
>
> - security_release_secctx(secctx, len);
> + lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> + security_release_secctx(&context);
> }
> #else
> static inline void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index bfa7f12fde99..cc3ef03ee198 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -395,6 +395,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> enum ip_conntrack_info uninitialized_var(ctinfo);
> struct nfnl_ct_hook *nfnl_ct;
> bool csum_verify;
> + struct lsmcontext scaff; /* scaffolding */
> char *secdata = NULL;
> u32 seclen = 0;
>
> @@ -625,8 +626,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> }
>
> nlh->nlmsg_len = skb->len;
> - if (seclen)
> - security_release_secctx(secdata, seclen);
> + if (seclen) {
> + lsmcontext_init(&scaff, secdata, seclen, 0);
> + security_release_secctx(&scaff);
> + }
> return skb;
>
> nla_put_failure:
> @@ -634,8 +637,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> kfree_skb(skb);
> net_err_ratelimited("nf_queue: error creating packet message\n");
> nlmsg_failure:
> - if (seclen)
> - security_release_secctx(secdata, seclen);
> + if (seclen) {
> + lsmcontext_init(&scaff, secdata, seclen, 0);
> + security_release_secctx(&scaff);
> + }
> return NULL;
> }
>
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index e279b81d9545..288c005b44c7 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -373,6 +373,7 @@ int netlbl_unlhsh_add(struct net *net,
> struct net_device *dev;
> struct netlbl_unlhsh_iface *iface;
> struct audit_buffer *audit_buf = NULL;
> + struct lsmcontext context;
> char *secctx = NULL;
> u32 secctx_len;
> struct lsmblob blob;
> @@ -443,7 +444,9 @@ int netlbl_unlhsh_add(struct net *net,
> &secctx,
> &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + /* scaffolding */
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", ret_val == 0 ? 1 : 0);
> audit_log_end(audit_buf);
> @@ -474,6 +477,7 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct netlbl_unlhsh_addr4 *entry;
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> + struct lsmcontext context;
> char *secctx;
> u32 secctx_len;
> struct lsmblob blob;
> @@ -502,7 +506,9 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> security_secid_to_secctx(&blob,
> &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + /* scaffolding */
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> audit_log_end(audit_buf);
> @@ -539,6 +545,7 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct netlbl_unlhsh_addr6 *entry;
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> + struct lsmcontext context;
> char *secctx;
> u32 secctx_len;
> struct lsmblob blob;
> @@ -566,7 +573,8 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> security_secid_to_secctx(&blob,
> &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> audit_log_end(audit_buf);
> @@ -1080,6 +1088,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> int ret_val = -ENOMEM;
> struct netlbl_unlhsh_walk_arg *cb_arg = arg;
> struct net_device *dev;
> + struct lsmcontext context;
> void *data;
> u32 secid;
> char *secctx;
> @@ -1147,7 +1156,9 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> NLBL_UNLABEL_A_SECCTX,
> secctx_len,
> secctx);
> - security_release_secctx(secctx, secctx_len);
> + /* scaffolding */
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> if (ret_val != 0)
> goto list_cb_failure;
>
> diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
> index 893301ae0131..ef139d8ae7cd 100644
> --- a/net/netlabel/netlabel_user.c
> +++ b/net/netlabel/netlabel_user.c
> @@ -84,6 +84,7 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> struct netlbl_audit *audit_info)
> {
> struct audit_buffer *audit_buf;
> + struct lsmcontext context;
> char *secctx;
> u32 secctx_len;
> struct lsmblob blob;
> @@ -103,7 +104,8 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> if (audit_info->secid != 0 &&
> security_secid_to_secctx(&blob, &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " subj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + lsmcontext_init(&context, secctx, secctx_len, 0);/*scaffolding*/
> + security_release_secctx(&context);
> }
>
> return audit_buf;
> diff --git a/security/security.c b/security/security.c
> index c2874f6587d2..c05ef9d0c8ed 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2144,17 +2144,23 @@ int security_secctx_to_secid(const char *secdata, u32 seclen,
> }
> EXPORT_SYMBOL(security_secctx_to_secid);
>
> -void security_release_secctx(char *secdata, u32 seclen)
> +void security_release_secctx(struct lsmcontext *cp)
> {
> struct security_hook_list *hp;
> - int *display = current->security;
> + bool found = false;
>
> hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
> - if (display == NULL || *display == LSMBLOB_INVALID ||
> - *display == hp->lsmid->slot) {
> - hp->hook.release_secctx(secdata, seclen);
> - return;
> + if (cp->slot == hp->lsmid->slot) {
> + hp->hook.release_secctx(cp->context, cp->len);
> + found = true;
> + break;
> }
> +
> + memset(cp, 0, sizeof(*cp));
> +
> + if (!found)
> + pr_warn("%s context \"%s\" from slot %d not released\n",
> + __func__, cp->context, cp->slot);
> }
> EXPORT_SYMBOL(security_release_secctx);
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index aac8cb0de733..4e464e5e942e 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4483,11 +4483,16 @@ static int smack_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
> return 0;
> }
>
> -/*
> - * There used to be a smack_release_secctx hook
> - * that did nothing back when hooks were in a vector.
> - * Now that there's a list such a hook adds cost.
> +/**
> + * smack_release_secctx - do everything necessary to free a context
> + * @secdata: Unused
> + * @seclen: Unused
> + *
> + * Do nothing but hold a slot in the hooks list.
> */
> +static void smack_release_secctx(char *secdata, u32 seclen)
> +{
> +}
>
> static int smack_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
> {
> @@ -4730,6 +4735,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(ismaclabel, smack_ismaclabel),
> LSM_HOOK_INIT(secid_to_secctx, smack_secid_to_secctx),
> LSM_HOOK_INIT(secctx_to_secid, smack_secctx_to_secid),
> + LSM_HOOK_INIT(release_secctx, smack_release_secctx),
> LSM_HOOK_INIT(inode_notifysecctx, smack_inode_notifysecctx),
> LSM_HOOK_INIT(inode_setsecctx, smack_inode_setsecctx),
> LSM_HOOK_INIT(inode_getsecctx, smack_inode_getsecctx),
>
^ permalink raw reply
* Re: [PATCH] tpm_tis: Move setting of TPM_CHIP_FLAG_IRQ into tpm_tis_probe_irq_single
From: Jerry Snitselaar @ 2019-11-21 18:49 UTC (permalink / raw)
To: Stefan Berger
Cc: Jarkko Sakkinen, Stefan Berger, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <185664a9-58f2-2a4b-4e6b-8d7750a35690@linux.ibm.com>
On Sat Nov 16 19, Stefan Berger wrote:
>On 11/14/19 11:44 AM, Jarkko Sakkinen wrote:
>>On Thu, Nov 14, 2019 at 06:41:51PM +0200, Jarkko Sakkinen wrote:
>>>On Tue, Nov 12, 2019 at 03:27:25PM -0500, Stefan Berger wrote:
>>>>From: Stefan Berger <stefanb@linux.ibm.com>
>>>>
>>>>Move the setting of the TPM_CHIP_FLAG_IRQ for irq probing into
>>>>tpm_tis_probe_irq_single before calling tpm_tis_gen_interrupt.
>>>>This move handles error conditions better that may arise if anything
>>>>before fails in tpm_tis_probe_irq_single.
>>>>
>>>>Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>>Suggested-by: Jerry Snitselaar <jsnitsel@redhat.com>
>>>What about just changing the condition?
>>Also cannot take this since it is not a bug (no fixes tag).
>
>I'll repost but will wait until Jerry has tested it on that machine.
>
> Stefan
>
>
>>
>>/Jarkko
>
>
It appears they still have the problem. I'm still waiting on logistics
to send me a system to debug.
^ permalink raw reply
* Re: [PATCH v11 12/25] IMA: Change internal interfaces to use lsmblobs
From: John Johansen @ 2019-11-21 18:45 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20191113181925.2437-13-casey@schaufler-ca.com>
On 11/13/19 10:19 AM, Casey Schaufler wrote:
> The IMA interfaces ima_get_action() and ima_match_policy()
> call LSM functions that use lsmblobs. Change the IMA functions
> to pass the lsmblob to be compatible with the LSM functions.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: linux-integrity@vger.kernel.org
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> security/integrity/ima/ima.h | 11 ++++----
> security/integrity/ima/ima_api.c | 10 +++----
> security/integrity/ima/ima_appraise.c | 4 +--
> security/integrity/ima/ima_main.c | 38 +++++++++++----------------
> security/integrity/ima/ima_policy.c | 12 ++++-----
> 5 files changed, 34 insertions(+), 41 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 5bcd6011ef8c..4226622f50b1 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -205,9 +205,9 @@ extern const char *const func_tokens[];
> struct modsig;
>
> /* LIM API function definitions */
> -int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
> - int mask, enum ima_hooks func, int *pcr,
> - struct ima_template_desc **template_desc);
> +int ima_get_action(struct inode *inode, const struct cred *cred,
> + struct lsmblob *blob, int mask, enum ima_hooks func,
> + int *pcr, struct ima_template_desc **template_desc);
> int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
> int ima_collect_measurement(struct integrity_iint_cache *iint,
> struct file *file, void *buf, loff_t size,
> @@ -229,8 +229,9 @@ void ima_free_template_entry(struct ima_template_entry *entry);
> const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
>
> /* IMA policy related functions */
> -int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
> - enum ima_hooks func, int mask, int flags, int *pcr,
> +int ima_match_policy(struct inode *inode, const struct cred *cred,
> + struct lsmblob *blob, enum ima_hooks func, int mask,
> + int flags, int *pcr,
> struct ima_template_desc **template_desc);
> void ima_init_policy(void);
> void ima_update_policy(void);
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 610759fe63b8..1ab769fa7df6 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -163,7 +163,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
> * ima_get_action - appraise & measure decision based on policy.
> * @inode: pointer to inode to measure
> * @cred: pointer to credentials structure to validate
> - * @secid: secid of the task being validated
> + * @blob: LSM data of the task being validated
> * @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXEC,
> * MAY_APPEND)
> * @func: caller identifier
> @@ -181,15 +181,15 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
> * Returns IMA_MEASURE, IMA_APPRAISE mask.
> *
> */
> -int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
> - int mask, enum ima_hooks func, int *pcr,
> - struct ima_template_desc **template_desc)
> +int ima_get_action(struct inode *inode, const struct cred *cred,
> + struct lsmblob *blob, int mask, enum ima_hooks func,
> + int *pcr, struct ima_template_desc **template_desc)
> {
> int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
>
> flags &= ima_policy_flag;
>
> - return ima_match_policy(inode, cred, secid, func, mask, flags, pcr,
> + return ima_match_policy(inode, cred, blob, func, mask, flags, pcr,
> template_desc);
> }
>
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 7288a574459b..bc04c6f4bb20 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -47,15 +47,13 @@ bool is_ima_appraise_enabled(void)
> */
> int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
> {
> - u32 secid;
> struct lsmblob blob;
>
> if (!ima_appraise)
> return 0;
>
> security_task_getsecid(current, &blob);
> - lsmblob_secid(&blob, &secid);
> - return ima_match_policy(inode, current_cred(), secid, func, mask,
> + return ima_match_policy(inode, current_cred(), &blob, func, mask,
> IMA_APPRAISE | IMA_HASH, NULL, NULL);
> }
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 305a00a6b087..a8e7e11b1c84 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -190,8 +190,8 @@ void ima_file_free(struct file *file)
> }
>
> static int process_measurement(struct file *file, const struct cred *cred,
> - u32 secid, char *buf, loff_t size, int mask,
> - enum ima_hooks func)
> + struct lsmblob *blob, char *buf, loff_t size,
> + int mask, enum ima_hooks func)
> {
> struct inode *inode = file_inode(file);
> struct integrity_iint_cache *iint = NULL;
> @@ -214,7 +214,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
> * bitmask based on the appraise/audit/measurement policy.
> * Included is the appraise submask.
> */
> - action = ima_get_action(inode, cred, secid, mask, func, &pcr,
> + action = ima_get_action(inode, cred, blob, mask, func, &pcr,
> &template_desc);
> violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
> (ima_policy_flag & IMA_MEASURE));
> @@ -384,8 +384,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
>
> if (file && (prot & PROT_EXEC)) {
> security_task_getsecid(current, &blob);
> - /* scaffolding - until process_measurement changes */
> - return process_measurement(file, current_cred(), blob.secid[0],
> + return process_measurement(file, current_cred(), &blob,
> NULL, 0, MAY_EXEC, MMAP_CHECK);
> }
>
> @@ -411,16 +410,14 @@ int ima_bprm_check(struct linux_binprm *bprm)
> struct lsmblob blob;
>
> security_task_getsecid(current, &blob);
> - /* scaffolding until process_measurement changes */
> - ret = process_measurement(bprm->file, current_cred(), blob.secid[0],
> - NULL, 0, MAY_EXEC, BPRM_CHECK);
> + ret = process_measurement(bprm->file, current_cred(), &blob, NULL, 0,
> + MAY_EXEC, BPRM_CHECK);
> if (ret)
> return ret;
>
> security_cred_getsecid(bprm->cred, &blob);
> - /* scaffolding until process_measurement changes */
> - return process_measurement(bprm->file, bprm->cred, blob.secid[0],
> - NULL, 0, MAY_EXEC, CREDS_CHECK);
> + return process_measurement(bprm->file, bprm->cred, &blob, NULL, 0,
> + MAY_EXEC, CREDS_CHECK);
> }
>
> /**
> @@ -438,8 +435,7 @@ int ima_file_check(struct file *file, int mask)
> struct lsmblob blob;
>
> security_task_getsecid(current, &blob);
> - /* scaffolding until process_measurement changes */
> - return process_measurement(file, current_cred(), blob.secid[0], NULL, 0,
> + return process_measurement(file, current_cred(), &blob, NULL, 0,
> mask & (MAY_READ | MAY_WRITE | MAY_EXEC |
> MAY_APPEND), FILE_CHECK);
> }
> @@ -571,9 +567,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>
> func = read_idmap[read_id] ?: FILE_CHECK;
> security_task_getsecid(current, &blob);
> - /* scaffolding until process_measurement changes */
> - return process_measurement(file, current_cred(), blob.secid[0], buf,
> - size, MAY_READ, func);
> + return process_measurement(file, current_cred(), &blob, buf, size,
> + MAY_READ, func);
> }
>
> /**
> @@ -632,13 +627,14 @@ int ima_load_data(enum kernel_load_data_id id)
> * @size: size of buffer(in bytes).
> * @eventname: event name to be used for the buffer entry.
> * @cred: a pointer to a credentials structure for user validation.
> - * @secid: the secid of the task to be validated.
> + * @blob: the LSM data of the task to be validated.
> *
> * Based on policy, the buffer is measured into the ima log.
> */
> static void process_buffer_measurement(const void *buf, int size,
> const char *eventname,
> - const struct cred *cred, u32 secid)
> + const struct cred *cred,
> + struct lsmblob *blob)
> {
> int ret = 0;
> struct ima_template_entry *entry = NULL;
> @@ -656,7 +652,7 @@ static void process_buffer_measurement(const void *buf, int size,
> int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> int action = 0;
>
> - action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
> + action = ima_get_action(NULL, cred, blob, 0, KEXEC_CMDLINE, &pcr,
> &template_desc);
> if (!(action & IMA_MEASURE))
> return;
> @@ -691,14 +687,12 @@ static void process_buffer_measurement(const void *buf, int size,
> */
> void ima_kexec_cmdline(const void *buf, int size)
> {
> - u32 secid;
> struct lsmblob blob;
>
> if (buf && size != 0) {
> security_task_getsecid(current, &blob);
> - /* scaffolding */
> process_buffer_measurement(buf, size, "kexec-cmdline",
> - current_cred(), blob.secid[0]);
> + current_cred(), &blob);
> }
> }
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index c5417045e165..e863c0d0f9b7 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -368,7 +368,7 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> * Returns true on rule match, false on failure.
> */
> static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> - const struct cred *cred, u32 secid,
> + const struct cred *cred, struct lsmblob *blob,
> enum ima_hooks func, int mask)
> {
> int i;
> @@ -431,7 +431,6 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> case LSM_SUBJ_USER:
> case LSM_SUBJ_ROLE:
> case LSM_SUBJ_TYPE:
> - lsmblob_init(&blob, secid);
> rc = security_filter_rule_match(&blob,
> rule->lsm[i].type,
> Audit_equal,
> @@ -475,7 +474,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
> * @inode: pointer to an inode for which the policy decision is being made
> * @cred: pointer to a credentials structure for which the policy decision is
> * being made
> - * @secid: LSM secid of the task to be validated
> + * @blob: LSM data of the task to be validated
> * @func: IMA hook identifier
> * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
> * @pcr: set the pcr to extend
> @@ -488,8 +487,9 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
> * list when walking it. Reads are many orders of magnitude more numerous
> * than writes so ima_match_policy() is classical RCU candidate.
> */
> -int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
> - enum ima_hooks func, int mask, int flags, int *pcr,
> +int ima_match_policy(struct inode *inode, const struct cred *cred,
> + struct lsmblob *blob, enum ima_hooks func, int mask,
> + int flags, int *pcr,
> struct ima_template_desc **template_desc)
> {
> struct ima_rule_entry *entry;
> @@ -504,7 +504,7 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
> if (!(entry->action & actmask))
> continue;
>
> - if (!ima_match_rules(entry, inode, cred, secid, func, mask))
> + if (!ima_match_rules(entry, inode, cred, blob, func, mask))
> continue;
>
> action |= entry->flags & IMA_ACTION_FLAGS;
>
^ permalink raw reply
* Re: [PATCH v11 11/25] LSM: Use lsmblob in security_cred_getsecid
From: John Johansen @ 2019-11-21 18:44 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20191113181925.2437-12-casey@schaufler-ca.com>
On 11/13/19 10:19 AM, Casey Schaufler wrote:
> Change the security_cred_getsecid() interface to fill in a
> lsmblob instead of a u32 secid. The associated data elements
> in the audit sub-system are changed from a secid to a lsmblob
> to accommodate multiple possible LSM audit users.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: linux-integrity@vger.kernel.org
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> include/linux/security.h | 2 +-
> kernel/audit.c | 19 +++++++-----------
> kernel/audit.h | 5 +++--
> kernel/auditsc.c | 33 +++++++++++--------------------
> security/integrity/ima/ima_main.c | 8 ++++----
> security/security.c | 12 ++++++++---
> 6 files changed, 36 insertions(+), 43 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index a845254fc415..f7bc7aef95cb 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -434,7 +434,7 @@ int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
> void security_cred_free(struct cred *cred);
> int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
> void security_transfer_creds(struct cred *new, const struct cred *old);
> -void security_cred_getsecid(const struct cred *c, u32 *secid);
> +void security_cred_getsecid(const struct cred *c, struct lsmblob *blob);
> int security_kernel_act_as(struct cred *new, struct lsmblob *blob);
> int security_kernel_create_files_as(struct cred *new, struct inode *inode);
> int security_kernel_module_request(char *kmod_name);
> diff --git a/kernel/audit.c b/kernel/audit.c
> index fd29186ae977..ba9f78e36d1e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -124,7 +124,7 @@ static u32 audit_backlog_wait_time = AUDIT_BACKLOG_WAIT_TIME;
> /* The identity of the user shutting down the audit system. */
> kuid_t audit_sig_uid = INVALID_UID;
> pid_t audit_sig_pid = -1;
> -u32 audit_sig_sid = 0;
> +struct lsmblob audit_sig_lsm;
>
> /* Records can be lost in several ways:
> 0) [suppressed in audit_alloc]
> @@ -1416,23 +1416,21 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> }
> case AUDIT_SIGNAL_INFO:
> len = 0;
> - if (audit_sig_sid) {
> - struct lsmblob blob;
> -
> - lsmblob_init(&blob, audit_sig_sid);
> - err = security_secid_to_secctx(&blob, &ctx, &len);
> + if (lsmblob_is_set(&audit_sig_lsm)) {
> + err = security_secid_to_secctx(&audit_sig_lsm, &ctx,
> + &len);
> if (err)
> return err;
> }
> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
> if (!sig_data) {
> - if (audit_sig_sid)
> + if (lsmblob_is_set(&audit_sig_lsm))
> security_release_secctx(ctx, len);
> return -ENOMEM;
> }
> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
> sig_data->pid = audit_sig_pid;
> - if (audit_sig_sid) {
> + if (lsmblob_is_set(&audit_sig_lsm)) {
> memcpy(sig_data->ctx, ctx, len);
> security_release_secctx(ctx, len);
> }
> @@ -2274,7 +2272,6 @@ int audit_set_loginuid(kuid_t loginuid)
> int audit_signal_info(int sig, struct task_struct *t)
> {
> kuid_t uid = current_uid(), auid;
> - struct lsmblob blob;
>
> if (auditd_test_task(t) &&
> (sig == SIGTERM || sig == SIGHUP ||
> @@ -2285,9 +2282,7 @@ int audit_signal_info(int sig, struct task_struct *t)
> audit_sig_uid = auid;
> else
> audit_sig_uid = uid;
> - security_task_getsecid(current, &blob);
> - /* scaffolding until audit_sig_sid is converted */
> - audit_sig_sid = blob.secid[0];
> + security_task_getsecid(current, &audit_sig_lsm);
> }
>
> return audit_signal_info_syscall(t);
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 6fb7160412d4..af9bc09e656c 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -9,6 +9,7 @@
> #include <linux/fs.h>
> #include <linux/audit.h>
> #include <linux/skbuff.h>
> +#include <linux/security.h>
> #include <uapi/linux/mqueue.h>
> #include <linux/tty.h>
>
> @@ -134,7 +135,7 @@ struct audit_context {
> kuid_t target_auid;
> kuid_t target_uid;
> unsigned int target_sessionid;
> - u32 target_sid;
> + struct lsmblob target_lsm;
> char target_comm[TASK_COMM_LEN];
>
> struct audit_tree_refs *trees, *first_trees;
> @@ -329,7 +330,7 @@ extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
>
> extern pid_t audit_sig_pid;
> extern kuid_t audit_sig_uid;
> -extern u32 audit_sig_sid;
> +extern struct lsmblob audit_sig_lsm;
>
> extern int audit_filter(int msgtype, unsigned int listtype);
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 5752e51883d5..c1e3ac8eb1ad 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -112,7 +112,7 @@ struct audit_aux_data_pids {
> kuid_t target_auid[AUDIT_AUX_PIDS];
> kuid_t target_uid[AUDIT_AUX_PIDS];
> unsigned int target_sessionid[AUDIT_AUX_PIDS];
> - u32 target_sid[AUDIT_AUX_PIDS];
> + struct lsmblob target_lsm[AUDIT_AUX_PIDS];
> char target_comm[AUDIT_AUX_PIDS][TASK_COMM_LEN];
> int pid_count;
> };
> @@ -957,14 +957,14 @@ static inline void audit_free_context(struct audit_context *context)
> }
>
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> - kuid_t auid, kuid_t uid, unsigned int sessionid,
> - u32 sid, char *comm)
> + kuid_t auid, kuid_t uid,
> + unsigned int sessionid,
> + struct lsmblob *blob, char *comm)
> {
> struct audit_buffer *ab;
> char *ctx = NULL;
> u32 len;
> int rc = 0;
> - struct lsmblob blob;
>
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_OBJ_PID);
> if (!ab)
> @@ -973,9 +973,8 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> audit_log_format(ab, "opid=%d oauid=%d ouid=%d oses=%d", pid,
> from_kuid(&init_user_ns, auid),
> from_kuid(&init_user_ns, uid), sessionid);
> - if (sid) {
> - lsmblob_init(&blob, sid);
> - if (security_secid_to_secctx(&blob, &ctx, &len)) {
> + if (lsmblob_is_set(blob)) {
> + if (security_secid_to_secctx(blob, &ctx, &len)) {
> audit_log_format(ab, " obj=(none)");
> rc = 1;
> } else {
> @@ -1546,7 +1545,7 @@ static void audit_log_exit(void)
> axs->target_auid[i],
> axs->target_uid[i],
> axs->target_sessionid[i],
> - axs->target_sid[i],
> + &axs->target_lsm[i],
> axs->target_comm[i]))
> call_panic = 1;
> }
> @@ -1555,7 +1554,7 @@ static void audit_log_exit(void)
> audit_log_pid_context(context, context->target_pid,
> context->target_auid, context->target_uid,
> context->target_sessionid,
> - context->target_sid, context->target_comm))
> + &context->target_lsm, context->target_comm))
> call_panic = 1;
>
> if (context->pwd.dentry && context->pwd.mnt) {
> @@ -1733,7 +1732,7 @@ void __audit_syscall_exit(int success, long return_code)
> context->aux = NULL;
> context->aux_pids = NULL;
> context->target_pid = 0;
> - context->target_sid = 0;
> + lsmblob_init(&context->target_lsm, 0);
> context->sockaddr_len = 0;
> context->type = 0;
> context->fds[0] = -1;
> @@ -2384,15 +2383,12 @@ int __audit_sockaddr(int len, void *a)
> void __audit_ptrace(struct task_struct *t)
> {
> struct audit_context *context = audit_context();
> - struct lsmblob blob;
>
> context->target_pid = task_tgid_nr(t);
> context->target_auid = audit_get_loginuid(t);
> context->target_uid = task_uid(t);
> context->target_sessionid = audit_get_sessionid(t);
> - security_task_getsecid(t, &blob);
> - /* scaffolding - until target_sid is converted */
> - context->target_sid = blob.secid[0];
> + security_task_getsecid(t, &context->target_lsm);
> memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> }
>
> @@ -2408,7 +2404,6 @@ int audit_signal_info_syscall(struct task_struct *t)
> struct audit_aux_data_pids *axp;
> struct audit_context *ctx = audit_context();
> kuid_t t_uid = task_uid(t);
> - struct lsmblob blob;
>
> if (!audit_signals || audit_dummy_context())
> return 0;
> @@ -2420,9 +2415,7 @@ int audit_signal_info_syscall(struct task_struct *t)
> ctx->target_auid = audit_get_loginuid(t);
> ctx->target_uid = t_uid;
> ctx->target_sessionid = audit_get_sessionid(t);
> - security_task_getsecid(t, &blob);
> - /* scaffolding until target_sid is converted */
> - ctx->target_sid = blob.secid[0];
> + security_task_getsecid(t, &ctx->target_lsm);
> memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
> return 0;
> }
> @@ -2443,9 +2436,7 @@ int audit_signal_info_syscall(struct task_struct *t)
> axp->target_auid[axp->pid_count] = audit_get_loginuid(t);
> axp->target_uid[axp->pid_count] = t_uid;
> axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
> - security_task_getsecid(t, &blob);
> - /* scaffolding until target_sid is converted */
> - axp->target_sid[axp->pid_count] = blob.secid[0];
> + security_task_getsecid(t, &axp->target_lsm[axp->pid_count]);
> memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
> axp->pid_count++;
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index cac654c2faaf..305a00a6b087 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -408,7 +408,6 @@ int ima_file_mmap(struct file *file, unsigned long prot)
> int ima_bprm_check(struct linux_binprm *bprm)
> {
> int ret;
> - u32 secid;
> struct lsmblob blob;
>
> security_task_getsecid(current, &blob);
> @@ -418,9 +417,10 @@ int ima_bprm_check(struct linux_binprm *bprm)
> if (ret)
> return ret;
>
> - security_cred_getsecid(bprm->cred, &secid);
> - return process_measurement(bprm->file, bprm->cred, secid, NULL, 0,
> - MAY_EXEC, CREDS_CHECK);
> + security_cred_getsecid(bprm->cred, &blob);
> + /* scaffolding until process_measurement changes */
> + return process_measurement(bprm->file, bprm->cred, blob.secid[0],
> + NULL, 0, MAY_EXEC, CREDS_CHECK);
> }
>
> /**
> diff --git a/security/security.c b/security/security.c
> index bd279a24adfc..3aba440624f9 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1615,10 +1615,16 @@ void security_transfer_creds(struct cred *new, const struct cred *old)
> call_void_hook(cred_transfer, new, old);
> }
>
> -void security_cred_getsecid(const struct cred *c, u32 *secid)
> +void security_cred_getsecid(const struct cred *c, struct lsmblob *blob)
> {
> - *secid = 0;
> - call_void_hook(cred_getsecid, c, secid);
> + struct security_hook_list *hp;
> +
> + lsmblob_init(blob, 0);
> + hlist_for_each_entry(hp, &security_hook_heads.cred_getsecid, list) {
> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> + continue;
> + hp->hook.cred_getsecid(c, &blob->secid[hp->lsmid->slot]);
> + }
> }
> EXPORT_SYMBOL(security_cred_getsecid);
>
>
^ permalink raw reply
* Re: [PATCH v11 10/25] LSM: Use lsmblob in security_inode_getsecid
From: John Johansen @ 2019-11-21 18:44 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20191113181925.2437-11-casey@schaufler-ca.com>
On 11/13/19 10:19 AM, Casey Schaufler wrote:
> Change the security_inode_getsecid() interface to fill in a
> lsmblob structure instead of a u32 secid. This allows for its
> callers to gather data from all registered LSMs. Data is provided
> for IMA and audit.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: linux-integrity@vger.kernel.org
Acked-by: John Johansen <john.johansen@canonical.com>
> ---
> include/linux/security.h | 7 ++++---
> kernel/auditsc.c | 6 +++++-
> security/integrity/ima/ima_policy.c | 4 +---
> security/security.c | 11 +++++++++--
> 4 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 67f95a335b5d..a845254fc415 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -407,7 +407,7 @@ int security_inode_killpriv(struct dentry *dentry);
> int security_inode_getsecurity(struct inode *inode, const char *name, void **buffer, bool alloc);
> int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
> int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
> -void security_inode_getsecid(struct inode *inode, u32 *secid);
> +void security_inode_getsecid(struct inode *inode, struct lsmblob *blob);
> int security_inode_copy_up(struct dentry *src, struct cred **new);
> int security_inode_copy_up_xattr(const char *name);
> int security_kernfs_init_security(struct kernfs_node *kn_dir,
> @@ -922,9 +922,10 @@ static inline int security_inode_listsecurity(struct inode *inode, char *buffer,
> return 0;
> }
>
> -static inline void security_inode_getsecid(struct inode *inode, u32 *secid)
> +static inline void security_inode_getsecid(struct inode *inode,
> + struct lsmblob *blob)
> {
> - *secid = 0;
> + lsmblob_init(blob, 0);
> }
>
> static inline int security_inode_copy_up(struct dentry *src, struct cred **new)
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index cccb681ad081..5752e51883d5 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1931,13 +1931,17 @@ static void audit_copy_inode(struct audit_names *name,
> const struct dentry *dentry,
> struct inode *inode, unsigned int flags)
> {
> + struct lsmblob blob;
> +
> name->ino = inode->i_ino;
> name->dev = inode->i_sb->s_dev;
> name->mode = inode->i_mode;
> name->uid = inode->i_uid;
> name->gid = inode->i_gid;
> name->rdev = inode->i_rdev;
> - security_inode_getsecid(inode, &name->osid);
> + security_inode_getsecid(inode, &blob);
> + /* scaffolding until osid is updated */
> + name->osid = blob.secid[0];
> if (flags & AUDIT_INODE_NOEVAL) {
> name->fcap_ver = -1;
> return;
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 7711cc6a3fe3..c5417045e165 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -413,7 +413,6 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> return false;
> for (i = 0; i < MAX_LSM_RULES; i++) {
> int rc = 0;
> - u32 osid;
> struct lsmblob blob;
>
> if (!rule->lsm[i].rule)
> @@ -423,8 +422,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> case LSM_OBJ_USER:
> case LSM_OBJ_ROLE:
> case LSM_OBJ_TYPE:
> - security_inode_getsecid(inode, &osid);
> - lsmblob_init(&blob, osid);
> + security_inode_getsecid(inode, &blob);
> rc = security_filter_rule_match(&blob,
> rule->lsm[i].type,
> Audit_equal,
> diff --git a/security/security.c b/security/security.c
> index e1f216d453bf..bd279a24adfc 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1386,9 +1386,16 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer
> }
> EXPORT_SYMBOL(security_inode_listsecurity);
>
> -void security_inode_getsecid(struct inode *inode, u32 *secid)
> +void security_inode_getsecid(struct inode *inode, struct lsmblob *blob)
> {
> - call_void_hook(inode_getsecid, inode, secid);
> + struct security_hook_list *hp;
> +
> + lsmblob_init(blob, 0);
> + hlist_for_each_entry(hp, &security_hook_heads.inode_getsecid, list) {
> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> + continue;
> + hp->hook.inode_getsecid(inode, &blob->secid[hp->lsmid->slot]);
> + }
> }
>
> int security_inode_copy_up(struct dentry *src, struct cred **new)
>
^ 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