Re: [PATCH V4] audit: add tty field to LOGIN event

[Paul Moore]

On Wed, Apr 27, 2016 at 9:31 PM, Richard Guy Briggs  wrote:
> On 16/04/22, Peter Hurley wrote:
>> 2. The existing usage is always tsk==current
>
> My understanding is that when it is called via:
>
> copy_process()
> audit_free()
> __audit_free()
> audit_log_exit()
> audit_log_task_info()
>
> then tsk != current.  This appears to be the only case which appears to
> force lugging around tsk.  This is noted in that commit referenced
> above.

In the case where copy_process() ends up calling __audit_free(), the
call to audit_log_exit() is conditional on the audit context
in_syscall field being true and unless I missed something, the copied
process' audit context should not have in_syscall set to true.

-- 
paul moore
www.paul-moore.com

Re: [PATCH V4] audit: add tty field to LOGIN event

[Paul Moore]

On Wed, Apr 27, 2016 at 9:31 PM, Richard Guy Briggs  wrote:
> On 16/04/22, Peter Hurley wrote:
>> 2. The existing usage is always tsk==current
>
> My understanding is that when it is called via:
>
> copy_process()
> audit_free()
> __audit_free()
> audit_log_exit()
> audit_log_task_info()
>
> then tsk != current.  This appears to be the only case which appears to
> force lugging around tsk.  This is noted in that commit referenced
> above.

In the case where copy_process() ends up calling __audit_free(), the
call to audit_log_exit() is conditional on the audit context
in_syscall field being true and unless I missed something, the copied
process' audit context should not have in_syscall set to true.

-- 
paul moore
www.paul-moore.com

Re: [PATCH V4] audit: add tty field to LOGIN event

[Peter Hurley]

On 04/28/2016 12:28 PM, Richard Guy Briggs wrote:
> On 16/04/27, Peter Hurley wrote:
>> On 04/27/2016 06:31 PM, Richard Guy Briggs wrote:
>>> On 16/04/22, Peter Hurley wrote:
 On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
> The tty field was missing from AUDIT_LOGIN events.
>
> Refactor code to create a new function audit_get_tty(), using it to
> replace the call in audit_log_task_info() and to add it to
> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> audit_put_tty() alias to decrement it.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>
> V4: Add missing prototype for audit_put_tty() when audit syscall is not
> enabled (MIPS).
>
> V3: Introduce audit_put_tty() alias to decrement kref.
>
> V2: Use kref to protect tty signal struct while in use.
>
> ---
>
>  include/linux/audit.h |   24 
>  kernel/audit.c|   18 +-
>  kernel/auditsc.c  |8 ++--
>  3 files changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b40ed5d..32cdafb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -343,6 +344,23 @@ static inline unsigned int 
> audit_get_sessionid(struct task_struct *tsk)
>   return tsk->sessionid;
>  }
>  
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> + struct tty_struct *tty = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>sighand->siglock, flags);
> + if (tsk->signal)
> + tty = tty_kref_get(tsk->signal->tty);
> + spin_unlock_irqrestore(>sighand->siglock, flags);


 Not that I'm objecting because I get that you're just refactoring
 existing code, but I thought I'd point out some stuff.

 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
because if it is, this will blow up trying to dereference the
sighand_struct (ie tsk->sighand).
>>>
>>> Ok.  This logic goes back 10 years and one month less two days. (45d9bb0e)
>>>
 2. The existing usage is always tsk==current
>>>
>>> My understanding is that when it is called via:
>>>
>>> copy_process()
>>> audit_free()
>>> __audit_free()
>>> audit_log_exit()
>>> audit_log_task_info()
>>>
>>> then tsk != current.
>>
>> While it's true that tsk != current here, everything relevant to tty
>> in task_struct is the same because the nascent task is not even half-done.
>> So tsk->sighand == current->sighand, tsk->signal == current->signal etc.
> 
> I agree this is true except in the case of !CLONE_SIGHAND, if it fails
> after copy_sighand() or copy_signal() then it would be null and would
> get freed before audit_free() is called.  By the time tty gets copied
> from current in this case, it is past the point of failure in
> copy_process().

Oh, right.


>> If you're uncomfortable with pass-through execution like that, then the
>> simple solution is:
>>
>>  struct tty_struct *tty = NULL;
>>
>>  /* tsk != current when copy_process() failed */
>>  if (tsk == current)
>>  tty = get_current_tty();
>>
>> because tty_kref_put(tty) accepts NULL tty and (obviously) so does
>> tty_name(tty).
> 
> Given the circumstances above, this appears reasonable to me at first
> look.

Ok.

I'll spend more analysis time before I actually submit a patch for this.


>>>  This appears to be the only case which appears to
>>> force lugging around tsk.  This is noted in that commit referenced
>>> above.
>>>
 3. If the idea is to make this invulnerable to tsk being gone, then
the usage is unsafe anyway.


 So ultimately (but not necessarily for this patch) I'd prefer that either
 a. audit use existing tty api instead of open-coding, or
 b. add any tty api functions required.
>>>
>>> This latter option did cross my mind...
>>>
 Peter Hurley

> + return tty;
> +}
> +
> +static inline void audit_put_tty(struct tty_struct *tty)
> +{
> + tty_kref_put(tty);
> +}
> +
>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t 
> gid, umode_t mode);
>  extern void __audit_bprm(struct linux_binprm *bprm);
> @@ -500,6 +518,12 @@ static inline unsigned int 
> audit_get_sessionid(struct task_struct *tsk)
>  {
>   return -1;
>  }
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> + return NULL;
> +}
> 

Re: [PATCH V4] audit: add tty field to LOGIN event

[Peter Hurley]

On 04/28/2016 12:28 PM, Richard Guy Briggs wrote:
> On 16/04/27, Peter Hurley wrote:
>> On 04/27/2016 06:31 PM, Richard Guy Briggs wrote:
>>> On 16/04/22, Peter Hurley wrote:
 On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
> The tty field was missing from AUDIT_LOGIN events.
>
> Refactor code to create a new function audit_get_tty(), using it to
> replace the call in audit_log_task_info() and to add it to
> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> audit_put_tty() alias to decrement it.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>
> V4: Add missing prototype for audit_put_tty() when audit syscall is not
> enabled (MIPS).
>
> V3: Introduce audit_put_tty() alias to decrement kref.
>
> V2: Use kref to protect tty signal struct while in use.
>
> ---
>
>  include/linux/audit.h |   24 
>  kernel/audit.c|   18 +-
>  kernel/auditsc.c  |8 ++--
>  3 files changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b40ed5d..32cdafb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -343,6 +344,23 @@ static inline unsigned int 
> audit_get_sessionid(struct task_struct *tsk)
>   return tsk->sessionid;
>  }
>  
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> + struct tty_struct *tty = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>sighand->siglock, flags);
> + if (tsk->signal)
> + tty = tty_kref_get(tsk->signal->tty);
> + spin_unlock_irqrestore(>sighand->siglock, flags);


 Not that I'm objecting because I get that you're just refactoring
 existing code, but I thought I'd point out some stuff.

 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
because if it is, this will blow up trying to dereference the
sighand_struct (ie tsk->sighand).
>>>
>>> Ok.  This logic goes back 10 years and one month less two days. (45d9bb0e)
>>>
 2. The existing usage is always tsk==current
>>>
>>> My understanding is that when it is called via:
>>>
>>> copy_process()
>>> audit_free()
>>> __audit_free()
>>> audit_log_exit()
>>> audit_log_task_info()
>>>
>>> then tsk != current.
>>
>> While it's true that tsk != current here, everything relevant to tty
>> in task_struct is the same because the nascent task is not even half-done.
>> So tsk->sighand == current->sighand, tsk->signal == current->signal etc.
> 
> I agree this is true except in the case of !CLONE_SIGHAND, if it fails
> after copy_sighand() or copy_signal() then it would be null and would
> get freed before audit_free() is called.  By the time tty gets copied
> from current in this case, it is past the point of failure in
> copy_process().

Oh, right.


>> If you're uncomfortable with pass-through execution like that, then the
>> simple solution is:
>>
>>  struct tty_struct *tty = NULL;
>>
>>  /* tsk != current when copy_process() failed */
>>  if (tsk == current)
>>  tty = get_current_tty();
>>
>> because tty_kref_put(tty) accepts NULL tty and (obviously) so does
>> tty_name(tty).
> 
> Given the circumstances above, this appears reasonable to me at first
> look.

Ok.

I'll spend more analysis time before I actually submit a patch for this.


>>>  This appears to be the only case which appears to
>>> force lugging around tsk.  This is noted in that commit referenced
>>> above.
>>>
 3. If the idea is to make this invulnerable to tsk being gone, then
the usage is unsafe anyway.


 So ultimately (but not necessarily for this patch) I'd prefer that either
 a. audit use existing tty api instead of open-coding, or
 b. add any tty api functions required.
>>>
>>> This latter option did cross my mind...
>>>
 Peter Hurley

> + return tty;
> +}
> +
> +static inline void audit_put_tty(struct tty_struct *tty)
> +{
> + tty_kref_put(tty);
> +}
> +
>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t 
> gid, umode_t mode);
>  extern void __audit_bprm(struct linux_binprm *bprm);
> @@ -500,6 +518,12 @@ static inline unsigned int 
> audit_get_sessionid(struct task_struct *tsk)
>  {
>   return -1;
>  }
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> + return NULL;
> +}
> +static inline void 

Re: [PATCH V4] audit: add tty field to LOGIN event

[Richard Guy Briggs]

