linux-kernel - Re: [PATCH 4/8] x86: Add support for rd/wr fs/gs base

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150410202132.GQ15335@tassilo.jf.intel.com>
Date:	Fri, 10 Apr 2015 13:21:32 -0700
From:	Andi Kleen <ak@...ux.intel.com>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Andi Kleen <andi@...stfloor.org>, X86 ML <x86@...nel.org>,
	Andrew Lutomirski <luto@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH 4/8] x86: Add support for rd/wr fs/gs base

> We never run paranoid_exit if we interrupted userspace, and we can't
> context switch on the IST stack, so I don't see how this is possible.
> 
> > - Restore from R15 (with FSGSBASE), if the gs base was saved
> > in R15
> 
> What about case 4: we interrupted the kernel with usergs?  (The code
> seems more correct in this regard, but this description above is
> confusing to me.)

I'll fix the description.

> >                                 estacks = per_cpu(debug_stack, cpu);
> > +                       /* Store GS at bottom of stack for bootstrap access */
> > +                       *(void **)estacks = gs;
> >                         estacks += exception_stack_sizes[v];
> >                         oist->ist[v] = t->x86_tss.ist[v] =
> >                                         (unsigned long)estacks;
> 
> Seems reasonable to me.
> 
> You could possibly simplify some things if you wrote the address to
> the bottom of *each* debug stack.  Then you wouldn't need the extra
> alignment stuff.

It would waste 16K or so per CPU. I don't think the if is a problem.


> > +/*
> > + * Stack layout:
> > + * +16 pt_regs
> > + * +8  stack mask for ist or 0
> 
> What does that mean?
> 
> Oh, I get it.  It's the size of the IST stack we're on.  Let's please
> make all IST stacks the same size as suggested above and get rid of
> this.  After all, they really are all the same size -- there's just
> more than one debug stack.

I don't want to waste the memory here. A few instructions more is much
preferable.

> > +       movq_cfi rdx, RDX+OFF
> > +       movq_cfi rcx, RCX+OFF
> > +       movq_cfi rax, RAX+OFF
> > +       movq %r8, R8+OFF(%rsp)
> > +       movq %r9, R9+OFF(%rsp)
> > +       movq %r10, R10+OFF(%rsp)
> > +       movq %r11, R11+OFF(%rsp)
> > +       movq_cfi rbx, RBX+OFF
> > +       movq %rbp, RBP+OFF(%rsp)
> > +       movq %r12, R12+OFF(%rsp)
> > +       movq %r13, R13+OFF(%rsp)
> > +       movq %r14, R14+OFF(%rsp)
> > +       movq %r15, R15+OFF(%rsp)
> > +       movq $-1,ORIG_RAX+OFF(%rsp)     /* no syscall to restart */
> > +33:
> > +       ASM_NOP5        /* May be replaced with jump to paranoid_save_gs */
> 
> Is there some reason that the normal ALTERNATIVE macro can't be used here?

This is assembler, not C.

Since you asked for such extensive use I added a new macro for it now.

> > +.macro idtentry sym do_sym has_error_code:req paranoid=0 shift_ist=-1 \
> > +               stack_mask=-EXCEPTION_STKSZ
> 
> This can be removed as well, I think.

No with the different sized stacks.


-Andi
-- 
ak@...ux.intel.com -- Speaking for myself only
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/