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
Subject[PATCH] x86/paravirt: Detect oversized patching bugs as they happen and BUG_ON() to avoid later crashes

* Ingo Molnar <mingo@kernel.org> wrote:

> Third, beyond readability there's another advantage of my suggested
> approach as well: for example that way we could verify the passed in
> length with the patchlet length. Right now it's completely unverified:
>
> case PARAVIRT_PATCH(ops.m): \
> return PATCH(data, ops##_##m, ibuf, len)
>
> right now we don't check whether the 'len' passed in by the usage site
> matches the actual structure field length.
>
> Although maybe we could do that with your C space structure as well.

So I was wrong here, got confused by the 'len' name which doesn't mean
what it suggests: it's not the length of the patchlet, but the
maximum/original length of the patch site - which we trim down in
paravirt_patch_insns():

unsigned paravirt_patch_insns(void *insnbuf, unsigned len,
const char *start, const char *end)
{
unsigned insn_len = end - start;

if (insn_len > len || start == NULL)
insn_len = len;
else
memcpy(insnbuf, start, insn_len);

return insn_len;
}

What is the logic behind silently returning 'len' here and not copying
anything?

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.

Instead I think we should BUG_ON() that condition with the patch below -
there's no way to continue successfully at that point.

I've tested this patch, with the vanilla kernel check never triggers, and
if I intentionally increase the size of one of the patch templates to a
too high value the assert triggers:

[ 0.164385] kernel BUG at arch/x86/kernel/paravirt.c:167!

Without this patch a broken kernel randomly crashes in later places,
after the silent patching failure.

Thanks,

Ingo

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

--- tip.orig/arch/x86/kernel/paravirt.c
+++ tip/arch/x86/kernel/paravirt.c
@@ -163,10 +163,10 @@ unsigned paravirt_patch_insns(void *insn
{
unsigned insn_len = end - start;

- if (insn_len > len || start == NULL)
- insn_len = len;
- else
- memcpy(insnbuf, start, insn_len);
+ /* Alternative instruction is too large for the patch site and we cannot continue: */
+ BUG_ON(insn_len > len || start == NULL);
+
+ memcpy(insnbuf, start, insn_len);

return insn_len;
}
\
 
 \ /
  Last update: 2019-04-25 11:19    [W:0.076 / U:11.528 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site