| Message ID | 20190527064413.21304-1-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/module: revert to unsigned interpretation of ABS16/32 relocations | expand |
On Mon, May 27, 2019 at 08:44:13AM +0200, Ard Biesheuvel wrote: > Commit 1cf24a2cc3fd > > ("arm64/module: deal with ambiguity in PRELxx relocation ranges") > > updated the overflow checking logic in the relocation handling code to > ensure that PREL16/32 relocations don't overflow signed quantities. > > However, the same code path is used for absolute relocations, where the > interpretation is the opposite: the only current use case for absolute > relocations operating on non-native word size quantities is the CRC32 > handling in the CONFIG_MODVERSIONS code, and these CRCs are unsigned > 32-bit quantities, which are now being rejected by the module loader > if bit 31 happens to be set. > > So let's use different ranges for quanties subject to absolute vs. > relative relocations: > - ABS16/32 relocations should be in the range [0, Uxx_MAX) > - PREL16/32 relocations should be in the range [Sxx_MIN, Sxx_MAX) > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/kernel/module.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > index f32359cffb01..85fb63c1ba3a 100644 > --- a/arch/arm64/kernel/module.c > +++ b/arch/arm64/kernel/module.c > @@ -98,10 +98,10 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > /* > * The ELF psABI for AArch64 documents the 16-bit and 32-bit place > - * relative relocations as having a range of [-2^15, 2^16) or > - * [-2^31, 2^32), respectively. However, in order to be able to detect > - * overflows reliably, we have to choose whether we interpret such > - * quantities as signed or as unsigned, and stick with it. > + * relative and absolute relocations as having a range of [-2^15, 2^16) > + * or [-2^31, 2^32), respectively. However, in order to be able to > + * detect overflows reliably, we have to choose whether we interpret > + * such quantities as signed or as unsigned, and stick with it. > * The way we organize our address space requires a signed > * interpretation of 32-bit relative references, so let's use that > * for all R_AARCH64_PRELxx relocations. This means our upper > @@ -111,12 +111,14 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > switch (len) { > case 16: > *(s16 *)place = sval; > - if (sval < S16_MIN || sval > S16_MAX) > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U16_MAX)) || > + (op == RELOC_OP_PREL && (sval < S16_MIN || sval > S16_MAX))) > return -ERANGE; > break; > case 32: > *(s32 *)place = sval; > - if (sval < S32_MIN || sval > S32_MAX) > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U32_MAX)) || > + (op == RELOC_OP_PREL && (sval < S32_MIN || sval > S32_MAX))) > return -ERANGE; > break; > case 64: Hmm. I worry that this isn't the last time we're going to be tweaking this, so I'm wondering whether or not we should just revert 1cf24a2cc3fd instead. However, if you still want to pursue this, please can you restructure the check so that we do: if (op == RELOC_OP_PREL) { /* Comment about deviation from ELF ABI */ if (signed overflow check) return -ERANGE; } else if (unsigned overflow check) { return -ERANGE; } i.e. drop the explicit check of ABS so that the default behaviour follows the ELF spec? Will
On Tue, 28 May 2019 at 12:11, Will Deacon <will.deacon@arm.com> wrote: > > On Mon, May 27, 2019 at 08:44:13AM +0200, Ard Biesheuvel wrote: > > Commit 1cf24a2cc3fd > > > > ("arm64/module: deal with ambiguity in PRELxx relocation ranges") > > > > updated the overflow checking logic in the relocation handling code to > > ensure that PREL16/32 relocations don't overflow signed quantities. > > > > However, the same code path is used for absolute relocations, where the > > interpretation is the opposite: the only current use case for absolute > > relocations operating on non-native word size quantities is the CRC32 > > handling in the CONFIG_MODVERSIONS code, and these CRCs are unsigned > > 32-bit quantities, which are now being rejected by the module loader > > if bit 31 happens to be set. > > > > So let's use different ranges for quanties subject to absolute vs. > > relative relocations: > > - ABS16/32 relocations should be in the range [0, Uxx_MAX) > > - PREL16/32 relocations should be in the range [Sxx_MIN, Sxx_MAX) > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > arch/arm64/kernel/module.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > > index f32359cffb01..85fb63c1ba3a 100644 > > --- a/arch/arm64/kernel/module.c > > +++ b/arch/arm64/kernel/module.c > > @@ -98,10 +98,10 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > > > /* > > * The ELF psABI for AArch64 documents the 16-bit and 32-bit place > > - * relative relocations as having a range of [-2^15, 2^16) or > > - * [-2^31, 2^32), respectively. However, in order to be able to detect > > - * overflows reliably, we have to choose whether we interpret such > > - * quantities as signed or as unsigned, and stick with it. > > + * relative and absolute relocations as having a range of [-2^15, 2^16) > > + * or [-2^31, 2^32), respectively. However, in order to be able to > > + * detect overflows reliably, we have to choose whether we interpret > > + * such quantities as signed or as unsigned, and stick with it. > > * The way we organize our address space requires a signed > > * interpretation of 32-bit relative references, so let's use that > > * for all R_AARCH64_PRELxx relocations. This means our upper > > @@ -111,12 +111,14 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > switch (len) { > > case 16: > > *(s16 *)place = sval; > > - if (sval < S16_MIN || sval > S16_MAX) > > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U16_MAX)) || > > + (op == RELOC_OP_PREL && (sval < S16_MIN || sval > S16_MAX))) > > return -ERANGE; > > break; > > case 32: > > *(s32 *)place = sval; > > - if (sval < S32_MIN || sval > S32_MAX) > > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U32_MAX)) || > > + (op == RELOC_OP_PREL && (sval < S32_MIN || sval > S32_MAX))) > > return -ERANGE; > > break; > > case 64: > > Hmm. I worry that this isn't the last time we're going to be tweaking this, > so I'm wondering whether or not we should just revert 1cf24a2cc3fd instead. > Both absolute and relative 32-bit references can really only be emitted using assembler code, since C code only uses the former, and only uses it to resolve symbol addresses not bare values. ABS32 is only used by the CONFIG_MODVERSIONS code, and PREL32 is only used by the few data structures where we deliberately use relative references to get rid of the RELA entries associated with absolute references. So I don't share your concern here, although I understand where it is coming from. *However*, not being able to detect overflow is *really* bad, so even if there is another tweak behind the horizon, this is still better than silent data corruption because your data reference is off by 4 GB. > However, if you still want to pursue this, please can you restructure the > check so that we do: > > if (op == RELOC_OP_PREL) { > /* Comment about deviation from ELF ABI */ > if (signed overflow check) > return -ERANGE; > } else if (unsigned overflow check) { > return -ERANGE; > } > > i.e. drop the explicit check of ABS so that the default behaviour follows > the ELF spec? > My patch does not follow the ELF spec for either ABS16/32 or PREL16/32 relocations. It chooses an unsigned interpretation for the former, and a signed one for the latter, and I think this is the only way to deal with this properly. This is why I updated the comment as well: we should apply a strict (but different) interpretation to both kinds of relocations, not just the relative ones.
On Tue, May 28, 2019 at 12:20:53PM +0200, Ard Biesheuvel wrote: > On Tue, 28 May 2019 at 12:11, Will Deacon <will.deacon@arm.com> wrote: > > > > On Mon, May 27, 2019 at 08:44:13AM +0200, Ard Biesheuvel wrote: > > > Commit 1cf24a2cc3fd > > > > > > ("arm64/module: deal with ambiguity in PRELxx relocation ranges") > > > > > > updated the overflow checking logic in the relocation handling code to > > > ensure that PREL16/32 relocations don't overflow signed quantities. > > > > > > However, the same code path is used for absolute relocations, where the > > > interpretation is the opposite: the only current use case for absolute > > > relocations operating on non-native word size quantities is the CRC32 > > > handling in the CONFIG_MODVERSIONS code, and these CRCs are unsigned > > > 32-bit quantities, which are now being rejected by the module loader > > > if bit 31 happens to be set. > > > > > > So let's use different ranges for quanties subject to absolute vs. > > > relative relocations: > > > - ABS16/32 relocations should be in the range [0, Uxx_MAX) > > > - PREL16/32 relocations should be in the range [Sxx_MIN, Sxx_MAX) > > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > --- > > > arch/arm64/kernel/module.c | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > > > index f32359cffb01..85fb63c1ba3a 100644 > > > --- a/arch/arm64/kernel/module.c > > > +++ b/arch/arm64/kernel/module.c > > > @@ -98,10 +98,10 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > > > > > /* > > > * The ELF psABI for AArch64 documents the 16-bit and 32-bit place > > > - * relative relocations as having a range of [-2^15, 2^16) or > > > - * [-2^31, 2^32), respectively. However, in order to be able to detect > > > - * overflows reliably, we have to choose whether we interpret such > > > - * quantities as signed or as unsigned, and stick with it. > > > + * relative and absolute relocations as having a range of [-2^15, 2^16) > > > + * or [-2^31, 2^32), respectively. However, in order to be able to > > > + * detect overflows reliably, we have to choose whether we interpret > > > + * such quantities as signed or as unsigned, and stick with it. > > > * The way we organize our address space requires a signed > > > * interpretation of 32-bit relative references, so let's use that > > > * for all R_AARCH64_PRELxx relocations. This means our upper > > > @@ -111,12 +111,14 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > > switch (len) { > > > case 16: > > > *(s16 *)place = sval; > > > - if (sval < S16_MIN || sval > S16_MAX) > > > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U16_MAX)) || > > > + (op == RELOC_OP_PREL && (sval < S16_MIN || sval > S16_MAX))) > > > return -ERANGE; > > > break; > > > case 32: > > > *(s32 *)place = sval; > > > - if (sval < S32_MIN || sval > S32_MAX) > > > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U32_MAX)) || > > > + (op == RELOC_OP_PREL && (sval < S32_MIN || sval > S32_MAX))) > > > return -ERANGE; > > > break; > > > case 64: > > > > Hmm. I worry that this isn't the last time we're going to be tweaking this, > > so I'm wondering whether or not we should just revert 1cf24a2cc3fd instead. > > > > Both absolute and relative 32-bit references can really only be > emitted using assembler code, since C code only uses the former, and > only uses it to resolve symbol addresses not bare values. > > ABS32 is only used by the CONFIG_MODVERSIONS code, and PREL32 is only > used by the few data structures where we deliberately use relative > references to get rid of the RELA entries associated with absolute > references. > > So I don't share your concern here, although I understand where it is > coming from. Fair enough, although I'm not claiming that there's a bug in your patch (it looks fine), just more that I dislike the deviation we're introducing here. > *However*, not being able to detect overflow is *really* bad, so even > if there is another tweak behind the horizon, this is still better > than silent data corruption because your data reference is off by 4 > GB. Agreed. > > However, if you still want to pursue this, please can you restructure the > > check so that we do: > > > > if (op == RELOC_OP_PREL) { > > /* Comment about deviation from ELF ABI */ > > if (signed overflow check) > > return -ERANGE; > > } else if (unsigned overflow check) { > > return -ERANGE; > > } > > > > i.e. drop the explicit check of ABS so that the default behaviour follows > > the ELF spec? > > > > My patch does not follow the ELF spec for either ABS16/32 or PREL16/32 > relocations. It chooses an unsigned interpretation for the former, and > a signed one for the latter, and I think this is the only way to deal > with this properly. > > This is why I updated the comment as well: we should apply a strict > (but different) interpretation to both kinds of relocations, not just > the relative ones. Ah, apologies, I missed that you'd changed the ABS behaviour from what it was prior to 1cf24a2cc3fd. My main gripe is that our (currently unused) fallthrough case (for op != PREL or ABS) is to elide the check altogether, whereas I would prefer that we do our special casing with the comment, and then keep the old overflow check for everything else, even though it's really just there for documentation purposes. Will
On Tue, 28 May 2019 at 13:52, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, May 28, 2019 at 12:20:53PM +0200, Ard Biesheuvel wrote: > > On Tue, 28 May 2019 at 12:11, Will Deacon <will.deacon@arm.com> wrote: > > > > > > On Mon, May 27, 2019 at 08:44:13AM +0200, Ard Biesheuvel wrote: > > > > Commit 1cf24a2cc3fd > > > > > > > > ("arm64/module: deal with ambiguity in PRELxx relocation ranges") > > > > > > > > updated the overflow checking logic in the relocation handling code to > > > > ensure that PREL16/32 relocations don't overflow signed quantities. > > > > > > > > However, the same code path is used for absolute relocations, where the > > > > interpretation is the opposite: the only current use case for absolute > > > > relocations operating on non-native word size quantities is the CRC32 > > > > handling in the CONFIG_MODVERSIONS code, and these CRCs are unsigned > > > > 32-bit quantities, which are now being rejected by the module loader > > > > if bit 31 happens to be set. > > > > > > > > So let's use different ranges for quanties subject to absolute vs. > > > > relative relocations: > > > > - ABS16/32 relocations should be in the range [0, Uxx_MAX) > > > > - PREL16/32 relocations should be in the range [Sxx_MIN, Sxx_MAX) > > > > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > --- > > > > arch/arm64/kernel/module.c | 14 ++++++++------ > > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > > > > index f32359cffb01..85fb63c1ba3a 100644 > > > > --- a/arch/arm64/kernel/module.c > > > > +++ b/arch/arm64/kernel/module.c > > > > @@ -98,10 +98,10 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > > > > > > > /* > > > > * The ELF psABI for AArch64 documents the 16-bit and 32-bit place > > > > - * relative relocations as having a range of [-2^15, 2^16) or > > > > - * [-2^31, 2^32), respectively. However, in order to be able to detect > > > > - * overflows reliably, we have to choose whether we interpret such > > > > - * quantities as signed or as unsigned, and stick with it. > > > > + * relative and absolute relocations as having a range of [-2^15, 2^16) > > > > + * or [-2^31, 2^32), respectively. However, in order to be able to > > > > + * detect overflows reliably, we have to choose whether we interpret > > > > + * such quantities as signed or as unsigned, and stick with it. > > > > * The way we organize our address space requires a signed > > > > * interpretation of 32-bit relative references, so let's use that > > > > * for all R_AARCH64_PRELxx relocations. This means our upper > > > > @@ -111,12 +111,14 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > > > switch (len) { > > > > case 16: > > > > *(s16 *)place = sval; > > > > - if (sval < S16_MIN || sval > S16_MAX) > > > > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U16_MAX)) || > > > > + (op == RELOC_OP_PREL && (sval < S16_MIN || sval > S16_MAX))) > > > > return -ERANGE; > > > > break; > > > > case 32: > > > > *(s32 *)place = sval; > > > > - if (sval < S32_MIN || sval > S32_MAX) > > > > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U32_MAX)) || > > > > + (op == RELOC_OP_PREL && (sval < S32_MIN || sval > S32_MAX))) > > > > return -ERANGE; > > > > break; > > > > case 64: > > > > > > Hmm. I worry that this isn't the last time we're going to be tweaking this, > > > so I'm wondering whether or not we should just revert 1cf24a2cc3fd instead. > > > > > > > Both absolute and relative 32-bit references can really only be > > emitted using assembler code, since C code only uses the former, and > > only uses it to resolve symbol addresses not bare values. > > > > ABS32 is only used by the CONFIG_MODVERSIONS code, and PREL32 is only > > used by the few data structures where we deliberately use relative > > references to get rid of the RELA entries associated with absolute > > references. > > > > So I don't share your concern here, although I understand where it is > > coming from. > > Fair enough, although I'm not claiming that there's a bug in your patch > (it looks fine), just more that I dislike the deviation we're introducing > here. > > > *However*, not being able to detect overflow is *really* bad, so even > > if there is another tweak behind the horizon, this is still better > > than silent data corruption because your data reference is off by 4 > > GB. > > Agreed. > > > > However, if you still want to pursue this, please can you restructure the > > > check so that we do: > > > > > > if (op == RELOC_OP_PREL) { > > > /* Comment about deviation from ELF ABI */ > > > if (signed overflow check) > > > return -ERANGE; > > > } else if (unsigned overflow check) { > > > return -ERANGE; > > > } > > > > > > i.e. drop the explicit check of ABS so that the default behaviour follows > > > the ELF spec? > > > > > > > My patch does not follow the ELF spec for either ABS16/32 or PREL16/32 > > relocations. It chooses an unsigned interpretation for the former, and > > a signed one for the latter, and I think this is the only way to deal > > with this properly. > > > > This is why I updated the comment as well: we should apply a strict > > (but different) interpretation to both kinds of relocations, not just > > the relative ones. > > Ah, apologies, I missed that you'd changed the ABS behaviour from what it > was prior to 1cf24a2cc3fd. My main gripe is that our (currently unused) > fallthrough case (for op != PREL or ABS) is to elide the check altogether, > whereas I would prefer that we do our special casing with the comment, and > then keep the old overflow check for everything else, even though it's > really just there for documentation purposes. > Something like this perhaps? s64 min = (op != RELOC_OP_ABS ? S32_MIN : 0); s64 max = (op != RELOC_OP_PREL ? U32_MAX : S32_MAX); if (sval < min || sval > max) return -ERANGE; Or is that too obfuscated?
On Tue, May 28, 2019 at 02:03:37PM +0200, Ard Biesheuvel wrote: > On Tue, 28 May 2019 at 13:52, Will Deacon <will.deacon@arm.com> wrote: > > > > On Tue, May 28, 2019 at 12:20:53PM +0200, Ard Biesheuvel wrote: > > > On Tue, 28 May 2019 at 12:11, Will Deacon <will.deacon@arm.com> wrote: > > > > > > > > On Mon, May 27, 2019 at 08:44:13AM +0200, Ard Biesheuvel wrote: > > > > > Commit 1cf24a2cc3fd > > > > > > > > > > ("arm64/module: deal with ambiguity in PRELxx relocation ranges") > > > > > > > > > > updated the overflow checking logic in the relocation handling code to > > > > > ensure that PREL16/32 relocations don't overflow signed quantities. > > > > > > > > > > However, the same code path is used for absolute relocations, where the > > > > > interpretation is the opposite: the only current use case for absolute > > > > > relocations operating on non-native word size quantities is the CRC32 > > > > > handling in the CONFIG_MODVERSIONS code, and these CRCs are unsigned > > > > > 32-bit quantities, which are now being rejected by the module loader > > > > > if bit 31 happens to be set. > > > > > > > > > > So let's use different ranges for quanties subject to absolute vs. > > > > > relative relocations: > > > > > - ABS16/32 relocations should be in the range [0, Uxx_MAX) > > > > > - PREL16/32 relocations should be in the range [Sxx_MIN, Sxx_MAX) > > > > > > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > > --- > > > > > arch/arm64/kernel/module.c | 14 ++++++++------ > > > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > > > > > index f32359cffb01..85fb63c1ba3a 100644 > > > > > --- a/arch/arm64/kernel/module.c > > > > > +++ b/arch/arm64/kernel/module.c > > > > > @@ -98,10 +98,10 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > > > > > > > > > /* > > > > > * The ELF psABI for AArch64 documents the 16-bit and 32-bit place > > > > > - * relative relocations as having a range of [-2^15, 2^16) or > > > > > - * [-2^31, 2^32), respectively. However, in order to be able to detect > > > > > - * overflows reliably, we have to choose whether we interpret such > > > > > - * quantities as signed or as unsigned, and stick with it. > > > > > + * relative and absolute relocations as having a range of [-2^15, 2^16) > > > > > + * or [-2^31, 2^32), respectively. However, in order to be able to > > > > > + * detect overflows reliably, we have to choose whether we interpret > > > > > + * such quantities as signed or as unsigned, and stick with it. > > > > > * The way we organize our address space requires a signed > > > > > * interpretation of 32-bit relative references, so let's use that > > > > > * for all R_AARCH64_PRELxx relocations. This means our upper > > > > > @@ -111,12 +111,14 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > > > > switch (len) { > > > > > case 16: > > > > > *(s16 *)place = sval; > > > > > - if (sval < S16_MIN || sval > S16_MAX) > > > > > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U16_MAX)) || > > > > > + (op == RELOC_OP_PREL && (sval < S16_MIN || sval > S16_MAX))) > > > > > return -ERANGE; > > > > > break; > > > > > case 32: > > > > > *(s32 *)place = sval; > > > > > - if (sval < S32_MIN || sval > S32_MAX) > > > > > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U32_MAX)) || > > > > > + (op == RELOC_OP_PREL && (sval < S32_MIN || sval > S32_MAX))) > > > > > return -ERANGE; > > > > > break; > > > > > case 64: > > > > > > > > Hmm. I worry that this isn't the last time we're going to be tweaking this, > > > > so I'm wondering whether or not we should just revert 1cf24a2cc3fd instead. > > > > > > > > > > Both absolute and relative 32-bit references can really only be > > > emitted using assembler code, since C code only uses the former, and > > > only uses it to resolve symbol addresses not bare values. > > > > > > ABS32 is only used by the CONFIG_MODVERSIONS code, and PREL32 is only > > > used by the few data structures where we deliberately use relative > > > references to get rid of the RELA entries associated with absolute > > > references. > > > > > > So I don't share your concern here, although I understand where it is > > > coming from. > > > > Fair enough, although I'm not claiming that there's a bug in your patch > > (it looks fine), just more that I dislike the deviation we're introducing > > here. > > > > > *However*, not being able to detect overflow is *really* bad, so even > > > if there is another tweak behind the horizon, this is still better > > > than silent data corruption because your data reference is off by 4 > > > GB. > > > > Agreed. > > > > > > However, if you still want to pursue this, please can you restructure the > > > > check so that we do: > > > > > > > > if (op == RELOC_OP_PREL) { > > > > /* Comment about deviation from ELF ABI */ > > > > if (signed overflow check) > > > > return -ERANGE; > > > > } else if (unsigned overflow check) { > > > > return -ERANGE; > > > > } > > > > > > > > i.e. drop the explicit check of ABS so that the default behaviour follows > > > > the ELF spec? > > > > > > > > > > My patch does not follow the ELF spec for either ABS16/32 or PREL16/32 > > > relocations. It chooses an unsigned interpretation for the former, and > > > a signed one for the latter, and I think this is the only way to deal > > > with this properly. > > > > > > This is why I updated the comment as well: we should apply a strict > > > (but different) interpretation to both kinds of relocations, not just > > > the relative ones. > > > > Ah, apologies, I missed that you'd changed the ABS behaviour from what it > > was prior to 1cf24a2cc3fd. My main gripe is that our (currently unused) > > fallthrough case (for op != PREL or ABS) is to elide the check altogether, > > whereas I would prefer that we do our special casing with the comment, and > > then keep the old overflow check for everything else, even though it's > > really just there for documentation purposes. > > > > Something like this perhaps? > > s64 min = (op != RELOC_OP_ABS ? S32_MIN : 0); > s64 max = (op != RELOC_OP_PREL ? U32_MAX : S32_MAX); > if (sval < min || sval > max) > return -ERANGE; > > Or is that too obfuscated? It's a bit too smart for me ;) I think I'd just switch on the op and duplicate the check, tbh. Will
On Tue, 28 May 2019 at 14:13, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, May 28, 2019 at 02:03:37PM +0200, Ard Biesheuvel wrote: > > On Tue, 28 May 2019 at 13:52, Will Deacon <will.deacon@arm.com> wrote: > > > > > > On Tue, May 28, 2019 at 12:20:53PM +0200, Ard Biesheuvel wrote: > > > > On Tue, 28 May 2019 at 12:11, Will Deacon <will.deacon@arm.com> wrote: > > > > > > > > > > On Mon, May 27, 2019 at 08:44:13AM +0200, Ard Biesheuvel wrote: > > > > > > Commit 1cf24a2cc3fd > > > > > > > > > > > > ("arm64/module: deal with ambiguity in PRELxx relocation ranges") > > > > > > > > > > > > updated the overflow checking logic in the relocation handling code to > > > > > > ensure that PREL16/32 relocations don't overflow signed quantities. > > > > > > > > > > > > However, the same code path is used for absolute relocations, where the > > > > > > interpretation is the opposite: the only current use case for absolute > > > > > > relocations operating on non-native word size quantities is the CRC32 > > > > > > handling in the CONFIG_MODVERSIONS code, and these CRCs are unsigned > > > > > > 32-bit quantities, which are now being rejected by the module loader > > > > > > if bit 31 happens to be set. > > > > > > > > > > > > So let's use different ranges for quanties subject to absolute vs. > > > > > > relative relocations: > > > > > > - ABS16/32 relocations should be in the range [0, Uxx_MAX) > > > > > > - PREL16/32 relocations should be in the range [Sxx_MIN, Sxx_MAX) > > > > > > > > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > > > --- > > > > > > arch/arm64/kernel/module.c | 14 ++++++++------ > > > > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > > > > > > > diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > > > > > > index f32359cffb01..85fb63c1ba3a 100644 > > > > > > --- a/arch/arm64/kernel/module.c > > > > > > +++ b/arch/arm64/kernel/module.c > > > > > > @@ -98,10 +98,10 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > > > > > > > > > > > /* > > > > > > * The ELF psABI for AArch64 documents the 16-bit and 32-bit place > > > > > > - * relative relocations as having a range of [-2^15, 2^16) or > > > > > > - * [-2^31, 2^32), respectively. However, in order to be able to detect > > > > > > - * overflows reliably, we have to choose whether we interpret such > > > > > > - * quantities as signed or as unsigned, and stick with it. > > > > > > + * relative and absolute relocations as having a range of [-2^15, 2^16) > > > > > > + * or [-2^31, 2^32), respectively. However, in order to be able to > > > > > > + * detect overflows reliably, we have to choose whether we interpret > > > > > > + * such quantities as signed or as unsigned, and stick with it. > > > > > > * The way we organize our address space requires a signed > > > > > > * interpretation of 32-bit relative references, so let's use that > > > > > > * for all R_AARCH64_PRELxx relocations. This means our upper > > > > > > @@ -111,12 +111,14 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) > > > > > > switch (len) { > > > > > > case 16: > > > > > > *(s16 *)place = sval; > > > > > > - if (sval < S16_MIN || sval > S16_MAX) > > > > > > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U16_MAX)) || > > > > > > + (op == RELOC_OP_PREL && (sval < S16_MIN || sval > S16_MAX))) > > > > > > return -ERANGE; > > > > > > break; > > > > > > case 32: > > > > > > *(s32 *)place = sval; > > > > > > - if (sval < S32_MIN || sval > S32_MAX) > > > > > > + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U32_MAX)) || > > > > > > + (op == RELOC_OP_PREL && (sval < S32_MIN || sval > S32_MAX))) > > > > > > return -ERANGE; > > > > > > break; > > > > > > case 64: > > > > > > > > > > Hmm. I worry that this isn't the last time we're going to be tweaking this, > > > > > so I'm wondering whether or not we should just revert 1cf24a2cc3fd instead. > > > > > > > > > > > > > Both absolute and relative 32-bit references can really only be > > > > emitted using assembler code, since C code only uses the former, and > > > > only uses it to resolve symbol addresses not bare values. > > > > > > > > ABS32 is only used by the CONFIG_MODVERSIONS code, and PREL32 is only > > > > used by the few data structures where we deliberately use relative > > > > references to get rid of the RELA entries associated with absolute > > > > references. > > > > > > > > So I don't share your concern here, although I understand where it is > > > > coming from. > > > > > > Fair enough, although I'm not claiming that there's a bug in your patch > > > (it looks fine), just more that I dislike the deviation we're introducing > > > here. > > > > > > > *However*, not being able to detect overflow is *really* bad, so even > > > > if there is another tweak behind the horizon, this is still better > > > > than silent data corruption because your data reference is off by 4 > > > > GB. > > > > > > Agreed. > > > > > > > > However, if you still want to pursue this, please can you restructure the > > > > > check so that we do: > > > > > > > > > > if (op == RELOC_OP_PREL) { > > > > > /* Comment about deviation from ELF ABI */ > > > > > if (signed overflow check) > > > > > return -ERANGE; > > > > > } else if (unsigned overflow check) { > > > > > return -ERANGE; > > > > > } > > > > > > > > > > i.e. drop the explicit check of ABS so that the default behaviour follows > > > > > the ELF spec? > > > > > > > > > > > > > My patch does not follow the ELF spec for either ABS16/32 or PREL16/32 > > > > relocations. It chooses an unsigned interpretation for the former, and > > > > a signed one for the latter, and I think this is the only way to deal > > > > with this properly. > > > > > > > > This is why I updated the comment as well: we should apply a strict > > > > (but different) interpretation to both kinds of relocations, not just > > > > the relative ones. > > > > > > Ah, apologies, I missed that you'd changed the ABS behaviour from what it > > > was prior to 1cf24a2cc3fd. My main gripe is that our (currently unused) > > > fallthrough case (for op != PREL or ABS) is to elide the check altogether, > > > whereas I would prefer that we do our special casing with the comment, and > > > then keep the old overflow check for everything else, even though it's > > > really just there for documentation purposes. > > > > > > > Something like this perhaps? > > > > s64 min = (op != RELOC_OP_ABS ? S32_MIN : 0); > > s64 max = (op != RELOC_OP_PREL ? U32_MAX : S32_MAX); > > if (sval < min || sval > max) > > return -ERANGE; > > > > Or is that too obfuscated? > > It's a bit too smart for me ;) > > I think I'd just switch on the op and duplicate the check, tbh. > So like this? switch (op) { case RELOC_OP_ABS: if (sval < 0 || sval > U32_MAX) return -ERANGE; break; case RELOC_OP_PREL: if (sval < S32_MIN || sval > S32_MAX) return -ERANGE; break; default: if (sval < S32_MIN || sval > U32_MAX) return -ERANGE; }
On Tue, May 28, 2019 at 02:26:04PM +0200, Ard Biesheuvel wrote: > On Tue, 28 May 2019 at 14:13, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, May 28, 2019 at 02:03:37PM +0200, Ard Biesheuvel wrote: > > > On Tue, 28 May 2019 at 13:52, Will Deacon <will.deacon@arm.com> wrote: > > > > On Tue, May 28, 2019 at 12:20:53PM +0200, Ard Biesheuvel wrote: > > > > > > > > > > My patch does not follow the ELF spec for either ABS16/32 or PREL16/32 > > > > > relocations. It chooses an unsigned interpretation for the former, and > > > > > a signed one for the latter, and I think this is the only way to deal > > > > > with this properly. > > > > > > > > > > This is why I updated the comment as well: we should apply a strict > > > > > (but different) interpretation to both kinds of relocations, not just > > > > > the relative ones. > > > > > > > > Ah, apologies, I missed that you'd changed the ABS behaviour from what it > > > > was prior to 1cf24a2cc3fd. My main gripe is that our (currently unused) > > > > fallthrough case (for op != PREL or ABS) is to elide the check altogether, > > > > whereas I would prefer that we do our special casing with the comment, and > > > > then keep the old overflow check for everything else, even though it's > > > > really just there for documentation purposes. > > > > > > > > > > Something like this perhaps? > > > > > > s64 min = (op != RELOC_OP_ABS ? S32_MIN : 0); > > > s64 max = (op != RELOC_OP_PREL ? U32_MAX : S32_MAX); > > > if (sval < min || sval > max) > > > return -ERANGE; > > > > > > Or is that too obfuscated? > > > > It's a bit too smart for me ;) > > > > I think I'd just switch on the op and duplicate the check, tbh. > > > > So like this? > > switch (op) { > case RELOC_OP_ABS: > if (sval < 0 || sval > U32_MAX) > return -ERANGE; > break; > case RELOC_OP_PREL: > if (sval < S32_MIN || sval > S32_MAX) > return -ERANGE; > break; > default: > if (sval < S32_MIN || sval > U32_MAX) > return -ERANGE; Beautiful. You'll probably end up duplicating the logic for the 16-bit case, but whatever. Will
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c index f32359cffb01..85fb63c1ba3a 100644 --- a/arch/arm64/kernel/module.c +++ b/arch/arm64/kernel/module.c @@ -98,10 +98,10 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) /* * The ELF psABI for AArch64 documents the 16-bit and 32-bit place - * relative relocations as having a range of [-2^15, 2^16) or - * [-2^31, 2^32), respectively. However, in order to be able to detect - * overflows reliably, we have to choose whether we interpret such - * quantities as signed or as unsigned, and stick with it. + * relative and absolute relocations as having a range of [-2^15, 2^16) + * or [-2^31, 2^32), respectively. However, in order to be able to + * detect overflows reliably, we have to choose whether we interpret + * such quantities as signed or as unsigned, and stick with it. * The way we organize our address space requires a signed * interpretation of 32-bit relative references, so let's use that * for all R_AARCH64_PRELxx relocations. This means our upper @@ -111,12 +111,14 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len) switch (len) { case 16: *(s16 *)place = sval; - if (sval < S16_MIN || sval > S16_MAX) + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U16_MAX)) || + (op == RELOC_OP_PREL && (sval < S16_MIN || sval > S16_MAX))) return -ERANGE; break; case 32: *(s32 *)place = sval; - if (sval < S32_MIN || sval > S32_MAX) + if ((op == RELOC_OP_ABS && (sval < 0 || sval > U32_MAX)) || + (op == RELOC_OP_PREL && (sval < S32_MIN || sval > S32_MAX))) return -ERANGE; break; case 64:
Commit 1cf24a2cc3fd ("arm64/module: deal with ambiguity in PRELxx relocation ranges") updated the overflow checking logic in the relocation handling code to ensure that PREL16/32 relocations don't overflow signed quantities. However, the same code path is used for absolute relocations, where the interpretation is the opposite: the only current use case for absolute relocations operating on non-native word size quantities is the CRC32 handling in the CONFIG_MODVERSIONS code, and these CRCs are unsigned 32-bit quantities, which are now being rejected by the module loader if bit 31 happens to be set. So let's use different ranges for quanties subject to absolute vs. relative relocations: - ABS16/32 relocations should be in the range [0, Uxx_MAX) - PREL16/32 relocations should be in the range [Sxx_MIN, Sxx_MAX) Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/kernel/module.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)