On 16/04/27, Peter Hurley wrote:
> On 04/27/2016 06:31 PM, Richard Guy Briggs wrote:
> > On 16/04/22, Peter Hurley wrote:
> >> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
> >>> The tty field was missing from AUDIT_LOGIN events.
> >>>
> >>> Refactor code to create a new function audit_get_tty(), using it to
> >>> replace the call in audit_log_task_info() and to add it to
> >>> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> >>> audit_put_tty() alias to decrement it.
> >>>
> >>> Signed-off-by: Richard Guy Briggs 
> >>> ---
> >>>
> >>> V4: Add missing prototype for audit_put_tty() when audit syscall is not
> >>> enabled (MIPS).
> >>>
> >>> V3: Introduce audit_put_tty() alias to decrement kref.
> >>>
> >>> V2: Use kref to protect tty signal struct while in use.
> >>>
> >>> ---
> >>>
> >>>  include/linux/audit.h |   24 
> >>>  kernel/audit.c|   18 +-
> >>>  kernel/auditsc.c  |8 ++--
> >>>  3 files changed, 35 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/include/linux/audit.h b/include/linux/audit.h
> >>> index b40ed5d..32cdafb 100644
> >>> --- a/include/linux/audit.h
> >>> +++ b/include/linux/audit.h
> >>> @@ -26,6 +26,7 @@
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> +#include 
> >>>  
> >>>  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >>>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> >>> @@ -343,6 +344,23 @@ static inline unsigned int 
> >>> audit_get_sessionid(struct task_struct *tsk)
> >>>   return tsk->sessionid;
> >>>  }
> >>>  
> >>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> >>> +{
> >>> + struct tty_struct *tty = NULL;
> >>> + unsigned long flags;
> >>> +
> >>> + spin_lock_irqsave(>sighand->siglock, flags);
> >>> + if (tsk->signal)
> >>> + tty = tty_kref_get(tsk->signal->tty);
> >>> + spin_unlock_irqrestore(>sighand->siglock, flags);
> >>
> >>
> >> Not that I'm objecting because I get that you're just refactoring
> >> existing code, but I thought I'd point out some stuff.
> >>
> >> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
> >>because if it is, this will blow up trying to dereference the
> >>sighand_struct (ie tsk->sighand).
> > 
> > Ok.  This logic goes back 10 years and one month less two days. (45d9bb0e)
> > 
> >> 2. The existing usage is always tsk==current
> > 
> > My understanding is that when it is called via:
> > 
> > copy_process()
> > audit_free()
> > __audit_free()
> > audit_log_exit()
> > audit_log_task_info()
> > 
> > then tsk != current.
> 
> While it's true that tsk != current here, everything relevant to tty
> in task_struct is the same because the nascent task is not even half-done.
> So tsk->sighand == current->sighand, tsk->signal == current->signal etc.

I agree this is true except in the case of !CLONE_SIGHAND, if it fails
after copy_sighand() or copy_signal() then it would be null and would
get freed before audit_free() is called.  By the time tty gets copied
from current in this case, it is past the point of failure in
copy_process().

> If you're uncomfortable with pass-through execution like that, then the
> simple solution is:
> 
>   struct tty_struct *tty = NULL;
> 
>   /* tsk != current when copy_process() failed */
>   if (tsk == current)
>   tty = get_current_tty();
> 
> because tty_kref_put(tty) accepts NULL tty and (obviously) so does
> tty_name(tty).

Given the circumstances above, this appears reasonable to me at first
look.

> Peter Hurley
> 
> >  This appears to be the only case which appears to
> > force lugging around tsk.  This is noted in that commit referenced
> > above.
> > 
> >> 3. If the idea is to make this invulnerable to tsk being gone, then
> >>the usage is unsafe anyway.
> >>
> >>
> >> So ultimately (but not necessarily for this patch) I'd prefer that either
> >> a. audit use existing tty api instead of open-coding, or
> >> b. add any tty api functions required.
> > 
> > This latter option did cross my mind...
> > 
> >> Peter Hurley
> >>
> >>> + return tty;
> >>> +}
> >>> +
> >>> +static inline void audit_put_tty(struct tty_struct *tty)
> >>> +{
> >>> + tty_kref_put(tty);
> >>> +}
> >>> +
> >>>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
> >>>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t 
> >>> gid, umode_t mode);
> >>>  extern void __audit_bprm(struct linux_binprm *bprm);
> >>> @@ -500,6 +518,12 @@ static inline unsigned int 
> >>> audit_get_sessionid(struct task_struct *tsk)
> >>>  {
> >>>   return -1;
> >>>  }
> >>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> >>> +{
> >>> + return NULL;
> >>> +}
> >>> +static inline void audit_put_tty(struct tty_struct *tty)
> >>> +{ }
> >>>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >>>  { }
> 

Re: [PATCH V4] audit: add tty field to LOGIN event

[Richard Guy Briggs]

On 16/04/27, Peter Hurley wrote:
> On 04/27/2016 06:31 PM, Richard Guy Briggs wrote:
> > On 16/04/22, Peter Hurley wrote:
> >> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
> >>> The tty field was missing from AUDIT_LOGIN events.
> >>>
> >>> Refactor code to create a new function audit_get_tty(), using it to
> >>> replace the call in audit_log_task_info() and to add it to
> >>> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> >>> audit_put_tty() alias to decrement it.
> >>>
> >>> Signed-off-by: Richard Guy Briggs 
> >>> ---
> >>>
> >>> V4: Add missing prototype for audit_put_tty() when audit syscall is not
> >>> enabled (MIPS).
> >>>
> >>> V3: Introduce audit_put_tty() alias to decrement kref.
> >>>
> >>> V2: Use kref to protect tty signal struct while in use.
> >>>
> >>> ---
> >>>
> >>>  include/linux/audit.h |   24 
> >>>  kernel/audit.c|   18 +-
> >>>  kernel/auditsc.c  |8 ++--
> >>>  3 files changed, 35 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/include/linux/audit.h b/include/linux/audit.h
> >>> index b40ed5d..32cdafb 100644
> >>> --- a/include/linux/audit.h
> >>> +++ b/include/linux/audit.h
> >>> @@ -26,6 +26,7 @@
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> +#include 
> >>>  
> >>>  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >>>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> >>> @@ -343,6 +344,23 @@ static inline unsigned int 
> >>> audit_get_sessionid(struct task_struct *tsk)
> >>>   return tsk->sessionid;
> >>>  }
> >>>  
> >>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> >>> +{
> >>> + struct tty_struct *tty = NULL;
> >>> + unsigned long flags;
> >>> +
> >>> + spin_lock_irqsave(>sighand->siglock, flags);
> >>> + if (tsk->signal)
> >>> + tty = tty_kref_get(tsk->signal->tty);
> >>> + spin_unlock_irqrestore(>sighand->siglock, flags);
> >>
> >>
> >> Not that I'm objecting because I get that you're just refactoring
> >> existing code, but I thought I'd point out some stuff.
> >>
> >> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
> >>because if it is, this will blow up trying to dereference the
> >>sighand_struct (ie tsk->sighand).
> > 
> > Ok.  This logic goes back 10 years and one month less two days. (45d9bb0e)
> > 
> >> 2. The existing usage is always tsk==current
> > 
> > My understanding is that when it is called via:
> > 
> > copy_process()
> > audit_free()
> > __audit_free()
> > audit_log_exit()
> > audit_log_task_info()
> > 
> > then tsk != current.
> 
> While it's true that tsk != current here, everything relevant to tty
> in task_struct is the same because the nascent task is not even half-done.
> So tsk->sighand == current->sighand, tsk->signal == current->signal etc.

I agree this is true except in the case of !CLONE_SIGHAND, if it fails
after copy_sighand() or copy_signal() then it would be null and would
get freed before audit_free() is called.  By the time tty gets copied
from current in this case, it is past the point of failure in
copy_process().

> If you're uncomfortable with pass-through execution like that, then the
> simple solution is:
> 
>   struct tty_struct *tty = NULL;
> 
>   /* tsk != current when copy_process() failed */
>   if (tsk == current)
>   tty = get_current_tty();
> 
> because tty_kref_put(tty) accepts NULL tty and (obviously) so does
> tty_name(tty).

Given the circumstances above, this appears reasonable to me at first
look.

> Peter Hurley
> 
> >  This appears to be the only case which appears to
> > force lugging around tsk.  This is noted in that commit referenced
> > above.
> > 
> >> 3. If the idea is to make this invulnerable to tsk being gone, then
> >>the usage is unsafe anyway.
> >>
> >>
> >> So ultimately (but not necessarily for this patch) I'd prefer that either
> >> a. audit use existing tty api instead of open-coding, or
> >> b. add any tty api functions required.
> > 
> > This latter option did cross my mind...
> > 
> >> Peter Hurley
> >>
> >>> + return tty;
> >>> +}
> >>> +
> >>> +static inline void audit_put_tty(struct tty_struct *tty)
> >>> +{
> >>> + tty_kref_put(tty);
> >>> +}
> >>> +
> >>>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
> >>>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t 
> >>> gid, umode_t mode);
> >>>  extern void __audit_bprm(struct linux_binprm *bprm);
> >>> @@ -500,6 +518,12 @@ static inline unsigned int 
> >>> audit_get_sessionid(struct task_struct *tsk)
> >>>  {
> >>>   return -1;
> >>>  }
> >>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> >>> +{
> >>> + return NULL;
> >>> +}
> >>> +static inline void audit_put_tty(struct tty_struct *tty)
> >>> +{ }
> >>>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >>>  { }
> >>>  static 

Re: [PATCH V4] audit: add tty field to LOGIN event

[Peter Hurley]

