diff mbox series

elf: Simplify version test when searching a versioned symbol

Message ID 20220501074619.1744068-1-maskray@google.com
State Superseded
Headers
Series elf: Simplify version test when searching a versioned symbol |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Fangrui Song May 1, 2022, 7:46 a.m. UTC 
  ---
 elf/dl-lookup.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Fangrui Song May 1, 2022, 7:56 a.m. UTC | #1 
On 2022-05-01, Fangrui Song wrote:
>---
> elf/dl-lookup.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
>index 989b073e4f..3ad6c95d79 100644
>--- a/elf/dl-lookup.c
>+++ b/elf/dl-lookup.c
>@@ -110,14 +110,14 @@ check_match (const char *const undef_name,
> 	}
>       else
> 	{
>-	  /* We can match the version information or use the
>-	     default one if it is not hidden.  */
>-	  ElfW(Half) ndx = verstab[symidx] & 0x7fff;
>+	  /* When the version does not match, allow VER_NDX_GLOBAL fallback when
>+	     resolving relocations (version->hidden==0).  Don't bother with the
>+	     check done by the linker: VER_NDX_GLOBAL symbol cannot be hidden.
>+	     */
>+	  ElfW (Half) ndx = verstab[symidx] & 0x7fff;
> 	  if ((map->l_versions[ndx].hash != version->hash
> 	       || strcmp (map->l_versions[ndx].name, version->name))
>-	      && (version->hidden || map->l_versions[ndx].hash
>-		  || (verstab[symidx] & 0x8000)))
>-	    /* It's not the version we want.  */
>+	      && (version->hidden || ndx != VER_NDX_GLOBAL))
> 	    return NULL;
> 	}
>     }
>-- 
>2.36.0.464.gb9c8b46e94-goog

The existing code has a bug.

If a has foo@v1 referencing b.so. If I rebuild b.so and change foo@v1 to
foo VER_NDX_GLOBAL,

`strcmp (map->l_versions[ndx].name, version->name)` may trigger a null
pointer dereference:

   (rr) p map->l_versions[1]
   $7 = {name = 0x0, hash = 0, hidden = 0, filename = 0x0}

This can be fixed with `!map->l_versions[ndx].name || strcmp (map->l_versions[ndx].name, version->name)`
Florian Weimer May 2, 2022, 8:40 a.m. UTC | #2 
* Fangrui Song:

> On 2022-05-01, Fangrui Song wrote:
>>---
>> elf/dl-lookup.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>>diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
>>index 989b073e4f..3ad6c95d79 100644
>>--- a/elf/dl-lookup.c
>>+++ b/elf/dl-lookup.c
>>@@ -110,14 +110,14 @@ check_match (const char *const undef_name,
>> 	}
>>       else
>> 	{
>>-	  /* We can match the version information or use the
>>-	     default one if it is not hidden.  */
>>-	  ElfW(Half) ndx = verstab[symidx] & 0x7fff;
>>+	  /* When the version does not match, allow VER_NDX_GLOBAL fallback when
>>+	     resolving relocations (version->hidden==0).  Don't bother with the
>>+	     check done by the linker: VER_NDX_GLOBAL symbol cannot be hidden.
>>+	     */
>>+	  ElfW (Half) ndx = verstab[symidx] & 0x7fff;
>> 	  if ((map->l_versions[ndx].hash != version->hash
>> 	       || strcmp (map->l_versions[ndx].name, version->name))
>>-	      && (version->hidden || map->l_versions[ndx].hash
>>-		  || (verstab[symidx] & 0x8000)))
>>-	    /* It's not the version we want.  */
>>+	      && (version->hidden || ndx != VER_NDX_GLOBAL))
>> 	    return NULL;
>> 	}
>>     }
>> -- 2.36.0.464.gb9c8b46e94-goog
>
> The existing code has a bug.
>
> If a has foo@v1 referencing b.so. If I rebuild b.so and change foo@v1 to
> foo VER_NDX_GLOBAL,
>
> `strcmp (map->l_versions[ndx].name, version->name)` may trigger a null
> pointer dereference:
>
>   (rr) p map->l_versions[1]
>   $7 = {name = 0x0, hash = 0, hidden = 0, filename = 0x0}
>
> This can be fixed with `!map->l_versions[ndx].name || strcmp (map->l_versions[ndx].name, version->name)`

Hmm.  How do we handle VER_NDX_GLOBAL in the dynamic linker?

