linux-next: manual merge of the audit tree with the security tree

linux-next: manual merge of the audit tree with the security tree

[Stephen Rothwell]

Hi Paul,

Today's linux-next merge of the audit tree got a conflict in:

  arch/s390/kernel/ptrace.c

between commit:

  0208b9445bc0 ("s390/ptrace: run seccomp after ptrace")

from the security tree and commit:

  bba696c2c083 ("s390: ensure that syscall arguments are properly masked on s390")

from the audit tree.

I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging.  You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.

-- 
Cheers,
Stephen Rothwell

diff --cc arch/s390/kernel/ptrace.c
index cea17010448f,ac1dc74632b0..000000000000
--- a/arch/s390/kernel/ptrace.c
+++ b/arch/s390/kernel/ptrace.c
@@@ -821,6 -821,16 +821,8 @@@ long compat_arch_ptrace(struct task_str
  
  asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
  {
 -	long ret = 0;
+ 	unsigned long mask = -1UL;
+ 
 -	/* Do the secure computing check first. */
 -	if (secure_computing()) {
 -		/* seccomp failures shouldn't expose any additional code. */
 -		ret = -1;
 -		goto out;
 -	}
 -
  	/*
  	 * The sysc_tracesys code in entry.S stored the system
  	 * call number to gprs[2].
@@@ -846,11 -850,15 +848,14 @@@
  	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
  		trace_sys_enter(regs, regs->gprs[2]);
  
- 	audit_syscall_entry(regs->gprs[2], regs->orig_gpr2,
- 			    regs->gprs[3], regs->gprs[4],
- 			    regs->gprs[5]);
- 
+ #ifdef CONFIG_COMPAT
+         if (test_thread_flag(TIF_31BIT))
+                 mask = 0xffffffff;
+ #endif
+ 	audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
+ 			    regs->gprs[3] & mask, regs->gprs[4] & mask,
+ 			    regs->gprs[5] & mask);
 -out:
 -	return ret ?: regs->gprs[2];
 +	return regs->gprs[2];
  }
  
  asmlinkage void do_syscall_trace_exit(struct pt_regs *regs)

Re: linux-next: manual merge of the audit tree with the security tree

[Heiko Carstens]

On Thu, Jun 23, 2016 at 02:18:14PM +1000, Stephen Rothwell wrote:
> Hi Paul,
> 
> Today's linux-next merge of the audit tree got a conflict in:
> 
>   arch/s390/kernel/ptrace.c
> 
> between commit:
> 
>   0208b9445bc0 ("s390/ptrace: run seccomp after ptrace")
> 
> from the security tree and commit:
> 
>   bba696c2c083 ("s390: ensure that syscall arguments are properly masked on s390")
> 
> from the audit tree.

Hmm, I haven't seen that commit, therefore I'm just commenting on the
result ;)

> diff --cc arch/s390/kernel/ptrace.c
> index cea17010448f,ac1dc74632b0..000000000000
> --- a/arch/s390/kernel/ptrace.c
> +++ b/arch/s390/kernel/ptrace.c
> @@@ -821,6 -821,16 +821,8 @@@ long compat_arch_ptrace(struct task_str
>   
>   asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
>   {
>  -	long ret = 0;
> + 	unsigned long mask = -1UL;
> + 
>  -	/* Do the secure computing check first. */
>  -	if (secure_computing()) {
>  -		/* seccomp failures shouldn't expose any additional code. */
>  -		ret = -1;
>  -		goto out;
>  -	}
>  -
>   	/*
>   	 * The sysc_tracesys code in entry.S stored the system
>   	 * call number to gprs[2].
> @@@ -846,11 -850,15 +848,14 @@@
>   	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
>   		trace_sys_enter(regs, regs->gprs[2]);
>   
> - 	audit_syscall_entry(regs->gprs[2], regs->orig_gpr2,
> - 			    regs->gprs[3], regs->gprs[4],
> - 			    regs->gprs[5]);
> - 
> + #ifdef CONFIG_COMPAT
> +         if (test_thread_flag(TIF_31BIT))
> +                 mask = 0xffffffff;
> + #endif

Better: use is_compat_task() and avoid yet another ifdef.

> + 	audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
> + 			    regs->gprs[3] & mask, regs->gprs[4] & mask,
> + 			    regs->gprs[5] & mask);

With these masks it is more correct, however these are still not the values
used by the system call itself. This would be still incorrect for
e.g. compat pointers (31 bit on s390).

So it seems like audit_syscall_entry should be called after all sign, zero
and masking has been done?

Re: linux-next: manual merge of the audit tree with the security tree

[Paul Moore]

On Thu, Jun 23, 2016 at 2:01 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
> On Thu, Jun 23, 2016 at 02:18:14PM +1000, Stephen Rothwell wrote:
>> Hi Paul,
>>
>> Today's linux-next merge of the audit tree got a conflict in:
>>
>>   arch/s390/kernel/ptrace.c
>>
>> between commit:
>>
>>   0208b9445bc0 ("s390/ptrace: run seccomp after ptrace")
>>
>> from the security tree and commit:
>>
>>   bba696c2c083 ("s390: ensure that syscall arguments are properly masked on s390")
>>
>> from the audit tree.
>
> Hmm, I haven't seen that commit, therefore I'm just commenting on the
> result ;)

It was sent to the linux-audit and linux-s390 mailing lists yesterday
with a follow up comment that I was going to add it to the audit#next
branch and if anyone had any objections to let me know.

* https://www.redhat.com/archives/linux-audit/2016-June/msg00051.html

>> diff --cc arch/s390/kernel/ptrace.c
>> index cea17010448f,ac1dc74632b0..000000000000
>> --- a/arch/s390/kernel/ptrace.c
>> +++ b/arch/s390/kernel/ptrace.c
>> @@@ -821,6 -821,16 +821,8 @@@ long compat_arch_ptrace(struct task_str
>>
>>   asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
>>   {
>>  -    long ret = 0;
>> +     unsigned long mask = -1UL;
>> +
>>  -    /* Do the secure computing check first. */
>>  -    if (secure_computing()) {
>>  -            /* seccomp failures shouldn't expose any additional code. */
>>  -            ret = -1;
>>  -            goto out;
>>  -    }
>>  -
>>       /*
>>        * The sysc_tracesys code in entry.S stored the system
>>        * call number to gprs[2].
>> @@@ -846,11 -850,15 +848,14 @@@
>>       if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
>>               trace_sys_enter(regs, regs->gprs[2]);
>>
>> -     audit_syscall_entry(regs->gprs[2], regs->orig_gpr2,
>> -                         regs->gprs[3], regs->gprs[4],
>> -                         regs->gprs[5]);
>> -
>> + #ifdef CONFIG_COMPAT
>> +         if (test_thread_flag(TIF_31BIT))
>> +                 mask = 0xffffffff;
>> + #endif
>
> Better: use is_compat_task() and avoid yet another ifdef.

Sounds reasonable.

>> +     audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
>> +                         regs->gprs[3] & mask, regs->gprs[4] & mask,
>> +                         regs->gprs[5] & mask);
>
> With these masks it is more correct, however these are still not the values
> used by the system call itself. This would be still incorrect for
> e.g. compat pointers (31 bit on s390).
>
> So it seems like audit_syscall_entry should be called after all sign, zero
> and masking has been done?

For someone not familiar with s390, compat or not, where would you
suggest we place the audit_syscall_entry() call?

-- 
paul moore
www.paul-moore.com

Re: linux-next: manual merge of the audit tree with the security tree

[Heiko Carstens]

On Thu, Jun 23, 2016 at 12:14:11PM -0400, Paul Moore wrote:
> On Thu, Jun 23, 2016 at 2:01 AM, Heiko Carstens
> <heiko.carstens@de.ibm.com> wrote:
> > On Thu, Jun 23, 2016 at 02:18:14PM +1000, Stephen Rothwell wrote:
> >> Hi Paul,
> >>
> >> Today's linux-next merge of the audit tree got a conflict in:
> >>
> >>   arch/s390/kernel/ptrace.c
> >>
> >> between commit:
> >>
> >>   0208b9445bc0 ("s390/ptrace: run seccomp after ptrace")
> >>
> >> from the security tree and commit:
> >>
> >>   bba696c2c083 ("s390: ensure that syscall arguments are properly masked on s390")
> >>
> >> from the audit tree.
> >
> > Hmm, I haven't seen that commit, therefore I'm just commenting on the
> > result ;)
> 
> It was sent to the linux-audit and linux-s390 mailing lists yesterday
> with a follow up comment that I was going to add it to the audit#next
> branch and if anyone had any objections to let me know.
> 
> * https://www.redhat.com/archives/linux-audit/2016-June/msg00051.html

Yes, I missed that, sorry!

> >> +     audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
> >> +                         regs->gprs[3] & mask, regs->gprs[4] & mask,
> >> +                         regs->gprs[5] & mask);
> >
> > With these masks it is more correct, however these are still not the values
> > used by the system call itself. This would be still incorrect for
> > e.g. compat pointers (31 bit on s390).
> >
> > So it seems like audit_syscall_entry should be called after all sign, zero
> > and masking has been done?
> 
> For someone not familiar with s390, compat or not, where would you
> suggest we place the audit_syscall_entry() call?

I was thinking of a more generic solution for all architectures: for
example setting a new TIF flag within do_syscall_trace_enter which
indicates that audit_syscall_entry needs be called and then add a
conditional call to the SYSCALL_DEFINE and COMPAT_SYSCALL_DEFINE macros.

That way audit_syscall_entry would always receive already properly sign and
zero extended system call parameters. At the downside this would increase
the kernel text size by probably ~370 conditional branches and add two more
instructions on the system call hot path.

But that's something that could be done independently from your patch,
which already improves the current situation.

Re: linux-next: manual merge of the audit tree with the security tree

[Paul Moore]

On Fri, Jun 24, 2016 at 1:41 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
> On Thu, Jun 23, 2016 at 12:14:11PM -0400, Paul Moore wrote:
>> On Thu, Jun 23, 2016 at 2:01 AM, Heiko Carstens
>> <heiko.carstens@de.ibm.com> wrote:
>> > On Thu, Jun 23, 2016 at 02:18:14PM +1000, Stephen Rothwell wrote:
>> >> Hi Paul,
>> >>
>> >> Today's linux-next merge of the audit tree got a conflict in:
>> >>
>> >>   arch/s390/kernel/ptrace.c
>> >>
>> >> between commit:
>> >>
>> >>   0208b9445bc0 ("s390/ptrace: run seccomp after ptrace")
>> >>
>> >> from the security tree and commit:
>> >>
>> >>   bba696c2c083 ("s390: ensure that syscall arguments are properly masked on s390")
>> >>
>> >> from the audit tree.
>> >
>> > Hmm, I haven't seen that commit, therefore I'm just commenting on the
>> > result ;)
>>
>> It was sent to the linux-audit and linux-s390 mailing lists yesterday
>> with a follow up comment that I was going to add it to the audit#next
>> branch and if anyone had any objections to let me know.
>>
>> * https://www.redhat.com/archives/linux-audit/2016-June/msg00051.html
>
> Yes, I missed that, sorry!

No problem, my inbox eats a fair amount of mail too ;)

>> >> +     audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
>> >> +                         regs->gprs[3] & mask, regs->gprs[4] & mask,
>> >> +                         regs->gprs[5] & mask);
>> >
>> > With these masks it is more correct, however these are still not the values
>> > used by the system call itself. This would be still incorrect for
>> > e.g. compat pointers (31 bit on s390).
>> >
>> > So it seems like audit_syscall_entry should be called after all sign, zero
>> > and masking has been done?
>>
>> For someone not familiar with s390, compat or not, where would you
>> suggest we place the audit_syscall_entry() call?
>
> I was thinking of a more generic solution for all architectures: for
> example setting a new TIF flag within do_syscall_trace_enter which
> indicates that audit_syscall_entry needs be called and then add a
> conditional call to the SYSCALL_DEFINE and COMPAT_SYSCALL_DEFINE macros.
>
> That way audit_syscall_entry would always receive already properly sign and
> zero extended system call parameters. At the downside this would increase
> the kernel text size by probably ~370 conditional branches and add two more
> instructions on the system call hot path.
>
> But that's something that could be done independently from your patch,
> which already improves the current situation.

My immediate concern is making sure that we are at least recording the
arguments correctly in the audit record.  My simple tests look okay,
but as I said before, I'm far from a s390 expert and your initial
comment made it sound like there were still problems with how we were
recording the arguments.  Can you either confirm that we are logging
the arguments correctly, or provide a suggestion on how to get the
right values?  That would be most helpful at this point.

-- 
paul moore
www.paul-moore.com

Re: linux-next: manual merge of the audit tree with the security tree

[Heiko Carstens]

On Fri, Jun 24, 2016 at 11:05:33AM -0400, Paul Moore wrote:
> >> >> +     audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
> >> >> +                         regs->gprs[3] & mask, regs->gprs[4] & mask,
> >> >> +                         regs->gprs[5] & mask);
> >> >
> >> > With these masks it is more correct, however these are still not the values
> >> > used by the system call itself. This would be still incorrect for
> >> > e.g. compat pointers (31 bit on s390).
> >> >
> >> > So it seems like audit_syscall_entry should be called after all sign, zero
> >> > and masking has been done?
> >>
> >> For someone not familiar with s390, compat or not, where would you
> >> suggest we place the audit_syscall_entry() call?
> >
> > I was thinking of a more generic solution for all architectures: for
> > example setting a new TIF flag within do_syscall_trace_enter which
> > indicates that audit_syscall_entry needs be called and then add a
> > conditional call to the SYSCALL_DEFINE and COMPAT_SYSCALL_DEFINE macros.
> >
> > That way audit_syscall_entry would always receive already properly sign and
> > zero extended system call parameters. At the downside this would increase
> > the kernel text size by probably ~370 conditional branches and add two more
> > instructions on the system call hot path.
> >
> > But that's something that could be done independently from your patch,
> > which already improves the current situation.
> 
> My immediate concern is making sure that we are at least recording the
> arguments correctly in the audit record.  My simple tests look okay,
> but as I said before, I'm far from a s390 expert and your initial
> comment made it sound like there were still problems with how we were
> recording the arguments.  Can you either confirm that we are logging
> the arguments correctly, or provide a suggestion on how to get the
> right values?  That would be most helpful at this point.

The arguments are correct, except that they are missing sign and zero
extension to full 64 bit. However I would expect that the audit subsystem
will only work on the lower 32 bits anyway for compat tasks. So that
shouldn't be a problem.

I'm a bit concerned about user space pointers passed as argument for compat
tasks. These need to mask out 33 instead of 32 bits.  This is of course
system call specific and I don't know enough about audit to tell if it
could be a problem.

Re: linux-next: manual merge of the audit tree with the security tree

[Paul Moore]

On Fri, Jun 24, 2016 at 11:20 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
> On Fri, Jun 24, 2016 at 11:05:33AM -0400, Paul Moore wrote:
>> My immediate concern is making sure that we are at least recording the
>> arguments correctly in the audit record.  My simple tests look okay,
>> but as I said before, I'm far from a s390 expert and your initial
>> comment made it sound like there were still problems with how we were
>> recording the arguments.  Can you either confirm that we are logging
>> the arguments correctly, or provide a suggestion on how to get the
>> right values?  That would be most helpful at this point.
>
> The arguments are correct, except that they are missing sign and zero
> extension to full 64 bit. However I would expect that the audit subsystem
> will only work on the lower 32 bits anyway for compat tasks. So that
> shouldn't be a problem.

Yes, that should be fine.  We were seeing problems with s390 syscalls
on s390x showing garbage in the high 32-bits, that's the problem I'm
trying to solve.

For example, here is a snippet of an audit record generated from
calling the s390 socketcall() on a s390x system:

type=SYSCALL msg=audit(1458835510.860:652): arch=16 syscall=102
 success=yes exit=21
 a0=ffffffff0000000b
 a1=3ff7f82e3c0
 a2=3ff00000000
 a3=a72028

> I'm a bit concerned about user space pointers passed as argument for compat
> tasks. These need to mask out 33 instead of 32 bits.  This is of course
> system call specific and I don't know enough about audit to tell if it
> could be a problem.

>From a practical point of view I'm not sure how much of an impact that
will have as it is unlikely anyone will be doing anything useful with
those pointer values; for example, you aren't going to be inspecting a
process' memory space using just the audit log.  Also, at the very
least we aren't removing any information, just adding in an extra bit
of potential junk.  Anyone who does care about user space pointers in
the audit log, should have all the information the need to drop the
high bit.

Does that sound reasonable?

-- 
paul moore
www.paul-moore.com

Re: linux-next: manual merge of the audit tree with the security tree

[Heiko Carstens]

On Fri, Jun 24, 2016 at 12:20:52PM -0400, Paul Moore wrote:
> > I'm a bit concerned about user space pointers passed as argument for compat
> > tasks. These need to mask out 33 instead of 32 bits.  This is of course
> > system call specific and I don't know enough about audit to tell if it
> > could be a problem.
> 
> From a practical point of view I'm not sure how much of an impact that
> will have as it is unlikely anyone will be doing anything useful with
> those pointer values; for example, you aren't going to be inspecting a
> process' memory space using just the audit log.  Also, at the very
> least we aren't removing any information, just adding in an extra bit
> of potential junk.  Anyone who does care about user space pointers in
> the audit log, should have all the information the need to drop the
> high bit.
> 
> Does that sound reasonable?

Yes, it does. If there should be problems because of the one extra bit that
potentially contains garbage we still can look for a way to fix this.

Thanks!

linux-next: manual merge of the audit tree with the security tree

[Stephen Rothwell]

Hi Paul,

Today's linux-next merge of the audit tree got conflicts in:

  arch/s390/kernel/ptrace.c

between commit:

  0208b9445bc0 ("s390/ptrace: run seccomp after ptrace")

from the security tree and commit:

  da7f750c1ef5 ("s390: ensure that syscall arguments are properly masked on s390")

from the audit tree.

I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging.  You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.

-- 
Cheers,
Stephen Rothwell

diff --cc arch/s390/kernel/ptrace.c
index cea17010448f,defc0dca4510..000000000000
--- a/arch/s390/kernel/ptrace.c
+++ b/arch/s390/kernel/ptrace.c
@@@ -821,6 -821,16 +821,8 @@@ long compat_arch_ptrace(struct task_str
  
  asmlinkage long do_syscall_trace_enter(struct pt_regs *regs)
  {
 -	long ret = 0;
+ 	unsigned long mask = -1UL;
+ 
 -	/* Do the secure computing check first. */
 -	if (secure_computing()) {
 -		/* seccomp failures shouldn't expose any additional code. */
 -		ret = -1;
 -		goto out;
 -	}
 -
  	/*
  	 * The sysc_tracesys code in entry.S stored the system
  	 * call number to gprs[2].
@@@ -846,11 -850,14 +848,14 @@@
  	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT)))
  		trace_sys_enter(regs, regs->gprs[2]);
  
- 	audit_syscall_entry(regs->gprs[2], regs->orig_gpr2,
- 			    regs->gprs[3], regs->gprs[4],
- 			    regs->gprs[5]);
+ 	if (is_compat_task())
+ 		mask = 0xffffffff;
+ 
+ 	audit_syscall_entry(regs->gprs[2], regs->orig_gpr2 & mask,
+ 			    regs->gprs[3] & mask, regs->gprs[4] & mask,
+ 			    regs->gprs[5] & mask);
 -out:
 -	return ret ?: regs->gprs[2];
 +
 +	return regs->gprs[2];
  }
  
  asmlinkage void do_syscall_trace_exit(struct pt_regs *regs)