On 04/27/2016 06:31 PM, Richard Guy Briggs wrote:
> On 16/04/22, Peter Hurley wrote:
>> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
>>> The tty field was missing from AUDIT_LOGIN events.
>>>
>>> Refactor code to create a new function audit_get_tty(), using it to
>>> replace the call in audit_log_task_info() and to add it to
>>> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
>>> audit_put_tty() alias to decrement it.
>>>
>>> Signed-off-by: Richard Guy Briggs 
>>> ---
>>>
>>> V4: Add missing prototype for audit_put_tty() when audit syscall is not
>>> enabled (MIPS).
>>>
>>> V3: Introduce audit_put_tty() alias to decrement kref.
>>>
>>> V2: Use kref to protect tty signal struct while in use.
>>>
>>> ---
>>>
>>>  include/linux/audit.h |   24 
>>>  kernel/audit.c|   18 +-
>>>  kernel/auditsc.c  |8 ++--
>>>  3 files changed, 35 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>>> index b40ed5d..32cdafb 100644
>>> --- a/include/linux/audit.h
>>> +++ b/include/linux/audit.h
>>> @@ -26,6 +26,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  
>>>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>>>  #define AUDIT_DEV_UNSET ((dev_t)-1)
>>> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
>>> task_struct *tsk)
>>> return tsk->sessionid;
>>>  }
>>>  
>>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>>> +{
>>> +   struct tty_struct *tty = NULL;
>>> +   unsigned long flags;
>>> +
>>> +   spin_lock_irqsave(>sighand->siglock, flags);
>>> +   if (tsk->signal)
>>> +   tty = tty_kref_get(tsk->signal->tty);
>>> +   spin_unlock_irqrestore(>sighand->siglock, flags);
>>
>>
>> Not that I'm objecting because I get that you're just refactoring
>> existing code, but I thought I'd point out some stuff.
>>
>> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
>>because if it is, this will blow up trying to dereference the
>>sighand_struct (ie tsk->sighand).
> 
> Ok.  This logic goes back 10 years and one month less two days. (45d9bb0e)
> 
>> 2. The existing usage is always tsk==current
> 
> My understanding is that when it is called via:
> 
>   copy_process()
>   audit_free()
>   __audit_free()
>   audit_log_exit()
>   audit_log_task_info()
> 
> then tsk != current.

While it's true that tsk != current here, everything relevant to tty
in task_struct is the same because the nascent task is not even half-done.
So tsk->sighand == current->sighand, tsk->signal == current->signal etc.

If you're uncomfortable with pass-through execution like that, then the
simple solution is:


struct tty_struct *tty = NULL;

/* tsk != current when copy_process() failed */
if (tsk == current)
tty = get_current_tty();


because tty_kref_put(tty) accepts NULL tty and (obviously) so does
tty_name(tty).


Regards,
Peter Hurley


>  This appears to be the only case which appears to
> force lugging around tsk.  This is noted in that commit referenced
> above.
> 
>> 3. If the idea is to make this invulnerable to tsk being gone, then
>>the usage is unsafe anyway.
>>
>>
>> So ultimately (but not necessarily for this patch) I'd prefer that either
>> a. audit use existing tty api instead of open-coding, or
>> b. add any tty api functions required.
> 
> This latter option did cross my mind...
> 
>> Peter Hurley
>>
>>> +   return tty;
>>> +}
>>> +
>>> +static inline void audit_put_tty(struct tty_struct *tty)
>>> +{
>>> +   tty_kref_put(tty);
>>> +}
>>> +
>>>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>>>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t 
>>> gid, umode_t mode);
>>>  extern void __audit_bprm(struct linux_binprm *bprm);
>>> @@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct 
>>> task_struct *tsk)
>>>  {
>>> return -1;
>>>  }
>>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>>> +{
>>> +   return NULL;
>>> +}
>>> +static inline void audit_put_tty(struct tty_struct *tty)
>>> +{ }
>>>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>>>  { }
>>>  static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
>>> diff --git a/kernel/audit.c b/kernel/audit.c
>>> index 3a3e5de..7edd776 100644
>>> --- a/kernel/audit.c
>>> +++ b/kernel/audit.c
>>> @@ -64,7 +64,6 @@
>>>  #include 
>>>  #endif
>>>  #include 
>>> -#include 
>>>  #include 
>>>  #include 
>>>  
>>> @@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, 
>>> struct task_struct *tsk)
>>>  {
>>> const struct cred *cred;
>>> char comm[sizeof(tsk->comm)];
>>> -   char *tty;
>>> +   struct tty_struct *tty;
>>>  
>>> if (!ab)
>>> return;
>>>  

Re: [PATCH V4] audit: add tty field to LOGIN event

[Peter Hurley]

On 04/27/2016 06:31 PM, Richard Guy Briggs wrote:
> On 16/04/22, Peter Hurley wrote:
>> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
>>> The tty field was missing from AUDIT_LOGIN events.
>>>
>>> Refactor code to create a new function audit_get_tty(), using it to
>>> replace the call in audit_log_task_info() and to add it to
>>> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
>>> audit_put_tty() alias to decrement it.
>>>
>>> Signed-off-by: Richard Guy Briggs 
>>> ---
>>>
>>> V4: Add missing prototype for audit_put_tty() when audit syscall is not
>>> enabled (MIPS).
>>>
>>> V3: Introduce audit_put_tty() alias to decrement kref.
>>>
>>> V2: Use kref to protect tty signal struct while in use.
>>>
>>> ---
>>>
>>>  include/linux/audit.h |   24 
>>>  kernel/audit.c|   18 +-
>>>  kernel/auditsc.c  |8 ++--
>>>  3 files changed, 35 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>>> index b40ed5d..32cdafb 100644
>>> --- a/include/linux/audit.h
>>> +++ b/include/linux/audit.h
>>> @@ -26,6 +26,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  
>>>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>>>  #define AUDIT_DEV_UNSET ((dev_t)-1)
>>> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
>>> task_struct *tsk)
>>> return tsk->sessionid;
>>>  }
>>>  
>>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>>> +{
>>> +   struct tty_struct *tty = NULL;
>>> +   unsigned long flags;
>>> +
>>> +   spin_lock_irqsave(>sighand->siglock, flags);
>>> +   if (tsk->signal)
>>> +   tty = tty_kref_get(tsk->signal->tty);
>>> +   spin_unlock_irqrestore(>sighand->siglock, flags);
>>
>>
>> Not that I'm objecting because I get that you're just refactoring
>> existing code, but I thought I'd point out some stuff.
>>
>> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
>>because if it is, this will blow up trying to dereference the
>>sighand_struct (ie tsk->sighand).
> 
> Ok.  This logic goes back 10 years and one month less two days. (45d9bb0e)
> 
>> 2. The existing usage is always tsk==current
> 
> My understanding is that when it is called via:
> 
>   copy_process()
>   audit_free()
>   __audit_free()
>   audit_log_exit()
>   audit_log_task_info()
> 
> then tsk != current.

While it's true that tsk != current here, everything relevant to tty
in task_struct is the same because the nascent task is not even half-done.
So tsk->sighand == current->sighand, tsk->signal == current->signal etc.

If you're uncomfortable with pass-through execution like that, then the
simple solution is:


struct tty_struct *tty = NULL;

/* tsk != current when copy_process() failed */
if (tsk == current)
tty = get_current_tty();


because tty_kref_put(tty) accepts NULL tty and (obviously) so does
tty_name(tty).


Regards,
Peter Hurley


>  This appears to be the only case which appears to
> force lugging around tsk.  This is noted in that commit referenced
> above.
> 
>> 3. If the idea is to make this invulnerable to tsk being gone, then
>>the usage is unsafe anyway.
>>
>>
>> So ultimately (but not necessarily for this patch) I'd prefer that either
>> a. audit use existing tty api instead of open-coding, or
>> b. add any tty api functions required.
> 
> This latter option did cross my mind...
> 
>> Peter Hurley
>>
>>> +   return tty;
>>> +}
>>> +
>>> +static inline void audit_put_tty(struct tty_struct *tty)
>>> +{
>>> +   tty_kref_put(tty);
>>> +}
>>> +
>>>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>>>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t 
>>> gid, umode_t mode);
>>>  extern void __audit_bprm(struct linux_binprm *bprm);
>>> @@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct 
>>> task_struct *tsk)
>>>  {
>>> return -1;
>>>  }
>>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>>> +{
>>> +   return NULL;
>>> +}
>>> +static inline void audit_put_tty(struct tty_struct *tty)
>>> +{ }
>>>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>>>  { }
>>>  static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
>>> diff --git a/kernel/audit.c b/kernel/audit.c
>>> index 3a3e5de..7edd776 100644
>>> --- a/kernel/audit.c
>>> +++ b/kernel/audit.c
>>> @@ -64,7 +64,6 @@
>>>  #include 
>>>  #endif
>>>  #include 
>>> -#include 
>>>  #include 
>>>  #include 
>>>  
>>> @@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, 
>>> struct task_struct *tsk)
>>>  {
>>> const struct cred *cred;
>>> char comm[sizeof(tsk->comm)];
>>> -   char *tty;
>>> +   struct tty_struct *tty;
>>>  
>>> if (!ab)
>>> return;
>>>  
>>> /* tsk == 

Re: [PATCH V4] audit: add tty field to LOGIN event

[Richard Guy Briggs]

On 16/04/22, Peter Hurley wrote:
> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
> > The tty field was missing from AUDIT_LOGIN events.
> > 
> > Refactor code to create a new function audit_get_tty(), using it to
> > replace the call in audit_log_task_info() and to add it to
> > audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> > audit_put_tty() alias to decrement it.
> > 
> > Signed-off-by: Richard Guy Briggs 
> > ---
> > 
> > V4: Add missing prototype for audit_put_tty() when audit syscall is not
> > enabled (MIPS).
> > 
> > V3: Introduce audit_put_tty() alias to decrement kref.
> > 
> > V2: Use kref to protect tty signal struct while in use.
> > 
> > ---
> > 
> >  include/linux/audit.h |   24 
> >  kernel/audit.c|   18 +-
> >  kernel/auditsc.c  |8 ++--
> >  3 files changed, 35 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index b40ed5d..32cdafb 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -26,6 +26,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
> > task_struct *tsk)
> > return tsk->sessionid;
> >  }
> >  
> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> > +{
> > +   struct tty_struct *tty = NULL;
> > +   unsigned long flags;
> > +
> > +   spin_lock_irqsave(>sighand->siglock, flags);
> > +   if (tsk->signal)
> > +   tty = tty_kref_get(tsk->signal->tty);
> > +   spin_unlock_irqrestore(>sighand->siglock, flags);
> 
> 
> Not that I'm objecting because I get that you're just refactoring
> existing code, but I thought I'd point out some stuff.
> 
> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
>because if it is, this will blow up trying to dereference the
>sighand_struct (ie tsk->sighand).

Ok.  This logic goes back 10 years and one month less two days. (45d9bb0e)

> 2. The existing usage is always tsk==current

My understanding is that when it is called via:

copy_process()
audit_free()
__audit_free()
audit_log_exit()
audit_log_task_info()

then tsk != current.  This appears to be the only case which appears to
force lugging around tsk.  This is noted in that commit referenced
above.

> 3. If the idea is to make this invulnerable to tsk being gone, then
>the usage is unsafe anyway.
> 
> 
> So ultimately (but not necessarily for this patch) I'd prefer that either
> a. audit use existing tty api instead of open-coding, or
> b. add any tty api functions required.

This latter option did cross my mind...

> Peter Hurley
> 
> > +   return tty;
> > +}
> > +
> > +static inline void audit_put_tty(struct tty_struct *tty)
> > +{
> > +   tty_kref_put(tty);
> > +}
> > +
> >  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
> >  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t 
> > gid, umode_t mode);
> >  extern void __audit_bprm(struct linux_binprm *bprm);
> > @@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct 
> > task_struct *tsk)
> >  {
> > return -1;
> >  }
> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> > +{
> > +   return NULL;
> > +}
> > +static inline void audit_put_tty(struct tty_struct *tty)
> > +{ }
> >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >  { }
> >  static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 3a3e5de..7edd776 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -64,7 +64,6 @@
> >  #include 
> >  #endif
> >  #include 
> > -#include 
> >  #include 
> >  #include 
> >  
> > @@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, 
> > struct task_struct *tsk)
> >  {
> > const struct cred *cred;
> > char comm[sizeof(tsk->comm)];
> > -   char *tty;
> > +   struct tty_struct *tty;
> >  
> > if (!ab)
> > return;
> >  
> > /* tsk == current */
> > cred = current_cred();
> > -
> > -   spin_lock_irq(>sighand->siglock);
> > -   if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name)
> > -   tty = tsk->signal->tty->name;
> > -   else
> > -   tty = "(none)";
> > -   spin_unlock_irq(>sighand->siglock);
> > -
> > +   tty = audit_get_tty(tsk);
> > audit_log_format(ab,
> >  " ppid=%d pid=%d auid=%u uid=%u gid=%u"
> >  " euid=%u suid=%u fsuid=%u"
> > @@ -1903,11 +1895,11 @@ void audit_log_task_info(struct audit_buffer *ab, 
> > struct task_struct *tsk)
> >  from_kgid(_user_ns, cred->egid),
> >   

Re: [PATCH V4] audit: add tty field to LOGIN event

[Richard Guy Briggs]

On 16/04/22, Peter Hurley wrote:
> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
> > The tty field was missing from AUDIT_LOGIN events.
> > 
> > Refactor code to create a new function audit_get_tty(), using it to
> > replace the call in audit_log_task_info() and to add it to
> > audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> > audit_put_tty() alias to decrement it.
> > 
> > Signed-off-by: Richard Guy Briggs 
> > ---
> > 
> > V4: Add missing prototype for audit_put_tty() when audit syscall is not
> > enabled (MIPS).
> > 
> > V3: Introduce audit_put_tty() alias to decrement kref.
> > 
> > V2: Use kref to protect tty signal struct while in use.
> > 
> > ---
> > 
> >  include/linux/audit.h |   24 
> >  kernel/audit.c|   18 +-
> >  kernel/auditsc.c  |8 ++--
> >  3 files changed, 35 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index b40ed5d..32cdafb 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -26,6 +26,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
> > task_struct *tsk)
> > return tsk->sessionid;
> >  }
> >  
> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> > +{
> > +   struct tty_struct *tty = NULL;
> > +   unsigned long flags;
> > +
> > +   spin_lock_irqsave(>sighand->siglock, flags);
> > +   if (tsk->signal)
> > +   tty = tty_kref_get(tsk->signal->tty);
> > +   spin_unlock_irqrestore(>sighand->siglock, flags);
> 
> 
> Not that I'm objecting because I get that you're just refactoring
> existing code, but I thought I'd point out some stuff.
> 
> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
>because if it is, this will blow up trying to dereference the
>sighand_struct (ie tsk->sighand).

Ok.  This logic goes back 10 years and one month less two days. (45d9bb0e)

> 2. The existing usage is always tsk==current

My understanding is that when it is called via:

copy_process()
audit_free()
__audit_free()
audit_log_exit()
audit_log_task_info()

then tsk != current.  This appears to be the only case which appears to
force lugging around tsk.  This is noted in that commit referenced
above.

> 3. If the idea is to make this invulnerable to tsk being gone, then
>the usage is unsafe anyway.
> 
> 
> So ultimately (but not necessarily for this patch) I'd prefer that either
> a. audit use existing tty api instead of open-coding, or
> b. add any tty api functions required.

This latter option did cross my mind...

> Peter Hurley
> 
> > +   return tty;
> > +}
> > +
> > +static inline void audit_put_tty(struct tty_struct *tty)
> > +{
> > +   tty_kref_put(tty);
> > +}
> > +
> >  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
> >  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t 
> > gid, umode_t mode);
> >  extern void __audit_bprm(struct linux_binprm *bprm);
> > @@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct 
> > task_struct *tsk)
> >  {
> > return -1;
> >  }
> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> > +{
> > +   return NULL;
> > +}
> > +static inline void audit_put_tty(struct tty_struct *tty)
> > +{ }
> >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >  { }
> >  static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 3a3e5de..7edd776 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -64,7 +64,6 @@
> >  #include 
> >  #endif
> >  #include 
> > -#include 
> >  #include 
> >  #include 
> >  
> > @@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, 
> > struct task_struct *tsk)
> >  {
> > const struct cred *cred;
> > char comm[sizeof(tsk->comm)];
> > -   char *tty;
> > +   struct tty_struct *tty;
> >  
> > if (!ab)
> > return;
> >  
> > /* tsk == current */
> > cred = current_cred();
> > -
> > -   spin_lock_irq(>sighand->siglock);
> > -   if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name)
> > -   tty = tsk->signal->tty->name;
> > -   else
> > -   tty = "(none)";
> > -   spin_unlock_irq(>sighand->siglock);
> > -
> > +   tty = audit_get_tty(tsk);
> > audit_log_format(ab,
> >  " ppid=%d pid=%d auid=%u uid=%u gid=%u"
> >  " euid=%u suid=%u fsuid=%u"
> > @@ -1903,11 +1895,11 @@ void audit_log_task_info(struct audit_buffer *ab, 
> > struct task_struct *tsk)
> >  from_kgid(_user_ns, cred->egid),
> >  

Re: [PATCH V4] audit: add tty field to LOGIN event

[Peter Hurley]

On 04/26/2016 03:34 PM, Paul Moore wrote:
> On Fri, Apr 22, 2016 at 1:16 PM, Peter Hurley  
> wrote:
>> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
>>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>>> index b40ed5d..32cdafb 100644
>>> --- a/include/linux/audit.h
>>> +++ b/include/linux/audit.h
>>> @@ -26,6 +26,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>>>  #define AUDIT_DEV_UNSET ((dev_t)-1)
>>> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
>>> task_struct *tsk)
>>>   return tsk->sessionid;
>>>  }
>>>
>>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>>> +{
>>> + struct tty_struct *tty = NULL;
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(>sighand->siglock, flags);
>>> + if (tsk->signal)
>>> + tty = tty_kref_get(tsk->signal->tty);
>>> + spin_unlock_irqrestore(>sighand->siglock, flags);
> 
> I just merged Richard's patch, if nothing else it is better than it
> was.  However, I would like to talk about improving things, see below.
> 
>> Not that I'm objecting because I get that you're just refactoring
>> existing code, but I thought I'd point out some stuff.
>>
>> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
>>because if it is, this will blow up trying to dereference the
>>sighand_struct (ie tsk->sighand).
>>
>> 2. The existing usage is always tsk==current
> 
> Yep, there is only one caller I found that even works on task_structs
> other than current (see audit_log_exit() via audit_free()), although
> even then when it ends up calling into audit_log_task_info() tsk
> should always be current.
> 
> I've got a patch compiling now to get rid of passing around current as
> a a task_struct argument, assuming nothing blows up in testing I'll
> post/merge it.
> 
>> 3. If the idea is to make this invulnerable to tsk being gone, then
>>the usage is unsafe anyway.
> 
> I don't think that is our concern here.
> 
>> So ultimately (but not necessarily for this patch) I'd prefer that either
>> a. audit use existing tty api instead of open-coding, or
>> b. add any tty api functions required.
> 
> I'm open to suggestions, care to elaborate on either option?

So b) is only necessary if the answer to 3) was yes or if tsk != current.
Otherwise, the new audit_get_tty() is equivalent to get_current_tty()
which is the exported tty core interface for the identical operation.

