[1/2] RISC-V: Reject invalid relocation types

  From: Tsukasa OI <research_trasio@irq.a4lg.com>

In RISC-V BFD, there are several internal-only relocation types.  Such
relocation fed from the outside can be a cause of unexpected behaviors
and should be rejected before being parsed further.

This commit adds checks to make sure that we only handle known
relocation types.  For maintainability, internal-only relocation types
are listed separately.

Changes to riscv_elf_check_relocs applies to the linker (ld) and changes
to riscv_info_to_howto_rela and riscv_elf_rtype_to_howto applies to
other tools such like objdump and objcopy.

bfd/ChangeLog:

	* elfnn-riscv.c (riscv_reloc_is_internal_use_only): New to detect
	internal use only relocation type.
	(riscv_info_to_howto_rela): Reject invalid relocation types
	while handling ELF files but linking.
	(riscv_elf_check_relocs): Reject invalid relocation types
	while linking.
	* elfxx-riscv.c (riscv_elf_rtype_to_howto): Also reject types
	without name meaning unknown relocation type.
---
 bfd/elfnn-riscv.c | 77 +++++++++++++++++++++++++++++++++++++++++++++--
 bfd/elfxx-riscv.c |  2 +-
 2 files changed, 76 insertions(+), 3 deletions(-)
  

On Mon, Oct 16, 2023 at 2:03 PM Tsukasa OI <research_trasio@irq.a4lg.com>
wrote:

> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>
> In RISC-V BFD, there are several internal-only relocation types.  Such
> relocation fed from the outside can be a cause of unexpected behaviors
> and should be rejected before being parsed further.
>
> This commit adds checks to make sure that we only handle known
> relocation types.  For maintainability, internal-only relocation types
> are listed separately.
>
> Changes to riscv_elf_check_relocs applies to the linker (ld) and changes
> to riscv_info_to_howto_rela and riscv_elf_rtype_to_howto applies to
> other tools such like objdump and objcopy.
>
> bfd/ChangeLog:
>
>         * elfnn-riscv.c (riscv_reloc_is_internal_use_only): New to detect
>         internal use only relocation type.
>         (riscv_info_to_howto_rela): Reject invalid relocation types
>         while handling ELF files but linking.
>         (riscv_elf_check_relocs): Reject invalid relocation types
>         while linking.
>         * elfxx-riscv.c (riscv_elf_rtype_to_howto): Also reject types
>         without name meaning unknown relocation type.
> ---
>  bfd/elfnn-riscv.c | 77 +++++++++++++++++++++++++++++++++++++++++++++--
>  bfd/elfxx-riscv.c |  2 +-
>  2 files changed, 76 insertions(+), 3 deletions(-)
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 09aa7be225ef..dedfabe131ba 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -262,12 +262,37 @@ riscv_elfNN_set_options (struct bfd_link_info
> *link_info,
>    riscv_elf_hash_table (link_info)->params = params;
>  }
>
> +static bool
> +riscv_reloc_is_internal_use_only (unsigned int r_type)
> +{
> +  switch (r_type)
> +    {
> +      case R_RISCV_RVC_LUI:
> +      case R_RISCV_GPREL_I:
> +      case R_RISCV_GPREL_S:
> +      case R_RISCV_TPREL_I:
> +      case R_RISCV_TPREL_S:
> +      case R_RISCV_DELETE:
> +       return true;
> +      default:
> +       return false;
> +    }
> +}
> +
>  static bool
>  riscv_info_to_howto_rela (bfd *abfd,
>                           arelent *cache_ptr,
>                           Elf_Internal_Rela *dst)
>  {
> -  cache_ptr->howto = riscv_elf_rtype_to_howto (abfd, ELFNN_R_TYPE
> (dst->r_info));
> +  unsigned int r_type = ELFNN_R_TYPE (dst->r_info);
> +  cache_ptr->howto = riscv_elf_rtype_to_howto (abfd, r_type);
> +  if (cache_ptr->howto && riscv_reloc_is_internal_use_only (r_type))
> +    {
> +      (*_bfd_error_handler) (_("%pB: unsupported relocation type %#x"),
> +                            abfd, r_type);
> +      bfd_set_error (bfd_error_bad_value);
> +      cache_ptr->howto = NULL;
> +    }
>    return cache_ptr->howto != NULL;
>  }
>

