Re: [PATCH] objtool: include symbol value in check for contiguous objects

Re: [PATCH] objtool: include symbol value in check for contiguous objects

[Josh Poimboeuf]

On Wed, Apr 28, 2021 at 11:04:08PM +0200, Vanessa Hack wrote:
> While trying to adopt objtool's ability to generate ORC data for the use
> in our research project, we came across a problem with the detection of
> function pointers of contiguous objects introduced by commit fd35c88b7417
> ("objtool: Support GCC 8 switch tables"). Only the section and the
> addend/offset of the relocation/function are compared without the actual
> value of the involved symbols - false positives might be reported if the
> referenced symbols are different, but are in the same section. By adding
> (the value of) the referenced symbol as part of the comparison, those
> false positives are no longer reported.
> 
> Co-developed-by: Jonas Rabenstein <rabenstein@cs.fau.de>
> Signed-off-by: Jonas Rabenstein <rabenstein@cs.fau.de>
> Signed-off-by: Vanessa Hack <vanessa.hack@fau.de>
> ---
>  tools/objtool/check.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 1f4154f9b04b..4456f3278bb8 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1320,6 +1320,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>  	 * instruction.
>  	 */
>  	list_for_each_entry_from(reloc, &table->sec->reloc_list, list) {
> +		unsigned int pfunc_offset;
> +		unsigned int reloc_offset;
>  
>  		/* Check for the end of the table: */
>  		if (reloc != table && reloc->jump_table_start)
> @@ -1330,8 +1332,10 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>  			break;
>  
>  		/* Detect function pointers from contiguous objects: */
> +		pfunc_offset = pfunc->sym.st_value + pfunc->offset;
> +		reloc_offset = reloc->sym->sym.st_value + reloc->addend;
>  		if (reloc->sym->sec == pfunc->sec &&
> -		    reloc->addend == pfunc->offset)
> +		    pfunc_offset == reloc_offset)
>  			break;

Hi Vanessa & Jonas,

Thanks for the patch!

I'm a little confused about the issue -- do you have an example listing
of .rodata relocations (e.g. from 'readelf -aW') which confused objtool?

I believe add_jump_table() -- in addition to all the other jump table
code -- assumes that reloc->sym is a section symbol (STT_SECTION),
rather than a function symbol (STT_FUNC).  Was it an STT_FUNC symbol
which caused the problem?

Also I'm not quite convinced this patch is the right fix.  For the
'pfunc_offset' calculation, I believe 'pfunc->sym.st_value' is the same
value as 'pfunc->offset' -- they both represent the function's section
offset.  So it's basically adding pfunc->offset to itself, right?  Is
that intentional?

Any chance this patch fixes your issue?  If not, the above readelf would
help me understand it more.

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 5e5388a38e2a..4f30a763a4e3 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1344,6 +1344,10 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 		if (prev_offset && reloc->offset != prev_offset + 8)
 			break;
 
+		/* Jump table relocs are always STT_SECTION: */
+		if (reloc->sym->type != STT_SECTION)
+			break;
+
 		/* Detect function pointers from contiguous objects: */
 		if (reloc->sym->sec == pfunc->sec &&
 		    reloc->addend == pfunc->offset)

Re: [PATCH] objtool: include symbol value in check for contiguous objects

[Vanessa Hack]

Hi Josh,

I think there might have been a problem with my mail client, so I’m sorry
if this has been sent twice.

> I believe add_jump_table() -- in addition to all the other jump table
> code -- assumes that reloc->sym is a section symbol (STT_SECTION),
> rather than a function symbol (STT_FUNC).  Was it an STT_FUNC symbol
> which caused the problem?

Indeed as you proposed it was a symbol with another type than STT_SECTION 
that caused the problem. This resulted of our mistake of not having compiled 
our code using -fno-pic and the fact, that it was i386 code as we are trying to 
adapt objtool for our 32 bit operating system. Consequently, local labels for a 
switch jump table were created, which the existing objtool code seems not to 
be intended to handle. 

> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 5e5388a38e2a..4f30a763a4e3 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1344,6 +1344,10 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
> 		if (prev_offset && reloc->offset != prev_offset + 8)
> 			break;
> 
> +		/* Jump table relocs are always STT_SECTION: */
> +		if (reloc->sym->type != STT_SECTION)
> +			break;
> +
> 		/* Detect function pointers from contiguous objects: */
> 		if (reloc->sym->sec == pfunc->sec &&
> 		    reloc->addend == pfunc->offset)

Your suggested patch would have helped us detecting the problem earlier and 
seems like a good idea to check.

Regards,
Vanessa