Patch for GCC plugin hash table corruption bug (ID 80094)

Submitted by Brad Spengler on March 20, 2017, 11:05 p.m.

Details

Message ID 20170320230554.GA10291@grsecurity.net
State New
Headers show

Commit Message

Brad Spengler March 20, 2017, 11:05 p.m.
Hi,

As requested in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80094
i'm attaching a patch for the issue described.

Specifically:
when the plugin_name_args_tab hash table has its 11th entry inserted, it
trigers a hash table resize.  This resize performs the hash_f against
each slot's value.  Though the code was looking for matches in the hash
table via simple strings, the value of each slot was a pointer to a
plugin_name_args struct.  The resize would thus effectively treat the
plugin_name_args struct as a string, producing incorrect hashes that result
in subsequent lookups for previously inserted items generally failing.

To solve this, we use the correct hash function that operates on the
base_name field of the plugin_name_args struct and to minimize the changes
required, act in a similar way to tlink.c and other files by using the
_with_hash variants of lookup and removal functions, which allow us to
search based on just the names provided (which will match with the hash
formed from the base_name field).

The patch is untested, but can be tested via the reproducer provided
at the link above.  I have verified that it passes check_GNU_style.sh.

All versions of GCC that support plugins (4.5+) are affected by this bug,
and users of grsecurity (who enable all the GCC plugins we provide) can
potentially hit this bug today (we have over 11 plugins, though some require
specific steps to enable) and definitely will hit it in the near future
as we add more GCC plugins.  Since the bug results in a compile failure with
a deceptive error message (about arguments being out of order which aren't
in fact out of order), it's important to backport this to all affected
versions.

Let me know if you have any questions or need anything else.

Very Respectfully,
-Brad

Comments

Richard Guenther March 21, 2017, 9:25 a.m.
On Tue, Mar 21, 2017 at 12:05 AM, Brad Spengler <spender@grsecurity.net> wrote:
> Hi,
>
> As requested in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80094
> i'm attaching a patch for the issue described.
>
> Specifically:
> when the plugin_name_args_tab hash table has its 11th entry inserted, it
> trigers a hash table resize.  This resize performs the hash_f against
> each slot's value.  Though the code was looking for matches in the hash
> table via simple strings, the value of each slot was a pointer to a
> plugin_name_args struct.  The resize would thus effectively treat the
> plugin_name_args struct as a string, producing incorrect hashes that result
> in subsequent lookups for previously inserted items generally failing.
>
> To solve this, we use the correct hash function that operates on the
> base_name field of the plugin_name_args struct and to minimize the changes
> required, act in a similar way to tlink.c and other files by using the
> _with_hash variants of lookup and removal functions, which allow us to
> search based on just the names provided (which will match with the hash
> formed from the base_name field).
>
> The patch is untested, but can be tested via the reproducer provided
> at the link above.  I have verified that it passes check_GNU_style.sh.
>
> All versions of GCC that support plugins (4.5+) are affected by this bug,
> and users of grsecurity (who enable all the GCC plugins we provide) can
> potentially hit this bug today (we have over 11 plugins, though some require
> specific steps to enable) and definitely will hit it in the near future
> as we add more GCC plugins.  Since the bug results in a compile failure with
> a deceptive error message (about arguments being out of order which aren't
> in fact out of order), it's important to backport this to all affected
> versions.
>
> Let me know if you have any questions or need anything else.

I've picked it up for my current test run on trunk and will commit it.
It's a minor
enough change to not need a copyright assignment but if you're going to
do further contributions getting one is appreciated.

Thanks,
Richard.

> Very Respectfully,
> -Brad

Patch hide | download patch | download mbox

--- plugin.c	2014-01-02 17:23:26.000000000 -0500
+++ /root/plugin.c	2017-03-20 18:40:18.250214383 -0400
@@ -117,6 +117,16 @@  static const char *str_plugin_init_func_
 static const char *str_license = "plugin_is_GPL_compatible";
 #endif
 
+/* Helper function for hashing the base_name of the plugin_name_args
+   structure to be inserted into the hash table.  */
+
+static hashval_t
+htab_hash_plugin (const PTR p)
+{
+  const struct plugin_name_args *plugin = (const struct plugin_name_args *) p;
+  return htab_hash_string (plugin->base_name);
+ }
+
 /* Helper function for the hash table that compares the base_name of the
    existing entry (S1) with the given string (S2).  */
 
@@ -185,10 +195,11 @@  add_new_plugin (const char* plugin_name)
   /* If this is the first -fplugin= option we encounter, create
      'plugin_name_args_tab' hash table.  */
   if (!plugin_name_args_tab)
-    plugin_name_args_tab = htab_create (10, htab_hash_string, htab_str_eq,
+    plugin_name_args_tab = htab_create (10, htab_hash_plugin, htab_str_eq,
                                         NULL);
 
-  slot = htab_find_slot (plugin_name_args_tab, base_name, INSERT);
+  slot = htab_find_slot_with_hash (plugin_name_args_tab, base_name,
+				   htab_hash_string (base_name), INSERT);
 
   /* If the same plugin (name) has been specified earlier, either emit an
      error or a warning message depending on if they have identical full
@@ -275,7 +286,8 @@  parse_plugin_arg_opt (const char *arg)
   /* Check if the named plugin has already been specified earlier in the
      command-line.  */
   if (plugin_name_args_tab
-      && ((slot = htab_find_slot (plugin_name_args_tab, name, NO_INSERT))
+      && ((slot = htab_find_slot_with_hash (plugin_name_args_tab, name,
+					    htab_hash_string (name), NO_INSERT))
           != NULL))
     {
       struct plugin_name_args *plugin = (struct plugin_name_args *) *slot;
@@ -331,7 +343,8 @@  parse_plugin_arg_opt (const char *arg)
 static void
 register_plugin_info (const char* name, struct plugin_info *info)
 {
-  void **slot = htab_find_slot (plugin_name_args_tab, name, NO_INSERT);
+  void **slot = htab_find_slot_with_hash (plugin_name_args_tab, name,
+					  htab_hash_string (name), NO_INSERT);
   struct plugin_name_args *plugin = (struct plugin_name_args *) *slot;
   plugin->version = info->version;
   plugin->help = info->help;
@@ -628,7 +641,8 @@  init_one_plugin (void **slot, void * ARG
   bool ok = try_init_one_plugin (plugin);
   if (!ok)
     {
-      htab_remove_elt (plugin_name_args_tab, plugin->base_name);
+      htab_remove_elt_with_hash (plugin_name_args_tab, plugin->base_name,
+				 htab_hash_string (plugin->base_name));
       XDELETE (plugin);
     }
   return 1;