[PATCH 1/3] modpost: add array range check to sec_name()

[PATCH 1/3] modpost: add array range check to sec_name()

[Masahiro Yamada]

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 <masahiroy@kernel.org>
---

 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

[PATCH 2/3] modpost: use more reliable way to get fromsec in section_rel(a)()

[Masahiro Yamada]

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 <masahiroy@kernel.org>
---

 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

Re: [PATCH 2/3] modpost: use more reliable way to get fromsec in section_rel(a)()

[Nick Desaulniers]

On Sat, Jul 30, 2022 at 10:37 AM Masahiro Yamada <masahiroy@kernel.org> 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 <masahiroy@kernel.org>

Yes, this seems much safer; thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
>  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

[PATCH 3/3] Revert "Kbuild, lto, workaround: Don't warn for initcall_reference in modpost"

[Masahiro Yamada]

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 <masahiroy@kernel.org>
---

 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

Re: [PATCH 3/3] Revert "Kbuild, lto, workaround: Don't warn for initcall_reference in modpost"

[Nick Desaulniers]

On Sat, Jul 30, 2022 at 10:37 AM Masahiro Yamada <masahiroy@kernel.org> 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 <masahiroy@kernel.org>

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 <ndesaulniers@google.com>


> ---
>
>  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

Re: [PATCH 3/3] Revert "Kbuild, lto, workaround: Don't warn for initcall_reference in modpost"

[Jiri Slaby]

On 02. 08. 22, 20:04, Nick Desaulniers wrote:
> On Sat, Jul 30, 2022 at 10:37 AM Masahiro Yamada <masahiroy@kernel.org> 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 <masahiroy@kernel.org>
> 
> 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 <jirislaby@kernel.org>

> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

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

Re: [PATCH 1/3] modpost: add array range check to sec_name()

[Jeff Johnson]

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/

Re: [PATCH 1/3] modpost: add array range check to sec_name()

[Masahiro Yamada]

On Tue, Aug 2, 2022 at 5:29 AM Jeff Johnson <quic_jjohnson@quicinc.com> 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

Re: [PATCH 1/3] modpost: add array range check to sec_name()

[Nick Desaulniers]

On Sat, Jul 30, 2022 at 10:37 AM Masahiro Yamada <masahiroy@kernel.org> 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 <masahiroy@kernel.org>

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

Re: [PATCH 1/3] modpost: add array range check to sec_name()

[Masahiro Yamada]

On Wed, Aug 3, 2022 at 2:55 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Sat, Jul 30, 2022 at 10:37 AM Masahiro Yamada <masahiroy@kernel.org> 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 <masahiroy@kernel.org>
>
> 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