Maybe we could define these internal relocs from R_RISCV_max, and then
report errors when the number exceeds it?  Like sparc did,
https://github.com/bminor/binutils-gdb/blob/master/bfd/elfxx-sparc.c#L637.
But I'm not sure if disallowing internal relocs will cause problems or
not...


>
> @@ -834,8 +859,53 @@ riscv_elf_check_relocs (bfd *abfd, struct
> bfd_link_info *info,
>           h->ref_regular = 1;
>         }
>
> +      /* Explicitly reject internal use only relocation types.  */
> +      if (riscv_reloc_is_internal_use_only (r_type))
> +       {
> +         _bfd_error_handler
> +           (_("%pB: internal error: unsupported relocation type %#x"),
> +            abfd, r_type);
> +         return false;
> +       }
> +
>        switch (r_type)
>         {
> +       case R_RISCV_NONE:
> +       case R_RISCV_TLS_DTPMOD32:
> +       case R_RISCV_TLS_DTPMOD64:
> +       case R_RISCV_TLS_DTPREL32:
> +       case R_RISCV_TLS_DTPREL64:
> +       case R_RISCV_TLS_TPREL32:
> +       case R_RISCV_TLS_TPREL64:
> +       case R_RISCV_PCREL_LO12_I:
> +       case R_RISCV_PCREL_LO12_S:
> +       case R_RISCV_LO12_I:
> +       case R_RISCV_LO12_S:
> +       case R_RISCV_TPREL_LO12_I:
> +       case R_RISCV_TPREL_LO12_S:
> +       case R_RISCV_TPREL_ADD:
> +       case R_RISCV_ADD8:
> +       case R_RISCV_ADD16:
> +       case R_RISCV_ADD32:
> +       case R_RISCV_ADD64:
> +       case R_RISCV_SUB8:
> +       case R_RISCV_SUB16:
> +       case R_RISCV_SUB32:
> +       case R_RISCV_SUB64:
> +       case R_RISCV_ALIGN:
> +       case R_RISCV_RELAX:
> +       case R_RISCV_SUB6:
> +       case R_RISCV_SET6:
> +       case R_RISCV_SET8:
> +       case R_RISCV_SET16:
> +       case R_RISCV_SET32:
> +       case R_RISCV_32_PCREL:
> +       case R_RISCV_IRELATIVE:
> +       case R_RISCV_SET_ULEB128:
> +       case R_RISCV_SUB_ULEB128:
> +         /* Known relocation types without additional checks here.  */
> +         break;
> +
>

It seems like other targets don't have this similar check, so it will be
fine here as usual.  If something won't cause a problem, then it would be
safe to keep them as usual.


>         case R_RISCV_TLS_GD_HI20:
>           if (!riscv_elf_record_got_reference (abfd, info, h, r_symndx)
>               || !riscv_elf_record_tls_type (abfd, h, r_symndx,
> GOT_TLS_GD))
> @@ -1064,7 +1134,10 @@ riscv_elf_check_relocs (bfd *abfd, struct
> bfd_link_info *info,
>           break;
>
>         default:
> -         break;
> +         _bfd_error_handler
> +           (_("%pB: internal error: unsupported relocation type %#x"),
> +            abfd, r_type);
> +         return false;
>         }
>      }
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index c070394a3667..ffcdae341b2f 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -975,7 +975,7 @@ riscv_reloc_name_lookup (bfd *abfd ATTRIBUTE_UNUSED,
> const char *r_name)
>  reloc_howto_type *
>  riscv_elf_rtype_to_howto (bfd *abfd, unsigned int r_type)
>  {
> -  if (r_type >= ARRAY_SIZE (howto_table))
> +  if (r_type >= ARRAY_SIZE (howto_table) || !howto_table[r_type].name)
>

Seems like this will disallow the EMPTY_HOWTO?

Thanks
Nelson


>      {
>        (*_bfd_error_handler) (_("%pB: unsupported relocation type %#x"),
>                              abfd, r_type);
> --
> 2.42.0
>
>
  

On 2023/10/17 14:41, Nelson Chu wrote:
> 
> 
> On Mon, Oct 16, 2023 at 2:03 PM Tsukasa OI <research_trasio@irq.a4lg.com
> <mailto:research_trasio@irq.a4lg.com>> wrote:
> 
>     From: Tsukasa OI <research_trasio@irq.a4lg.com
>     <mailto:research_trasio@irq.a4lg.com>>
> 
>     In RISC-V BFD, there are several internal-only relocation types.  Such
>     relocation fed from the outside can be a cause of unexpected behaviors
>     and should be rejected before being parsed further.
> 
>     This commit adds checks to make sure that we only handle known
>     relocation types.  For maintainability, internal-only relocation types
>     are listed separately.
> 
>     Changes to riscv_elf_check_relocs applies to the linker (ld) and changes
>     to riscv_info_to_howto_rela and riscv_elf_rtype_to_howto applies to
>     other tools such like objdump and objcopy.
> 
>     bfd/ChangeLog:
> 
>             * elfnn-riscv.c (riscv_reloc_is_internal_use_only): New to
>     detect
>             internal use only relocation type.
>             (riscv_info_to_howto_rela): Reject invalid relocation types
>             while handling ELF files but linking.
>             (riscv_elf_check_relocs): Reject invalid relocation types
>             while linking.
>             * elfxx-riscv.c (riscv_elf_rtype_to_howto): Also reject types
>             without name meaning unknown relocation type.
>     ---
>      bfd/elfnn-riscv.c | 77 +++++++++++++++++++++++++++++++++++++++++++++--
>      bfd/elfxx-riscv.c |  2 +-
>      2 files changed, 76 insertions(+), 3 deletions(-)
> 
>     diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
>     index 09aa7be225ef..dedfabe131ba 100644
>     --- a/bfd/elfnn-riscv.c
>     +++ b/bfd/elfnn-riscv.c
>     @@ -262,12 +262,37 @@ riscv_elfNN_set_options (struct bfd_link_info
>     *link_info,
>        riscv_elf_hash_table (link_info)->params = params;
>      }
> 
>     +static bool
>     +riscv_reloc_is_internal_use_only (unsigned int r_type)
>     +{
>     +  switch (r_type)
>     +    {
>     +      case R_RISCV_RVC_LUI:
>     +      case R_RISCV_GPREL_I:
>     +      case R_RISCV_GPREL_S:
>     +      case R_RISCV_TPREL_I:
>     +      case R_RISCV_TPREL_S:
>     +      case R_RISCV_DELETE:
>     +       return true;
>     +      default:
>     +       return false;
>     +    }
>     +}
>     +
>      static bool
>      riscv_info_to_howto_rela (bfd *abfd,
>                               arelent *cache_ptr,
>                               Elf_Internal_Rela *dst)
>      {
>     -  cache_ptr->howto = riscv_elf_rtype_to_howto (abfd, ELFNN_R_TYPE
>     (dst->r_info));
>     +  unsigned int r_type = ELFNN_R_TYPE (dst->r_info);
>     +  cache_ptr->howto = riscv_elf_rtype_to_howto (abfd, r_type);
>     +  if (cache_ptr->howto && riscv_reloc_is_internal_use_only (r_type))
>     +    {
>     +      (*_bfd_error_handler) (_("%pB: unsupported relocation type %#x"),
>     +                            abfd, r_type);
>     +      bfd_set_error (bfd_error_bad_value);
>     +      cache_ptr->howto = NULL;
>     +    }
>        return cache_ptr->howto != NULL;
>      }
> 
> 
> Maybe we could define these internal relocs from R_RISCV_max, and then
> report errors when the number exceeds it?  Like sparc did,
> https://github.com/bminor/binutils-gdb/blob/master/bfd/elfxx-sparc.c#L637 <https://github.com/bminor/binutils-gdb/blob/master/bfd/elfxx-sparc.c#L637>.  But I'm not sure if disallowing internal relocs will cause problems or not...

We could.  But rejecting internal use only relocs is an absolute *MUST*
for the next release (short term solution should be merged if we don't
succeed to move internal only relocs after R_RISCV_max).

Fortunately, we won't accept R_RISCV_DELETE from outside (that would be
the worst case if it did but it doesn't happen because R_RISCV_DELETE is
defined relative to R_RISCV_max) but others can cause erroneous results.

>  
> 
> 
>     @@ -834,8 +859,53 @@ riscv_elf_check_relocs (bfd *abfd, struct
>     bfd_link_info *info,
>               h->ref_regular = 1;
>             }
> 
>     +      /* Explicitly reject internal use only relocation types.  */
>     +      if (riscv_reloc_is_internal_use_only (r_type))
>     +       {
>     +         _bfd_error_handler
>     +           (_("%pB: internal error: unsupported relocation type %#x"),
>     +            abfd, r_type);
>     +         return false;
>     +       }
>     +
>            switch (r_type)
>             {
>     +       case R_RISCV_NONE:
>     +       case R_RISCV_TLS_DTPMOD32:
>     +       case R_RISCV_TLS_DTPMOD64:
>     +       case R_RISCV_TLS_DTPREL32:
>     +       case R_RISCV_TLS_DTPREL64:
>     +       case R_RISCV_TLS_TPREL32:
>     +       case R_RISCV_TLS_TPREL64:
>     +       case R_RISCV_PCREL_LO12_I:
>     +       case R_RISCV_PCREL_LO12_S:
>     +       case R_RISCV_LO12_I:
>     +       case R_RISCV_LO12_S:
>     +       case R_RISCV_TPREL_LO12_I:
>     +       case R_RISCV_TPREL_LO12_S:
>     +       case R_RISCV_TPREL_ADD:
>     +       case R_RISCV_ADD8:
>     +       case R_RISCV_ADD16:
>     +       case R_RISCV_ADD32:
>     +       case R_RISCV_ADD64:
>     +       case R_RISCV_SUB8:
>     +       case R_RISCV_SUB16:
>     +       case R_RISCV_SUB32:
>     +       case R_RISCV_SUB64:
>     +       case R_RISCV_ALIGN:
>     +       case R_RISCV_RELAX:
>     +       case R_RISCV_SUB6:
>     +       case R_RISCV_SET6:
>     +       case R_RISCV_SET8:
>     +       case R_RISCV_SET16:
>     +       case R_RISCV_SET32:
>     +       case R_RISCV_32_PCREL:
>     +       case R_RISCV_IRELATIVE:
>     +       case R_RISCV_SET_ULEB128:
>     +       case R_RISCV_SUB_ULEB128:
>     +         /* Known relocation types without additional checks here.  */
>     +         break;
>     +
> 
> 
> It seems like other targets don't have this similar check, so it will be
> fine here as usual.  If something won't cause a problem, then it would
> be safe to keep them as usual.

I saw elf32_frv_check_relocs from FR-V architecture code which does and
I thought this is valid to do that (even if rare).  Also note that raw
reloc entries fed into riscv_elf_check_relocs are not checked by
riscv_elf_rtype_to_howto and if we miss internal use relocation types
here, that will be used (I don't want that).

>  
> 
>             case R_RISCV_TLS_GD_HI20:
>               if (!riscv_elf_record_got_reference (abfd, info, h, r_symndx)
>                   || !riscv_elf_record_tls_type (abfd, h, r_symndx,
>     GOT_TLS_GD))
>     @@ -1064,7 +1134,10 @@ riscv_elf_check_relocs (bfd *abfd, struct
>     bfd_link_info *info,
>               break;
> 
>             default:
>     -         break;
>     +         _bfd_error_handler
>     +           (_("%pB: internal error: unsupported relocation type %#x"),
>     +            abfd, r_type);
>     +         return false;
>             }
>          }
> 
>     diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
>     index c070394a3667..ffcdae341b2f 100644
>     --- a/bfd/elfxx-riscv.c
>     +++ b/bfd/elfxx-riscv.c
>     @@ -975,7 +975,7 @@ riscv_reloc_name_lookup (bfd *abfd
>     ATTRIBUTE_UNUSED, const char *r_name)
>      reloc_howto_type *
>      riscv_elf_rtype_to_howto (bfd *abfd, unsigned int r_type)
>      {
>     -  if (r_type >= ARRAY_SIZE (howto_table))
>     +  if (r_type >= ARRAY_SIZE (howto_table) || !howto_table[r_type].name)
> 
> 
> Seems like this will disallow the EMPTY_HOWTO?

Yes, I think we need it.  I was surprised by the fact that the most
targets don't do that (some of them don't have EMPTY_HOWTO but others
have such entries even if the most of them reject if the relocation type
is too large for the howto table; that's too arbitrary [same unknown
relocation type but different result between the value of the relocation
type; big or small] isn't it?).

Maybe we should note "what are we rejecting" or wrapping the check is
necessary but I'm going to reject EMPTY_HOWTO cases.


I'll report whether "move internal use only relocs after R_RISCV_max"
works (I'm working on it) and if not, please accept PATCH 1/2 (this
version or the next version with a minor change).


Thanks,
Tsukasa

> 
> Thanks
> Nelson
>  
> 
>          {
>            (*_bfd_error_handler) (_("%pB: unsupported relocation type %#x"),
>                                  abfd, r_type);
>     -- 
>     2.42.0
>