I was only suggesting b) if get_current_tty() wasn't going to be
sufficient.

> Feel free to elaborate by patch too ;)

I can do that.

Regards,
Peter Hurley

Re: [PATCH V4] audit: add tty field to LOGIN event

[Peter Hurley]

On 04/26/2016 03:34 PM, Paul Moore wrote:
> On Fri, Apr 22, 2016 at 1:16 PM, Peter Hurley  
> wrote:
>> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
>>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>>> index b40ed5d..32cdafb 100644
>>> --- a/include/linux/audit.h
>>> +++ b/include/linux/audit.h
>>> @@ -26,6 +26,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>>>  #define AUDIT_DEV_UNSET ((dev_t)-1)
>>> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
>>> task_struct *tsk)
>>>   return tsk->sessionid;
>>>  }
>>>
>>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>>> +{
>>> + struct tty_struct *tty = NULL;
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(>sighand->siglock, flags);
>>> + if (tsk->signal)
>>> + tty = tty_kref_get(tsk->signal->tty);
>>> + spin_unlock_irqrestore(>sighand->siglock, flags);
> 
> I just merged Richard's patch, if nothing else it is better than it
> was.  However, I would like to talk about improving things, see below.
> 
>> Not that I'm objecting because I get that you're just refactoring
>> existing code, but I thought I'd point out some stuff.
>>
>> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
>>because if it is, this will blow up trying to dereference the
>>sighand_struct (ie tsk->sighand).
>>
>> 2. The existing usage is always tsk==current
> 
> Yep, there is only one caller I found that even works on task_structs
> other than current (see audit_log_exit() via audit_free()), although
> even then when it ends up calling into audit_log_task_info() tsk
> should always be current.
> 
> I've got a patch compiling now to get rid of passing around current as
> a a task_struct argument, assuming nothing blows up in testing I'll
> post/merge it.
> 
>> 3. If the idea is to make this invulnerable to tsk being gone, then
>>the usage is unsafe anyway.
> 
> I don't think that is our concern here.
> 
>> So ultimately (but not necessarily for this patch) I'd prefer that either
>> a. audit use existing tty api instead of open-coding, or
>> b. add any tty api functions required.
> 
> I'm open to suggestions, care to elaborate on either option?

So b) is only necessary if the answer to 3) was yes or if tsk != current.
Otherwise, the new audit_get_tty() is equivalent to get_current_tty()
which is the exported tty core interface for the identical operation.

I was only suggesting b) if get_current_tty() wasn't going to be
sufficient.

> Feel free to elaborate by patch too ;)

I can do that.

Regards,
Peter Hurley

Re: [PATCH V4] audit: add tty field to LOGIN event

[Paul Moore]

On Fri, Apr 22, 2016 at 1:16 PM, Peter Hurley  wrote:
> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index b40ed5d..32cdafb 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -26,6 +26,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>>  #define AUDIT_DEV_UNSET ((dev_t)-1)
>> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
>> task_struct *tsk)
>>   return tsk->sessionid;
>>  }
>>
>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>> +{
>> + struct tty_struct *tty = NULL;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(>sighand->siglock, flags);
>> + if (tsk->signal)
>> + tty = tty_kref_get(tsk->signal->tty);
>> + spin_unlock_irqrestore(>sighand->siglock, flags);

I just merged Richard's patch, if nothing else it is better than it
was.  However, I would like to talk about improving things, see below.

> Not that I'm objecting because I get that you're just refactoring
> existing code, but I thought I'd point out some stuff.
>
> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
>because if it is, this will blow up trying to dereference the
>sighand_struct (ie tsk->sighand).
>
> 2. The existing usage is always tsk==current

Yep, there is only one caller I found that even works on task_structs
other than current (see audit_log_exit() via audit_free()), although
even then when it ends up calling into audit_log_task_info() tsk
should always be current.

I've got a patch compiling now to get rid of passing around current as
a a task_struct argument, assuming nothing blows up in testing I'll
post/merge it.

> 3. If the idea is to make this invulnerable to tsk being gone, then
>the usage is unsafe anyway.

I don't think that is our concern here.

> So ultimately (but not necessarily for this patch) I'd prefer that either
> a. audit use existing tty api instead of open-coding, or
> b. add any tty api functions required.

I'm open to suggestions, care to elaborate on either option?  Feel
free to elaborate by patch too ;)

-- 
paul moore
www.paul-moore.com

Re: [PATCH V4] audit: add tty field to LOGIN event

[Paul Moore]

On Fri, Apr 22, 2016 at 1:16 PM, Peter Hurley  wrote:
> On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index b40ed5d..32cdafb 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -26,6 +26,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>>  #define AUDIT_DEV_UNSET ((dev_t)-1)
>> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
>> task_struct *tsk)
>>   return tsk->sessionid;
>>  }
>>
>> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>> +{
>> + struct tty_struct *tty = NULL;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(>sighand->siglock, flags);
>> + if (tsk->signal)
>> + tty = tty_kref_get(tsk->signal->tty);
>> + spin_unlock_irqrestore(>sighand->siglock, flags);

I just merged Richard's patch, if nothing else it is better than it
was.  However, I would like to talk about improving things, see below.

> Not that I'm objecting because I get that you're just refactoring
> existing code, but I thought I'd point out some stuff.
>
> 1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
>because if it is, this will blow up trying to dereference the
>sighand_struct (ie tsk->sighand).
>
> 2. The existing usage is always tsk==current

Yep, there is only one caller I found that even works on task_structs
other than current (see audit_log_exit() via audit_free()), although
even then when it ends up calling into audit_log_task_info() tsk
should always be current.

I've got a patch compiling now to get rid of passing around current as
a a task_struct argument, assuming nothing blows up in testing I'll
post/merge it.

> 3. If the idea is to make this invulnerable to tsk being gone, then
>the usage is unsafe anyway.

I don't think that is our concern here.

> So ultimately (but not necessarily for this patch) I'd prefer that either
> a. audit use existing tty api instead of open-coding, or
> b. add any tty api functions required.

I'm open to suggestions, care to elaborate on either option?  Feel
free to elaborate by patch too ;)

-- 
paul moore
www.paul-moore.com

Re: [PATCH V4] audit: add tty field to LOGIN event

