From 6b4f3d01052a479c7ebbe99d52a663558dc1be2a Mon Sep 17 00:00:00 2001 From: Stephen Smalley Date: Fri, 8 Sep 2017 12:40:01 -0400 Subject: usb, signal, security: only pass the cred, not the secid, to kill_pid_info_as_cred and security_task_kill commit d178bc3a708f39cbfefc3fab37032d3f2511b4ec ("user namespace: usb: make usb urbs user namespace aware (v2)") changed kill_pid_info_as_uid to kill_pid_info_as_cred, saving and passing a cred structure instead of uids. Since the secid can be obtained from the cred, drop the secid fields from the usb_dev_state and async structures, and drop the secid argument to kill_pid_info_as_cred. Replace the secid argument to security_task_kill with the cred. Update SELinux, Smack, and AppArmor to use the cred, which avoids the need for Smack and AppArmor to use a secid at all in this hook. Further changes to Smack might still be required to take full advantage of this change, since it should now be possible to perform capability checking based on the supplied cred. The changes to Smack and AppArmor have only been compile-tested. Signed-off-by: Stephen Smalley Acked-by: Paul Moore Acked-by: Casey Schaufler Acked-by: Greg Kroah-Hartman Acked-by: John Johansen Signed-off-by: James Morris --- security/apparmor/lsm.c | 17 ++++++++++++----- security/security.c | 4 ++-- security/selinux/hooks.c | 7 +++++-- security/smack/smack_lsm.c | 12 +++++------- 4 files changed, 24 insertions(+), 16 deletions(-) (limited to 'security') diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 9a65eea..77bdfa7 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -717,16 +717,23 @@ static int apparmor_task_setrlimit(struct task_struct *task, } static int apparmor_task_kill(struct task_struct *target, struct siginfo *info, - int sig, u32 secid) + int sig, const struct cred *cred) { struct aa_label *cl, *tl; int error; - if (secid) - /* TODO: after secid to label mapping is done. - * Dealing with USB IO specific behavior + if (cred) { + /* + * Dealing with USB IO specific behavior */ - return 0; + cl = aa_get_newest_cred_label(cred); + tl = aa_get_task_label(target); + error = aa_may_signal(cl, tl, sig); + aa_put_label(cl); + aa_put_label(tl); + return error; + } + cl = __begin_current_label_crit_section(); tl = aa_get_task_label(target); error = aa_may_signal(cl, tl, sig); diff --git a/security/security.c b/security/security.c index 1cd8526..14c2919 100644 --- a/security/security.c +++ b/security/security.c @@ -1114,9 +1114,9 @@ int security_task_movememory(struct task_struct *p) } int security_task_kill(struct task_struct *p, struct siginfo *info, - int sig, u32 secid) + int sig, const struct cred *cred) { - return call_int_hook(task_kill, 0, p, info, sig, secid); + return call_int_hook(task_kill, 0, p, info, sig, cred); } int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 8644d86..8abd542 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4036,16 +4036,19 @@ static int selinux_task_movememory(struct task_struct *p) } static int selinux_task_kill(struct task_struct *p, struct siginfo *info, - int sig, u32 secid) + int sig, const struct cred *cred) { + u32 secid; u32 perm; if (!sig) perm = PROCESS__SIGNULL; /* null signal; existence test */ else perm = signal_to_av(sig); - if (!secid) + if (!cred) secid = current_sid(); + else + secid = cred_sid(cred); return avc_has_perm(secid, task_sid(p), SECCLASS_PROCESS, perm, NULL); } diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 03fdecb..feada26 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -2228,15 +2228,13 @@ static int smack_task_movememory(struct task_struct *p) * @p: the task object * @info: unused * @sig: unused - * @secid: identifies the smack to use in lieu of current's + * @cred: identifies the cred to use in lieu of current's * * Return 0 if write access is permitted * - * The secid behavior is an artifact of an SELinux hack - * in the USB code. Someday it may go away. */ static int smack_task_kill(struct task_struct *p, struct siginfo *info, - int sig, u32 secid) + int sig, const struct cred *cred) { struct smk_audit_info ad; struct smack_known *skp; @@ -2252,17 +2250,17 @@ static int smack_task_kill(struct task_struct *p, struct siginfo *info, * Sending a signal requires that the sender * can write the receiver. */ - if (secid == 0) { + if (cred == NULL) { rc = smk_curacc(tkp, MAY_DELIVER, &ad); rc = smk_bu_task(p, MAY_DELIVER, rc); return rc; } /* - * If the secid isn't 0 we're dealing with some USB IO + * If the cred isn't NULL we're dealing with some USB IO * specific behavior. This is not clean. For one thing * we can't take privilege into account. */ - skp = smack_from_secid(secid); + skp = smk_of_task(cred->security); rc = smk_access(skp, tkp, MAY_DELIVER, &ad); rc = smk_bu_note("USB signal", skp, tkp, MAY_DELIVER, rc); return rc; -- cgit v1.1 From df0ce17331e2501dbffc060041dfc6c5f85227b5 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Thu, 29 Mar 2018 01:28:23 +0000 Subject: security: convert security hooks to use hlist This changes security_hook_heads to use hlist_heads instead of the circular doubly-linked list heads. This should cut down the size of the struct by about half. In addition, it allows mutation of the hooks at the tail of the callback list without having to modify the head. The longer-term purpose of this is to enable making the heads read only. Signed-off-by: Sargun Dhillon Reviewed-by: Tetsuo Handa Acked-by: Casey Schaufler Signed-off-by: James Morris --- security/security.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) (limited to 'security') diff --git a/security/security.c b/security/security.c index 14c2919..dd246a3 100644 --- a/security/security.c +++ b/security/security.c @@ -61,11 +61,11 @@ static void __init do_security_initcalls(void) int __init security_init(void) { int i; - struct list_head *list = (struct list_head *) &security_hook_heads; + struct hlist_head *list = (struct hlist_head *) &security_hook_heads; - for (i = 0; i < sizeof(security_hook_heads) / sizeof(struct list_head); + for (i = 0; i < sizeof(security_hook_heads) / sizeof(struct hlist_head); i++) - INIT_LIST_HEAD(&list[i]); + INIT_HLIST_HEAD(&list[i]); pr_info("Security Framework initialized\n"); /* @@ -163,7 +163,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, for (i = 0; i < count; i++) { hooks[i].lsm = lsm; - list_add_tail_rcu(&hooks[i].list, hooks[i].head); + hlist_add_tail_rcu(&hooks[i].list, hooks[i].head); } if (lsm_append(lsm, &lsm_names) < 0) panic("%s - Cannot get early memory.\n", __func__); @@ -201,7 +201,7 @@ EXPORT_SYMBOL(unregister_lsm_notifier); do { \ struct security_hook_list *P; \ \ - list_for_each_entry(P, &security_hook_heads.FUNC, list) \ + hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \ P->hook.FUNC(__VA_ARGS__); \ } while (0) @@ -210,7 +210,7 @@ EXPORT_SYMBOL(unregister_lsm_notifier); do { \ struct security_hook_list *P; \ \ - list_for_each_entry(P, &security_hook_heads.FUNC, list) { \ + hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ RC = P->hook.FUNC(__VA_ARGS__); \ if (RC != 0) \ break; \ @@ -317,7 +317,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) * agree that it should be set it will. If any module * thinks it should not be set it won't. */ - list_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) { + hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) { rc = hp->hook.vm_enough_memory(mm, pages); if (rc <= 0) { cap_sys_admin = 0; @@ -805,7 +805,7 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf /* * Only one module will provide an attribute with a given name. */ - list_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) { + hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) { rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc); if (rc != -EOPNOTSUPP) return rc; @@ -823,7 +823,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void /* * Only one module will provide an attribute with a given name. */ - list_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) { + hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) { rc = hp->hook.inode_setsecurity(inode, name, value, size, flags); if (rc != -EOPNOTSUPP) @@ -1126,7 +1126,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, int rc = -ENOSYS; struct security_hook_list *hp; - list_for_each_entry(hp, &security_hook_heads.task_prctl, list) { + hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) { thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5); if (thisrc != -ENOSYS) { rc = thisrc; @@ -1629,7 +1629,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x, * For speed optimization, we explicitly break the loop rather than * using the macro */ - list_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match, + hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match, list) { rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl); break; -- cgit v1.1