[PATCH] arm: module: handle negative R_ARM_PREL31 addends correctly

[PATCH] arm: module: handle negative R_ARM_PREL31 addends correctly

[Ard Biesheuvel]

According to the spec 'ELF for the ARM Architecture' (IHI 0044E),
addends for R_ARM_PREL31 relocations are 31-bit signed quantities,
so we need to sign extend the value to 32 bits before it can be used
as an offset in the calculation of the relocated value.

We have not been bitten by this because these relocations are usually
emitted against the start of a section, which means the addends never
assume negative values in practice. But it is a bug nonetheless, so fix
it.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
This is something I spotted while looking into adding support for
R_ARM_REL32 relocations. Feel free to ignore if it is guaranteed in some
way that these relocations can never be emitted with negative addends.

 arch/arm/kernel/module.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 29629fe02ce5..20d8374711e2 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -155,7 +155,15 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 		       break;
 
 		case R_ARM_PREL31:
-			offset = *(u32 *)loc + sym->st_value - loc;
+			offset = (*(s32 *)loc << 1) >> 1; /* sign extend */
+			offset += sym->st_value - loc;
+			if (offset >= 0x40000000 || offset < -0x40000000) {
+				pr_err("%s: section %u reloc %u sym '%s': relocation %u out of range (%#lx -> %#x)\n",
+				       module->name, relindex, i, symname,
+				       ELF32_R_TYPE(rel->r_info), loc,
+				       sym->st_value);
+				return -ENOEXEC;
+			}
 			*(u32 *)loc = offset & 0x7fffffff;
 			break;
 
-- 
2.7.4

[PATCH] arm: module: handle negative R_ARM_PREL31 addends correctly

[Russell King - ARM Linux]

On Fri, Jan 20, 2017 at 10:21:03AM +0000, Ard Biesheuvel wrote:
> According to the spec 'ELF for the ARM Architecture' (IHI 0044E),
> addends for R_ARM_PREL31 relocations are 31-bit signed quantities,
> so we need to sign extend the value to 32 bits before it can be used
> as an offset in the calculation of the relocated value.
> 
> We have not been bitten by this because these relocations are usually
> emitted against the start of a section, which means the addends never
> assume negative values in practice. But it is a bug nonetheless, so fix
> it.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> This is something I spotted while looking into adding support for
> R_ARM_REL32 relocations. Feel free to ignore if it is guaranteed in some
> way that these relocations can never be emitted with negative addends.

I still think this is a good thing to have, even if we should never
see them - it avoids updating the relocation with an incorrect value.
Any relocations we perform should be guaranteed to be correct, or
we should fail.

Please put it in the patch system, thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm: module: handle negative R_ARM_PREL31 addends correctly

[Ard Biesheuvel]

On 30 January 2017 at 12:39, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Fri, Jan 20, 2017 at 10:21:03AM +0000, Ard Biesheuvel wrote:
>> According to the spec 'ELF for the ARM Architecture' (IHI 0044E),
>> addends for R_ARM_PREL31 relocations are 31-bit signed quantities,
>> so we need to sign extend the value to 32 bits before it can be used
>> as an offset in the calculation of the relocated value.
>>
>> We have not been bitten by this because these relocations are usually
>> emitted against the start of a section, which means the addends never
>> assume negative values in practice. But it is a bug nonetheless, so fix
>> it.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> This is something I spotted while looking into adding support for
>> R_ARM_REL32 relocations. Feel free to ignore if it is guaranteed in some
>> way that these relocations can never be emitted with negative addends.
>
> I still think this is a good thing to have, even if we should never
> see them - it avoids updating the relocation with an incorrect value.
> Any relocations we perform should be guaranteed to be correct, or
> we should fail.
>

Agreed.

> Please put it in the patch system, thanks.
>

Actually, I think it may still be incorrect: it seems to me that we
should preserve bit 31 rather than clear it. From the EHABI

"""
Bit 31 of the relocated word does not participate in the relocation
and may be used as data.
"""
(IHI0038B page 13)

and

"""
... The landing pad word contains a prel31 offset to a landing pad,
with bit 31 set if the catch handler catches a reference type and bit
31 clear otherwise. ...
"""
(IHI0038B page 39)

which suggests to me that bit 31 could legally assume either truth
value, and should not be touched by the relocation machinery.

[PATCH] arm: module: handle negative R_ARM_PREL31 addends correctly

[Russell King - ARM Linux]

On Mon, Jan 30, 2017 at 03:08:56PM +0000, Ard Biesheuvel wrote:
> Actually, I think it may still be incorrect: it seems to me that we
> should preserve bit 31 rather than clear it. From the EHABI

Agreed.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.