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

[James Morris]

On Tue, 6 Mar 2018, Casey Schaufler wrote:

> On 3/6/2018 11:01 AM, Paul Moore wrote:
> > On Fri, Sep 8, 2017 at 6:09 PM, James Morris  wrote:
> >> On Fri, 8 Sep 2017, Paul Moore wrote:
> >>> 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.
> > 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.
> 
> I was also surprised that it had not been included.
> 
> >
> > * 
> > 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

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

[James Morris]

On Tue, 6 Mar 2018, Casey Schaufler wrote:

> On 3/6/2018 11:01 AM, Paul Moore wrote:
> > On Fri, Sep 8, 2017 at 6:09 PM, James Morris  wrote:
> >> On Fri, 8 Sep 2017, Paul Moore wrote:
> >>> 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.
> > 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.
> 
> I was also surprised that it had not been included.
> 
> >
> > * 
> > 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

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

[Casey Schaufler]

On 3/6/2018 11:01 AM, Paul Moore wrote:
> On Fri, Sep 8, 2017 at 6:09 PM, James Morris  wrote:
>> On Fri, 8 Sep 2017, Paul Moore wrote:
>>> 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.
> 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.

I was also surprised that it had not been included.

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

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

[Casey Schaufler]

On 3/6/2018 11:01 AM, Paul Moore wrote:
> On Fri, Sep 8, 2017 at 6:09 PM, James Morris  wrote:
>> On Fri, 8 Sep 2017, Paul Moore wrote:
>>> 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.
> 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.

I was also surprised that it had not been included.

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

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

[Paul Moore]

On Fri, Sep 8, 2017 at 6:09 PM, James Morris  wrote:
> On Fri, 8 Sep 2017, Paul Moore wrote:
>> 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.

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

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

[Paul Moore]

On Fri, Sep 8, 2017 at 6:09 PM, James Morris  wrote:
> On Fri, 8 Sep 2017, Paul Moore wrote:
>> 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.

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

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

[Greg KH]

On Fri, Sep 08, 2017 at 12:40:01PM -0400, Stephen Smalley wrote:
> 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 
> ---
>  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(-)


Acked-by: Greg Kroah-Hartman 

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

[Greg KH]

On Fri, Sep 08, 2017 at 12:40:01PM -0400, Stephen Smalley wrote:
> 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 
> ---
>  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(-)


Acked-by: Greg Kroah-Hartman 

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

[Casey Schaufler]

On 9/8/2017 9:40 AM, Stephen Smalley wrote:
> 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 

Smack tests seem ok with this.

Acked-by: Casey Schaufler 

> ---
>  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(>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(>lock);
>  
>   if (signr) {
> - kill_pid_info_as_cred(sinfo.si_signo, , pid, cred, secid);
> + kill_pid_info_as_cred(sinfo.si_signo, , 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(>wait);
>   ps->disc_pid = get_pid(task_pid(current));
>   ps->cred = get_current_cred();
> - security_task_getsecid(current, >secid);
>   smp_wmb();
>   list_add_tail(>list, >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, >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, ,
> - 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 @@ 

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

[Casey Schaufler]

On 9/8/2017 9:40 AM, Stephen Smalley wrote:
> 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 

Smack tests seem ok with this.

Acked-by: Casey Schaufler 

> ---
>  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(>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(>lock);
>  
>   if (signr) {
> - kill_pid_info_as_cred(sinfo.si_signo, , pid, cred, secid);
> + kill_pid_info_as_cred(sinfo.si_signo, , 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(>wait);
>   ps->disc_pid = get_pid(task_pid(current));
>   ps->cred = get_current_cred();
> - security_task_getsecid(current, >secid);
>   smp_wmb();
>   list_add_tail(>list, >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, >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, ,
> - 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 

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

[James Morris]

On Fri, 8 Sep 2017, Paul Moore wrote:

> 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

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

[James Morris]

On Fri, 8 Sep 2017, Paul Moore wrote:

> 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

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

[Paul Moore]

On Fri, Sep 8, 2017 at 12:40 PM, Stephen Smalley  wrote:
> 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 
> ---
>  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
www.paul-moore.com

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

[Paul Moore]

On Fri, Sep 8, 2017 at 12:40 PM, Stephen Smalley  wrote:
> 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 
> ---
>  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
www.paul-moore.com

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

[Casey Schaufler]

On 9/8/2017 9:40 AM, Stephen Smalley wrote:
> 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.

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

>   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.

>   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.

>
> Signed-off-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(>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(>lock);
>  
>   if (signr) {
> - kill_pid_info_as_cred(sinfo.si_signo, , pid, cred, secid);
> + kill_pid_info_as_cred(sinfo.si_signo, , 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(>wait);
>   ps->disc_pid = get_pid(task_pid(current));
>   ps->cred = get_current_cred();
> - security_task_getsecid(current, >secid);
>   smp_wmb();
>   list_add_tail(>list, >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, >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, ,
> - 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.
>   *   

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

[Casey Schaufler]

On 9/8/2017 9:40 AM, Stephen Smalley wrote:
> 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.

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

>   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.

>   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.

>
> Signed-off-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(>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(>lock);
>  
>   if (signr) {
> - kill_pid_info_as_cred(sinfo.si_signo, , pid, cred, secid);
> + kill_pid_info_as_cred(sinfo.si_signo, , 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(>wait);
>   ps->disc_pid = get_pid(task_pid(current));
>   ps->cred = get_current_cred();
> - security_task_getsecid(current, >secid);
>   smp_wmb();
>   list_add_tail(>list, >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, >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, ,
> - 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