linux-next: manual merge of the audit tree with the security tree
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.. --- 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 = 0x; + + 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
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!
Re: linux-next: manual merge of the audit tree with the security tree
On Fri, Jun 24, 2016 at 11:20 AM, Heiko Carstens 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=000b a1=3ff7f82e3c0 a2=3ff 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
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
On Fri, Jun 24, 2016 at 1:41 AM, Heiko Carstens 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 >> 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
On Thu, Jun 23, 2016 at 12:14:11PM -0400, Paul Moore wrote: > On Thu, Jun 23, 2016 at 2:01 AM, Heiko Carstens > 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
On Thu, Jun 23, 2016 at 2:01 AM, Heiko Carstens 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.. >> --- 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 = 0x; >> + #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
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.. > --- 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 = 0x; > + #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?
linux-next: manual merge of the audit tree with the security tree
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.. --- 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 = 0x; + #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)