| Message ID | 546E7748.4000603@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Laura Abbott <lauraa@codeaurora.org> writes: > (cc Rusty Russell for reference) > > On 11/19/2014 3:01 PM, Kees Cook wrote: >> On Mon, Nov 17, 2014 at 2:18 PM, Laura Abbott <lauraa@codeaurora.org> wrote: >>> In a similar manner to arm, it's useful to be able to dump the page >>> tables to verify permissions and memory types. Add a debugfs file >>> to check the page tables. >>> >>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org> >> >> This seems to behave well for me. Thanks! >> >> Tested-by: Kees Cook <keescook@chromium.org> >> >> In my configuration, though, with CONFIG_DEBUG_SET_MODULE_RONX=y, I'm >> only seeing RO, and no NX changes. I haven't constructed a test for >> the module memory behavior directly (to see if this is a page table >> issue or a PTDUMP reporting issue). This series I'm testing has gone >> through some backporting on my end, so I wanted to just double-check >> and see if you saw this too, or if it's specific to my environment: >> >> ---[ Modules ]--- >> 0xffffffbffc000000-0xffffffbffc005000 20K ro x SHD AF >> UXN MEM/NORMAL >> 0xffffffbffc005000-0xffffffbffc007000 8K RW x SHD AF >> UXN MEM/NORMAL >> 0xffffffbffc00c000-0xffffffbffc00e000 8K ro x SHD AF >> UXN MEM/NORMAL >> 0xffffffbffc00e000-0xffffffbffc010000 8K RW x SHD AF >> UXN MEM/NORMAL >> ... >> >> Thanks, >> >> -Kees >> > > Yep, I'm seeing the same thing. We're failing the bounds check: > > if (!is_module_address(start) || !is_module_address(end - 1)) > return -EINVAL; That's a weird test, but I can agree that it's now broken. What's it for? > There are now two problems with this check > > 1) 4982223e51e8 module: set nx before marking module MODULE_STATE_COMING > moved around the order of when nx was set. Now we hit the mod->state == > MODULE_STATE_UNFORMED in __module_address so module_address on anything > always returns false. I think my previous testing must have been done > on a branch without that patch. > > 2) It's possible for the end of the region we are trying to set as nx > to not be fully page size aligned. This seems to be caused by things > getting aligned in layout_section but becoming unaligned in layout_symtab Yes, but you only need the first change in your patch: mod->init_size should already be aligned (and unlike mod->core_size, we haven't modified it). > diff --git a/kernel/module.c b/kernel/module.c > index 972151b..3791330 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -2316,10 +2316,14 @@ static void layout_symtab(struct module *mod, struct load_info *info) > info->stroffs = mod->core_size = info->symoffs + ndst * sizeof(Elf_Sym); > mod->core_size += strtab_size; > > + mod->core_size = debug_align(mod->core_size); > + > /* Put string table section at end of init part of module. */ > strsect->sh_flags |= SHF_ALLOC; > strsect->sh_entsize = get_offset(mod, &mod->init_size, strsect, > info->index.str) | INIT_OFFSET_MASK; > + > + mod->init_size = debug_align(mod->init_size); > pr_debug("\t%s\n", info->secstrings + strsect->sh_name); > } > > I haven't tried a bisect to see if this is new. > > I'm kind of tempted to switch the bounds check back to > (addr >= MODULES_VADDR && addr < MODULES_END) unless there is a clean way to > fixup module.c Thanks, Rusty.
On 11/25/2014 10:05 PM, Rusty Russell wrote: >> >> Yep, I'm seeing the same thing. We're failing the bounds check: >> >> if (!is_module_address(start) || !is_module_address(end - 1)) >> return -EINVAL; > > That's a weird test, but I can agree that it's now broken. What's it for? > Both arm and arm64 map underlying memory with page table mappings that may be greater than PAGE_SIZE. Rather than deal with the hassle of trying to tear down the larger mappings and call stop_machine to flush the TLBs, we just disallow changing attributes of arbitrary memory. Module memory is always mapped with 4K pages so it's safe to change the attributes, hence the bounds check above. On arm we just bounds check via MODULE_START <= addr < MODULE_END so that wasn't affected. >> There are now two problems with this check >> >> 1) 4982223e51e8 module: set nx before marking module MODULE_STATE_COMING >> moved around the order of when nx was set. Now we hit the mod->state == >> MODULE_STATE_UNFORMED in __module_address so module_address on anything >> always returns false. I think my previous testing must have been done >> on a branch without that patch. >> >> 2) It's possible for the end of the region we are trying to set as nx >> to not be fully page size aligned. This seems to be caused by things >> getting aligned in layout_section but becoming unaligned in layout_symtab > > Yes, but you only need the first change in your patch: mod->init_size > should already be aligned (and unlike mod->core_size, we haven't > modified it). > the init size can be modified via get_offset though. In my testing I needed to align up both mod->init_size and mod->core_size for is_module_address to pass on all set_memory_* calls made by the module layer. >> diff --git a/kernel/module.c b/kernel/module.c >> index 972151b..3791330 100644 >> --- a/kernel/module.c >> +++ b/kernel/module.c >> @@ -2316,10 +2316,14 @@ static void layout_symtab(struct module *mod, struct load_info *info) >> info->stroffs = mod->core_size = info->symoffs + ndst * sizeof(Elf_Sym); >> mod->core_size += strtab_size; >> >> + mod->core_size = debug_align(mod->core_size); >> + >> /* Put string table section at end of init part of module. */ >> strsect->sh_flags |= SHF_ALLOC; >> strsect->sh_entsize = get_offset(mod, &mod->init_size, strsect, >> info->index.str) | INIT_OFFSET_MASK; >> + >> + mod->init_size = debug_align(mod->init_size); >> pr_debug("\t%s\n", info->secstrings + strsect->sh_name); >> } >> >> I haven't tried a bisect to see if this is new. >> >> I'm kind of tempted to switch the bounds check back to >> (addr >= MODULES_VADDR && addr < MODULES_END) unless there is a clean way to >> fixup module.c > > Thanks, > Rusty. > Thanks, Laura
Laura Abbott <lauraa@codeaurora.org> writes: > On 11/25/2014 10:05 PM, Rusty Russell wrote: >>> >>> Yep, I'm seeing the same thing. We're failing the bounds check: >>> >>> if (!is_module_address(start) || !is_module_address(end - 1)) >>> return -EINVAL; >> >> That's a weird test, but I can agree that it's now broken. What's it for? >> > > Both arm and arm64 map underlying memory with page table mappings that may > be greater than PAGE_SIZE. Rather than deal with the hassle of trying to > tear down the larger mappings and call stop_machine to flush the TLBs, we > just disallow changing attributes of arbitrary memory. Module memory is > always mapped with 4K pages so it's safe to change the attributes, hence > the bounds check above. On arm we just bounds check via > MODULE_START <= addr < MODULE_END so that wasn't affected. OK, perhaps as you say, we should do this. >> Yes, but you only need the first change in your patch: mod->init_size >> should already be aligned (and unlike mod->core_size, we haven't >> modified it). >> > > the init size can be modified via get_offset though. In my testing I needed > to align up both mod->init_size and mod->core_size for is_module_address to > pass on all set_memory_* calls made by the module layer. You're right. It doesn't seem to hurt any other code path, but if you want this, please send me a proper explanation w/ signed-off-by. Thanks! Rusty. >>> diff --git a/kernel/module.c b/kernel/module.c >>> index 972151b..3791330 100644 >>> --- a/kernel/module.c >>> +++ b/kernel/module.c >>> @@ -2316,10 +2316,14 @@ static void layout_symtab(struct module *mod, struct load_info *info) >>> info->stroffs = mod->core_size = info->symoffs + ndst * sizeof(Elf_Sym); >>> mod->core_size += strtab_size; >>> >>> + mod->core_size = debug_align(mod->core_size); >>> + >>> /* Put string table section at end of init part of module. */ >>> strsect->sh_flags |= SHF_ALLOC; >>> strsect->sh_entsize = get_offset(mod, &mod->init_size, strsect, >>> info->index.str) | INIT_OFFSET_MASK; >>> + >>> + mod->init_size = debug_align(mod->init_size); >>> pr_debug("\t%s\n", info->secstrings + strsect->sh_name); >>> } >>> >>> I haven't tried a bisect to see if this is new. >>> >>> I'm kind of tempted to switch the bounds check back to >>> (addr >= MODULES_VADDR && addr < MODULES_END) unless there is a clean way to >>> fixup module.c >> >> Thanks, >> Rusty. >> > > Thanks, > Laura > > -- > Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project
diff --git a/kernel/module.c b/kernel/module.c index 972151b..3791330 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -2316,10 +2316,14 @@ static void layout_symtab(struct module *mod, struct load_info *info) info->stroffs = mod->core_size = info->symoffs + ndst * sizeof(Elf_Sym); mod->core_size += strtab_size; + mod->core_size = debug_align(mod->core_size); + /* Put string table section at end of init part of module. */ strsect->sh_flags |= SHF_ALLOC; strsect->sh_entsize = get_offset(mod, &mod->init_size, strsect, info->index.str) | INIT_OFFSET_MASK; + + mod->init_size = debug_align(mod->init_size); pr_debug("\t%s\n", info->secstrings + strsect->sh_name); }