Differential D7408

mem.
ClosedPublic
Actions

Authored by jhb on Aug 3 2016, 4:55 PM.

Details

Reviewers

kib

Commits

rS308004: MFamd64: Add bounds checks on addresses used with /dev/mem.

Summary

Don't permit mappings of invalid physical addresses on x86 via /dev/mem.

While here, add a bounds check on the physical address used with
memrw on i386. (amd64 already has a check)

Test Plan

mmap an invalid address via /dev/mem. Triggers a panic in an unpatched kernel on access.
I have only tested amd64 so far, need to verify on i386 still.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5716
Build 6011: arc lint + arc unit

Event Timeline

jhb updated this revision to Diff 19005.Aug 3 2016, 4:55 PM

jhb retitled this revision from to Don't permit mappings of invalid physical addresses on x86 via /dev/mem..

jhb updated this object.

jhb edited the test plan for this revision. (Show Details)

jhb added a reviewer: kib.

Herald added a subscriber: imp. · View Herald TranscriptAug 3 2016, 4:55 PM

I think that this may be not completely correct for i386. cpu_maxphyaddr contains raw value from CPUID. On non-PAE systems the result could be that offsets are allowed behind 4G. The result would be non-fatal, but incorrect.

x86/nexus.c has same bug, it seems.

We always set cpu_maxphyaddr. If the cpuid leaf isn't available we set it to either 32 or 36 depending on PAE:

if (cpu_exthigh >= 0x80000008) {
        do_cpuid(0x80000008, regs);
        cpu_maxphyaddr = regs[0] & 0xff;
        cpu_procinfo2 = regs[2];
} else {
        cpu_maxphyaddr = (cpu_feature & CPUID_PAE) != 0 ? 36 : 32;
}

In D7408#154024, @jhb wrote:

We always set cpu_maxphyaddr. If the cpuid leaf isn't available we set it to either 32 or 36 depending on PAE:

I mean that if CPUID leaf is available and typically wider than 32, on !PAE config the offsets > 4G are not representable. They will be wrapped mod 4G, which, as I said above, not fatal, but not correct either.

Hmm, so perhaps the code to set cpu_maxphyaddr should force it to 32 in the i386 && !PAE case?

In D7408#154070, @jhb wrote:

Hmm, so perhaps the code to set cpu_maxphyaddr should force it to 32 in the i386 && !PAE case?

I suspect this might be too limiting overall. If you want to avoid duplicated limiting code repeated several times for i386, perhaps inline function is due in x86_var.h ?

static inline vm_paddr_t
cpu_getmaxusedphyaddr(void)
{
#if defined(__i386__) && !defined(PAE)
   return (0xffffffffU);
#else
   return (1ULL << cpu_maxphyaddr);
#endif
}

I do not think that PAE_TABLES option should result in the full width return.

Export maxphyaddr() as x86-wide cpu_getmaxphyaddr().
Drop debug printf.
Rebase after EFER_NXE fix and /dev/kmem mmap removal.

jhb added inline comments.Oct 26 2016, 7:13 PM

sys/i386/i386/mem.c
111	It was important to check uio_offset and not 'pa' to reject addresses above 4G on i386 'pa' was only 32 bits and would silently truncate on assignment (I originally checked pa). Also, using trunc_page() instead of inlining it seemed more consistent (I can't recall if it mattered for PAE as well)