[PATCH] x86/module: Detect corrupt relocations against nonzero data

[PATCH] x86/module: Detect corrupt relocations against nonzero data

[Josh Poimboeuf]

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 <jpoimboe@redhat.com>
---
 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;
 #if 0
@@ -200,6 +208,11 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 	}
 	return 0;
 
+nonzero:
+	pr_err("corrupt relocation: existing data is nonzero for type %d loc %p val %Lx\n",
+	       (int)ELF64_R_TYPE(rel[i].r_info), loc, val);
+	return -ENOEXEC;
+
 overflow:
 	pr_err("overflow in relocation type %d val %Lx\n",
 	       (int)ELF64_R_TYPE(rel[i].r_info), val);
-- 
2.13.6

Re: [PATCH] x86/module: Detect corrupt relocations against nonzero data

[Josh Poimboeuf]

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 <jpoimboe@redhat.com>
> ---
>  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

Re: x86/module: Detect corrupt relocations against nonzero data

[Jessica Yu]

+++ Josh Poimboeuf [02/11/17 21:19 -0500]:
>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 <jpoimboe@redhat.com>
>> ---
>>  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...

Thanks for the patch Josh, btw - could you also CC the x86 folks when
you send out v2? 

Thanks!

Jessica