[PATCH] usb, signal, security: only pass the cred, not the secid, to kill_pid_info_as_cred and security_task

Discussion:

[PATCH] usb, signal, security: only pass the cred, not the secid, to kill_pid_info_as_cred and security_task_kill

Stephen Smalley

2017-09-08 16:40:01 UTC

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 <***@tycho.nsa.gov>
---
drivers/usb/core/devio.c | 10 ++--------
include/linux/lsm_hooks.h | 5 +++--
include/linux/sched/signal.h | 2 +-
include/linux/security.h | 4 ++--
kernel/signal.c | 6 +++---
security/apparmor/lsm.c | 17 ++++++++++++-----
security/security.c | 4 ++--
security/selinux/hooks.c | 7 +++++--
security/smack/smack_lsm.c | 12 +++++-------
9 files changed, 35 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index ebe2759..b44f74c 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -78,7 +78,6 @@ struct usb_dev_state {
const struct cred *cred;
void __user *disccontext;
unsigned long ifclaimed;
- u32 secid;
u32 disabled_bulk_eps;
bool privileges_dropped;
unsigned long interface_allowed_mask;
@@ -108,7 +107,6 @@ struct async {
struct usb_memory *usbm;
unsigned int mem_usage;
int status;
- u32 secid;
u8 bulk_addr;
u8 bulk_status;
};
@@ -596,7 +594,6 @@ static void async_completed(struct urb *urb)
struct usb_dev_state *ps = as->ps;
struct siginfo sinfo;
struct pid *pid = NULL;
- u32 secid = 0;
const struct cred *cred = NULL;
int signr;

@@ -612,7 +609,6 @@ static void async_completed(struct urb *urb)
sinfo.si_addr = as->userurb;
pid = get_pid(as->pid);
cred = get_cred(as->cred);
- secid = as->secid;
}
snoop(&urb->dev->dev, "urb complete\n");
snoop_urb(urb->dev, as->userurb, urb->pipe, urb->actual_length,
@@ -626,7 +622,7 @@ static void async_completed(struct urb *urb)
spin_unlock(&ps->lock);

if (signr) {
- kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid, cred, secid);
+ kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid, cred);
put_pid(pid);
put_cred(cred);
}
@@ -1023,7 +1019,6 @@ static int usbdev_open(struct inode *inode, struct file *file)
init_waitqueue_head(&ps->wait);
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
- security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
file->private_data = ps;
@@ -1733,7 +1728,6 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
as->ifnum = ifnum;
as->pid = get_pid(task_pid(current));
as->cred = get_current_cred();
- security_task_getsecid(current, &as->secid);
snoop_urb(ps->dev, as->userurb, as->urb->pipe,
as->urb->transfer_buffer_length, 0, SUBMIT,
NULL, 0);
@@ -2609,7 +2603,7 @@ static void usbdev_remove(struct usb_device *udev)
sinfo.si_code = SI_ASYNCIO;
sinfo.si_addr = ps->disccontext;
kill_pid_info_as_cred(ps->discsignr, &sinfo,
- ps->disc_pid, ps->cred, ps->secid);
+ ps->disc_pid, ps->cred);
}
}
}
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ce02f76..b0b663b2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -674,7 +674,8 @@
* @p contains the task_struct for process.
* @info contains the signal information.
* @sig contains the signal value.
- * @secid contains the sid of the process where the signal originated
+ * @cred contains the cred of the process where the signal originated, or
+ * NULL if the current task is the originator.
* Return 0 if permission is granted.
* @task_prctl:
* Check permission before performing a process control operation on the
@@ -1533,7 +1534,7 @@ union security_list_options {
int (*task_getscheduler)(struct task_struct *p);
int (*task_movememory)(struct task_struct *p);
int (*task_kill)(struct task_struct *p, struct siginfo *info,
- int sig, u32 secid);
+ int sig, const struct cred *cred);
int (*task_prctl)(int option, unsigned long arg2, unsigned long arg3,
unsigned long arg4, unsigned long arg5);
void (*task_to_inode)(struct task_struct *p, struct inode *inode);
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 2a0dd40..ae4fe12 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -290,7 +290,7 @@ extern int force_sig_info(int, struct siginfo *, struct task_struct *);
extern int __kill_pgrp_info(int sig, struct siginfo *info, struct pid *pgrp);
extern int kill_pid_info(int sig, struct siginfo *info, struct pid *pid);
extern int kill_pid_info_as_cred(int, struct siginfo *, struct pid *,
- const struct cred *, u32);
+ const struct cred *);
extern int kill_pgrp(struct pid *pid, int sig, int priv);
extern int kill_pid(struct pid *pid, int sig, int priv);
extern __must_check bool do_notify_parent(struct task_struct *, int);
diff --git a/include/linux/security.h b/include/linux/security.h
index 458e24b..9655621 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -347,7 +347,7 @@ int security_task_setscheduler(struct task_struct *p);
int security_task_getscheduler(struct task_struct *p);
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);
int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
unsigned long arg4, unsigned long arg5);
void security_task_to_inode(struct task_struct *p, struct inode *inode);
@@ -1015,7 +1015,7 @@ static inline int security_task_movememory(struct task_struct *p)

