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
>