[Peter Hurley]

On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
> The tty field was missing from AUDIT_LOGIN events.
> 
> Refactor code to create a new function audit_get_tty(), using it to
> replace the call in audit_log_task_info() and to add it to
> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> audit_put_tty() alias to decrement it.
> 
> Signed-off-by: Richard Guy Briggs 
> ---
> 
> V4: Add missing prototype for audit_put_tty() when audit syscall is not
> enabled (MIPS).
> 
> V3: Introduce audit_put_tty() alias to decrement kref.
> 
> V2: Use kref to protect tty signal struct while in use.
> 
> ---
> 
>  include/linux/audit.h |   24 
>  kernel/audit.c|   18 +-
>  kernel/auditsc.c  |8 ++--
>  3 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b40ed5d..32cdafb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
> task_struct *tsk)
>   return tsk->sessionid;
>  }
>  
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> + struct tty_struct *tty = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>sighand->siglock, flags);
> + if (tsk->signal)
> + tty = tty_kref_get(tsk->signal->tty);
> + spin_unlock_irqrestore(>sighand->siglock, flags);


Not that I'm objecting because I get that you're just refactoring
existing code, but I thought I'd point out some stuff.

1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
   because if it is, this will blow up trying to dereference the
   sighand_struct (ie tsk->sighand).

2. The existing usage is always tsk==current

3. If the idea is to make this invulnerable to tsk being gone, then
   the usage is unsafe anyway.


So ultimately (but not necessarily for this patch) I'd prefer that either
a. audit use existing tty api instead of open-coding, or
b. add any tty api functions required.


Regards,
Peter Hurley


> + return tty;
> +}
> +
> +static inline void audit_put_tty(struct tty_struct *tty)
> +{
> + tty_kref_put(tty);
> +}
> +
>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, 
> umode_t mode);
>  extern void __audit_bprm(struct linux_binprm *bprm);
> @@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct 
> task_struct *tsk)
>  {
>   return -1;
>  }
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> + return NULL;
> +}
> +static inline void audit_put_tty(struct tty_struct *tty)
> +{ }
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  { }
>  static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3a3e5de..7edd776 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -64,7 +64,6 @@
>  #include 
>  #endif
>  #include 
> -#include 
>  #include 
>  #include 
>  
> @@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, 
> struct task_struct *tsk)
>  {
>   const struct cred *cred;
>   char comm[sizeof(tsk->comm)];
> - char *tty;
> + struct tty_struct *tty;
>  
>   if (!ab)
>   return;
>  
>   /* tsk == current */
>   cred = current_cred();
> -
> - spin_lock_irq(>sighand->siglock);
> - if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name)
> - tty = tsk->signal->tty->name;
> - else
> - tty = "(none)";
> - spin_unlock_irq(>sighand->siglock);
> -
> + tty = audit_get_tty(tsk);
>   audit_log_format(ab,
>" ppid=%d pid=%d auid=%u uid=%u gid=%u"
>" euid=%u suid=%u fsuid=%u"
> @@ -1903,11 +1895,11 @@ void audit_log_task_info(struct audit_buffer *ab, 
> struct task_struct *tsk)
>from_kgid(_user_ns, cred->egid),
>from_kgid(_user_ns, cred->sgid),
>from_kgid(_user_ns, cred->fsgid),
> -  tty, audit_get_sessionid(tsk));
> -
> +  tty ? tty_name(tty) : "(none)",
> +  audit_get_sessionid(tsk));
> + audit_put_tty(tty);
>   audit_log_format(ab, " comm=");
>   audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
> -
>   audit_log_d_path_exe(ab, tsk->mm);
>   audit_log_task_context(ab);
>  }
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 195ffae..71e14d8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, 
> kuid_t kloginuid,
>  {
>   

Re: [PATCH V4] audit: add tty field to LOGIN event

[Peter Hurley]

On 04/21/2016 11:14 AM, Richard Guy Briggs wrote:
> The tty field was missing from AUDIT_LOGIN events.
> 
> Refactor code to create a new function audit_get_tty(), using it to
> replace the call in audit_log_task_info() and to add it to
> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> audit_put_tty() alias to decrement it.
> 
> Signed-off-by: Richard Guy Briggs 
> ---
> 
> V4: Add missing prototype for audit_put_tty() when audit syscall is not
> enabled (MIPS).
> 
> V3: Introduce audit_put_tty() alias to decrement kref.
> 
> V2: Use kref to protect tty signal struct while in use.
> 
> ---
> 
>  include/linux/audit.h |   24 
>  kernel/audit.c|   18 +-
>  kernel/auditsc.c  |8 ++--
>  3 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b40ed5d..32cdafb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
> task_struct *tsk)
>   return tsk->sessionid;
>  }
>  
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> + struct tty_struct *tty = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>sighand->siglock, flags);
> + if (tsk->signal)
> + tty = tty_kref_get(tsk->signal->tty);
> + spin_unlock_irqrestore(>sighand->siglock, flags);


Not that I'm objecting because I get that you're just refactoring
existing code, but I thought I'd point out some stuff.

1. There's no need to check if signal_struct is NULL (ie. tsk->signal)
   because if it is, this will blow up trying to dereference the
   sighand_struct (ie tsk->sighand).

2. The existing usage is always tsk==current

3. If the idea is to make this invulnerable to tsk being gone, then
   the usage is unsafe anyway.


So ultimately (but not necessarily for this patch) I'd prefer that either
a. audit use existing tty api instead of open-coding, or
b. add any tty api functions required.


Regards,
Peter Hurley


> + return tty;
> +}
> +
> +static inline void audit_put_tty(struct tty_struct *tty)
> +{
> + tty_kref_put(tty);
> +}
> +
>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, 
> umode_t mode);
>  extern void __audit_bprm(struct linux_binprm *bprm);
> @@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct 
> task_struct *tsk)
>  {
>   return -1;
>  }
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> + return NULL;
> +}
> +static inline void audit_put_tty(struct tty_struct *tty)
> +{ }
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  { }
>  static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3a3e5de..7edd776 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -64,7 +64,6 @@
>  #include 
>  #endif
>  #include 
> -#include 
>  #include 
>  #include 
>  
> @@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, 
> struct task_struct *tsk)
>  {
>   const struct cred *cred;
>   char comm[sizeof(tsk->comm)];
> - char *tty;
> + struct tty_struct *tty;
>  
>   if (!ab)
>   return;
>  
>   /* tsk == current */
>   cred = current_cred();
> -
> - spin_lock_irq(>sighand->siglock);
> - if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name)
> - tty = tsk->signal->tty->name;
> - else
> - tty = "(none)";
> - spin_unlock_irq(>sighand->siglock);
> -
> + tty = audit_get_tty(tsk);
>   audit_log_format(ab,
>" ppid=%d pid=%d auid=%u uid=%u gid=%u"
>" euid=%u suid=%u fsuid=%u"
> @@ -1903,11 +1895,11 @@ void audit_log_task_info(struct audit_buffer *ab, 
> struct task_struct *tsk)
>from_kgid(_user_ns, cred->egid),
>from_kgid(_user_ns, cred->sgid),
>from_kgid(_user_ns, cred->fsgid),
> -  tty, audit_get_sessionid(tsk));
> -
> +  tty ? tty_name(tty) : "(none)",
> +  audit_get_sessionid(tsk));
> + audit_put_tty(tty);
>   audit_log_format(ab, " comm=");
>   audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
> -
>   audit_log_d_path_exe(ab, tsk->mm);
>   audit_log_task_context(ab);
>  }
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 195ffae..71e14d8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, 
> kuid_t kloginuid,
>  {
>   struct audit_buffer 

Re: [PATCH V4] audit: add tty field to LOGIN event

[Paul Moore]

On Thu, Apr 21, 2016 at 11:50 PM, Richard Guy Briggs  wrote:
> On 16/04/21, Paul Moore wrote:
>> On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs  wrote:
>> > The tty field was missing from AUDIT_LOGIN events.
>> >
>> > Refactor code to create a new function audit_get_tty(), using it to
>> > replace the call in audit_log_task_info() and to add it to
>> > audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
>> > audit_put_tty() alias to decrement it.
>> >
>> > Signed-off-by: Richard Guy Briggs 
>> > ---
>> >
>> > V4: Add missing prototype for audit_put_tty() when audit syscall is not
>> > enabled (MIPS).
>> >
>> > V3: Introduce audit_put_tty() alias to decrement kref.
>> >
>> > V2: Use kref to protect tty signal struct while in use.
>> >
>> > ---
>> >
>> >  include/linux/audit.h |   24 
>> >  kernel/audit.c|   18 +-
>> >  kernel/auditsc.c  |8 ++--
>> >  3 files changed, 35 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/include/linux/audit.h b/include/linux/audit.h
>> > index b40ed5d..32cdafb 100644
>> > --- a/include/linux/audit.h
>> > +++ b/include/linux/audit.h
>> > @@ -26,6 +26,7 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > +#include 
>> >
>> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
>> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
>> > @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
>> > task_struct *tsk)
>> > return tsk->sessionid;
>> >  }
>> >
>> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>> > +{
>> > +   struct tty_struct *tty = NULL;
>> > +   unsigned long flags;
>> > +
>> > +   spin_lock_irqsave(>sighand->siglock, flags);
>> > +   if (tsk->signal)
>> > +   tty = tty_kref_get(tsk->signal->tty);
>> > +   spin_unlock_irqrestore(>sighand->siglock, flags);
>> > +   return tty;
>> > +}
>> > +
>> > +static inline void audit_put_tty(struct tty_struct *tty)
>> > +{
>> > +   tty_kref_put(tty);
>> > +}
>>
>> I'm generally not a big fan of defining functions as inlines in header
>> files, with the general exception of dummy functions that will get
>> compiled out.  Although I guess there might be some performance
>> argument to be made wrt audit_log_task_info().
>
> I did reflect on that initially and decided this was the least
> disruptive way to implement it.  There are others similar around it and
> when I started it wasn't as involved, but now it is starting to push the
> limits with the kref bits...

Yeah, that is why I mentioned it, it is sort of borderline in my
opinion of what I would consider acceptable in a header file, barring
some special case.

>> I guess I'm fine with this, but what was the idea behind the static
>> inlines in audit.h?  Performance, or something else?
>
> Can't say I remember...  I was tempted to put it in as a define, but
> decided to stick with the existing style, right?  :-)

No, definitely not a cpp macro, we'd lose type checking and other
stuff.  My debate is whether or not this should life fully in the
header file vs simply a prototype in the header and the definition in
a C file.  This is the first function where we are actually putting
content into linux/audit.h, all of the existing function definitions
there are dummy functions or simple wrappers for performance reasons.

