lkml.org 
[lkml]   [2019]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
Subjectx86/paravirt: Detect over-sized patching bugs in paravirt_patch_call()

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Apr 25, 2019 at 11:17:17AM +0200, Ingo Molnar wrote:
> > It basically means that we silently won't do any patching and the kernel
> > will crash later on in mysterious ways, because paravirt patching is
> > usually relied on.
>
> That's OK. The compiler emits an indirect CALL/JMP to the pv_ops
> structure contents. That _should_ stay valid and function correctly at
> all times.

It might result in a correctly executing kernel in terms of code
generation, but it doesn't result in a viable kernel: some of the places
rely on the patching going through and don't know what to do when it
doesn't and misbehave or crash in interesting ways.

Guess how I know this. ;-)

> Not patching should at the very least cause a WARN with RETPOLINE
> kernels though, we hard rely on the patching actually working and
> writing at least a direct call.

We hard rely in other places too.

How about just BUG_ON()-ing in paravirt_patch_call() as well? It's not
like these are *supposed* to fail, and if they do we want to know it,
even if the outcome might be more benign in some cases?

I.e. how about the patch below? This not only makes it more apparent when
patching fails, it also makes the kernel smaller and removes an #ifdef
ugly.

I tried it with a richly paravirt-enabled kernel and no patching bugs
were detected.

Thanks,

Ingo

Signed-off-by: Ingo Molnar <mingo@kernel.org>

---
arch/x86/kernel/paravirt.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 7f9121f2fdac..544d386ded45 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -73,21 +73,21 @@ struct branch {
static unsigned paravirt_patch_call(void *insnbuf, const void *target,
unsigned long addr, unsigned len)
{
+ const int call_len = 5;
struct branch *b = insnbuf;
- unsigned long delta = (unsigned long)target - (addr+5);
+ unsigned long delta = (unsigned long)target - (addr+call_len);

- if (len < 5) {
-#ifdef CONFIG_RETPOLINE
- WARN_ONCE(1, "Failing to patch indirect CALL in %ps\n", (void *)addr);
-#endif
- return len; /* call too long for patch site */
+ if (len < call_len) {
+ pr_warn("paravirt: Failed to patch indirect CALL at %ps\n", (void *)addr);
+ /* Kernel might not be viable if patching fails, bail out: */
+ BUG_ON(1);
}

b->opcode = 0xe8; /* call */
b->delta = delta;
- BUILD_BUG_ON(sizeof(*b) != 5);
+ BUILD_BUG_ON(sizeof(*b) != call_len);

- return 5;
+ return call_len;
}

#ifdef CONFIG_PARAVIRT_XXL
\
 
 \ /
  Last update: 2019-04-25 11:50    [W:0.091 / U:7.788 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site