The section index is always positive, so the argunent, secindex, should
be unsigned.
Also, inserted the array range check.
If sym->st_shndx is a special section index (between SHN_LORESERVE and
SHN_HIRESERVE), there is no corresponding section header.
For example, if a symbol specifies an absolute value, sym->st_shndx is
SHN_ABS (=0xfff1).
The current users do not cause the out-of-range access of
info->sechddrs[], but it is better to avoid such a pitfall.
Signed-off-by: Masahiro Yamada <[email protected]>
---
scripts/mod/modpost.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 08411fff3e17..148b38699889 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -336,8 +336,16 @@ static const char *sech_name(const struct elf_info *info, Elf_Shdr *sechdr)
sechdr->sh_name);
}
-static const char *sec_name(const struct elf_info *info, int secindex)
+static const char *sec_name(const struct elf_info *info, unsigned int secindex)
{
+ /*
+ * If sym->st_shndx is a special section index, there is no
+ * corresponding section header.
+ * Return "" if the index is out of range of info->sechdrs[] array.
+ */
+ if (secindex >= info->num_sections)
+ return "";
+
return sech_name(info, &info->sechdrs[secindex]);
}
--
2.34.1
This reverts commit 77ab21adae509c5540956729e2d03bc1a59bc82a.
That commit was 8 years old, and it said "This is a workaround".
If this is needed for GCC LTO, it should be added in a proper way.
Signed-off-by: Masahiro Yamada <[email protected]>
---
scripts/mod/modpost.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index c6a055c0291e..a8ee27496da7 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1462,9 +1462,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
from = find_elf_symbol2(elf, r->r_offset, fromsec);
fromsym = sym_name(elf, from);
- if (strstarts(fromsym, "reference___initcall"))
- return;
-
tosec = sec_name(elf, get_secindex(elf, sym));
to = find_elf_symbol(elf, r->r_addend, sym);
tosym = sym_name(elf, to);
--
2.34.1
The section name of Rel and Rela starts with ".rel" and ".rela"
respectively (but, I do not know whether this is specification or
convention).
For example, ".rela.text" holds relocation entries applied to the
".text" section.
So, the code chops the ".rel" or ".rela" prefix to get the name of
the section to which the relocation applies.
However, I do not like to skip 4 or 5 bytes blindly because it is
potential memory overrun.
The ELF specification provides a more reliable way to do this.
- The sh_info field holds extra information, whose interpretation
depends on the section type
- If the section type is SHT_REL or SHT_RELA, the sh_info field holds
the section header index of the section to which the relocation
applies.
Signed-off-by: Masahiro Yamada <[email protected]>
---
scripts/mod/modpost.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 148b38699889..c6a055c0291e 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1723,8 +1723,7 @@ static void section_rela(const char *modname, struct elf_info *elf,
Elf_Rela *start = (void *)elf->hdr + sechdr->sh_offset;
Elf_Rela *stop = (void *)start + sechdr->sh_size;
- fromsec = sech_name(elf, sechdr);
- fromsec += strlen(".rela");
+ fromsec = sec_name(elf, sechdr->sh_info);
/* if from section (name) is know good then skip it */
if (match(fromsec, section_white_list))
return;
@@ -1776,8 +1775,7 @@ static void section_rel(const char *modname, struct elf_info *elf,
Elf_Rel *start = (void *)elf->hdr + sechdr->sh_offset;
Elf_Rel *stop = (void *)start + sechdr->sh_size;
- fromsec = sech_name(elf, sechdr);
- fromsec += strlen(".rel");
+ fromsec = sec_name(elf, sechdr->sh_info);
/* if from section (name) is know good then skip it */
if (match(fromsec, section_white_list))
return;
--
2.34.1
On 7/30/2022 10:36 AM, Masahiro Yamada wrote:
> The section index is always positive, so the argunent, secindex, should
nit: s/argunent/argument/
On Tue, Aug 2, 2022 at 5:29 AM Jeff Johnson <[email protected]> wrote:
>
> On 7/30/2022 10:36 AM, Masahiro Yamada wrote:
> > The section index is always positive, so the argunent, secindex, should
>
> nit: s/argunent/argument/
Thanks.
I will not send v2 just because of this typo.
I locally fixed it.
--
Best Regards
Masahiro Yamada
On Sat, Jul 30, 2022 at 10:37 AM Masahiro Yamada <[email protected]> wrote:
>
> The section index is always positive, so the argunent, secindex, should
> be unsigned.
>
> Also, inserted the array range check.
>
> If sym->st_shndx is a special section index (between SHN_LORESERVE and
> SHN_HIRESERVE), there is no corresponding section header.
>
> For example, if a symbol specifies an absolute value, sym->st_shndx is
> SHN_ABS (=0xfff1).
>
> The current users do not cause the out-of-range access of
> info->sechddrs[], but it is better to avoid such a pitfall.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
I don't mind adding this check; though if it's anomalous I think we
could also just print to stderr and abort.
I would prefer Elf_Sym over unsigned int though. WDYT?
> ---
>
> scripts/mod/modpost.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 08411fff3e17..148b38699889 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -336,8 +336,16 @@ static const char *sech_name(const struct elf_info *info, Elf_Shdr *sechdr)
> sechdr->sh_name);
> }
>
> -static const char *sec_name(const struct elf_info *info, int secindex)
> +static const char *sec_name(const struct elf_info *info, unsigned int secindex)
> {
> + /*
> + * If sym->st_shndx is a special section index, there is no
> + * corresponding section header.
> + * Return "" if the index is out of range of info->sechdrs[] array.
> + */
> + if (secindex >= info->num_sections)
> + return "";
> +
> return sech_name(info, &info->sechdrs[secindex]);
> }
>
> --
> 2.34.1
>
--
Thanks,
~Nick Desaulniers
On Sat, Jul 30, 2022 at 10:37 AM Masahiro Yamada <[email protected]> wrote:
>
> This reverts commit 77ab21adae509c5540956729e2d03bc1a59bc82a.
>
> That commit was 8 years old, and it said "This is a workaround".
> If this is needed for GCC LTO, it should be added in a proper way.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Please don't forget to cc the author & reviewers for a patch when
submitting a revert.
+ Jiri in case a patch needs to be carried in any downstream trees for
re-application.
Reviewed-by: Nick Desaulniers <[email protected]>
> ---
>
> scripts/mod/modpost.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index c6a055c0291e..a8ee27496da7 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1462,9 +1462,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
> from = find_elf_symbol2(elf, r->r_offset, fromsec);
> fromsym = sym_name(elf, from);
>
> - if (strstarts(fromsym, "reference___initcall"))
> - return;
> -
> tosec = sec_name(elf, get_secindex(elf, sym));
> to = find_elf_symbol(elf, r->r_addend, sym);
> tosym = sym_name(elf, to);
> --
> 2.34.1
>
--
Thanks,
~Nick Desaulniers
On Sat, Jul 30, 2022 at 10:37 AM Masahiro Yamada <[email protected]> wrote:
>
> The section name of Rel and Rela starts with ".rel" and ".rela"
> respectively (but, I do not know whether this is specification or
> convention).
>
> For example, ".rela.text" holds relocation entries applied to the
> ".text" section.
>
> So, the code chops the ".rel" or ".rela" prefix to get the name of
> the section to which the relocation applies.
>
> However, I do not like to skip 4 or 5 bytes blindly because it is
> potential memory overrun.
>
> The ELF specification provides a more reliable way to do this.
>
> - The sh_info field holds extra information, whose interpretation
> depends on the section type
>
> - If the section type is SHT_REL or SHT_RELA, the sh_info field holds
> the section header index of the section to which the relocation
> applies.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
Yes, this seems much safer; thanks for the patch!
Reviewed-by: Nick Desaulniers <[email protected]>
> ---
>
> scripts/mod/modpost.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 148b38699889..c6a055c0291e 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1723,8 +1723,7 @@ static void section_rela(const char *modname, struct elf_info *elf,
> Elf_Rela *start = (void *)elf->hdr + sechdr->sh_offset;
> Elf_Rela *stop = (void *)start + sechdr->sh_size;
>
> - fromsec = sech_name(elf, sechdr);
> - fromsec += strlen(".rela");
> + fromsec = sec_name(elf, sechdr->sh_info);
> /* if from section (name) is know good then skip it */
> if (match(fromsec, section_white_list))
> return;
> @@ -1776,8 +1775,7 @@ static void section_rel(const char *modname, struct elf_info *elf,
> Elf_Rel *start = (void *)elf->hdr + sechdr->sh_offset;
> Elf_Rel *stop = (void *)start + sechdr->sh_size;
>
> - fromsec = sech_name(elf, sechdr);
> - fromsec += strlen(".rel");
> + fromsec = sec_name(elf, sechdr->sh_info);
> /* if from section (name) is know good then skip it */
> if (match(fromsec, section_white_list))
> return;
> --
> 2.34.1
>
--
Thanks,
~Nick Desaulniers
On Wed, Aug 3, 2022 at 2:55 AM Nick Desaulniers <[email protected]> wrote:
>
> On Sat, Jul 30, 2022 at 10:37 AM Masahiro Yamada <[email protected]> wrote:
> >
> > The section index is always positive, so the argunent, secindex, should
> > be unsigned.
> >
> > Also, inserted the array range check.
> >
> > If sym->st_shndx is a special section index (between SHN_LORESERVE and
> > SHN_HIRESERVE), there is no corresponding section header.
> >
> > For example, if a symbol specifies an absolute value, sym->st_shndx is
> > SHN_ABS (=0xfff1).
> >
> > The current users do not cause the out-of-range access of
> > info->sechddrs[], but it is better to avoid such a pitfall.
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
>
> I don't mind adding this check; though if it's anomalous I think we
> could also just print to stderr and abort.
If sec_name() has a failure path,
I need to add another check before calling sec_name().
I want to get a return value that can be safely passed
to strcmp(), etc.
For example,
strcmp(!sec_name(elf, secindex), "some_pattern");
Returning "" for special sections
will work nicely without additional check code.
I am changing the code with a bigger picture in my mind,
although that may not be so clear if you look at this patch only.
> I would prefer Elf_Sym over unsigned int though. WDYT?
>
In /usr/include/elf.h, Elf{32,64}_Sym are structures.
How to use it instead of unsigned int?
typedef struct
{
Elf32_Word st_name; /* Symbol name (string tbl index) */
Elf32_Addr st_value; /* Symbol value */
Elf32_Word st_size; /* Symbol size */
unsigned char st_info; /* Symbol type and binding */
unsigned char st_other; /* Symbol visibility */
Elf32_Section st_shndx; /* Section index */
} Elf32_Sym;
typedef struct
{
Elf64_Word st_name; /* Symbol name (string tbl index) */
unsigned char st_info; /* Symbol type and binding */
unsigned char st_other; /* Symbol visibility */
Elf64_Section st_shndx; /* Section index */
Elf64_Addr st_value; /* Symbol value */
Elf64_Xword st_size; /* Symbol size */
} Elf64_Sym;
Best Regards
Masahiro Yamada
On 02. 08. 22, 20:04, Nick Desaulniers wrote:
> On Sat, Jul 30, 2022 at 10:37 AM Masahiro Yamada <[email protected]> wrote:
>>
>> This reverts commit 77ab21adae509c5540956729e2d03bc1a59bc82a.
>>
>> That commit was 8 years old, and it said "This is a workaround".
>> If this is needed for GCC LTO, it should be added in a proper way.
>>
>> Signed-off-by: Masahiro Yamada <[email protected]>
>
> Please don't forget to cc the author & reviewers for a patch when
> submitting a revert.
>
> + Jiri in case a patch needs to be carried in any downstream trees for
> re-application.
IMO we already got rid of -fno-toplevel-reorder (we have noreorder
attribute now), so:
Acked-by: Jiri Slaby <[email protected]>
> Reviewed-by: Nick Desaulniers <[email protected]>
Thanks for letting me know.
>> scripts/mod/modpost.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>> index c6a055c0291e..a8ee27496da7 100644
>> --- a/scripts/mod/modpost.c
>> +++ b/scripts/mod/modpost.c
>> @@ -1462,9 +1462,6 @@ static void default_mismatch_handler(const char *modname, struct elf_info *elf,
>> from = find_elf_symbol2(elf, r->r_offset, fromsec);
>> fromsym = sym_name(elf, from);
>>
>> - if (strstarts(fromsym, "reference___initcall"))
>> - return;
>> -
>> tosec = sec_name(elf, get_secindex(elf, sym));
>> to = find_elf_symbol(elf, r->r_addend, sym);
>> tosym = sym_name(elf, to);
>> --
>> 2.34.1
>>
>
>
--
js
suse labs