>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 195ffae..71e14d8 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t 
>> > koldloginuid, kuid_t kloginuid,
>> >  {
>> > struct audit_buffer *ab;
>> > uid_t uid, oldloginuid, loginuid;
>> > +   struct tty_struct *tty;
>> >
>> > if (!audit_enabled)
>> > return;
>> > @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t 
>> > koldloginuid, kuid_t kloginuid,
>> > uid = from_kuid(_user_ns, task_uid(current));
>> > oldloginuid = from_kuid(_user_ns, koldloginuid);
>> > loginuid = from_kuid(_user_ns, kloginuid),
>> > +   tty = audit_get_tty(current);
>> >
>> > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
>> > if (!ab)
>> > return;
>> > audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
>> > audit_log_task_context(ab);
>> > -   audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u 
>> > res=%d",
>> > -oldloginuid, loginuid, oldsessionid, sessionid, 
>> > !rc);
>> > +   audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u 
>> > ses=%u res=%d",
>> > +oldloginuid, loginuid, tty ? tty_name(tty) : 
>> > "(none)",
>> > +oldsessionid, sessionid, !rc);
>> > +   audit_put_tty(tty);
>> > audit_log_end(ab);
>> >  }
>>
>> Because we are still using the crappy fixed 

Re: [PATCH V4] audit: add tty field to LOGIN event

[Paul Moore]

On Thu, Apr 21, 2016 at 11:50 PM, Richard Guy Briggs  wrote:
> On 16/04/21, Paul Moore wrote:
>> On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs  wrote:
>> > The tty field was missing from AUDIT_LOGIN events.
>> >
>> > Refactor code to create a new function audit_get_tty(), using it to
>> > replace the call in audit_log_task_info() and to add it to
>> > audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
>> > audit_put_tty() alias to decrement it.
>> >
>> > Signed-off-by: Richard Guy Briggs 
>> > ---
>> >
>> > V4: Add missing prototype for audit_put_tty() when audit syscall is not
>> > enabled (MIPS).
>> >
>> > V3: Introduce audit_put_tty() alias to decrement kref.
>> >
>> > V2: Use kref to protect tty signal struct while in use.
>> >
>> > ---
>> >
>> >  include/linux/audit.h |   24 
>> >  kernel/audit.c|   18 +-
>> >  kernel/auditsc.c  |8 ++--
>> >  3 files changed, 35 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/include/linux/audit.h b/include/linux/audit.h
>> > index b40ed5d..32cdafb 100644
>> > --- a/include/linux/audit.h
>> > +++ b/include/linux/audit.h
>> > @@ -26,6 +26,7 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > +#include 
>> >
>> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
>> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
>> > @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
>> > task_struct *tsk)
>> > return tsk->sessionid;
>> >  }
>> >
>> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
>> > +{
>> > +   struct tty_struct *tty = NULL;
>> > +   unsigned long flags;
>> > +
>> > +   spin_lock_irqsave(>sighand->siglock, flags);
>> > +   if (tsk->signal)
>> > +   tty = tty_kref_get(tsk->signal->tty);
>> > +   spin_unlock_irqrestore(>sighand->siglock, flags);
>> > +   return tty;
>> > +}
>> > +
>> > +static inline void audit_put_tty(struct tty_struct *tty)
>> > +{
>> > +   tty_kref_put(tty);
>> > +}
>>
>> I'm generally not a big fan of defining functions as inlines in header
>> files, with the general exception of dummy functions that will get
>> compiled out.  Although I guess there might be some performance
>> argument to be made wrt audit_log_task_info().
>
> I did reflect on that initially and decided this was the least
> disruptive way to implement it.  There are others similar around it and
> when I started it wasn't as involved, but now it is starting to push the
> limits with the kref bits...

Yeah, that is why I mentioned it, it is sort of borderline in my
opinion of what I would consider acceptable in a header file, barring
some special case.

>> I guess I'm fine with this, but what was the idea behind the static
>> inlines in audit.h?  Performance, or something else?
>
> Can't say I remember...  I was tempted to put it in as a define, but
> decided to stick with the existing style, right?  :-)

No, definitely not a cpp macro, we'd lose type checking and other
stuff.  My debate is whether or not this should life fully in the
header file vs simply a prototype in the header and the definition in
a C file.  This is the first function where we are actually putting
content into linux/audit.h, all of the existing function definitions
there are dummy functions or simple wrappers for performance reasons.

>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 195ffae..71e14d8 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t 
>> > koldloginuid, kuid_t kloginuid,
>> >  {
>> > struct audit_buffer *ab;
>> > uid_t uid, oldloginuid, loginuid;
>> > +   struct tty_struct *tty;
>> >
>> > if (!audit_enabled)
>> > return;
>> > @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t 
>> > koldloginuid, kuid_t kloginuid,
>> > uid = from_kuid(_user_ns, task_uid(current));
>> > oldloginuid = from_kuid(_user_ns, koldloginuid);
>> > loginuid = from_kuid(_user_ns, kloginuid),
>> > +   tty = audit_get_tty(current);
>> >
>> > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
>> > if (!ab)
>> > return;
>> > audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
>> > audit_log_task_context(ab);
>> > -   audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u 
>> > res=%d",
>> > -oldloginuid, loginuid, oldsessionid, sessionid, 
>> > !rc);
>> > +   audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u 
>> > ses=%u res=%d",
>> > +oldloginuid, loginuid, tty ? tty_name(tty) : 
>> > "(none)",
>> > +oldsessionid, sessionid, !rc);
>> > +   audit_put_tty(tty);
>> > audit_log_end(ab);
>> >  }
>>
>> Because we are still using the crappy fixed string format for
>> kernel<->userspace 

Re: [PATCH V4] audit: add tty field to LOGIN event

[Steve Grubb]

On Thursday, April 21, 2016 09:29:57 PM Paul Moore wrote:
> On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs  wrote:
> > The tty field was missing from AUDIT_LOGIN events.
> > 
> > Refactor code to create a new function audit_get_tty(), using it to
> > replace the call in audit_log_task_info() and to add it to
> > audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> > audit_put_tty() alias to decrement it.
> > 
> > Signed-off-by: Richard Guy Briggs 
> > ---
> > 
> > V4: Add missing prototype for audit_put_tty() when audit syscall is not
> > 
> > enabled (MIPS).
> > 
> > V3: Introduce audit_put_tty() alias to decrement kref.
> > 
> > V2: Use kref to protect tty signal struct while in use.
> > 
> > ---
> > 
> >  include/linux/audit.h |   24 
> >  kernel/audit.c|   18 +-
> >  kernel/auditsc.c  |8 ++--
> >  3 files changed, 35 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index b40ed5d..32cdafb 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -26,6 +26,7 @@
> > 
> >  #include 
> >  #include 
> >  #include 
> > 
> > +#include 
> > 
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > 
> > @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct
> > task_struct *tsk)> 
> > return tsk->sessionid;
> >  
> >  }
> > 
> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> > +{
> > +   struct tty_struct *tty = NULL;
> > +   unsigned long flags;
> > +
> > +   spin_lock_irqsave(>sighand->siglock, flags);
> > +   if (tsk->signal)
> > +   tty = tty_kref_get(tsk->signal->tty);
> > +   spin_unlock_irqrestore(>sighand->siglock, flags);
> > +   return tty;
> > +}
> > +
> > +static inline void audit_put_tty(struct tty_struct *tty)
> > +{
> > +   tty_kref_put(tty);
> > +}
> 
> I'm generally not a big fan of defining functions as inlines in header
> files, with the general exception of dummy functions that will get
> compiled out.  Although I guess there might be some performance
> argument to be made wrt audit_log_task_info().
> 
> I guess I'm fine with this, but what was the idea behind the static
> inlines in audit.h?  Performance, or something else?

I think that is normal to prevent multiple definitions at link time.


> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 195ffae..71e14d8 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t
> > koldloginuid, kuid_t kloginuid,> 
> >  {
> >  
> > struct audit_buffer *ab;
> > uid_t uid, oldloginuid, loginuid;
> > 
> > +   struct tty_struct *tty;
> > 
> > if (!audit_enabled)
> > 
> > return;
> > 
> > @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t
> > koldloginuid, kuid_t kloginuid,> 
> > uid = from_kuid(_user_ns, task_uid(current));
> > oldloginuid = from_kuid(_user_ns, koldloginuid);
> > loginuid = from_kuid(_user_ns, kloginuid),
> > 
> > +   tty = audit_get_tty(current);
> > 
> > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
> > if (!ab)
> > 
> > return;
> > 
> > audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
> > audit_log_task_context(ab);
> > 
> > -   audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u
> > res=%d", -oldloginuid, loginuid, oldsessionid,
> > sessionid, !rc); +   audit_log_format(ab, " old-auid=%u auid=%u
> > tty=%s old-ses=%u ses=%u res=%d", +oldloginuid,
> > loginuid, tty ? tty_name(tty) : "(none)", +   
> > oldsessionid, sessionid, !rc);
> > +   audit_put_tty(tty);
> > 
> > audit_log_end(ab);
> >  
> >  }
> 
> Because we are still using the crappy fixed string format for
> kernel<->userspace communication, this patch will likely "break the
> world" ... let's check with Steve but I believe the way to handle this
> is to add the tty information to the end of the record.

The placement is OK. Thanks for asking.

-Steve

Re: [PATCH V4] audit: add tty field to LOGIN event

[Steve Grubb]

On Thursday, April 21, 2016 09:29:57 PM Paul Moore wrote:
> On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs  wrote:
> > The tty field was missing from AUDIT_LOGIN events.
> > 
> > Refactor code to create a new function audit_get_tty(), using it to
> > replace the call in audit_log_task_info() and to add it to
> > audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> > audit_put_tty() alias to decrement it.
> > 
> > Signed-off-by: Richard Guy Briggs 
> > ---
> > 
> > V4: Add missing prototype for audit_put_tty() when audit syscall is not
> > 
> > enabled (MIPS).
> > 
> > V3: Introduce audit_put_tty() alias to decrement kref.
> > 
> > V2: Use kref to protect tty signal struct while in use.
> > 
> > ---
> > 
> >  include/linux/audit.h |   24 
> >  kernel/audit.c|   18 +-
> >  kernel/auditsc.c  |8 ++--
> >  3 files changed, 35 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index b40ed5d..32cdafb 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -26,6 +26,7 @@
> > 
> >  #include 
> >  #include 
> >  #include 
> > 
> > +#include 
> > 
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > 
> > @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct
> > task_struct *tsk)> 
> > return tsk->sessionid;
> >  
> >  }
> > 
> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> > +{
> > +   struct tty_struct *tty = NULL;
> > +   unsigned long flags;
> > +
> > +   spin_lock_irqsave(>sighand->siglock, flags);
> > +   if (tsk->signal)
> > +   tty = tty_kref_get(tsk->signal->tty);
> > +   spin_unlock_irqrestore(>sighand->siglock, flags);
> > +   return tty;
> > +}
> > +
> > +static inline void audit_put_tty(struct tty_struct *tty)
> > +{
> > +   tty_kref_put(tty);
> > +}
> 
> I'm generally not a big fan of defining functions as inlines in header
> files, with the general exception of dummy functions that will get
> compiled out.  Although I guess there might be some performance
> argument to be made wrt audit_log_task_info().
> 
> I guess I'm fine with this, but what was the idea behind the static
> inlines in audit.h?  Performance, or something else?

I think that is normal to prevent multiple definitions at link time.


> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 195ffae..71e14d8 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t
> > koldloginuid, kuid_t kloginuid,> 
> >  {
> >  
> > struct audit_buffer *ab;
> > uid_t uid, oldloginuid, loginuid;
> > 
> > +   struct tty_struct *tty;
> > 
> > if (!audit_enabled)
> > 
> > return;
> > 
> > @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t
> > koldloginuid, kuid_t kloginuid,> 
> > uid = from_kuid(_user_ns, task_uid(current));
> > oldloginuid = from_kuid(_user_ns, koldloginuid);
> > loginuid = from_kuid(_user_ns, kloginuid),
> > 
> > +   tty = audit_get_tty(current);
> > 
> > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
> > if (!ab)
> > 
> > return;
> > 
> > audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
> > audit_log_task_context(ab);
> > 
> > -   audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u
> > res=%d", -oldloginuid, loginuid, oldsessionid,
> > sessionid, !rc); +   audit_log_format(ab, " old-auid=%u auid=%u
> > tty=%s old-ses=%u ses=%u res=%d", +oldloginuid,
> > loginuid, tty ? tty_name(tty) : "(none)", +   
> > oldsessionid, sessionid, !rc);
> > +   audit_put_tty(tty);
> > 
> > audit_log_end(ab);
> >  
> >  }
> 
> Because we are still using the crappy fixed string format for
> kernel<->userspace communication, this patch will likely "break the
> world" ... let's check with Steve but I believe the way to handle this
> is to add the tty information to the end of the record.

The placement is OK. Thanks for asking.

-Steve

Re: [PATCH V4] audit: add tty field to LOGIN event

[Richard Guy Briggs]

On 16/04/21, Paul Moore wrote:
> On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs  wrote:
> > The tty field was missing from AUDIT_LOGIN events.
> >
> > Refactor code to create a new function audit_get_tty(), using it to
> > replace the call in audit_log_task_info() and to add it to
> > audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> > audit_put_tty() alias to decrement it.
> >
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >
> > V4: Add missing prototype for audit_put_tty() when audit syscall is not
> > enabled (MIPS).
> >
> > V3: Introduce audit_put_tty() alias to decrement kref.
> >
> > V2: Use kref to protect tty signal struct while in use.
> >
> > ---
> >
> >  include/linux/audit.h |   24 
> >  kernel/audit.c|   18 +-
> >  kernel/auditsc.c  |8 ++--
> >  3 files changed, 35 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index b40ed5d..32cdafb 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -26,6 +26,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
> > task_struct *tsk)
> > return tsk->sessionid;
> >  }
> >
> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> > +{
> > +   struct tty_struct *tty = NULL;
> > +   unsigned long flags;
> > +
> > +   spin_lock_irqsave(>sighand->siglock, flags);
> > +   if (tsk->signal)
> > +   tty = tty_kref_get(tsk->signal->tty);
> > +   spin_unlock_irqrestore(>sighand->siglock, flags);
> > +   return tty;
> > +}
> > +
> > +static inline void audit_put_tty(struct tty_struct *tty)
> > +{
> > +   tty_kref_put(tty);
> > +}
> 
> I'm generally not a big fan of defining functions as inlines in header
> files, with the general exception of dummy functions that will get
> compiled out.  Although I guess there might be some performance
> argument to be made wrt audit_log_task_info().

I did reflect on that initially and decided this was the least
disruptive way to implement it.  There are others similar around it and
when I started it wasn't as involved, but now it is starting to push the
limits with the kref bits...

> I guess I'm fine with this, but what was the idea behind the static
> inlines in audit.h?  Performance, or something else?

Can't say I remember...  I was tempted to put it in as a define, but
decided to stick with the existing style, right?  :-)

> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 195ffae..71e14d8 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t 
> > koldloginuid, kuid_t kloginuid,
> >  {
> > struct audit_buffer *ab;
> > uid_t uid, oldloginuid, loginuid;
> > +   struct tty_struct *tty;
> >
> > if (!audit_enabled)
> > return;
> > @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t 
> > koldloginuid, kuid_t kloginuid,
> > uid = from_kuid(_user_ns, task_uid(current));
> > oldloginuid = from_kuid(_user_ns, koldloginuid);
> > loginuid = from_kuid(_user_ns, kloginuid),
> > +   tty = audit_get_tty(current);
> >
> > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
> > if (!ab)
> > return;
> > audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
> > audit_log_task_context(ab);
> > -   audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u 
> > res=%d",
> > -oldloginuid, loginuid, oldsessionid, sessionid, 
> > !rc);
> > +   audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u ses=%u 
> > res=%d",
> > +oldloginuid, loginuid, tty ? tty_name(tty) : 
> > "(none)",
> > +oldsessionid, sessionid, !rc);
> > +   audit_put_tty(tty);
> > audit_log_end(ab);
> >  }
> 
> Because we are still using the crappy fixed string format for
> kernel<->userspace communication, this patch will likely "break the
> world" ... let's check with Steve but I believe the way to handle this
> is to add the tty information to the end of the record.

Well, I did try to put that keyword consistently with where it was
inserted in other messages.  My understanding was that adding an extra
in the middle wouldn't cause a problem because the keyword scanner
looked ahead until it found the keyword it sought.  This way, older
scanners will just hop over this keyword it wasn't seeking.

> paul moore

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH V4] audit: add tty field to LOGIN event

[Richard Guy Briggs]

On 16/04/21, Paul Moore wrote:
> On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs  wrote:
> > The tty field was missing from AUDIT_LOGIN events.
> >
> > Refactor code to create a new function audit_get_tty(), using it to
> > replace the call in audit_log_task_info() and to add it to
> > audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> > audit_put_tty() alias to decrement it.
> >
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >
> > V4: Add missing prototype for audit_put_tty() when audit syscall is not
> > enabled (MIPS).
> >
> > V3: Introduce audit_put_tty() alias to decrement kref.
> >
> > V2: Use kref to protect tty signal struct while in use.
> >
> > ---
> >
> >  include/linux/audit.h |   24 
> >  kernel/audit.c|   18 +-
> >  kernel/auditsc.c  |8 ++--
> >  3 files changed, 35 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index b40ed5d..32cdafb 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -26,6 +26,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
> > task_struct *tsk)
> > return tsk->sessionid;
> >  }
> >
> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> > +{
> > +   struct tty_struct *tty = NULL;
> > +   unsigned long flags;
> > +
> > +   spin_lock_irqsave(>sighand->siglock, flags);
> > +   if (tsk->signal)
> > +   tty = tty_kref_get(tsk->signal->tty);
> > +   spin_unlock_irqrestore(>sighand->siglock, flags);
> > +   return tty;
> > +}
> > +
> > +static inline void audit_put_tty(struct tty_struct *tty)
> > +{
> > +   tty_kref_put(tty);
> > +}
> 
> I'm generally not a big fan of defining functions as inlines in header
> files, with the general exception of dummy functions that will get
> compiled out.  Although I guess there might be some performance
> argument to be made wrt audit_log_task_info().

I did reflect on that initially and decided this was the least
disruptive way to implement it.  There are others similar around it and
when I started it wasn't as involved, but now it is starting to push the
limits with the kref bits...

> I guess I'm fine with this, but what was the idea behind the static
> inlines in audit.h?  Performance, or something else?

Can't say I remember...  I was tempted to put it in as a define, but
decided to stick with the existing style, right?  :-)

> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 195ffae..71e14d8 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t 
> > koldloginuid, kuid_t kloginuid,
> >  {
> > struct audit_buffer *ab;
> > uid_t uid, oldloginuid, loginuid;
> > +   struct tty_struct *tty;
> >
> > if (!audit_enabled)
> > return;
> > @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t 
> > koldloginuid, kuid_t kloginuid,
> > uid = from_kuid(_user_ns, task_uid(current));
> > oldloginuid = from_kuid(_user_ns, koldloginuid);
> > loginuid = from_kuid(_user_ns, kloginuid),
> > +   tty = audit_get_tty(current);
> >
> > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
> > if (!ab)
> > return;
> > audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
> > audit_log_task_context(ab);
> > -   audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u 
> > res=%d",
> > -oldloginuid, loginuid, oldsessionid, sessionid, 
> > !rc);
> > +   audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u ses=%u 
> > res=%d",
> > +oldloginuid, loginuid, tty ? tty_name(tty) : 
> > "(none)",
> > +oldsessionid, sessionid, !rc);
> > +   audit_put_tty(tty);
> > audit_log_end(ab);
> >  }
> 
> Because we are still using the crappy fixed string format for
> kernel<->userspace communication, this patch will likely "break the
> world" ... let's check with Steve but I believe the way to handle this
> is to add the tty information to the end of the record.

Well, I did try to put that keyword consistently with where it was
inserted in other messages.  My understanding was that adding an extra
in the middle wouldn't cause a problem because the keyword scanner
looked ahead until it found the keyword it sought.  This way, older
scanners will just hop over this keyword it wasn't seeking.

> paul moore

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH V4] audit: add tty field to LOGIN event

[Paul Moore]

On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs  wrote:
> The tty field was missing from AUDIT_LOGIN events.
>
> Refactor code to create a new function audit_get_tty(), using it to
> replace the call in audit_log_task_info() and to add it to
> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> audit_put_tty() alias to decrement it.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>
> V4: Add missing prototype for audit_put_tty() when audit syscall is not
> enabled (MIPS).
>
> V3: Introduce audit_put_tty() alias to decrement kref.
>
> V2: Use kref to protect tty signal struct while in use.
>
> ---
>
>  include/linux/audit.h |   24 
>  kernel/audit.c|   18 +-
>  kernel/auditsc.c  |8 ++--
>  3 files changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b40ed5d..32cdafb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
> task_struct *tsk)
> return tsk->sessionid;
>  }
>
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> +   struct tty_struct *tty = NULL;
> +   unsigned long flags;
> +
> +   spin_lock_irqsave(>sighand->siglock, flags);
> +   if (tsk->signal)
> +   tty = tty_kref_get(tsk->signal->tty);
> +   spin_unlock_irqrestore(>sighand->siglock, flags);
> +   return tty;
> +}
> +
> +static inline void audit_put_tty(struct tty_struct *tty)
> +{
> +   tty_kref_put(tty);
> +}

I'm generally not a big fan of defining functions as inlines in header
files, with the general exception of dummy functions that will get
compiled out.  Although I guess there might be some performance
argument to be made wrt audit_log_task_info().

I guess I'm fine with this, but what was the idea behind the static
inlines in audit.h?  Performance, or something else?

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 195ffae..71e14d8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, 
> kuid_t kloginuid,
>  {
> struct audit_buffer *ab;
> uid_t uid, oldloginuid, loginuid;
> +   struct tty_struct *tty;
>
> if (!audit_enabled)
> return;
> @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t 
> koldloginuid, kuid_t kloginuid,
> uid = from_kuid(_user_ns, task_uid(current));
> oldloginuid = from_kuid(_user_ns, koldloginuid);
> loginuid = from_kuid(_user_ns, kloginuid),
> +   tty = audit_get_tty(current);
>
> ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
> if (!ab)
> return;
> audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
> audit_log_task_context(ab);
> -   audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u res=%d",
> -oldloginuid, loginuid, oldsessionid, sessionid, !rc);
> +   audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u ses=%u 
> res=%d",
> +oldloginuid, loginuid, tty ? tty_name(tty) : 
> "(none)",
> +oldsessionid, sessionid, !rc);
> +   audit_put_tty(tty);
> audit_log_end(ab);
>  }

