ddb(4) userland trace and SMAP

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

ddb(4) userland trace and SMAP

Martin Pieuchot
Since SMAP is enabled ddb(4)'s 'trace /u' and 'trace /p' for a userland
processes result, as expected, in page faults.

Diff below disable SMAP for the duration of the command.  This allows us
to see any possible frame corruption.

ok?

Index: arch/amd64/amd64/db_trace.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/db_trace.c,v
retrieving revision 1.36
diff -u -p -r1.36 db_trace.c
--- arch/amd64/amd64/db_trace.c 3 Nov 2017 11:29:46 -0000 1.36
+++ arch/amd64/amd64/db_trace.c 4 Dec 2017 10:42:42 -0000
@@ -169,6 +169,7 @@ db_stack_trace_print(db_expr_t addr, boo
  struct callframe *frame, *lastframe;
  unsigned long *argp, *arg0;
  db_addr_t callpc;
+ unsigned int cr4save;
  int is_trap = 0;
  boolean_t kernel_only = TRUE;
  boolean_t trace_proc = FALSE;
@@ -185,6 +186,10 @@ db_stack_trace_print(db_expr_t addr, boo
  }
  }
 
+ cr4save = rcr4();
+ if (cr4save & CR4_SMAP)
+ lcr4(cr4save & ~CR4_SMAP);
+
  if (!have_addr) {
  frame = (struct callframe *)ddb_regs.tf_rbp;
  callpc = (db_addr_t)ddb_regs.tf_rip;
@@ -193,7 +198,7 @@ db_stack_trace_print(db_expr_t addr, boo
  struct proc *p = tfind((pid_t)addr);
  if (p == NULL) {
  (*pr) ("not found\n");
- return;
+ goto out;
  }
  frame = (struct callframe *)p->p_addr->u_pcb.pcb_rbp;
  } else {
@@ -212,8 +217,13 @@ db_stack_trace_print(db_expr_t addr, boo
  db_expr_t offset;
  Elf_Sym * sym;
 
- sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
- db_symbol_values(sym, &name, NULL);
+ if (INKERNEL(frame)) {
+ sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
+ db_symbol_values(sym, &name, NULL);
+ } else {
+ sym = NULL;
+ name = NULL;
+ }
 
  if (lastframe == 0 && sym == NULL) {
  /* Symbol not found, peek at code */
@@ -332,6 +342,9 @@ db_stack_trace_print(db_expr_t addr, boo
  db_printsym(callpc, DB_STGY_XTRN, pr);
  (*pr)(":\n");
  }
+
+out:
+ lcr4(cr4save);
 }
 
 void
Index: arch/i386/i386/db_trace.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/db_trace.c,v
retrieving revision 1.31
diff -u -p -r1.31 db_trace.c
--- arch/i386/i386/db_trace.c 3 Nov 2017 11:29:46 -0000 1.31
+++ arch/i386/i386/db_trace.c 4 Dec 2017 11:18:38 -0000
@@ -184,9 +184,9 @@ db_stack_trace_print(db_expr_t addr, boo
  struct callframe *frame, *lastframe;
  int *argp, *arg0;
  db_addr_t callpc;
+ unsigned int cr4save;
  int is_trap = 0;
  boolean_t kernel_only = TRUE;
- boolean_t trace_thread = FALSE;
  boolean_t trace_proc = FALSE;
 
  {
@@ -194,8 +194,6 @@ db_stack_trace_print(db_expr_t addr, boo
  char c;
 
  while ((c = *cp++) != 0) {
- if (c == 't')
- trace_thread = TRUE;
  if (c == 'p')
  trace_proc = TRUE;
  if (c == 'u')
@@ -206,16 +204,18 @@ db_stack_trace_print(db_expr_t addr, boo
  if (count == -1)
  count = 65535;
 
+ cr4save = rcr4();
+ if (cr4save & CR4_SMAP)
+ lcr4(cr4save & ~CR4_SMAP);
+
  if (!have_addr) {
  frame = (struct callframe *)ddb_regs.tf_ebp;
  callpc = (db_addr_t)ddb_regs.tf_eip;
- } else if (trace_thread) {
- (*pr) ("%s: can't trace thread\n", __func__);
  } else if (trace_proc) {
  struct proc *p = tfind((pid_t)addr);
  if (p == NULL) {
  (*pr) ("not found\n");
- return;
+ goto out;
  }
  frame = (struct callframe *)p->p_addr->u_pcb.pcb_ebp;
  callpc = (db_addr_t)
@@ -233,8 +233,13 @@ db_stack_trace_print(db_expr_t addr, boo
  db_expr_t offset;
  Elf_Sym *sym;
 
- sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
- db_symbol_values(sym, &name, NULL);
+ if (INKERNEL(frame)) {
+ sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
+ db_symbol_values(sym, &name, NULL);
+ } else {
+ sym = NULL;
+ name = NULL;
+ }
 
  if (lastframe == 0 && sym == NULL) {
  /* Symbol not found, peek at code */
@@ -306,8 +311,10 @@ db_stack_trace_print(db_expr_t addr, boo
  }
  } else if (INKERNEL(lastframe)) {
  /* switch from user to kernel */
- if (kernel_only)
+ if (kernel_only) {
+ (*pr)("end of kernel\n");
  break; /* kernel stack only */
+ }
  } else {
  /* in user */
  if (frame <= lastframe) {
@@ -323,6 +330,9 @@ db_stack_trace_print(db_expr_t addr, boo
  db_printsym(callpc, DB_STGY_XTRN, pr);
  (*pr)(":\n");
  }
+
+out:
+ lcr4(cr4save);
 }
 
 void

Reply | Threaded
Open this post in threaded view
|

Re: ddb(4) userland trace and SMAP

Mike Larkin
On Mon, Dec 04, 2017 at 12:24:00PM +0100, Martin Pieuchot wrote:
> Since SMAP is enabled ddb(4)'s 'trace /u' and 'trace /p' for a userland
> processes result, as expected, in page faults.
>
> Diff below disable SMAP for the duration of the command.  This allows us
> to see any possible frame corruption.
>
> ok?
>

This looks ok to me.

-ml

> Index: arch/amd64/amd64/db_trace.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/db_trace.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 db_trace.c
> --- arch/amd64/amd64/db_trace.c 3 Nov 2017 11:29:46 -0000 1.36
> +++ arch/amd64/amd64/db_trace.c 4 Dec 2017 10:42:42 -0000
> @@ -169,6 +169,7 @@ db_stack_trace_print(db_expr_t addr, boo
>   struct callframe *frame, *lastframe;
>   unsigned long *argp, *arg0;
>   db_addr_t callpc;
> + unsigned int cr4save;
>   int is_trap = 0;
>   boolean_t kernel_only = TRUE;
>   boolean_t trace_proc = FALSE;
> @@ -185,6 +186,10 @@ db_stack_trace_print(db_expr_t addr, boo
>   }
>   }
>  
> + cr4save = rcr4();
> + if (cr4save & CR4_SMAP)
> + lcr4(cr4save & ~CR4_SMAP);
> +
>   if (!have_addr) {
>   frame = (struct callframe *)ddb_regs.tf_rbp;
>   callpc = (db_addr_t)ddb_regs.tf_rip;
> @@ -193,7 +198,7 @@ db_stack_trace_print(db_expr_t addr, boo
>   struct proc *p = tfind((pid_t)addr);
>   if (p == NULL) {
>   (*pr) ("not found\n");
> - return;
> + goto out;
>   }
>   frame = (struct callframe *)p->p_addr->u_pcb.pcb_rbp;
>   } else {
> @@ -212,8 +217,13 @@ db_stack_trace_print(db_expr_t addr, boo
>   db_expr_t offset;
>   Elf_Sym * sym;
>  
> - sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
> - db_symbol_values(sym, &name, NULL);
> + if (INKERNEL(frame)) {
> + sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
> + db_symbol_values(sym, &name, NULL);
> + } else {
> + sym = NULL;
> + name = NULL;
> + }
>  
>   if (lastframe == 0 && sym == NULL) {
>   /* Symbol not found, peek at code */
> @@ -332,6 +342,9 @@ db_stack_trace_print(db_expr_t addr, boo
>   db_printsym(callpc, DB_STGY_XTRN, pr);
>   (*pr)(":\n");
>   }
> +
> +out:
> + lcr4(cr4save);
>  }
>  
>  void
> Index: arch/i386/i386/db_trace.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/i386/i386/db_trace.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 db_trace.c
> --- arch/i386/i386/db_trace.c 3 Nov 2017 11:29:46 -0000 1.31
> +++ arch/i386/i386/db_trace.c 4 Dec 2017 11:18:38 -0000
> @@ -184,9 +184,9 @@ db_stack_trace_print(db_expr_t addr, boo
>   struct callframe *frame, *lastframe;
>   int *argp, *arg0;
>   db_addr_t callpc;
> + unsigned int cr4save;
>   int is_trap = 0;
>   boolean_t kernel_only = TRUE;
> - boolean_t trace_thread = FALSE;
>   boolean_t trace_proc = FALSE;
>  
>   {
> @@ -194,8 +194,6 @@ db_stack_trace_print(db_expr_t addr, boo
>   char c;
>  
>   while ((c = *cp++) != 0) {
> - if (c == 't')
> - trace_thread = TRUE;
>   if (c == 'p')
>   trace_proc = TRUE;
>   if (c == 'u')
> @@ -206,16 +204,18 @@ db_stack_trace_print(db_expr_t addr, boo
>   if (count == -1)
>   count = 65535;
>  
> + cr4save = rcr4();
> + if (cr4save & CR4_SMAP)
> + lcr4(cr4save & ~CR4_SMAP);
> +
>   if (!have_addr) {
>   frame = (struct callframe *)ddb_regs.tf_ebp;
>   callpc = (db_addr_t)ddb_regs.tf_eip;
> - } else if (trace_thread) {
> - (*pr) ("%s: can't trace thread\n", __func__);
>   } else if (trace_proc) {
>   struct proc *p = tfind((pid_t)addr);
>   if (p == NULL) {
>   (*pr) ("not found\n");
> - return;
> + goto out;
>   }
>   frame = (struct callframe *)p->p_addr->u_pcb.pcb_ebp;
>   callpc = (db_addr_t)
> @@ -233,8 +233,13 @@ db_stack_trace_print(db_expr_t addr, boo
>   db_expr_t offset;
>   Elf_Sym *sym;
>  
> - sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
> - db_symbol_values(sym, &name, NULL);
> + if (INKERNEL(frame)) {
> + sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
> + db_symbol_values(sym, &name, NULL);
> + } else {
> + sym = NULL;
> + name = NULL;
> + }
>  
>   if (lastframe == 0 && sym == NULL) {
>   /* Symbol not found, peek at code */
> @@ -306,8 +311,10 @@ db_stack_trace_print(db_expr_t addr, boo
>   }
>   } else if (INKERNEL(lastframe)) {
>   /* switch from user to kernel */
> - if (kernel_only)
> + if (kernel_only) {
> + (*pr)("end of kernel\n");
>   break; /* kernel stack only */
> + }
>   } else {
>   /* in user */
>   if (frame <= lastframe) {
> @@ -323,6 +330,9 @@ db_stack_trace_print(db_expr_t addr, boo
>   db_printsym(callpc, DB_STGY_XTRN, pr);
>   (*pr)(":\n");
>   }
> +
> +out:
> + lcr4(cr4save);
>  }
>  
>  void
>

Reply | Threaded
Open this post in threaded view
|

Re: ddb(4) userland trace and SMAP

Martin Pieuchot
In reply to this post by Martin Pieuchot
On 04/12/17(Mon) 12:24, Martin Pieuchot wrote:
> Since SMAP is enabled ddb(4)'s 'trace /u' and 'trace /p' for a userland
> processes result, as expected, in page faults.
>
> Diff below disable SMAP for the duration of the command.  This allows us
> to see any possible frame corruption.

Updated version that:

 - Removes the goto by shuffling parameter tests
 - Initializes cr4save to limit the effect of this gadget.
 - Skip lcr4() completely if the CPU doesn't support SMAP.

ok?

Index: arch/amd64/amd64/db_trace.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/db_trace.c,v
retrieving revision 1.36
diff -u -p -r1.36 db_trace.c
--- arch/amd64/amd64/db_trace.c 3 Nov 2017 11:29:46 -0000 1.36
+++ arch/amd64/amd64/db_trace.c 5 Dec 2017 10:21:03 -0000
@@ -169,9 +169,11 @@ db_stack_trace_print(db_expr_t addr, boo
  struct callframe *frame, *lastframe;
  unsigned long *argp, *arg0;
  db_addr_t callpc;
+ unsigned int cr4save = CR4_SMEP|CR4_SMAP;
  int is_trap = 0;
  boolean_t kernel_only = TRUE;
  boolean_t trace_proc = FALSE;
+ struct proc *p;
 
  {
  char *cp = modif;
@@ -185,22 +187,30 @@ db_stack_trace_print(db_expr_t addr, boo
  }
  }
 
+ if (trace_proc) {
+ p = tfind((pid_t)addr);
+ if (p == NULL) {
+ (*pr) ("not found\n");
+ return;
+ }
+ }
+
+ cr4save = rcr4();
+ if (cr4save & CR4_SMAP)
+ lcr4(cr4save & ~CR4_SMAP);
+
  if (!have_addr) {
  frame = (struct callframe *)ddb_regs.tf_rbp;
  callpc = (db_addr_t)ddb_regs.tf_rip;
+ } else if (trace_proc) {
+ frame = (struct callframe *)p->p_addr->u_pcb.pcb_rbp;
+ callpc = (db_addr_t)
+    db_get_value((db_addr_t)&frame->f_retaddr, 8, FALSE);
+ frame = (struct callframe *)frame->f_frame;
  } else {
- if (trace_proc) {
- struct proc *p = tfind((pid_t)addr);
- if (p == NULL) {
- (*pr) ("not found\n");
- return;
- }
- frame = (struct callframe *)p->p_addr->u_pcb.pcb_rbp;
- } else {
- frame = (struct callframe *)addr;
- }
+ frame = (struct callframe *)addr;
  callpc = (db_addr_t)
- db_get_value((db_addr_t)&frame->f_retaddr, 8, FALSE);
+    db_get_value((db_addr_t)&frame->f_retaddr, 8, FALSE);
  frame = (struct callframe *)frame->f_frame;
  }
 
@@ -212,8 +222,13 @@ db_stack_trace_print(db_expr_t addr, boo
  db_expr_t offset;
  Elf_Sym * sym;
 
- sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
- db_symbol_values(sym, &name, NULL);
+ if (INKERNEL(frame)) {
+ sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
+ db_symbol_values(sym, &name, NULL);
+ } else {
+ sym = NULL;
+ name = NULL;
+ }
 
  if (lastframe == 0 && sym == NULL) {
  /* Symbol not found, peek at code */
@@ -332,6 +347,9 @@ db_stack_trace_print(db_expr_t addr, boo
  db_printsym(callpc, DB_STGY_XTRN, pr);
  (*pr)(":\n");
  }
+
+ if (cr4save & CR4_SMAP)
+ lcr4(cr4save);
 }
 
 void
Index: arch/i386/i386/db_trace.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/db_trace.c,v
retrieving revision 1.31
diff -u -p -r1.31 db_trace.c
--- arch/i386/i386/db_trace.c 3 Nov 2017 11:29:46 -0000 1.31
+++ arch/i386/i386/db_trace.c 5 Dec 2017 10:21:12 -0000
@@ -184,18 +184,17 @@ db_stack_trace_print(db_expr_t addr, boo
  struct callframe *frame, *lastframe;
  int *argp, *arg0;
  db_addr_t callpc;
+ unsigned int cr4save = CR4_SMEP|CR4_SMAP;
  int is_trap = 0;
  boolean_t kernel_only = TRUE;
- boolean_t trace_thread = FALSE;
  boolean_t trace_proc = FALSE;
+ struct proc *p;
 
  {
  char *cp = modif;
  char c;
 
  while ((c = *cp++) != 0) {
- if (c == 't')
- trace_thread = TRUE;
  if (c == 'p')
  trace_proc = TRUE;
  if (c == 'u')
@@ -206,24 +205,29 @@ db_stack_trace_print(db_expr_t addr, boo
  if (count == -1)
  count = 65535;
 
- if (!have_addr) {
- frame = (struct callframe *)ddb_regs.tf_ebp;
- callpc = (db_addr_t)ddb_regs.tf_eip;
- } else if (trace_thread) {
- (*pr) ("%s: can't trace thread\n", __func__);
- } else if (trace_proc) {
- struct proc *p = tfind((pid_t)addr);
+ if (trace_proc) {
+ p = tfind((pid_t)addr);
  if (p == NULL) {
  (*pr) ("not found\n");
  return;
  }
+ }
+
+ cr4save = rcr4();
+ if (cr4save & CR4_SMAP)
+ lcr4(cr4save & ~CR4_SMAP);
+
+ if (!have_addr) {
+ frame = (struct callframe *)ddb_regs.tf_ebp;
+ callpc = (db_addr_t)ddb_regs.tf_eip;
+ } else if (trace_proc) {
  frame = (struct callframe *)p->p_addr->u_pcb.pcb_ebp;
  callpc = (db_addr_t)
     db_get_value((int)&frame->f_retaddr, 4, FALSE);
  } else {
  frame = (struct callframe *)addr;
  callpc = (db_addr_t)
- db_get_value((int)&frame->f_retaddr, 4, FALSE);
+    db_get_value((int)&frame->f_retaddr, 4, FALSE);
  }
 
  lastframe = 0;
@@ -233,8 +237,13 @@ db_stack_trace_print(db_expr_t addr, boo
  db_expr_t offset;
  Elf_Sym *sym;
 
- sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
- db_symbol_values(sym, &name, NULL);
+ if (INKERNEL(frame)) {
+ sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
+ db_symbol_values(sym, &name, NULL);
+ } else {
+ sym = NULL;
+ name = NULL;
+ }
 
  if (lastframe == 0 && sym == NULL) {
  /* Symbol not found, peek at code */
@@ -306,8 +315,10 @@ db_stack_trace_print(db_expr_t addr, boo
  }
  } else if (INKERNEL(lastframe)) {
  /* switch from user to kernel */
- if (kernel_only)
+ if (kernel_only) {
+ (*pr)("end of kernel\n");
  break; /* kernel stack only */
+ }
  } else {
  /* in user */
  if (frame <= lastframe) {
@@ -323,6 +334,9 @@ db_stack_trace_print(db_expr_t addr, boo
  db_printsym(callpc, DB_STGY_XTRN, pr);
  (*pr)(":\n");
  }
+
+ if (cr4save & CR4_SMAP)
+ lcr4(cr4save);
 }
 
 void

Reply | Threaded
Open this post in threaded view
|

Re: ddb(4) userland trace and SMAP

Visa Hankala-2
On Tue, Dec 05, 2017 at 11:32:53AM +0100, Martin Pieuchot wrote:

> On 04/12/17(Mon) 12:24, Martin Pieuchot wrote:
> > Since SMAP is enabled ddb(4)'s 'trace /u' and 'trace /p' for a userland
> > processes result, as expected, in page faults.
> >
> > Diff below disable SMAP for the duration of the command.  This allows us
> > to see any possible frame corruption.
>
> Updated version that:
>
>  - Removes the goto by shuffling parameter tests
>  - Initializes cr4save to limit the effect of this gadget.
>  - Skip lcr4() completely if the CPU doesn't support SMAP.

On i386, it might be necessary to make the rcr4() conditional to a CPU
feature flag because olden x86 processors do not have the CR4 register.
Condition curcpu()->ci_feature_sefflags_ebx & SEFF0EBX_SMAP might be
good enough in this case.

With that issue fixed, OK visa@

Reply | Threaded
Open this post in threaded view
|

Re: ddb(4) userland trace and SMAP

Martin Pieuchot
On 05/12/17(Tue) 14:52, Visa Hankala wrote:

> On Tue, Dec 05, 2017 at 11:32:53AM +0100, Martin Pieuchot wrote:
> > On 04/12/17(Mon) 12:24, Martin Pieuchot wrote:
> > > Since SMAP is enabled ddb(4)'s 'trace /u' and 'trace /p' for a userland
> > > processes result, as expected, in page faults.
> > >
> > > Diff below disable SMAP for the duration of the command.  This allows us
> > > to see any possible frame corruption.
> >
> > Updated version that:
> >
> >  - Removes the goto by shuffling parameter tests
> >  - Initializes cr4save to limit the effect of this gadget.
> >  - Skip lcr4() completely if the CPU doesn't support SMAP.
>
> On i386, it might be necessary to make the rcr4() conditional to a CPU
> feature flag because olden x86 processors do not have the CR4 register.
> Condition curcpu()->ci_feature_sefflags_ebx & SEFF0EBX_SMAP might be
> good enough in this case.
>
> With that issue fixed, OK visa@

Good point, I'd like to commit the diff below then.


Index: arch/amd64/amd64/db_trace.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/db_trace.c,v
retrieving revision 1.36
diff -u -p -r1.36 db_trace.c
--- arch/amd64/amd64/db_trace.c 3 Nov 2017 11:29:46 -0000 1.36
+++ arch/amd64/amd64/db_trace.c 5 Dec 2017 10:21:03 -0000
@@ -169,9 +169,11 @@ db_stack_trace_print(db_expr_t addr, boo
  struct callframe *frame, *lastframe;
  unsigned long *argp, *arg0;
  db_addr_t callpc;
+ unsigned int cr4save = CR4_SMEP|CR4_SMAP;
  int is_trap = 0;
  boolean_t kernel_only = TRUE;
  boolean_t trace_proc = FALSE;
+ struct proc *p;
 
  {
  char *cp = modif;
@@ -185,22 +187,30 @@ db_stack_trace_print(db_expr_t addr, boo
  }
  }
 
+ if (trace_proc) {
+ p = tfind((pid_t)addr);
+ if (p == NULL) {
+ (*pr) ("not found\n");
+ return;
+ }
+ }
+
+ cr4save = rcr4();
+ if (cr4save & CR4_SMAP)
+ lcr4(cr4save & ~CR4_SMAP);
+
  if (!have_addr) {
  frame = (struct callframe *)ddb_regs.tf_rbp;
  callpc = (db_addr_t)ddb_regs.tf_rip;
+ } else if (trace_proc) {
+ frame = (struct callframe *)p->p_addr->u_pcb.pcb_rbp;
+ callpc = (db_addr_t)
+    db_get_value((db_addr_t)&frame->f_retaddr, 8, FALSE);
+ frame = (struct callframe *)frame->f_frame;
  } else {
- if (trace_proc) {
- struct proc *p = tfind((pid_t)addr);
- if (p == NULL) {
- (*pr) ("not found\n");
- return;
- }
- frame = (struct callframe *)p->p_addr->u_pcb.pcb_rbp;
- } else {
- frame = (struct callframe *)addr;
- }
+ frame = (struct callframe *)addr;
  callpc = (db_addr_t)
- db_get_value((db_addr_t)&frame->f_retaddr, 8, FALSE);
+    db_get_value((db_addr_t)&frame->f_retaddr, 8, FALSE);
  frame = (struct callframe *)frame->f_frame;
  }
 
@@ -212,8 +222,13 @@ db_stack_trace_print(db_expr_t addr, boo
  db_expr_t offset;
  Elf_Sym * sym;
 
- sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
- db_symbol_values(sym, &name, NULL);
+ if (INKERNEL(frame)) {
+ sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
+ db_symbol_values(sym, &name, NULL);
+ } else {
+ sym = NULL;
+ name = NULL;
+ }
 
  if (lastframe == 0 && sym == NULL) {
  /* Symbol not found, peek at code */
@@ -332,6 +347,9 @@ db_stack_trace_print(db_expr_t addr, boo
  db_printsym(callpc, DB_STGY_XTRN, pr);
  (*pr)(":\n");
  }
+
+ if (cr4save & CR4_SMAP)
+ lcr4(cr4save);
 }
 
 void
Index: arch/i386/i386/db_trace.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/db_trace.c,v
retrieving revision 1.31
diff -u -p -r1.31 db_trace.c
--- arch/i386/i386/db_trace.c 3 Nov 2017 11:29:46 -0000 1.31
+++ arch/i386/i386/db_trace.c 7 Dec 2017 10:40:00 -0000
@@ -184,18 +184,17 @@ db_stack_trace_print(db_expr_t addr, boo
  struct callframe *frame, *lastframe;
  int *argp, *arg0;
  db_addr_t callpc;
+ unsigned int cr4save = CR4_SMEP|CR4_SMAP;
  int is_trap = 0;
  boolean_t kernel_only = TRUE;
- boolean_t trace_thread = FALSE;
  boolean_t trace_proc = FALSE;
+ struct proc *p;
 
  {
  char *cp = modif;
  char c;
 
  while ((c = *cp++) != 0) {
- if (c == 't')
- trace_thread = TRUE;
  if (c == 'p')
  trace_proc = TRUE;
  if (c == 'u')
@@ -206,24 +205,33 @@ db_stack_trace_print(db_expr_t addr, boo
  if (count == -1)
  count = 65535;
 
- if (!have_addr) {
- frame = (struct callframe *)ddb_regs.tf_ebp;
- callpc = (db_addr_t)ddb_regs.tf_eip;
- } else if (trace_thread) {
- (*pr) ("%s: can't trace thread\n", __func__);
- } else if (trace_proc) {
- struct proc *p = tfind((pid_t)addr);
+ if (trace_proc) {
+ p = tfind((pid_t)addr);
  if (p == NULL) {
  (*pr) ("not found\n");
  return;
  }
+ }
+
+ if (curcpu()->ci_feature_sefflags_ebx & SEFF0EBX_SMAP) {
+ cr4save = rcr4();
+ if (cr4save & CR4_SMAP)
o lcr4(cr4save & ~CR4_SMAP);
+ } else {
+ cr4save = 0;
+ }
+
+ if (!have_addr) {
+ frame = (struct callframe *)ddb_regs.tf_ebp;
+ callpc = (db_addr_t)ddb_regs.tf_eip;
+ } else if (trace_proc) {
  frame = (struct callframe *)p->p_addr->u_pcb.pcb_ebp;
  callpc = (db_addr_t)
     db_get_value((int)&frame->f_retaddr, 4, FALSE);
  } else {
  frame = (struct callframe *)addr;
  callpc = (db_addr_t)
- db_get_value((int)&frame->f_retaddr, 4, FALSE);
+    db_get_value((int)&frame->f_retaddr, 4, FALSE);
  }
 
  lastframe = 0;
@@ -233,8 +241,13 @@ db_stack_trace_print(db_expr_t addr, boo
  db_expr_t offset;
  Elf_Sym *sym;
 
- sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
- db_symbol_values(sym, &name, NULL);
+ if (INKERNEL(frame)) {
+ sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
+ db_symbol_values(sym, &name, NULL);
+ } else {
+ sym = NULL;
+ name = NULL;
+ }
 
  if (lastframe == 0 && sym == NULL) {
  /* Symbol not found, peek at code */
@@ -306,8 +319,10 @@ db_stack_trace_print(db_expr_t addr, boo
  }
  } else if (INKERNEL(lastframe)) {
  /* switch from user to kernel */
- if (kernel_only)
+ if (kernel_only) {
+ (*pr)("end of kernel\n");
  break; /* kernel stack only */
+ }
  } else {
  /* in user */
  if (frame <= lastframe) {
@@ -323,6 +338,9 @@ db_stack_trace_print(db_expr_t addr, boo
  db_printsym(callpc, DB_STGY_XTRN, pr);
  (*pr)(":\n");
  }
+
+ if (cr4save & CR4_SMAP)
+ lcr4(cr4save);
 }
 
 void

Reply | Threaded
Open this post in threaded view
|

Re: ddb(4) userland trace and SMAP

Visa Hankala-2
On Thu, Dec 07, 2017 at 11:43:09AM +0100, Martin Pieuchot wrote:

> On 05/12/17(Tue) 14:52, Visa Hankala wrote:
> > On Tue, Dec 05, 2017 at 11:32:53AM +0100, Martin Pieuchot wrote:
> > > On 04/12/17(Mon) 12:24, Martin Pieuchot wrote:
> > > > Since SMAP is enabled ddb(4)'s 'trace /u' and 'trace /p' for a userland
> > > > processes result, as expected, in page faults.
> > > >
> > > > Diff below disable SMAP for the duration of the command.  This allows us
> > > > to see any possible frame corruption.
> > >
> > > Updated version that:
> > >
> > >  - Removes the goto by shuffling parameter tests
> > >  - Initializes cr4save to limit the effect of this gadget.
> > >  - Skip lcr4() completely if the CPU doesn't support SMAP.
> >
> > On i386, it might be necessary to make the rcr4() conditional to a CPU
> > feature flag because olden x86 processors do not have the CR4 register.
> > Condition curcpu()->ci_feature_sefflags_ebx & SEFF0EBX_SMAP might be
> > good enough in this case.

> Good point, I'd like to commit the diff below then.

OK visa@

Reply | Threaded
Open this post in threaded view
|

Re: ddb(4) userland trace and SMAP

Mike Larkin
In reply to this post by Martin Pieuchot
On Thu, Dec 07, 2017 at 11:43:09AM +0100, Martin Pieuchot wrote:

> On 05/12/17(Tue) 14:52, Visa Hankala wrote:
> > On Tue, Dec 05, 2017 at 11:32:53AM +0100, Martin Pieuchot wrote:
> > > On 04/12/17(Mon) 12:24, Martin Pieuchot wrote:
> > > > Since SMAP is enabled ddb(4)'s 'trace /u' and 'trace /p' for a userland
> > > > processes result, as expected, in page faults.
> > > >
> > > > Diff below disable SMAP for the duration of the command.  This allows us
> > > > to see any possible frame corruption.
> > >
> > > Updated version that:
> > >
> > >  - Removes the goto by shuffling parameter tests
> > >  - Initializes cr4save to limit the effect of this gadget.
> > >  - Skip lcr4() completely if the CPU doesn't support SMAP.
> >
> > On i386, it might be necessary to make the rcr4() conditional to a CPU
> > feature flag because olden x86 processors do not have the CR4 register.
> > Condition curcpu()->ci_feature_sefflags_ebx & SEFF0EBX_SMAP might be
> > good enough in this case.
> >
> > With that issue fixed, OK visa@
>
> Good point, I'd like to commit the diff below then.
>

Ah. I also forgot about that :) Thanks visa!

>
> Index: arch/amd64/amd64/db_trace.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/db_trace.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 db_trace.c
> --- arch/amd64/amd64/db_trace.c 3 Nov 2017 11:29:46 -0000 1.36
> +++ arch/amd64/amd64/db_trace.c 5 Dec 2017 10:21:03 -0000
> @@ -169,9 +169,11 @@ db_stack_trace_print(db_expr_t addr, boo
>   struct callframe *frame, *lastframe;
>   unsigned long *argp, *arg0;
>   db_addr_t callpc;
> + unsigned int cr4save = CR4_SMEP|CR4_SMAP;
>   int is_trap = 0;
>   boolean_t kernel_only = TRUE;
>   boolean_t trace_proc = FALSE;
> + struct proc *p;
>  
>   {
>   char *cp = modif;
> @@ -185,22 +187,30 @@ db_stack_trace_print(db_expr_t addr, boo
>   }
>   }
>  
> + if (trace_proc) {
> + p = tfind((pid_t)addr);
> + if (p == NULL) {
> + (*pr) ("not found\n");
> + return;
> + }
> + }
> +
> + cr4save = rcr4();
> + if (cr4save & CR4_SMAP)
> + lcr4(cr4save & ~CR4_SMAP);
> +
>   if (!have_addr) {
>   frame = (struct callframe *)ddb_regs.tf_rbp;
>   callpc = (db_addr_t)ddb_regs.tf_rip;
> + } else if (trace_proc) {
> + frame = (struct callframe *)p->p_addr->u_pcb.pcb_rbp;
> + callpc = (db_addr_t)
> +    db_get_value((db_addr_t)&frame->f_retaddr, 8, FALSE);
> + frame = (struct callframe *)frame->f_frame;
>   } else {
> - if (trace_proc) {
> - struct proc *p = tfind((pid_t)addr);
> - if (p == NULL) {
> - (*pr) ("not found\n");
> - return;
> - }
> - frame = (struct callframe *)p->p_addr->u_pcb.pcb_rbp;
> - } else {
> - frame = (struct callframe *)addr;
> - }
> + frame = (struct callframe *)addr;
>   callpc = (db_addr_t)
> - db_get_value((db_addr_t)&frame->f_retaddr, 8, FALSE);
> +    db_get_value((db_addr_t)&frame->f_retaddr, 8, FALSE);
>   frame = (struct callframe *)frame->f_frame;
>   }
>  
> @@ -212,8 +222,13 @@ db_stack_trace_print(db_expr_t addr, boo
>   db_expr_t offset;
>   Elf_Sym * sym;
>  
> - sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
> - db_symbol_values(sym, &name, NULL);
> + if (INKERNEL(frame)) {
> + sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
> + db_symbol_values(sym, &name, NULL);
> + } else {
> + sym = NULL;
> + name = NULL;
> + }
>  
>   if (lastframe == 0 && sym == NULL) {
>   /* Symbol not found, peek at code */
> @@ -332,6 +347,9 @@ db_stack_trace_print(db_expr_t addr, boo
>   db_printsym(callpc, DB_STGY_XTRN, pr);
>   (*pr)(":\n");
>   }
> +
> + if (cr4save & CR4_SMAP)
> + lcr4(cr4save);
>  }
>  
>  void
> Index: arch/i386/i386/db_trace.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/i386/i386/db_trace.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 db_trace.c
> --- arch/i386/i386/db_trace.c 3 Nov 2017 11:29:46 -0000 1.31
> +++ arch/i386/i386/db_trace.c 7 Dec 2017 10:40:00 -0000
> @@ -184,18 +184,17 @@ db_stack_trace_print(db_expr_t addr, boo
>   struct callframe *frame, *lastframe;
>   int *argp, *arg0;
>   db_addr_t callpc;
> + unsigned int cr4save = CR4_SMEP|CR4_SMAP;
>   int is_trap = 0;
>   boolean_t kernel_only = TRUE;
> - boolean_t trace_thread = FALSE;
>   boolean_t trace_proc = FALSE;
> + struct proc *p;
>  
>   {
>   char *cp = modif;
>   char c;
>  
>   while ((c = *cp++) != 0) {
> - if (c == 't')
> - trace_thread = TRUE;
>   if (c == 'p')
>   trace_proc = TRUE;
>   if (c == 'u')
> @@ -206,24 +205,33 @@ db_stack_trace_print(db_expr_t addr, boo
>   if (count == -1)
>   count = 65535;
>  
> - if (!have_addr) {
> - frame = (struct callframe *)ddb_regs.tf_ebp;
> - callpc = (db_addr_t)ddb_regs.tf_eip;
> - } else if (trace_thread) {
> - (*pr) ("%s: can't trace thread\n", __func__);
> - } else if (trace_proc) {
> - struct proc *p = tfind((pid_t)addr);
> + if (trace_proc) {
> + p = tfind((pid_t)addr);
>   if (p == NULL) {
>   (*pr) ("not found\n");
>   return;
>   }
> + }
> +
> + if (curcpu()->ci_feature_sefflags_ebx & SEFF0EBX_SMAP) {
> + cr4save = rcr4();
> + if (cr4save & CR4_SMAP)
> o lcr4(cr4save & ~CR4_SMAP);
> + } else {
> + cr4save = 0;
> + }
> +
> + if (!have_addr) {
> + frame = (struct callframe *)ddb_regs.tf_ebp;
> + callpc = (db_addr_t)ddb_regs.tf_eip;
> + } else if (trace_proc) {
>   frame = (struct callframe *)p->p_addr->u_pcb.pcb_ebp;
>   callpc = (db_addr_t)
>      db_get_value((int)&frame->f_retaddr, 4, FALSE);
>   } else {
>   frame = (struct callframe *)addr;
>   callpc = (db_addr_t)
> - db_get_value((int)&frame->f_retaddr, 4, FALSE);
> +    db_get_value((int)&frame->f_retaddr, 4, FALSE);
>   }
>  
>   lastframe = 0;
> @@ -233,8 +241,13 @@ db_stack_trace_print(db_expr_t addr, boo
>   db_expr_t offset;
>   Elf_Sym *sym;
>  
> - sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
> - db_symbol_values(sym, &name, NULL);
> + if (INKERNEL(frame)) {
> + sym = db_search_symbol(callpc, DB_STGY_ANY, &offset);
> + db_symbol_values(sym, &name, NULL);
> + } else {
> + sym = NULL;
> + name = NULL;
> + }
>  
>   if (lastframe == 0 && sym == NULL) {
>   /* Symbol not found, peek at code */
> @@ -306,8 +319,10 @@ db_stack_trace_print(db_expr_t addr, boo
>   }
>   } else if (INKERNEL(lastframe)) {
>   /* switch from user to kernel */
> - if (kernel_only)
> + if (kernel_only) {
> + (*pr)("end of kernel\n");
>   break; /* kernel stack only */
> + }
>   } else {
>   /* in user */
>   if (frame <= lastframe) {
> @@ -323,6 +338,9 @@ db_stack_trace_print(db_expr_t addr, boo
>   db_printsym(callpc, DB_STGY_XTRN, pr);
>   (*pr)(":\n");
>   }
> +
> + if (cr4save & CR4_SMAP)
> + lcr4(cr4save);
>  }
>  
>  void
>