Hi list,
during development of the PaX module patch[1], Lianbo pointed out an
issue about adding new members to struct offset_table. They should be
added to the end to ensure binary compatibility with already compiled
extensions, as these won't be aware of the new members and, even worse,
might be using wrong offsets within the offset_table if we didn't. As
this seems to be rather fragile, here's a proposal to make it less so.
To not blindly make already compiled extensions misbehave by using the
wrong offsets when new members get added in the middle of shared types,
we need to have some validation step, preferably during extension
registration to catch these.
If we want to keep binary compatibility, we need to add members to the
end. That's a rule maintainers can enforce but as we all make mistakes,
code can help us here. Changing a shared data structure has, at least,
implications on its size. So we can use this as a base to build checks
on. If the size of a data structure used during compilation of an
extension differs from the size of that same data structure as seen
within 'crash', we have an ABI issue. The response should be, at least,
a warning to inform the user to recompile its extension to ensure ABI
compatibility. Another option would be to decline loading the extension
but that might be inconvenient for users, if we indeed made sure to add
members only at the end of the involved data structures.
These checks can be implemented "transparently" as sketched in the
attached patch. It changes register_extension() to be a macro that
stores the sizes of critical types in a data structure that gets passed
to _register_extension() which is implemented in 'crash' and will do the
size checks against its version of the involved types.
The patch also takes care about backward compatibility by providing a
stub register_extension() function that passes a NULL structure, as all
current extension modules refer to register_extension() instead of the
newly introduced _register_extension().
Open TODOs:
1/ Compile a list of all types we want to protect and add checks for
these.
2/ Put these types into a 'shared.h' header file and use that
exclusively within extensions -- maybe even enforce it.
Do we want to follow that approach or look for alternatives?
Regards,
Mathias
[1]: https://www.redhat.com/archives/crash-utility/2020-August/msg00024.html
diff --git a/defs.h b/defs.h
index 17e98763362b..e968709148dc 100644
--- a/defs.h
+++ b/defs.h
@@ -2283,6 +2283,13 @@ struct array_table {
int vm_numa_stat;
};
+struct table_sizes {
+ size_t offset_table;
+ size_t size_table;
+ size_t array_table;
+ // more sizes...
+};
+
/*
* The following set of macros use gdb to determine structure, union,
* or member sizes/offsets. They should be used only during initialization
@@ -5545,7 +5552,17 @@ void check_stack_overflow(void);
/*
* extensions.c
*/
-void register_extension(struct command_table_entry *);
+#define register_extension(cte) \
+ do { \
+ static const struct table_sizes ts = { \
+ .offset_table = sizeof(struct offset_table), \
+ .size_table = sizeof(struct size_table), \
+ .array_table = sizeof(struct array_table), \
+ }; \
+ _register_extension(cte, &ts); \
+ } while (0)
+
+void _register_extension(struct command_table_entry *, const struct table_sizes *);
void dump_extension_table(int);
void load_extension(char *);
void unload_extension(char *);
diff --git a/extensions.c b/extensions.c
index d23b1e3cc26f..92accb59cf65 100644
--- a/extensions.c
+++ b/extensions.c
@@ -545,6 +545,30 @@ unload_extension(char *lib)
error(INFO, "%s: not loaded\n", lib);
}
+static void verify_table_sizes(const struct table_sizes *tab_sizes)
+{
+ int mismatch = 0;
+
+ if (tab_sizes == NULL) {
+ mismatch++;
+ goto out;
+ }
+
+ if (tab_sizes->offset_table != sizeof(struct offset_table)) {
+ error(WARNING, "%s: offset_table size mismatch (%zu vs. %zu)\n",
+ tab_sizes->offset_table, sizeof(struct offset_table),
+ pc->curext->filename);
+ mismatch++;
+ }
+
+ // more size tests...
+
+out:
+ if (mismatch)
+ error(INFO, "Please consider recompiling %s to ensure binary compatibility with crash\n",
+ pc->curext->filename);
+}
+
/*
* Register the command_table as long as there are no command namespace
* clashes with the currently-existing command set. Also delete any aliases
@@ -556,10 +580,13 @@ unload_extension(char *lib)
* REGISTERED bit in the "current" extension_table structure flags.
*/
void
-register_extension(struct command_table_entry *command_table)
+_register_extension(struct command_table_entry *command_table,
+ const struct table_sizes *tab_sizes)
{
struct command_table_entry *cp;
+ verify_table_sizes(tab_sizes);
+
pc->curext->flags |= NO_MINIMAL_COMMANDS;
for (cp = command_table; cp->name; cp++) {
@@ -604,6 +631,15 @@ register_extension(struct command_table_entry *command_table)
pc->curext->flags |= REGISTERED; /* Mark of approval */
}
+/* for backwards compatibility with existing extensions */
+#undef register_extension
+void register_extension(struct command_table_entry *);
+void
+register_extension(struct command_table_entry *command_table)
+{
+ _register_extension(command_table, NULL);
+}
+
/*
* Hooks for sial.
*/
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
