no, your patch is not fine at all. It introduces a security hole, as
Thomas pointed it out to you.
The IST pointer in the _TSS_ can go out of bounds by your patch! That
corruption can be controlled by malicious userspace. Read the analysis
from Thomas.
That percpu_tss.ist[] location is not just a random pointer. It is the
TSS that is read by the CPU on every trap. It is a security-critical
structure and every modification to it has to be treated very, very
carefully. If it goes out of bounds that leads to very nasty problems.
This is a pretty bad security bug, and your reply shows ignorance about
the code your patch is modifying - _after_ Thomas has pointed it out to
you.
read the analysis from Thomas. Here is a sample buggy sequence:
task #1 runs, does int3, goes on DEBUG_STACK IST stack,
we do -= 4096 of the tss.ist[DEBUG_STACK-1] pointer, handler schedules
task #2 runs, does int3, goes on DEBUG_STACK IST stack, handler schedules
we do -= 4096 of the tss.ist[DEBUG_STACK-1] pointer, handler schedules
task #3 runs, does int3, goes on DEBUG_STACK IST
=> poof, stack underflow, memory corruption of nearby data structures,
because the DEBUG_STACK is only 8KB...
this can be triggered in almost arbitrary depth, from user-space.
Wrong. The pointer the subq/addq instructions modify are in fact used by
_EVERY SINGLE_ IST trap sequence that the 64-bit kernel executes (!).
This is the modification that the paranoidentry macro does to the TSS in
entry_64.S:
subq $EXCEPTION_STKSZ, per_cpu__init_tss + TSS_ist + (\ist - 1) * 8(%rbp)
...
addq $EXCEPTION_STKSZ, per_cpu__init_tss + TSS_ist + (\ist - 1) * 8(%rbp)
that modifies the TSS _directly_. The TSS is directly read by the CPU on
every trap entry. The percpu_tss.ist[] array of pointers is then used by
the CPU for every single IST trap. (hw-debug, NMI, int3, double-fault,
stacksegment-overflow, MCE)
This is nothing new or unexpected, it is basic x86-64 IST architectural
behavior implemented in the CPU.
The bug you introduce is that if the handler schedules away (it blocks
or gets preempted on CONFIG_PREEMPT), it would keep the per-CPU IST
offset for that IST type decreased by 4096 => if done enough times the
pointer goes out of bounds and it's kaboom.
yes but it is restored on the wrong CPU, after the task has scheduled
and migrated - moving the IST pointer in the TSS out of bounds, so the
next IST trap (which user-space can trigger arbitrarily - just generate
a stream of INT3 breakpoints) will corrupt memory!
Thomas is not forgetting anything.
Thomas is doing security analysis about how the hole in your patch can
be exploited: the wrong IST offsets are used to corrupt nearby data
structures - a malicious nonprivileged user task can modify a good
portion of the ~64 bytes of pt_regs at the end of every page near the
IST stack address (and randomly corrupt some bytes below that) - just by
virtue of generating an int3 (or hw-debug) trap with prepared register
contents.
You first need to understand the hole you are introducing to understand
the finer aspects of his security analysis though...
Ingo
--