The if branch has an assert.  I think it fires if we drop symbol
versioning completely.  I think we should report a proper error for
that.

Should the switch to VER_NDX_GLOBAL also result in an error
(eventually)?

Thanks,
Florian
Fangrui Song May 2, 2022, 6:58 p.m. UTC | #3 
On 2022-05-02, Florian Weimer wrote:
>* Fangrui Song:
>
>> On 2022-05-01, Fangrui Song wrote:
>>>---
>>> elf/dl-lookup.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>>diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
>>>index 989b073e4f..3ad6c95d79 100644
>>>--- a/elf/dl-lookup.c
>>>+++ b/elf/dl-lookup.c
>>>@@ -110,14 +110,14 @@ check_match (const char *const undef_name,
>>> 	}
>>>       else
>>> 	{
>>>-	  /* We can match the version information or use the
>>>-	     default one if it is not hidden.  */
>>>-	  ElfW(Half) ndx = verstab[symidx] & 0x7fff;
>>>+	  /* When the version does not match, allow VER_NDX_GLOBAL fallback when
>>>+	     resolving relocations (version->hidden==0).  Don't bother with the
>>>+	     check done by the linker: VER_NDX_GLOBAL symbol cannot be hidden.
>>>+	     */
>>>+	  ElfW (Half) ndx = verstab[symidx] & 0x7fff;
>>> 	  if ((map->l_versions[ndx].hash != version->hash
>>> 	       || strcmp (map->l_versions[ndx].name, version->name))
>>>-	      && (version->hidden || map->l_versions[ndx].hash
>>>-		  || (verstab[symidx] & 0x8000)))
>>>-	    /* It's not the version we want.  */
>>>+	      && (version->hidden || ndx != VER_NDX_GLOBAL))
>>> 	    return NULL;
>>> 	}
>>>     }
>>> -- 2.36.0.464.gb9c8b46e94-goog
>>
>> The existing code has a bug.
>>
>> If a has foo@v1 referencing b.so. If I rebuild b.so and change foo@v1 to
>> foo VER_NDX_GLOBAL,
>>
>> `strcmp (map->l_versions[ndx].name, version->name)` may trigger a null
>> pointer dereference:
>>
>>   (rr) p map->l_versions[1]
>>   $7 = {name = 0x0, hash = 0, hidden = 0, filename = 0x0}
>>
>> This can be fixed with `!map->l_versions[ndx].name || strcmp (map->l_versions[ndx].name, version->name)`
>
>Hmm.  How do we handle VER_NDX_GLOBAL in the dynamic linker?

VER_NDX_GLOBAL (1) is handled the same way as versions with index >= 2,
in _dl_check_map_versions in dl-version.c. I think l_versions[1] is
typically filled in due to VER_FLG_BASE in DT_VERDEF (.gnu.version_d).

An object may have DT_VERDEF but not DT_VERNEED. In that case, the field
for VER_NDX_GLOBAL are all zeroes.

>The if branch has an assert.  I think it fires if we drop symbol
>versioning completely.  I think we should report a proper error for
>that.
>
>Should the switch to VER_NDX_GLOBAL also result in an error
>(eventually)?

When playing around this yesterday, I have triggered the assert failure
but I don't remember how I did.

       assert (version->filename == NULL
               || ! _dl_name_match_p (version->filename, map));
diff mbox series

Patch

diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index 989b073e4f..3ad6c95d79 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -110,14 +110,14 @@  check_match (const char *const undef_name,
 	}
       else
 	{
-	  /* We can match the version information or use the
-	     default one if it is not hidden.  */
-	  ElfW(Half) ndx = verstab[symidx] & 0x7fff;
+	  /* When the version does not match, allow VER_NDX_GLOBAL fallback when
+	     resolving relocations (version->hidden==0).  Don't bother with the
+	     check done by the linker: VER_NDX_GLOBAL symbol cannot be hidden.
+	     */
+	  ElfW (Half) ndx = verstab[symidx] & 0x7fff;
 	  if ((map->l_versions[ndx].hash != version->hash
 	       || strcmp (map->l_versions[ndx].name, version->name))
-	      && (version->hidden || map->l_versions[ndx].hash
-		  || (verstab[symidx] & 0x8000)))
-	    /* It's not the version we want.  */
+	      && (version->hidden || ndx != VER_NDX_GLOBAL))
 	    return NULL;
 	}
     }