From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964988AbdKCCTp (ORCPT ); Thu, 2 Nov 2017 22:19:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46820 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934708AbdKCCTn (ORCPT ); Thu, 2 Nov 2017 22:19:43 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 839275D5E9 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=jpoimboe@redhat.com Date: Thu, 2 Nov 2017 21:19:42 -0500 From: Josh Poimboeuf To: jeyu@kernel.org Cc: linux-kernel@vger.kernel.org, live-patching@vger.kernel.org Subject: Re: [PATCH] x86/module: Detect corrupt relocations against nonzero data Message-ID: <20171103021942.nfxsdp43cziaoiow@treble> References: <3999d59e87ad73d4030e98bd90a6ffc00bc054d2.1509659611.git.jpoimboe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <3999d59e87ad73d4030e98bd90a6ffc00bc054d2.1509659611.git.jpoimboe@redhat.com> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 03 Nov 2017 02:19:43 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 02, 2017 at 04:57:11PM -0500, Josh Poimboeuf wrote: > There have been some cases where external tooling (e.g., kpatch-build) > creates a corrupt relocation which targets the wrong address. This is a > silent failure which can corrupt memory in unexpected places. > > On x86, the bytes of data being overwritten by relocations are always > initialized to zero beforehand. Use that knowledge to add sanity checks > to detect such cases before they corrupt memory. > > Signed-off-by: Josh Poimboeuf > --- > arch/x86/kernel/module.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c > index 62e7d70aadd5..a69b12617820 100644 > --- a/arch/x86/kernel/module.c > +++ b/arch/x86/kernel/module.c > @@ -172,19 +172,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs, > case R_X86_64_NONE: > break; > case R_X86_64_64: > + if (*(u64 *)loc != 0) > + goto nonzero; > *(u64 *)loc = val; > break; > case R_X86_64_32: > + if (*(u32 *)loc != 0) > + goto nonzero; > *(u32 *)loc = val; > if (val != *(u32 *)loc) > goto overflow; > break; > case R_X86_64_32S: > + if (*(s32 *)loc != 0) > + goto nonzero; > *(s32 *)loc = val; > if ((s64)val != *(s32 *)loc) > goto overflow; > break; > case R_X86_64_PC32: > + if (*(u64 *)loc != 0) > + goto nonzero; > val -= (u64)loc; > *(u32 *)loc = val; NACK - this last bit is obviously a bug, not sure how it passed my testing without module load failures... -- Josh