Re: [PATCH] arm: Use kernel mm when updating section permissions

[Kees Cook <keescook@chromium.org>]

On Thu, Nov 5, 2015 at 5:05 PM, Laura Abbott <labbott@redhat.com> wrote:
> On 11/05/2015 08:27 AM, Russell King - ARM Linux wrote:
>>
>> On Thu, Nov 05, 2015 at 08:20:42AM -0800, Laura Abbott wrote:
>>>
>>> On 11/05/2015 01:46 AM, Russell King - ARM Linux wrote:
>>>>
>>>> On Wed, Nov 04, 2015 at 05:00:39PM -0800, Laura Abbott wrote:
>>>>>
>>>>> Currently, read only permissions are not being applied even
>>>>> when CONFIG_DEBUG_RODATA is set. This is because section_update
>>>>> uses current->mm for adjusting the page tables. current->mm
>>>>> need not be equivalent to the kernel version. Use pgd_offset_k
>>>>> to get the proper page directory for updating.
>>>>
>>>>
>>>> What are you trying to achieve here?  You can't use these functions
>>>> at run time (after the first thread has been spawned) to change
>>>> permissions, because there will be multiple copies of the kernel
>>>> section mappings, and those copies will not get updated.
>>>>
>>>> In any case, this change will probably break kexec and ftrace, as
>>>> the running thread will no longer see the updated page tables.
>>>>
>>>
>>> I think I was hitting that exact problem with multiple copies
>>> not getting updated. The section_update code was being called
>>> and I was seeing the tables get updated but nothing was being
>>> applied when I tried to write to text or check the debugfs
>>> page table. The current flow is:
>>>
>>> rest_init -> kernel_thread(kernel_init) and from that thread
>>> mark_rodata_ro. So mark_rodata_ro is always going to happen
>>> in a thread.
>>>
>>> Do we need to update for both init_mm and the first running
>>> thread?
>>
>>
>> The "first running thread" is merely coincidental for things like kexec.
>>
>> Hmm.  Actually, I think the existing code _should_ be fine.  At the
>> point where mark_rodata_ro() is, we should still be using init_mm, so
>> updating the current threads page tables should actually be updating
>> the swapper_pg_dir.
>
>
> That doesn't seem to hold true. Based on what I'm seeing, we lose
> the the guarantee of init_mm after the first exec. If usermodehelper
> gets called to load a module, that triggers an exec and the kernel
> thread is no longer using init_mm after that. I'm testing with the
> multi-v7 defconfig which uses the smsc911x driver which loads a
> module during initcall. That gets called before mark_rodata_ro so
> the init_mm is never updated. I verified that disabling smsc911x
> makes things work as expected. I suspect the testing was never done
> with a driver that tried to call usermodehelper during init time.

Ooooh. Nice catch. Yeah, my testing didn't include that case.

> I got as far as narrowing it down that it happens after the usermodehelper
> but I wasn't able to pinpoint where exactly the switch happened. It seems
> like we need to have the page tables set up before any initcalls
> happen otherwise we risk having an exec create stray processes which we
> can't update.

Can we just make mark_rodata_ro() a no-op and do the RO setting
earlier when we do the NX setting?

-Kees

-- 
Kees Cook
Chrome OS Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>