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

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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 4779 bytes --]

Hi all,

Today's linux-next merge of the tip tree got a conflict in
arch/x86/kernel/ptrace.c between commit 91397401bb50 ("ARCH: AUDIT:
audit_syscall_entry() should not require the arch") from the audit tree
and commit e0ffbaabc46d ("x86: Split syscall_trace_enter into two
phases") from the tip tree.

I fixed it up (see below - there is more cleanup possible since
do_audit_syscall_entry() no longer needs its "arch" argument) and can
carry the fix as necessary (no action is required).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc arch/x86/kernel/ptrace.c
index eb1c87f0b03b,29576c244699..000000000000
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@@ -1441,24 -1441,126 +1441,126 @@@ void send_sigtrap(struct task_struct *t
  	force_sig_info(SIGTRAP, &info, tsk);
  }
  
- 
- #ifdef CONFIG_X86_32
- # define IS_IA32	1
- #elif defined CONFIG_IA32_EMULATION
- # define IS_IA32	is_compat_task()
- #else
- # define IS_IA32	0
+ static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
+ {
+ #ifdef CONFIG_X86_64
+ 	if (arch == AUDIT_ARCH_X86_64) {
 -		audit_syscall_entry(arch, regs->orig_ax, regs->di,
++		audit_syscall_entry(regs->orig_ax, regs->di,
+ 				    regs->si, regs->dx, regs->r10);
+ 	} else
  #endif
+ 	{
 -		audit_syscall_entry(arch, regs->orig_ax, regs->bx,
++		audit_syscall_entry(regs->orig_ax, regs->bx,
+ 				    regs->cx, regs->dx, regs->si);
+ 	}
+ }
  
  /*
-  * We must return the syscall number to actually look up in the table.
-  * This can be -1L to skip running any syscall at all.
+  * We can return 0 to resume the syscall or anything else to go to phase
+  * 2.  If we resume the syscall, we need to put something appropriate in
+  * regs->orig_ax.
+  *
+  * NB: We don't have full pt_regs here, but regs->orig_ax and regs->ax
+  * are fully functional.
+  *
+  * For phase 2's benefit, our return value is:
+  * 0:			resume the syscall
+  * 1:			go to phase 2; no seccomp phase 2 needed
+  * anything else:	go to phase 2; pass return value to seccomp
   */
- long syscall_trace_enter(struct pt_regs *regs)
+ unsigned long syscall_trace_enter_phase1(struct pt_regs *regs, u32 arch)
+ {
+ 	unsigned long ret = 0;
+ 	u32 work;
+ 
+ 	BUG_ON(regs != task_pt_regs(current));
+ 
+ 	work = ACCESS_ONCE(current_thread_info()->flags) &
+ 		_TIF_WORK_SYSCALL_ENTRY;
+ 
+ 	/*
+ 	 * If TIF_NOHZ is set, we are required to call user_exit() before
+ 	 * doing anything that could touch RCU.
+ 	 */
+ 	if (work & _TIF_NOHZ) {
+ 		user_exit();
+ 		work &= ~TIF_NOHZ;
+ 	}
+ 
+ #ifdef CONFIG_SECCOMP
+ 	/*
+ 	 * Do seccomp first -- it should minimize exposure of other
+ 	 * code, and keeping seccomp fast is probably more valuable
+ 	 * than the rest of this.
+ 	 */
+ 	if (work & _TIF_SECCOMP) {
+ 		struct seccomp_data sd;
+ 
+ 		sd.arch = arch;
+ 		sd.nr = regs->orig_ax;
+ 		sd.instruction_pointer = regs->ip;
+ #ifdef CONFIG_X86_64
+ 		if (arch == AUDIT_ARCH_X86_64) {
+ 			sd.args[0] = regs->di;
+ 			sd.args[1] = regs->si;
+ 			sd.args[2] = regs->dx;
+ 			sd.args[3] = regs->r10;
+ 			sd.args[4] = regs->r8;
+ 			sd.args[5] = regs->r9;
+ 		} else
+ #endif
+ 		{
+ 			sd.args[0] = regs->bx;
+ 			sd.args[1] = regs->cx;
+ 			sd.args[2] = regs->dx;
+ 			sd.args[3] = regs->si;
+ 			sd.args[4] = regs->di;
+ 			sd.args[5] = regs->bp;
+ 		}
+ 
+ 		BUILD_BUG_ON(SECCOMP_PHASE1_OK != 0);
+ 		BUILD_BUG_ON(SECCOMP_PHASE1_SKIP != 1);
+ 
+ 		ret = seccomp_phase1(&sd);
+ 		if (ret == SECCOMP_PHASE1_SKIP) {
+ 			regs->orig_ax = -1;
+ 			ret = 0;
+ 		} else if (ret != SECCOMP_PHASE1_OK) {
+ 			return ret;  /* Go directly to phase 2 */
+ 		}
+ 
+ 		work &= ~_TIF_SECCOMP;
+ 	}
+ #endif
+ 
+ 	/* Do our best to finish without phase 2. */
+ 	if (work == 0)
+ 		return ret;  /* seccomp and/or nohz only (ret == 0 here) */
+ 
+ #ifdef CONFIG_AUDITSYSCALL
+ 	if (work == _TIF_SYSCALL_AUDIT) {
+ 		/*
+ 		 * If there is no more work to be done except auditing,
+ 		 * then audit in phase 1.  Phase 2 always audits, so, if
+ 		 * we audit here, then we can't go on to phase 2.
+ 		 */
+ 		do_audit_syscall_entry(regs, arch);
+ 		return 0;
+ 	}
+ #endif
+ 
+ 	return 1;  /* Something is enabled that we can't handle in phase 1 */
+ }
+ 
+ /* Returns the syscall nr to run (which should match regs->orig_ax). */
+ long syscall_trace_enter_phase2(struct pt_regs *regs, u32 arch,
+ 				unsigned long phase1_result)
  {
  	long ret = 0;
+ 	u32 work = ACCESS_ONCE(current_thread_info()->flags) &
+ 		_TIF_WORK_SYSCALL_ENTRY;
  
- 	user_exit();
+ 	BUG_ON(regs != task_pt_regs(current));
  
  	/*
  	 * If we stepped into a sysenter/syscall insn, it trapped in

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Andy Lutomirski]

On Tue, Sep 23, 2014 at 10:47 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi all,
>
> Today's linux-next merge of the tip tree got a conflict in
> arch/x86/kernel/ptrace.c between commit 91397401bb50 ("ARCH: AUDIT:
> audit_syscall_entry() should not require the arch") from the audit tree
> and commit e0ffbaabc46d ("x86: Split syscall_trace_enter into two
> phases") from the tip tree.
>
> I fixed it up (see below - there is more cleanup possible since
> do_audit_syscall_entry() no longer needs its "arch" argument) and can
> carry the fix as necessary (no action is required).

[mainly for Eric]

This appears to re-introduce the bug fixed in:

commit 81f49a8fd7088cfcb588d182eeede862c0e3303e
Author: Andy Lutomirski <luto@amacapital.net>
Date:   Fri Sep 5 15:13:52 2014 -0700

    x86, x32, audit: Fix x32's AUDIT_ARCH wrt audit

This bug is not currently observable because enabling x32 disables
syscall auditing.

Eric, do you want to fix this or should I?

--Andy

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

[Andy Lutomirski]

On Wed, Sep 24, 2014 at 11:12 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Tue, Sep 23, 2014 at 10:47 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>> Hi all,
>>
>> Today's linux-next merge of the tip tree got a conflict in
>> arch/x86/kernel/ptrace.c between commit 91397401bb50 ("ARCH: AUDIT:
>> audit_syscall_entry() should not require the arch") from the audit tree
>> and commit e0ffbaabc46d ("x86: Split syscall_trace_enter into two
>> phases") from the tip tree.
>>
>> I fixed it up (see below - there is more cleanup possible since
>> do_audit_syscall_entry() no longer needs its "arch" argument) and can
>> carry the fix as necessary (no action is required).
>
> [mainly for Eric]
>
> This appears to re-introduce the bug fixed in:
>
> commit 81f49a8fd7088cfcb588d182eeede862c0e3303e
> Author: Andy Lutomirski <luto@amacapital.net>
> Date:   Fri Sep 5 15:13:52 2014 -0700
>
>     x86, x32, audit: Fix x32's AUDIT_ARCH wrt audit
>
> This bug is not currently observable because enabling x32 disables
> syscall auditing.
>
> Eric, do you want to fix this or should I?

Never mind, -ETOOEARLY.  There's no bug.  TS_COMPAT != is_compat_task.

--Andy

>
> --Andy



-- 
Andy Lutomirski
AMA Capital Management, LLC

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

[Andy Lutomirski]

On Tue, Sep 23, 2014 at 10:47 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi all,
>
> Today's linux-next merge of the tip tree got a conflict in
> arch/x86/kernel/ptrace.c between commit 91397401bb50 ("ARCH: AUDIT:
> audit_syscall_entry() should not require the arch") from the audit tree
> and commit e0ffbaabc46d ("x86: Split syscall_trace_enter into two
> phases") from the tip tree.
>
> I fixed it up (see below - there is more cleanup possible since
> do_audit_syscall_entry() no longer needs its "arch" argument) and can
> carry the fix as necessary (no action is required).

I don't think that more cleanup is possible after all.
do_audit_syscall_entry may not need to pass the arch parameter to the
audit code, but it still needs it to choose the set of registers to
use.

--Andy

>
> --
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au
>
> diff --cc arch/x86/kernel/ptrace.c
> index eb1c87f0b03b,29576c244699..000000000000
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@@ -1441,24 -1441,126 +1441,126 @@@ void send_sigtrap(struct task_struct *t
>         force_sig_info(SIGTRAP, &info, tsk);
>   }
>
> -
> - #ifdef CONFIG_X86_32
> - # define IS_IA32      1
> - #elif defined CONFIG_IA32_EMULATION
> - # define IS_IA32      is_compat_task()
> - #else
> - # define IS_IA32      0
> + static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
> + {
> + #ifdef CONFIG_X86_64
> +       if (arch == AUDIT_ARCH_X86_64) {
>  -              audit_syscall_entry(arch, regs->orig_ax, regs->di,
> ++              audit_syscall_entry(regs->orig_ax, regs->di,
> +                                   regs->si, regs->dx, regs->r10);
> +       } else
>   #endif
> +       {
>  -              audit_syscall_entry(arch, regs->orig_ax, regs->bx,
> ++              audit_syscall_entry(regs->orig_ax, regs->bx,
> +                                   regs->cx, regs->dx, regs->si);
> +       }
> + }
>
>   /*
> -  * We must return the syscall number to actually look up in the table.
> -  * This can be -1L to skip running any syscall at all.
> +  * We can return 0 to resume the syscall or anything else to go to phase
> +  * 2.  If we resume the syscall, we need to put something appropriate in
> +  * regs->orig_ax.
> +  *
> +  * NB: We don't have full pt_regs here, but regs->orig_ax and regs->ax
> +  * are fully functional.
> +  *
> +  * For phase 2's benefit, our return value is:
> +  * 0:                 resume the syscall
> +  * 1:                 go to phase 2; no seccomp phase 2 needed
> +  * anything else:     go to phase 2; pass return value to seccomp
>    */
> - long syscall_trace_enter(struct pt_regs *regs)
> + unsigned long syscall_trace_enter_phase1(struct pt_regs *regs, u32 arch)
> + {
> +       unsigned long ret = 0;
> +       u32 work;
> +
> +       BUG_ON(regs != task_pt_regs(current));
> +
> +       work = ACCESS_ONCE(current_thread_info()->flags) &
> +               _TIF_WORK_SYSCALL_ENTRY;
> +
> +       /*
> +        * If TIF_NOHZ is set, we are required to call user_exit() before
> +        * doing anything that could touch RCU.
> +        */
> +       if (work & _TIF_NOHZ) {
> +               user_exit();
> +               work &= ~TIF_NOHZ;
> +       }
> +
> + #ifdef CONFIG_SECCOMP
> +       /*
> +        * Do seccomp first -- it should minimize exposure of other
> +        * code, and keeping seccomp fast is probably more valuable
> +        * than the rest of this.
> +        */
> +       if (work & _TIF_SECCOMP) {
> +               struct seccomp_data sd;
> +
> +               sd.arch = arch;
> +               sd.nr = regs->orig_ax;
> +               sd.instruction_pointer = regs->ip;
> + #ifdef CONFIG_X86_64
> +               if (arch == AUDIT_ARCH_X86_64) {
> +                       sd.args[0] = regs->di;
> +                       sd.args[1] = regs->si;
> +                       sd.args[2] = regs->dx;
> +                       sd.args[3] = regs->r10;
> +                       sd.args[4] = regs->r8;
> +                       sd.args[5] = regs->r9;
> +               } else
> + #endif
> +               {
> +                       sd.args[0] = regs->bx;
> +                       sd.args[1] = regs->cx;
> +                       sd.args[2] = regs->dx;
> +                       sd.args[3] = regs->si;
> +                       sd.args[4] = regs->di;
> +                       sd.args[5] = regs->bp;
> +               }
> +
> +               BUILD_BUG_ON(SECCOMP_PHASE1_OK != 0);
> +               BUILD_BUG_ON(SECCOMP_PHASE1_SKIP != 1);
> +
> +               ret = seccomp_phase1(&sd);
> +               if (ret == SECCOMP_PHASE1_SKIP) {
> +                       regs->orig_ax = -1;
> +                       ret = 0;
> +               } else if (ret != SECCOMP_PHASE1_OK) {
> +                       return ret;  /* Go directly to phase 2 */
> +               }
> +
> +               work &= ~_TIF_SECCOMP;
> +       }
> + #endif
> +
> +       /* Do our best to finish without phase 2. */
> +       if (work == 0)
> +               return ret;  /* seccomp and/or nohz only (ret == 0 here) */
> +
> + #ifdef CONFIG_AUDITSYSCALL
> +       if (work == _TIF_SYSCALL_AUDIT) {
> +               /*
> +                * If there is no more work to be done except auditing,
> +                * then audit in phase 1.  Phase 2 always audits, so, if
> +                * we audit here, then we can't go on to phase 2.
> +                */
> +               do_audit_syscall_entry(regs, arch);
> +               return 0;
> +       }
> + #endif
> +
> +       return 1;  /* Something is enabled that we can't handle in phase 1 */
> + }
> +
> + /* Returns the syscall nr to run (which should match regs->orig_ax). */
> + long syscall_trace_enter_phase2(struct pt_regs *regs, u32 arch,
> +                               unsigned long phase1_result)
>   {
>         long ret = 0;
> +       u32 work = ACCESS_ONCE(current_thread_info()->flags) &
> +               _TIF_WORK_SYSCALL_ENTRY;
>
> -       user_exit();
> +       BUG_ON(regs != task_pt_regs(current));
>
>         /*
>          * If we stepped into a sysenter/syscall insn, it trapped in



-- 
Andy Lutomirski
AMA Capital Management, LLC

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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

Hi Andy,

On Fri, 26 Sep 2014 11:02:27 -0700 Andy Lutomirski <luto@amacapital.net> wrote:
>
> I don't think that more cleanup is possible after all.
> do_audit_syscall_entry may not need to pass the arch parameter to the
> audit code, but it still needs it to choose the set of registers to
> use.

Yes, indeed, I did not look clearly enough :-(

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Richard Guy Briggs]

On 14/09/24, Stephen Rothwell wrote:
> Hi all,
> 
> Today's linux-next merge of the tip tree got a conflict in
> arch/x86/kernel/entry_64.S between commit b4f0d3755c5e ("audit: x86:
> drop arch from __audit_syscall_entry() interface") from the audit tree
> and commit 1dcf74f6edfc ("x86_64, entry: Use split-phase
> syscall_trace_enter for 64-bit syscalls") from the tip tree.
> 
> I fixed it up (probably incorrectly - I removed the auditsys section
> as that is what the latter did) and can carry the fix as necessary (no
> action is required).

The fix looks fine to me.  The auditsys section was no longer needed
since it was taken care of via the phase1 and phase2 calls.  It was the
part in ptrace.c that was important (it would not have compiled) and you
updated that fine to remove the arch arg.

Thanks, Stephen.

> Stephen Rothwell                    sfr@canb.auug.org.au



- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

Hi all,

Today's linux-next merge of the tip tree got a conflict in
arch/x86/kernel/entry_64.S between commit b4f0d3755c5e ("audit: x86:
drop arch from __audit_syscall_entry() interface") from the audit tree
and commit 1dcf74f6edfc ("x86_64, entry: Use split-phase
syscall_trace_enter for 64-bit syscalls") from the tip tree.

I fixed it up (probably incorrectly - I removed the auditsys section
as that is what the latter did) and can carry the fix as necessary (no
action is required).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]