Skip to content

Commit

Permalink
Kernel: Start implementing x86 SMAP support
Browse files Browse the repository at this point in the history
Supervisor Mode Access Prevention (SMAP) is an x86 CPU feature that
prevents the kernel from accessing userspace memory. With SMAP enabled,
trying to read/write a userspace memory address while in the kernel
will now generate a page fault.

Since it's sometimes necessary to read/write userspace memory, there
are two new instructions that quickly switch the protection on/off:
STAC (disables protection) and CLAC (enables protection.)
These are exposed in kernel code via the stac() and clac() helpers.

There's also a SmapDisabler RAII object that can be used to ensure
that you don't forget to re-enable protection before returning to
userspace code.

THis patch also adds copy_to_user(), copy_from_user() and memset_user()
which are the "correct" way of doing things. These functions allow us
to briefly disable protection for a specific purpose, and then turn it
back on immediately after it's done. Going forward all kernel code
should be moved to using these and all uses of SmapDisabler are to be
considered FIXME's.

Note that we're not realizing the full potential of this feature since
I've used SmapDisabler quite liberally in this initial bring-up patch.
  • Loading branch information
awesomekling committed Jan 5, 2020
1 parent 04b7345 commit 9eef39d
Show file tree
Hide file tree
Showing 11 changed files with 245 additions and 60 deletions.
23 changes: 21 additions & 2 deletions Kernel/Arch/i386/CPU.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,8 @@ void page_fault_handler(RegisterDump regs)

#ifdef PAGE_FAULT_DEBUG
dbgprintf("%s(%u): ring%u %s page fault in PD=%x, %s V%08x\n",
current->process().name().characters(),
current->pid(),
current ? current->process().name().characters() : "(none)",
current ? current->pid() : 0,
regs.cs & 3,
regs.exception_code & 1 ? "PV" : "NP",
fault_page_directory,
Expand Down Expand Up @@ -525,6 +525,7 @@ bool g_cpu_supports_nx;
bool g_cpu_supports_pae;
bool g_cpu_supports_pge;
bool g_cpu_supports_rdrand;
bool g_cpu_supports_smap;
bool g_cpu_supports_smep;
bool g_cpu_supports_sse;
bool g_cpu_supports_tsc;
Expand All @@ -543,6 +544,24 @@ void detect_cpu_features()
g_cpu_supports_nx = (extended_processor_info.edx() & (1 << 20));

CPUID extended_features(0x7);
g_cpu_supports_smap = (extended_features.ebx() & (1 << 20));
g_cpu_supports_smep = (extended_features.ebx() & (1 << 7));
g_cpu_supports_umip = (extended_features.ecx() & (1 << 2));
}


void stac()
{
if (!g_cpu_supports_smap)
return;
asm volatile("stac" ::
: "cc");
}

void clac()
{
if (!g_cpu_supports_smap)
return;
asm volatile("clac" ::
: "cc");
}
23 changes: 23 additions & 0 deletions Kernel/Arch/i386/CPU.h
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,30 @@ extern bool g_cpu_supports_nx;
extern bool g_cpu_supports_pae;
extern bool g_cpu_supports_pge;
extern bool g_cpu_supports_rdrand;
extern bool g_cpu_supports_smap;
extern bool g_cpu_supports_smep;
extern bool g_cpu_supports_sse;
extern bool g_cpu_supports_tsc;
extern bool g_cpu_supports_umip;

void stac();

void clac();

class SmapDisabler {
public:
SmapDisabler()
{
m_flags = cpu_flags();
stac();
}

~SmapDisabler()
{
if (!(m_flags & 0x40000))
clac();
}

private:
u32 m_flags;
};
5 changes: 4 additions & 1 deletion Kernel/FileSystem/FileDescription.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ FileDescription::~FileDescription()