Because we are still using the crappy fixed string format for
kernel<->userspace communication, this patch will likely "break the
world" ... let's check with Steve but I believe the way to handle this
is to add the tty information to the end of the record.

-- 
paul moore
www.paul-moore.com

Re: [PATCH V4] audit: add tty field to LOGIN event

[Paul Moore]

On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs  wrote:
> The tty field was missing from AUDIT_LOGIN events.
>
> Refactor code to create a new function audit_get_tty(), using it to
> replace the call in audit_log_task_info() and to add it to
> audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
> audit_put_tty() alias to decrement it.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>
> V4: Add missing prototype for audit_put_tty() when audit syscall is not
> enabled (MIPS).
>
> V3: Introduce audit_put_tty() alias to decrement kref.
>
> V2: Use kref to protect tty signal struct while in use.
>
> ---
>
>  include/linux/audit.h |   24 
>  kernel/audit.c|   18 +-
>  kernel/auditsc.c  |8 ++--
>  3 files changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b40ed5d..32cdafb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
> task_struct *tsk)
> return tsk->sessionid;
>  }
>
> +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> +{
> +   struct tty_struct *tty = NULL;
> +   unsigned long flags;
> +
> +   spin_lock_irqsave(>sighand->siglock, flags);
> +   if (tsk->signal)
> +   tty = tty_kref_get(tsk->signal->tty);
> +   spin_unlock_irqrestore(>sighand->siglock, flags);
> +   return tty;
> +}
> +
> +static inline void audit_put_tty(struct tty_struct *tty)
> +{
> +   tty_kref_put(tty);
> +}

I'm generally not a big fan of defining functions as inlines in header
files, with the general exception of dummy functions that will get
compiled out.  Although I guess there might be some performance
argument to be made wrt audit_log_task_info().

I guess I'm fine with this, but what was the idea behind the static
inlines in audit.h?  Performance, or something else?

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 195ffae..71e14d8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, 
> kuid_t kloginuid,
>  {
> struct audit_buffer *ab;
> uid_t uid, oldloginuid, loginuid;
> +   struct tty_struct *tty;
>
> if (!audit_enabled)
> return;
> @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t 
> koldloginuid, kuid_t kloginuid,
> uid = from_kuid(_user_ns, task_uid(current));
> oldloginuid = from_kuid(_user_ns, koldloginuid);
> loginuid = from_kuid(_user_ns, kloginuid),
> +   tty = audit_get_tty(current);
>
> ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
> if (!ab)
> return;
> audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
> audit_log_task_context(ab);
> -   audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u res=%d",
> -oldloginuid, loginuid, oldsessionid, sessionid, !rc);
> +   audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u ses=%u 
> res=%d",
> +oldloginuid, loginuid, tty ? tty_name(tty) : 
> "(none)",
> +oldsessionid, sessionid, !rc);
> +   audit_put_tty(tty);
> audit_log_end(ab);
>  }

Because we are still using the crappy fixed string format for
kernel<->userspace communication, this patch will likely "break the
world" ... let's check with Steve but I believe the way to handle this
is to add the tty information to the end of the record.

-- 
paul moore
www.paul-moore.com

[PATCH V4] audit: add tty field to LOGIN event

[Richard Guy Briggs]

The tty field was missing from AUDIT_LOGIN events.

Refactor code to create a new function audit_get_tty(), using it to
replace the call in audit_log_task_info() and to add it to
audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
audit_put_tty() alias to decrement it.

Signed-off-by: Richard Guy Briggs 
---

V4: Add missing prototype for audit_put_tty() when audit syscall is not
enabled (MIPS).

V3: Introduce audit_put_tty() alias to decrement kref.

V2: Use kref to protect tty signal struct while in use.

---

 include/linux/audit.h |   24 
 kernel/audit.c|   18 +-
 kernel/auditsc.c  |8 ++--
 3 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index b40ed5d..32cdafb 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define AUDIT_INO_UNSET ((unsigned long)-1)
 #define AUDIT_DEV_UNSET ((dev_t)-1)
@@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
task_struct *tsk)
return tsk->sessionid;
 }
 
+static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
+{
+   struct tty_struct *tty = NULL;
+   unsigned long flags;
+
+   spin_lock_irqsave(>sighand->siglock, flags);
+   if (tsk->signal)
+   tty = tty_kref_get(tsk->signal->tty);
+   spin_unlock_irqrestore(>sighand->siglock, flags);
+   return tty;
+}
+
+static inline void audit_put_tty(struct tty_struct *tty)
+{
+   tty_kref_put(tty);
+}
+
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
 extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, 
umode_t mode);
 extern void __audit_bprm(struct linux_binprm *bprm);
@@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct 
task_struct *tsk)
 {
return -1;
 }
+static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
+{
+   return NULL;
+}
+static inline void audit_put_tty(struct tty_struct *tty)
+{ }
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 { }
 static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
diff --git a/kernel/audit.c b/kernel/audit.c
index 3a3e5de..7edd776 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -64,7 +64,6 @@
 #include 
 #endif
 #include 
-#include 
 #include 
 #include 
 
@@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, 
struct task_struct *tsk)
 {
const struct cred *cred;
char comm[sizeof(tsk->comm)];
-   char *tty;
+   struct tty_struct *tty;
 
if (!ab)
return;
 
/* tsk == current */
cred = current_cred();
-
-   spin_lock_irq(>sighand->siglock);
-   if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name)
-   tty = tsk->signal->tty->name;
-   else
-   tty = "(none)";
-   spin_unlock_irq(>sighand->siglock);
-
+   tty = audit_get_tty(tsk);
audit_log_format(ab,
 " ppid=%d pid=%d auid=%u uid=%u gid=%u"
 " euid=%u suid=%u fsuid=%u"
@@ -1903,11 +1895,11 @@ void audit_log_task_info(struct audit_buffer *ab, 
struct task_struct *tsk)
 from_kgid(_user_ns, cred->egid),
 from_kgid(_user_ns, cred->sgid),
 from_kgid(_user_ns, cred->fsgid),
-tty, audit_get_sessionid(tsk));
-
+tty ? tty_name(tty) : "(none)",
+audit_get_sessionid(tsk));
+   audit_put_tty(tty);
audit_log_format(ab, " comm=");
audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
-
audit_log_d_path_exe(ab, tsk->mm);
audit_log_task_context(ab);
 }
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 195ffae..71e14d8 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, 
kuid_t kloginuid,
 {
struct audit_buffer *ab;
uid_t uid, oldloginuid, loginuid;
+   struct tty_struct *tty;
 
if (!audit_enabled)
return;
@@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, 
kuid_t kloginuid,
uid = from_kuid(_user_ns, task_uid(current));
oldloginuid = from_kuid(_user_ns, koldloginuid);
loginuid = from_kuid(_user_ns, kloginuid),
+   tty = audit_get_tty(current);
 
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
if (!ab)
return;
audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
audit_log_task_context(ab);
-   audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u res=%d",
-oldloginuid, loginuid, oldsessionid, sessionid, !rc);
+   audit_log_format(ab, " old-auid=%u auid=%u tty=%s 

[PATCH V4] audit: add tty field to LOGIN event

[Richard Guy Briggs]

The tty field was missing from AUDIT_LOGIN events.

Refactor code to create a new function audit_get_tty(), using it to
replace the call in audit_log_task_info() and to add it to
audit_log_set_loginuid().  Lock and bump the kref to protect it, adding
audit_put_tty() alias to decrement it.

Signed-off-by: Richard Guy Briggs 
---

V4: Add missing prototype for audit_put_tty() when audit syscall is not
enabled (MIPS).

V3: Introduce audit_put_tty() alias to decrement kref.

V2: Use kref to protect tty signal struct while in use.

---

 include/linux/audit.h |   24 
 kernel/audit.c|   18 +-
 kernel/auditsc.c  |8 ++--
 3 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index b40ed5d..32cdafb 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define AUDIT_INO_UNSET ((unsigned long)-1)
 #define AUDIT_DEV_UNSET ((dev_t)-1)
@@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct 
task_struct *tsk)
return tsk->sessionid;
 }
 
+static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
+{
+   struct tty_struct *tty = NULL;
+   unsigned long flags;
+
+   spin_lock_irqsave(>sighand->siglock, flags);
+   if (tsk->signal)
+   tty = tty_kref_get(tsk->signal->tty);
+   spin_unlock_irqrestore(>sighand->siglock, flags);
+   return tty;
+}
+
+static inline void audit_put_tty(struct tty_struct *tty)
+{
+   tty_kref_put(tty);
+}
+
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
 extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, 
umode_t mode);
 extern void __audit_bprm(struct linux_binprm *bprm);
@@ -500,6 +518,12 @@ static inline unsigned int audit_get_sessionid(struct 
task_struct *tsk)
 {
return -1;
 }
+static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
+{
+   return NULL;
+}
+static inline void audit_put_tty(struct tty_struct *tty)
+{ }
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 { }
 static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t uid,
diff --git a/kernel/audit.c b/kernel/audit.c
index 3a3e5de..7edd776 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -64,7 +64,6 @@
 #include 
 #endif
 #include 
-#include 
 #include 
 #include 
 
@@ -1873,21 +1872,14 @@ void audit_log_task_info(struct audit_buffer *ab, 
struct task_struct *tsk)
 {
const struct cred *cred;
char comm[sizeof(tsk->comm)];
-   char *tty;
+   struct tty_struct *tty;
 
if (!ab)
return;
 
/* tsk == current */
cred = current_cred();
-
-   spin_lock_irq(>sighand->siglock);
-   if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name)
-   tty = tsk->signal->tty->name;
-   else
-   tty = "(none)";
-   spin_unlock_irq(>sighand->siglock);
-
+   tty = audit_get_tty(tsk);
audit_log_format(ab,
 " ppid=%d pid=%d auid=%u uid=%u gid=%u"
 " euid=%u suid=%u fsuid=%u"
@@ -1903,11 +1895,11 @@ void audit_log_task_info(struct audit_buffer *ab, 
struct task_struct *tsk)
 from_kgid(_user_ns, cred->egid),
 from_kgid(_user_ns, cred->sgid),
 from_kgid(_user_ns, cred->fsgid),
-tty, audit_get_sessionid(tsk));
-
+tty ? tty_name(tty) : "(none)",
+audit_get_sessionid(tsk));
+   audit_put_tty(tty);
audit_log_format(ab, " comm=");
audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
-
audit_log_d_path_exe(ab, tsk->mm);
audit_log_task_context(ab);
 }
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 195ffae..71e14d8 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, 
kuid_t kloginuid,
 {
struct audit_buffer *ab;
uid_t uid, oldloginuid, loginuid;
+   struct tty_struct *tty;
 
if (!audit_enabled)
return;
@@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, 
kuid_t kloginuid,
uid = from_kuid(_user_ns, task_uid(current));
oldloginuid = from_kuid(_user_ns, koldloginuid);
loginuid = from_kuid(_user_ns, kloginuid),
+   tty = audit_get_tty(current);
 
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
if (!ab)
return;
audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
audit_log_task_context(ab);
-   audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u res=%d",
-oldloginuid, loginuid, oldsessionid, sessionid, !rc);
+   audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u ses=%u