LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Miroslav Benes <mbenes@suse.cz>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
	Jiri Kosina <jikos@kernel.org>, Petr Mladek <pmladek@suse.com>,
	Jessica Yu <jeyu@kernel.org>,
	Joe Lawrence <joe.lawrence@redhat.com>,
	linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
	Johannes Erdfelt <johannes@erdfelt.com>,
	Ingo Molnar <mingo@kernel.org>
Subject: Re: [PATCH] livepatch: Fix ftrace module text permissions race
Date: Fri, 31 May 2019 10:49:02 +0200 (CEST)	[thread overview]
Message-ID: <alpine.LSU.2.21.1905311045040.742@pobox.suse.cz> (raw)
In-Reply-To: <bb69d4ac34111bbd9cb16180a6fafe471a88d80b.1559156299.git.jpoimboe@redhat.com>

On Wed, 29 May 2019, Josh Poimboeuf wrote:

> It's possible for livepatch and ftrace to be toggling a module's text
> permissions at the same time, resulting in the following panic:
> 
>   BUG: unable to handle page fault for address: ffffffffc005b1d9
>   #PF: supervisor write access in kernel mode
>   #PF: error_code(0x0003) - permissions violation
>   PGD 3ea0c067 P4D 3ea0c067 PUD 3ea0e067 PMD 3cc13067 PTE 3b8a1061
>   Oops: 0003 [#1] PREEMPT SMP PTI
>   CPU: 1 PID: 453 Comm: insmod Tainted: G           O  K   5.2.0-rc1-a188339ca5 #1
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-20181126_142135-anatol 04/01/2014
>   RIP: 0010:apply_relocate_add+0xbe/0x14c
>   Code: fa 0b 74 21 48 83 fa 18 74 38 48 83 fa 0a 75 40 eb 08 48 83 38 00 74 33 eb 53 83 38 00 75 4e 89 08 89 c8 eb 0a 83 38 00 75 43 <89> 08 48 63 c1 48 39 c8 74 2e eb 48 83 38 00 75 32 48 29 c1 89 08
>   RSP: 0018:ffffb223c00dbb10 EFLAGS: 00010246
>   RAX: ffffffffc005b1d9 RBX: 0000000000000000 RCX: ffffffff8b200060
>   RDX: 000000000000000b RSI: 0000004b0000000b RDI: ffff96bdfcd33000
>   RBP: ffffb223c00dbb38 R08: ffffffffc005d040 R09: ffffffffc005c1f0
>   R10: ffff96bdfcd33c40 R11: ffff96bdfcd33b80 R12: 0000000000000018
>   R13: ffffffffc005c1f0 R14: ffffffffc005e708 R15: ffffffff8b2fbc74
>   FS:  00007f5f447beba8(0000) GS:ffff96bdff900000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: ffffffffc005b1d9 CR3: 000000003cedc002 CR4: 0000000000360ea0
>   DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>   DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>   Call Trace:
>    klp_init_object_loaded+0x10f/0x219
>    ? preempt_latency_start+0x21/0x57
>    klp_enable_patch+0x662/0x809
>    ? virt_to_head_page+0x3a/0x3c
>    ? kfree+0x8c/0x126
>    patch_init+0x2ed/0x1000 [livepatch_test02]
>    ? 0xffffffffc0060000
>    do_one_initcall+0x9f/0x1c5
>    ? kmem_cache_alloc_trace+0xc4/0xd4
>    ? do_init_module+0x27/0x210
>    do_init_module+0x5f/0x210
>    load_module+0x1c41/0x2290
>    ? fsnotify_path+0x3b/0x42
>    ? strstarts+0x2b/0x2b
>    ? kernel_read+0x58/0x65
>    __do_sys_finit_module+0x9f/0xc3
>    ? __do_sys_finit_module+0x9f/0xc3
>    __x64_sys_finit_module+0x1a/0x1c
>    do_syscall_64+0x52/0x61
>    entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> The above panic occurs when loading two modules at the same time with
> ftrace enabled, where at least one of the modules is a livepatch module:
> 
> CPU0					CPU1
> klp_enable_patch()
>   klp_init_object_loaded()
>     module_disable_ro()
>     					ftrace_module_enable()
> 					  ftrace_arch_code_modify_post_process()
> 				    	    set_all_modules_text_ro()
>       klp_write_object_relocations()
>         apply_relocate_add()
> 	  *patches read-only code* - BOOM
> 
> A similar race exists when toggling ftrace while loading a livepatch
> module.
> 
> Fix it by ensuring that the livepatch and ftrace code patching
> operations -- and their respective permissions changes -- are protected
> by the text_mutex.
> 
> Reported-by: Johannes Erdfelt <johannes@erdfelt.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

For the code

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

However, shouldn't the patch be split in two? One adding text_mutex 
protection to livepatch and ftrace. The other adding lockdep_assert_held() 
and __module_enable_ro()? The current changelog does not mention lockdep 
changes at all.

Miroslav


      parent reply	other threads:[~2019-05-31  8:49 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-29 19:02 Josh Poimboeuf
2019-05-30 11:57 ` Jessica Yu
2019-05-30 13:54 ` Petr Mladek
2019-05-31 19:12   ` Josh Poimboeuf
2019-05-31 22:25     ` Josh Poimboeuf
2019-06-13 21:38       ` Steven Rostedt
2019-06-14  0:57         ` Josh Poimboeuf
2019-05-31  8:49 ` Miroslav Benes [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LSU.2.21.1905311045040.742@pobox.suse.cz \
    --to=mbenes@suse.cz \
    --cc=jeyu@kernel.org \
    --cc=jikos@kernel.org \
    --cc=joe.lawrence@redhat.com \
    --cc=johannes@erdfelt.com \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    --subject='Re: [PATCH] livepatch: Fix ftrace module text permissions race' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).