Messages in this thread Patch in this message | | | Date | Thu, 25 Apr 2019 11:17:17 +0200 | From | Ingo Molnar <> | 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; }
| |