KResult FileDescription::fstat(stat& buffer)
{
SmapDisabler disabler;
if (is_fifo()) {
memset(&buffer, 0, sizeof(buffer));
buffer.st_mode = 001000;
Expand Down Expand Up @@ -102,6 +103,7 @@ off_t FileDescription::seek(off_t offset, int whence)

ssize_t FileDescription::read(u8* buffer, ssize_t count)
{
SmapDisabler disabler;
int nread = m_file->read(*this, buffer, count);
if (nread > 0 && m_file->is_seekable())
m_current_offset += nread;
Expand All @@ -110,6 +112,7 @@ ssize_t FileDescription::read(u8* buffer, ssize_t count)

ssize_t FileDescription::write(const u8* data, ssize_t size)
{
SmapDisabler disabler;
int nwritten = m_file->write(*this, data, size);
if (nwritten > 0 && m_file->is_seekable())
m_current_offset += nwritten;
Expand Down Expand Up @@ -160,7 +163,7 @@ ssize_t FileDescription::get_dir_entries(u8* buffer, ssize_t size)
if (size < temp_buffer.size())
return -1;

memcpy(buffer, temp_buffer.data(), temp_buffer.size());
copy_to_user(buffer, temp_buffer.data(), temp_buffer.size());
return stream.offset();
}

Expand Down
11 changes: 8 additions & 3 deletions Kernel/KSyms.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,13 @@ static void load_ksyms_from_data(const ByteBuffer& buffer)

[[gnu::noinline]] void dump_backtrace_impl(u32 ebp, bool use_ksyms)
{
SmapDisabler disabler;
#if 0
if (!current) {
//hang();
return;
}
#endif
if (use_ksyms && !ksyms_ready) {
hang();
return;
Expand All @@ -104,12 +107,14 @@ static void load_ksyms_from_data(const ByteBuffer& buffer)
RecognizedSymbol recognized_symbols[max_recognized_symbol_count];
int recognized_symbol_count = 0;
if (use_ksyms) {
for (u32* stack_ptr = (u32*)ebp; current->process().validate_read_from_kernel(VirtualAddress((u32)stack_ptr), sizeof(void*) * 2) && recognized_symbol_count < max_recognized_symbol_count; stack_ptr = (u32*)*stack_ptr) {
for (u32* stack_ptr = (u32*)ebp;
(current ? current->process().validate_read_from_kernel(VirtualAddress((u32)stack_ptr), sizeof(void*) * 2) : 1) && recognized_symbol_count < max_recognized_symbol_count; stack_ptr = (u32*)*stack_ptr) {
u32 retaddr = stack_ptr[1];
recognized_symbols[recognized_symbol_count++] = { retaddr, ksymbolicate(retaddr) };
}
} else {
for (u32* stack_ptr = (u32*)ebp; current->process().validate_read_from_kernel(VirtualAddress((u32)stack_ptr), sizeof(void*) * 2); stack_ptr = (u32*)*stack_ptr) {
for (u32* stack_ptr = (u32*)ebp;
(current ? current->process().validate_read_from_kernel(VirtualAddress((u32)stack_ptr), sizeof(void*) * 2) : 1); stack_ptr = (u32*)*stack_ptr) {
u32 retaddr = stack_ptr[1];
dbgprintf("%x (next: %x)\n", retaddr, stack_ptr ? (u32*)*stack_ptr : 0);
}
Expand All @@ -121,7 +126,7 @@ static void load_ksyms_from_data(const ByteBuffer& buffer)
if (!symbol.address)
break;
if (!symbol.ksym) {
if (current->process().elf_loader() && current->process().elf_loader()->has_symbols()) {
if (current && current->process().elf_loader() && current->process().elf_loader()->has_symbols()) {
dbgprintf("%p %s\n", symbol.address, current->process().elf_loader()->symbolicate(symbol.address).characters());
} else {
dbgprintf("%p (no ELF symbols for process)\n", symbol.address);
Expand Down
Loading

3 comments on commit 9eef39d

@bugaevc
Copy link
Member

@bugaevc bugaevc commented on 9eef39d Jan 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wild idea: what if, instead of explicit Linux-like copy_to_user() & copy_from_user() there was a wrapper type that would let you do this naturally? It could actually be a variation of the same SmapDisabler type (or it could be named differently for clarity):

Instead of

copy_to_user(dest_ptr, value, sizeof(value));

you'd write

*SmapDisabler(dest_ptr) = value;

or say

*UserMemory(dest_ptr) = value;

@awesomekling
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, something like a Userspace<T*> or similar might be an interesting option.

@bugaevc
Copy link
Member

@bugaevc bugaevc commented on 9eef39d Jan 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, it's not okay to first validate a read/write and then do it. The thread can get preempted in the middle and change the regions and/or the pointer, even without an explicit sleep.

So Userspace<T*> would have to:

  • disable interrupts
  • disable SMAP
  • validate read/write; somehow propagate that to an EFAULT in the caller
  • return the pointer

Please sign in to comment.