[PATCH 3/3] PR4886: Check build-id in module_init function if able.

[PATCH 3/3] PR4886: Check build-id in module_init function if able.

[Wenji Huang]

There are several cases to be considered, whether build-id exists in 
debuginfo file or not, whether module is loaded or not and whether 
build-id exists in loaded module/kernel or not.
---
  runtime/sym.c |   66 
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  translate.cxx |    1 +
  2 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/runtime/sym.c b/runtime/sym.c
index 1a9e26b..38323c9 100644
--- a/runtime/sym.c
+++ b/runtime/sym.c
@@ -160,6 +160,72 @@ static const char *_stp_kallsyms_lookup(unsigned 
long addr, unsigned long *symbo
         return NULL;
  }

+/* Validate module/kernel based on build-id if there
+*  The completed case is the following combination:
+*         Debuginfo             Module                          Kernel
+*                         X                            X
+*      has build-id/not        unloaded                      has 
build-id/not
+*                              loaded && (has build-id/not)
+*
+*  NB: build-id exists only if ld>=2.18 and kernel>= 2.6.23
+*/
+static int _stp_module_check(void)
+{
+       struct _stp_module *m = NULL;
+       unsigned long notes_addr, base_addr;
+       unsigned i;
+
+       for (i = 0; i < _stp_num_modules; i++)
+       {
+               m = _stp_modules[i];
+
+               /* unloaded module */
+               if (m->notes_sect == 0) {
+                           _stp_warn("skip checking %s\n", m->name);
+                    continue;
+                }
+               if (m->build_id_len > 0) { /* build-id in debuginfo file */
+                   dbug_sym(1, "validate %s based on build-id\n", m->name);
+
+                   /* loaded module/kernel, but without build-id */
+                   if (m->notes_sect == 1) {
+                       _stp_error("missing build-id in %s\n", m->name);
+                       return 1;
+                   }
+                   /* notes end address */
+                   if (!strcmp(m->name, "kernel")) {
+                         notes_addr = m->build_id_offset;
+                         base_addr = _stp_module_relocate("kernel",
+                                                          "_stext", 0);
+                    } else {
+                         notes_addr = m->notes_sect + m->build_id_offset;
+                         base_addr = m->notes_sect;
+                   }
+                   /* notes start address */
+                   notes_addr -= m->build_id_len;
+                   if (notes_addr > base_addr) {
+                       if (memcmp(m->build_id_bits,
+                                   (unsigned char *) notes_addr,
+                                   m->build_id_len))
+                       {
+                          _stp_error("inconsistent build-id in %s with 
debuginfo\n", m->name);
+                          return 1;
+                       }
+                   } else { /* bug, shouldn't come here */
+                            _stp_error("unknown failure in checking %s\n",
+                                                               m->name);
+                            return 1;
+                          } /* end comparing */
+               } else {
+                         /* build-id in module/kernel, absent in 
debuginfo */
+                         if (m->notes_sect > 1) {
+                           _stp_error("unexpected build-id in %s\n", 
m->name);
+                           return 1;
+                         }
+                } /* end checking */
+       } /* end loop */
+       return 0;
+}

  /** Print an address symbolically.
   * @param address The address to lookup.
diff --git a/translate.cxx b/translate.cxx
index a9695f1..0ee5179 100644
--- a/translate.cxx
+++ b/translate.cxx
@@ -1113,6 +1113,7 @@ c_unparser::emit_module_init ()
    o->newline(-1) << "}";

    // XXX: perform buildid-based checking if able
+  o->newline() << "if (_stp_module_check()) rc = -EINVAL;";

    o->newline(-1) << "}";
    o->newline() << "if (rc) goto out;";
-- 
1.5.6

Re: [PATCH 3/3] PR4886: Check build-id in module_init function if able.

[Frank Ch. Eigler]

Hi, Wenji -

Excellent work.  If it passes regression tests, please check it in.
It shouldn't be hard to apply it to userspace too, if it doesn't
already just work.

Some tweaks to the various messages may be helpful later, such as to
print parts of the buildid strings for eyeballing by the user, but
otherwise it looks just fine.

- FChE

Re: [PATCH 3/3] PR4886: Check build-id in module_init function if able.

[Elena Zannoni]

Thanks for the review, Frank.
Wenji is out this week, for the Chinese National holiday. He'll check it 
in when he gets back.

elena

Frank Ch. Eigler wrote:
> Hi, Wenji -
>
> Excellent work.  If it passes regression tests, please check it in.
> It shouldn't be hard to apply it to userspace too, if it doesn't
> already just work.
>
> Some tweaks to the various messages may be helpful later, such as to
> print parts of the buildid strings for eyeballing by the user, but
> otherwise it looks just fine.
>
> - FChE
>
>   


-- 
Elena Zannoni, Oracle
Senior Engineering Manager, Tools/Languages - Linux Engineering
Blog: http://blogs.oracle.com/ezannoni
Email: elena.zannoni@oracle.com

Re: [PATCH 3/3] PR4886: Check build-id in module_init function if able.

[Masami Hiramatsu]

Hi Wenji,

I'm not sure when the build-id is updated, but interested in your
enhancement. :-)
If I'm using an out-of-tree driver, is build-id updated when I updated
the driver's source code but didn't change running kernel?
In other words, would each driver have its build-id which is different
from other drivers and kernel?

Thank you,

Wenji Huang wrote:
> There are several cases to be considered, whether build-id exists in 
> debuginfo file or not, whether module is loaded or not and whether 
> build-id exists in loaded module/kernel or not.
> ---
>   runtime/sym.c |   66 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   translate.cxx |    1 +
>   2 files changed, 67 insertions(+), 0 deletions(-)
> 
> diff --git a/runtime/sym.c b/runtime/sym.c
> index 1a9e26b..38323c9 100644
> --- a/runtime/sym.c
> +++ b/runtime/sym.c
> @@ -160,6 +160,72 @@ static const char *_stp_kallsyms_lookup(unsigned 
> long addr, unsigned long *symbo
>          return NULL;
>   }
> 
> +/* Validate module/kernel based on build-id if there
> +*  The completed case is the following combination:
> +*         Debuginfo             Module                          Kernel
> +*                         X                            X
> +*      has build-id/not        unloaded                      has 
> build-id/not
> +*                              loaded && (has build-id/not)
> +*
> +*  NB: build-id exists only if ld>=2.18 and kernel>= 2.6.23
> +*/
> +static int _stp_module_check(void)
> +{
> +       struct _stp_module *m = NULL;
> +       unsigned long notes_addr, base_addr;
> +       unsigned i;
> +
> +       for (i = 0; i < _stp_num_modules; i++)
> +       {
> +               m = _stp_modules[i];
> +
> +               /* unloaded module */
> +               if (m->notes_sect == 0) {
> +                           _stp_warn("skip checking %s\n", m->name);
> +                    continue;
> +                }
> +               if (m->build_id_len > 0) { /* build-id in debuginfo file */
> +                   dbug_sym(1, "validate %s based on build-id\n", m->name);
> +
> +                   /* loaded module/kernel, but without build-id */
> +                   if (m->notes_sect == 1) {
> +                       _stp_error("missing build-id in %s\n", m->name);
> +                       return 1;
> +                   }
> +                   /* notes end address */
> +                   if (!strcmp(m->name, "kernel")) {
> +                         notes_addr = m->build_id_offset;
> +                         base_addr = _stp_module_relocate("kernel",
> +                                                          "_stext", 0);
> +                    } else {
> +                         notes_addr = m->notes_sect + m->build_id_offset;
> +                         base_addr = m->notes_sect;
> +                   }
> +                   /* notes start address */
> +                   notes_addr -= m->build_id_len;
> +                   if (notes_addr > base_addr) {
> +                       if (memcmp(m->build_id_bits,
> +                                   (unsigned char *) notes_addr,
> +                                   m->build_id_len))
> +                       {
> +                          _stp_error("inconsistent build-id in %s with 
> debuginfo\n", m->name);
> +                          return 1;
> +                       }
> +                   } else { /* bug, shouldn't come here */
> +                            _stp_error("unknown failure in checking %s\n",
> +                                                               m->name);
> +                            return 1;
> +                          } /* end comparing */
> +               } else {
> +                         /* build-id in module/kernel, absent in 
> debuginfo */
> +                         if (m->notes_sect > 1) {
> +                           _stp_error("unexpected build-id in %s\n", 
> m->name);
> +                           return 1;
> +                         }
> +                } /* end checking */
> +       } /* end loop */
> +       return 0;
> +}
> 
>   /** Print an address symbolically.
>    * @param address The address to lookup.
> diff --git a/translate.cxx b/translate.cxx
> index a9695f1..0ee5179 100644
> --- a/translate.cxx
> +++ b/translate.cxx
> @@ -1113,6 +1113,7 @@ c_unparser::emit_module_init ()
>     o->newline(-1) << "}";
> 
>     // XXX: perform buildid-based checking if able
> +  o->newline() << "if (_stp_module_check()) rc = -EINVAL;";
> 
>     o->newline(-1) << "}";
>     o->newline() << "if (rc) goto out;";

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH 3/3] PR4886: Check build-id in module_init function if able.

[Roland McGrath]

> I'm not sure when the build-id is updated, but interested in your
> enhancement. :-)
> If I'm using an out-of-tree driver, is build-id updated when I updated
> the driver's source code but didn't change running kernel?
> In other words, would each driver have its build-id which is different
> from other drivers and kernel?

Yes, every non-identical build of a .ko file should result in a distinct
build ID embedded in that .ko file.  Only if you rebuild the same code to
get the same binary should get you the same build ID.  (The build IDs of
the related kernel or of other .ko's have nothing to do with this.)


Thanks,
Roland

Re: [PATCH 3/3] PR4886: Check build-id in module_init function if able.

[Frank Ch. Eigler]

Masami Hiramatsu <mhiramat@redhat.com> writes:

> [...]
> I'm not sure when the build-id is updated, but interested in your
> enhancement. :-)

The assembler/linker computes is based upon object file content.

> [...] In other words, would each driver have its build-id which is
> different from other drivers and kernel?

Yes.

- FChE