Hi all,
Driven by the SLD vs VMX interaction, here are some patches that provide means
to analyze the text of out-of-tree modules.
The first user of that is refusing to load modules on VMX-SLD conflicts, but it
also has a second patch that refulses to load any module that tries to modify
CRn/DRn.
I'm thinking people will quickly come up with more and more elaborate tests to
which to subject out-of-tree modules.
On 07/04/2020 12:02, Peter Zijlstra wrote:
> Hi all,
>
> Driven by the SLD vs VMX interaction, here are some patches that provide means
> to analyze the text of out-of-tree modules.
>
> The first user of that is refusing to load modules on VMX-SLD conflicts, but it
> also has a second patch that refulses to load any module that tries to modify
> CRn/DRn.
>
> I'm thinking people will quickly come up with more and more elaborate tests to
> which to subject out-of-tree modules.
Anything playing with LGDT & friends? Shouldn't be substantially more
elaborate than CR/DR to check for.
~Andrew
On Tue, Apr 07, 2020 at 06:23:27PM +0100, Andrew Cooper wrote:
> On 07/04/2020 12:02, Peter Zijlstra wrote:
> > Hi all,
> >
> > Driven by the SLD vs VMX interaction, here are some patches that provide means
> > to analyze the text of out-of-tree modules.
> >
> > The first user of that is refusing to load modules on VMX-SLD conflicts, but it
> > also has a second patch that refulses to load any module that tries to modify
> > CRn/DRn.
> >
> > I'm thinking people will quickly come up with more and more elaborate tests to
> > which to subject out-of-tree modules.
>
> Anything playing with LGDT & friends?? Shouldn't be substantially more
> elaborate than CR/DR to check for.
More friends? (I wasn't sure on the Sxxx instructions, they appear
harmless, but what do I know..)
I was also eyeing LSL LTR LSS, none of which I figured a module has any
business of using. Are there more?
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -282,6 +282,50 @@ static bool insn_is_mov_DRn(struct insn
return false;
}
+static bool insn_is_LxDT(struct insn *insn)
+{
+ u8 modrm = insn->modrm.bytes[0];
+ u8 modrm_mod = X86_MODRM_MOD(modrm);
+ u8 modrm_reg = X86_MODRM_REG(modrm);
+
+ if (insn->opcode.bytes[0] != 0x0f)
+ return false;
+
+ switch (insn->opcode.bytes[1]) {
+ case 0x00:
+ if (modrm_mod != 0x03)
+ break;
+
+ switch (modrm_reg) {
+ case 0x0: /* SLDT */
+ case 0x2: /* LLDT */
+ return true;
+
+ default:
+ break;
+ }
+ break;
+
+ case 0x01:
+ switch (modrm_reg) {
+ case 0x0: /* SGDT */
+ case 0x1: /* SIDT */
+ case 0x2: /* LGDT */
+ case 0x3: /* LIDT */
+ return true;
+
+ default:
+ break;
+ }
+ break;
+
+ default:
+ break;
+ }
+
+ return false;
+}
+
static int decode_module(struct module *mod, void *text, void *text_end, bool sld_safe)
{
bool allow_vmx = sld_safe || !split_lock_enabled();
On 07/04/2020 20:41, Peter Zijlstra wrote:
> On Tue, Apr 07, 2020 at 06:23:27PM +0100, Andrew Cooper wrote:
>> On 07/04/2020 12:02, Peter Zijlstra wrote:
>>> Hi all,
>>>
>>> Driven by the SLD vs VMX interaction, here are some patches that provide means
>>> to analyze the text of out-of-tree modules.
>>>
>>> The first user of that is refusing to load modules on VMX-SLD conflicts, but it
>>> also has a second patch that refulses to load any module that tries to modify
>>> CRn/DRn.
>>>
>>> I'm thinking people will quickly come up with more and more elaborate tests to
>>> which to subject out-of-tree modules.
>> Anything playing with LGDT & friends? Shouldn't be substantially more
>> elaborate than CR/DR to check for.
> More friends? (I wasn't sure on the Sxxx instructions, they appear
> harmless, but what do I know..)
>
> I was also eyeing LSL LTR LSS, none of which I figured a module has any
> business of using. Are there more?
Sorry - should have been more clear. By friends, I meant LGDT, LIDT,
LLDT and LTR which are the 4 system table loading instructions. LLDT
and LTR depend on being able to write into the GDT, but still have no
business being used.
Also, LMSW if you care about it, but its utility is somewhere close to 0
these days, so probably not worth the cycles searching for.
The Sxxx instructions have no business being used, but are also harmless
and similarly, probably not worth spending cycles searching for.
L{D,E,F,S}S are functional equivalents to "MOV val1, %sreg; mov val2,
%reg" so harmless (also mode specific as to whether they are useable).
Other things to consider, while we're on a roll:
WRMSR and RDMSR: There is a lot of damage which can be done with these,
and at least forcing people to use the regular hooks will get proper
paravirt support and/or exception support. That said, this might cause
large carnage to out-of-tree modules which are a device driver for
random platform things.
POPF: Don't really want someone being able to set IOPL3. However, this
might quite easily show up as a false positive depending on how the
irqsafe infrastructure gets inlined.
SYSRET/SYSEXIT/IRET: Don't want a module returning to userspace behind
the kernels back. IRET may be a false positive for serialising
purposes, as may be a write to CR2 for that matter.
Looking over the list of other privileged instructions, CLTS,
{,WB,WBNO}INVD, INVLPG and HLT might be candidates for "clearly doing
something which shouldn't be done". Not on the list is INVPCID which
falls into the same category.
Come to think about it, it might be easier to gauge on CPL0 instructions
and whitelist the ok ones, such as VMX and SVM for out-of-tree hypervisors.
~Andrew
On 07/04/2020 20:41, Peter Zijlstra wrote:
> On Tue, Apr 07, 2020 at 06:23:27PM +0100, Andrew Cooper wrote:
>> On 07/04/2020 12:02, Peter Zijlstra wrote:
>>> Hi all,
>>>
>>> Driven by the SLD vs VMX interaction, here are some patches that provide means
>>> to analyze the text of out-of-tree modules.
>>>
>>> The first user of that is refusing to load modules on VMX-SLD conflicts, but it
>>> also has a second patch that refulses to load any module that tries to modify
>>> CRn/DRn.
>>>
>>> I'm thinking people will quickly come up with more and more elaborate tests to
>>> which to subject out-of-tree modules.
>> Anything playing with LGDT & friends? Shouldn't be substantially more
>> elaborate than CR/DR to check for.
> More friends? (I wasn't sure on the Sxxx instructions, they appear
> harmless, but what do I know..)
>
> I was also eyeing LSL LTR LSS, none of which I figured a module has any
> business of using. Are there more?
>
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -282,6 +282,50 @@ static bool insn_is_mov_DRn(struct insn
> return false;
> }
>
> +static bool insn_is_LxDT(struct insn *insn)
> +{
> + u8 modrm = insn->modrm.bytes[0];
> + u8 modrm_mod = X86_MODRM_MOD(modrm);
> + u8 modrm_reg = X86_MODRM_REG(modrm);
> +
> + if (insn->opcode.bytes[0] != 0x0f)
> + return false;
> +
> + switch (insn->opcode.bytes[1]) {
> + case 0x00:
> + if (modrm_mod != 0x03)
> + break;
> +
Apologies - missed this before. LLDT and LTR can be encoded with a
memory operand, so you need to drop the modrm_mod check to spot all
instances.
~Andrew
On Tue, Apr 07, 2020 at 09:11:17PM +0100, Andrew Cooper wrote:
> Sorry - should have been more clear.? By friends, I meant LGDT, LIDT,
> LLDT and LTR which are the 4 system table loading instructions.? LLDT
> and LTR depend on being able to write into the GDT, but still have no
> business being used.
>
> Also, LMSW if you care about it, but its utility is somewhere close to 0
> these days, so probably not worth the cycles searching for.
>
> The Sxxx instructions have no business being used, but are also harmless
> and similarly, probably not worth spending cycles searching for.
>
> L{D,E,F,S}S are functional equivalents to "MOV val1, %sreg; mov val2,
> %reg"? so harmless (also mode specific as to whether they are useable).
OK, LxDT + LTR it is.
> Other things to consider, while we're on a roll:
>
> WRMSR and RDMSR:? There is a lot of damage which can be done with these,
> and at least forcing people to use the regular hooks will get proper
> paravirt support and/or exception support.? That said, this might cause
> large carnage to out-of-tree modules which are a device driver for
> random platform things.
Yeah, I had already considered that, didn't want to touch that just yet.
> POPF: Don't really want someone being able to set IOPL3.? However, this
> might quite easily show up as a false positive depending on how the
> irqsafe infrastructure gets inlined.
local_irq_restore() will be a POPF :/
> SYSRET/SYSEXIT/IRET: Don't want a module returning to userspace behind
> the kernels back.?
Good thinking, let me add this.
> IRET may be a false positive for serialising
> purposes, as may be a write to CR2 for that matter.
We can out-of-line and export sync_core() for that.
> Looking over the list of other privileged instructions, CLTS,
> {,WB,WBNO}INVD, INVLPG and HLT might be candidates for "clearly doing
> something which shouldn't be done".? Not on the list is INVPCID which
> falls into the same category.
>
> Come to think about it, it might be easier to gauge on CPL0 instructions
> and whitelist the ok ones, such as VMX and SVM for out-of-tree hypervisors.
Fair enough, I'll go over those tomorrow.
For now I ended up with:
---
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -282,6 +282,68 @@ static bool insn_is_mov_DRn(struct insn
return false;
}
+static bool insn_is_GDT_modifier(struct insn *insn)
+{
+ u8 modrm = insn->modrm.bytes[0];
+ u8 modrm_mod = X86_MODRM_MOD(modrm);
+ u8 modrm_reg = X86_MODRM_REG(modrm);
+
+ if (insn->opcode.bytes[0] != 0x0f)
+ return false;
+
+ switch (insn->opcode.bytes[1]) {
+ case 0x00: /* Grp6 */
+ switch (modrm_reg) {
+ /* case 0x0: SLDT */
+ case 0x2: /* LLDT */
+ case 0x3: /* LTR */
+ return true;
+
+ default:
+ break;
+ }
+ break;
+
+ case 0x01: /* Grp7 */
+ if (modrm_mod == 0x03)
+ break;
+
+ switch (modrm_reg) {
+ /* case 0x0: SGDT */
+ /* case 0x1: SIDT */
+ case 0x2: /* LGDT */
+ case 0x3: /* LIDT */
+ return true;
+
+ default:
+ break;
+ }
+ break;
+
+ default:
+ break;
+ }
+
+ return false;
+}
+
+static bool insn_is_xRET(struct insn *insn)
+{
+ u8 op1 = insn->opcode.bytes[0];
+ u8 op2 = insn->opcode.bytes[1];
+
+ if (op1 == 0xcf) /* IRET */
+ return true;
+
+ if (op1 != 0x0f)
+ return false;
+
+ if (op2 == 0x07 || op2 == 0x35) /* SYSRET, SYSEXIT */
+ return true;
+
+ return false;
+}
+
static int decode_module(struct module *mod, void *text, void *text_end, bool sld_safe)
{
bool allow_vmx = sld_safe || !split_lock_enabled();
On Tue, Apr 07, 2020 at 09:21:31PM +0100, Andrew Cooper wrote:
> > + switch (insn->opcode.bytes[1]) {
> > + case 0x00:
> > + if (modrm_mod != 0x03)
> > + break;
> > +
>
> Apologies - missed this before.? LLDT and LTR can be encoded with a
> memory operand, so you need to drop the modrm_mod check to spot all
> instances.
I spotted the same, already fixed. Sorry for the mistake, reading opcode
tables it a pain at the best of times :/
On 07/04/2020 21:45, Peter Zijlstra wrote:
>> POPF: Don't really want someone being able to set IOPL3. However, this
>> might quite easily show up as a false positive depending on how the
>> irqsafe infrastructure gets inlined.
> local_irq_restore() will be a POPF :/
Ok. Something to consider in an orthogonal direction. A while ago, I
put this into Xen as a security fix:
iret_exit_to_guest:
andl $~(X86_EFLAGS_IOPL|X86_EFLAGS_NT|X86_EFLAGS_VM),24(%rsp)
orl $X86_EFLAGS_IF,24(%rsp)
addq $8,%rsp
.Lft0: iretq
which unconditionally fixes up the unsafe flags even if something
manages to slips through (e.g. local_irq_restore() against stack
rubble). It turns out that it has saved us several CVEs in the
intervening time.
Is this the kind of things the hardening folk would be interested in?
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -282,6 +282,68 @@ static bool insn_is_mov_DRn(struct insn
> return false;
> }
>
> +static bool insn_is_GDT_modifier(struct insn *insn)
> +{
> + u8 modrm = insn->modrm.bytes[0];
> + u8 modrm_mod = X86_MODRM_MOD(modrm);
> + u8 modrm_reg = X86_MODRM_REG(modrm);
> +
> + if (insn->opcode.bytes[0] != 0x0f)
> + return false;
> +
> + switch (insn->opcode.bytes[1]) {
> + case 0x00: /* Grp6 */
> + switch (modrm_reg) {
> + /* case 0x0: SLDT */
> + case 0x2: /* LLDT */
> + case 0x3: /* LTR */
> + return true;
Come to think of it, if you include the Sxxx variants, a sufficiently
clever compiler can collapse this entire switch statement into a single
"and $~3, modrm_reg" instruction, rather than being forced to use "and
$~1, modrm_reg; cmp $2, ...".
Probably on the extreme end of micro-optimising however.
~Andrew