static inline int security_task_kill(struct task_struct *p,
struct siginfo *info, int sig,
- u32 secid)
+ const struct cred *cred)
{
return 0;
}
diff --git a/kernel/signal.c b/kernel/signal.c
index caed913..a397bb9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -762,7 +762,7 @@ static int check_kill_permission(int sig, struct siginfo *info,
}
}

- return security_task_kill(t, info, sig, 0);
+ return security_task_kill(t, info, sig, NULL);
}

/**
@@ -1348,7 +1348,7 @@ static int kill_as_cred_perm(const struct cred *cred,

/* like kill_pid_info(), but doesn't use uid/euid of "current" */
int kill_pid_info_as_cred(int sig, struct siginfo *info, struct pid *pid,
- const struct cred *cred, u32 secid)
+ const struct cred *cred)
{
int ret = -EINVAL;
struct task_struct *p;
@@ -1367,7 +1367,7 @@ int kill_pid_info_as_cred(int sig, struct siginfo *info, struct pid *pid,
ret = -EPERM;
goto out_unlock;
}
- ret = security_task_kill(p, info, sig, secid);
+ ret = security_task_kill(p, info, sig, cred);
if (ret)
goto out_unlock;

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index cc5ab23..2fbec6d 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -718,16 +718,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 55b5997..3b67842 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1118,9 +1118,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 45943e1..68bc634 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4041,16 +4041,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 463af86..65fcede 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2259,15 +2259,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;
@@ -2283,17 +2281,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;

--
2.9.4

Casey Schaufler

2017-09-08 16:52:58 UTC

Permalink

Post by Stephen Smalley
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.

That's a change I've wanted to make for some time,
but was never confident that the cred would remain
valid.

Post by Stephen Smalley
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.

Perfect.

Post by Stephen Smalley
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.

I will run some tests on Smack. I don't expect to implement
the additional privilege checks at this time.

Post by Stephen Smalley
---
drivers/usb/core/devio.c | 10 ++--------
include/linux/lsm_hooks.h | 5 +++--
include/linux/sched/signal.h | 2 +-
include/linux/security.h | 4 ++--
kernel/signal.c | 6 +++---
security/apparmor/lsm.c | 17 ++++++++++++-----
security/security.c | 4 ++--
security/selinux/hooks.c | 7 +++++--
security/smack/smack_lsm.c | 12 +++++-------
9 files changed, 35 insertions(+), 32 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index ebe2759..b44f74c 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -78,7 +78,6 @@ struct usb_dev_state {
const struct cred *cred;
void __user *disccontext;
unsigned long ifclaimed;
- u32 secid;
u32 disabled_bulk_eps;
bool privileges_dropped;
unsigned long interface_allowed_mask;
@@ -108,7 +107,6 @@ struct async {
struct usb_memory *usbm;
unsigned int mem_usage;
int status;
- u32 secid;
u8 bulk_addr;
u8 bulk_status;
};
@@ -596,7 +594,6 @@ static void async_completed(struct urb *urb)
struct usb_dev_state *ps = as->ps;
struct siginfo sinfo;
struct pid *pid = NULL;
- u32 secid = 0;
const struct cred *cred = NULL;
int signr;
@@ -612,7 +609,6 @@ static void async_completed(struct urb *urb)
sinfo.si_addr = as->userurb;
pid = get_pid(as->pid);
cred = get_cred(as->cred);
- secid = as->secid;
}
snoop(&urb->dev->dev, "urb complete\n");
snoop_urb(urb->dev, as->userurb, urb->pipe, urb->actual_length,
@@ -626,7 +622,7 @@ static void async_completed(struct urb *urb)
spin_unlock(&ps->lock);
if (signr) {
- kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid, cred, secid);
+ kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid, cred);
put_pid(pid);
put_cred(cred);
}
@@ -1023,7 +1019,6 @@ static int usbdev_open(struct inode *inode, struct file *file)
init_waitqueue_head(&ps->wait);
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
- security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
file->private_data = ps;
@@ -1733,7 +1728,6 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb
as->ifnum = ifnum;
as->pid = get_pid(task_pid(current));
as->cred = get_current_cred();
- security_task_getsecid(current, &as->secid);
snoop_urb(ps->dev, as->userurb, as->urb->pipe,
as->urb->transfer_buffer_length, 0, SUBMIT,
NULL, 0);
@@ -2609,7 +2603,7 @@ static void usbdev_remove(struct usb_device *udev)
sinfo.si_code = SI_ASYNCIO;
sinfo.si_addr = ps->disccontext;
kill_pid_info_as_cred(ps->discsignr, &sinfo,
- ps->disc_pid, ps->cred, ps->secid);
+ ps->disc_pid, ps->cred);
}
}
}
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ce02f76..b0b663b2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -674,7 +674,8 @@
+ * NULL if the current task is the originator.
* Return 0 if permission is granted.
* Check permission before performing a process control operation on the
@@ -1533,7 +1534,7 @@ union security_list_options {
int (*task_getscheduler)(struct task_struct *p);
int (*task_movememory)(struct task_struct *p);
int (*task_kill)(struct task_struct *p, struct siginfo *info,
- int sig, u32 secid);
+ int sig, const struct cred *cred);
int (*task_prctl)(int option, unsigned long arg2, unsigned long arg3,
unsigned long arg4, unsigned long arg5);
void (*task_to_inode)(struct task_struct *p, struct inode *inode);
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 2a0dd40..ae4fe12 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -290,7 +290,7 @@ extern int force_sig_info(int, struct siginfo *, struct task_struct *);
extern int __kill_pgrp_info(int sig, struct siginfo *info, struct pid *pgrp);
extern int kill_pid_info(int sig, struct siginfo *info, struct pid *pid);
extern int kill_pid_info_as_cred(int, struct siginfo *, struct pid *,
- const struct cred *, u32);
+ const struct cred *);
extern int kill_pgrp(struct pid *pid, int sig, int priv);
extern int kill_pid(struct pid *pid, int sig, int priv);
extern __must_check bool do_notify_parent(struct task_struct *, int);
diff --git a/include/linux/security.h b/include/linux/security.h
index 458e24b..9655621 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -347,7 +347,7 @@ int security_task_setscheduler(struct task_struct *p);
int security_task_getscheduler(struct task_struct *p);
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);
int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
unsigned long arg4, unsigned long arg5);
void security_task_to_inode(struct task_struct *p, struct inode *inode);
@@ -1015,7 +1015,7 @@ static inline int security_task_movememory(struct task_struct *p)
static inline int security_task_kill(struct task_struct *p,
struct siginfo *info, int sig,
- u32 secid)
+ const struct cred *cred)
{
return 0;
}
diff --git a/kernel/signal.c b/kernel/signal.c
index caed913..a397bb9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -762,7 +762,7 @@ static int check_kill_permission(int sig, struct siginfo *info,
}
}
- return security_task_kill(t, info, sig, 0);
+ return security_task_kill(t, info, sig, NULL);
}
/**
@@ -1348,7 +1348,7 @@ static int kill_as_cred_perm(const struct cred *cred,
/* like kill_pid_info(), but doesn't use uid/euid of "current" */
int kill_pid_info_as_cred(int sig, struct siginfo *info, struct pid *pid,
- const struct cred *cred, u32 secid)
+ const struct cred *cred)
{
int ret = -EINVAL;
struct task_struct *p;
@@ -1367,7 +1367,7 @@ int kill_pid_info_as_cred(int sig, struct siginfo *info, struct pid *pid,
ret = -EPERM;
goto out_unlock;
}
- ret = security_task_kill(p, info, sig, secid);
+ ret = security_task_kill(p, info, sig, cred);
if (ret)
goto out_unlock;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index cc5ab23..2fbec6d 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -718,16 +718,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 55b5997..3b67842 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1118,9 +1118,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 45943e1..68bc634 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4041,16 +4041,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 463af86..65fcede 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2259,15 +2259,13 @@ static int smack_task_movememory(struct task_struct *p)
*
* 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;
@@ -2283,17 +2281,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;

Paul Moore

2017-09-08 17:11:54 UTC

Permalink

Post by Stephen Smalley
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.
---
drivers/usb/core/devio.c | 10 ++--------
include/linux/lsm_hooks.h | 5 +++--
include/linux/sched/signal.h | 2 +-
include/linux/security.h | 4 ++--
kernel/signal.c | 6 +++---
security/apparmor/lsm.c | 17 ++++++++++++-----
security/security.c | 4 ++--
security/selinux/hooks.c | 7 +++++--
security/smack/smack_lsm.c | 12 +++++-------
9 files changed, 35 insertions(+), 32 deletions(-)

Looks fine to me from a SELinux perspective. If Casey and John are
happy with this I can volunteer to pull it into the selinux/next tree
(once the merge window closes), otherwise if someone else wants to
merge this my ack is below.

Acked-by: Paul Moore <***@paul-moore.com>

--
paul moore
www.paul-moore.com

James Morris

2017-09-08 22:09:01 UTC

Permalink

Post by Paul Moore
Looks fine to me from a SELinux perspective. If Casey and John are
happy with this I can volunteer to pull it into the selinux/next tree
(once the merge window closes), otherwise if someone else wants to
merge this my ack is below.

As this impacts multiple LSMs, I'd prefer to take it via my tree.

--
James Morris
<***@namei.org>

Paul Moore

2018-03-06 19:01:01 UTC

Permalink

Post by James Morris

As this impacts multiple LSMs, I'd prefer to take it via my tree.

What happened to this James? As best I can tell there were never any
objections, and plenty of ACKs, but I don't see it in Linus' tree.
I'll extend my offer to merge it, but I know you expressed a desire to
pull this via your tree.

* http://kernsec.org/pipermail/linux-security-module-archive/2017-September/003156.html

--
paul moore
www.paul-moore.com

Casey Schaufler

2018-03-06 19:28:34 UTC

Permalink

Post by Paul Moore

Post by James Morris

As this impacts multiple LSMs, I'd prefer to take it via my tree.

I was also surprised that it had not been included.

Post by Paul Moore
* http://kernsec.org/pipermail/linux-security-module-archive/2017-September/003156.html

James Morris

2018-03-06 22:10:10 UTC

Permalink

Post by Casey Schaufler

Post by Paul Moore

Post by James Morris

As this impacts multiple LSMs, I'd prefer to take it via my tree.

I was also surprised that it had not been included.

Post by Paul Moore
* http://kernsec.org/pipermail/linux-security-module-archive/2017-September/003156.html

Oops,

Now applied to:

git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
and next-testing

--
James Morris
<***@namei.org>

Casey Schaufler

2017-09-08 22:27:16 UTC

Permalink

Smack tests seem ok with this.

Greg KH

2017-09-09 05:05:52 UTC

Permalink

Acked-by: Greg Kroah-Hartman <***@linuxfoundation.org>

John Johansen

2017-09-09 07:14:24 UTC

Permalink

Sorry, meant to cc you on this.
-------- Forwarded Message --------
.nsa.gov>
Subject: [PATCH] usb,signal,security: only pass the cred, not the
secid, to kill_pid_info_as_cred and security_task_kill
Date: Fri, 8 Sep 2017 12:40:01 -0400
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.

this looks good to me

---
drivers/usb/core/devio.c | 10 ++--------
include/linux/lsm_hooks.h | 5 +++--
include/linux/sched/signal.h | 2 +-
include/linux/security.h | 4 ++--
kernel/signal.c | 6 +++---
security/apparmor/lsm.c | 17 ++++++++++++-----
security/security.c | 4 ++--
security/selinux/hooks.c | 7 +++++--
security/smack/smack_lsm.c | 12 +++++-------
9 files changed, 35 insertions(+), 32 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index ebe2759..b44f74c 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -78,7 +78,6 @@ struct usb_dev_state {
const struct cred *cred;
void __user *disccontext;
unsigned long ifclaimed;
- u32 secid;
u32 disabled_bulk_eps;
bool privileges_dropped;
unsigned long interface_allowed_mask;
@@ -108,7 +107,6 @@ struct async {
struct usb_memory *usbm;
unsigned int mem_usage;
int status;
- u32 secid;
u8 bulk_addr;
u8 bulk_status;
};
@@ -596,7 +594,6 @@ static void async_completed(struct urb *urb)
struct usb_dev_state *ps = as->ps;
struct siginfo sinfo;
struct pid *pid = NULL;
- u32 secid = 0;
const struct cred *cred = NULL;
int signr;
@@ -612,7 +609,6 @@ static void async_completed(struct urb *urb)
sinfo.si_addr = as->userurb;
pid = get_pid(as->pid);
cred = get_cred(as->cred);
- secid = as->secid;
}
snoop(&urb->dev->dev, "urb complete\n");
snoop_urb(urb->dev, as->userurb, urb->pipe, urb-

Post by Stephen Smalley
actual_length,

@@ -626,7 +622,7 @@ static void async_completed(struct urb *urb)
spin_unlock(&ps->lock);
if (signr) {
- kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid,
cred, secid);
+ kill_pid_info_as_cred(sinfo.si_signo, &sinfo, pid,
cred);
put_pid(pid);
put_cred(cred);
}
@@ -1023,7 +1019,6 @@ static int usbdev_open(struct inode *inode, struct file *file)
init_waitqueue_head(&ps->wait);
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
- security_task_getsecid(current, &ps->secid);
smp_wmb();
list_add_tail(&ps->list, &dev->filelist);
file->private_data = ps;
@@ -1733,7 +1728,6 @@ static int proc_do_submiturb(struct usb_dev_state
*ps, struct usbdevfs_urb *uurb
as->ifnum = ifnum;
as->pid = get_pid(task_pid(current));
as->cred = get_current_cred();
- security_task_getsecid(current, &as->secid);
snoop_urb(ps->dev, as->userurb, as->urb->pipe,
as->urb->transfer_buffer_length, 0, SUBMIT,
NULL, 0);
@@ -2609,7 +2603,7 @@ static void usbdev_remove(struct usb_device *udev)
sinfo.si_code = SI_ASYNCIO;
sinfo.si_addr = ps->disccontext;
kill_pid_info_as_cred(ps->discsignr, &sinfo,
- ps->disc_pid, ps->cred, ps-

Post by Stephen Smalley
secid);

+ ps->disc_pid, ps->cred);
}
}
}
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ce02f76..b0b663b2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -674,7 +674,8 @@
originated
originated, or
+ * NULL if the current task is the originator.
* Return 0 if permission is granted.
* Check permission before performing a process control
operation on the
@@ -1533,7 +1534,7 @@ union security_list_options {
int (*task_getscheduler)(struct task_struct *p);
int (*task_movememory)(struct task_struct *p);
int (*task_kill)(struct task_struct *p, struct siginfo *info,
- int sig, u32 secid);
+ int sig, const struct cred *cred);
int (*task_prctl)(int option, unsigned long arg2, unsigned
long arg3,
unsigned long arg4, unsigned long
arg5);
void (*task_to_inode)(struct task_struct *p, struct inode
*inode);
diff --git a/include/linux/sched/signal.h
b/include/linux/sched/signal.h
index 2a0dd40..ae4fe12 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -290,7 +290,7 @@ extern int force_sig_info(int, struct siginfo *, struct task_struct *);
extern int __kill_pgrp_info(int sig, struct siginfo *info, struct pid *pgrp);
extern int kill_pid_info(int sig, struct siginfo *info, struct pid *pid);
extern int kill_pid_info_as_cred(int, struct siginfo *, struct pid *,
- const struct cred *, u32);
+ const struct cred *);
extern int kill_pgrp(struct pid *pid, int sig, int priv);
extern int kill_pid(struct pid *pid, int sig, int priv);
extern __must_check bool do_notify_parent(struct task_struct *, int);
diff --git a/include/linux/security.h b/include/linux/security.h
index 458e24b..9655621 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -347,7 +347,7 @@ int security_task_setscheduler(struct task_struct *p);
int security_task_getscheduler(struct task_struct *p);
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);
int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
unsigned long arg4, unsigned long arg5);
void security_task_to_inode(struct task_struct *p, struct inode *inode);
@@ -1015,7 +1015,7 @@ static inline int security_task_movememory(struct task_struct *p)
static inline int security_task_kill(struct task_struct *p,
struct siginfo *info, int sig,
- u32 secid)
+ const struct cred *cred)
{
return 0;
}
diff --git a/kernel/signal.c b/kernel/signal.c
index caed913..a397bb9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -762,7 +762,7 @@ static int check_kill_permission(int sig, struct siginfo *info,
}
}
- return security_task_kill(t, info, sig, 0);
+ return security_task_kill(t, info, sig, NULL);
}
/**
@@ -1348,7 +1348,7 @@ static int kill_as_cred_perm(const struct cred *cred,
/* like kill_pid_info(), but doesn't use uid/euid of "current" */
int kill_pid_info_as_cred(int sig, struct siginfo *info, struct pid *pid,
- const struct cred *cred, u32 secid)
+ const struct cred *cred)
{
int ret = -EINVAL;
struct task_struct *p;
@@ -1367,7 +1367,7 @@ int kill_pid_info_as_cred(int sig, struct siginfo
*info, struct pid *pid,
ret = -EPERM;
goto out_unlock;
}
- ret = security_task_kill(p, info, sig, secid);
+ ret = security_task_kill(p, info, sig, cred);
if (ret)
goto out_unlock;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index cc5ab23..2fbec6d 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -718,16 +718,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 55b5997..3b67842 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1118,9 +1118,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 45943e1..68bc634 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4041,16 +4041,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 463af86..65fcede 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2259,15 +2259,13 @@ static int smack_task_movememory(struct task_struct *p)
*
* 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;
@@ -2283,17 +2281,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;
--
2.9.4