Re: [PATCH v2] Print module name on license check failure

[Daniel Kiper]

On Wed, Oct 20, 2021 at 11:19:13AM -0400, Robbie Harwood wrote:
> Before performing the license check, resolve the module name so that it
> can be printed if the license check fails.  Prior to this change, grub
> would only indicate that the check had been failed, but not by what
> module.  This made it difficult to track down either the problem module,
> or debug the false positive further.
>
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  grub-core/kern/dl.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 48f8a7907..90e83e4d4 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -457,14 +457,16 @@ grub_dl_find_section (Elf_Ehdr *e, const char *name)
>     Be sure to understand your license obligations.
>  */
>  static grub_err_t
> -grub_dl_check_license (Elf_Ehdr *e)
> +grub_dl_check_license (grub_dl_t mod, Elf_Ehdr *e)
>  {
>    Elf_Shdr *s = grub_dl_find_section (e, ".module_license");
>    if (s && (grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3") == 0
>           || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3+") == 0
>           || grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv2+") == 0))
>      return GRUB_ERR_NONE;
> -  return grub_error (GRUB_ERR_BAD_MODULE, "incompatible license");
> +  return grub_error (GRUB_ERR_BAD_MODULE,
> +                  "incompatible license in module %s: %s", mod->name,
> +                  (char *) e + s->sh_offset);

The Covertiy complains with this:

  *** CID 361072:  Null pointer dereferences  (FORWARD_NULL)
  /grub-core/kern/dl.c: 467 in grub_dl_check_license()
  461     {
  462       Elf_Shdr *s = grub_dl_find_section (e, ".module_license");
  463       if (s && (grub_strcmp ((char *) e + s->sh_offset, "LICENSE=GPLv3") 
== 0
  464               || grub_strcmp ((char *) e + s->sh_offset, 
"LICENSE=GPLv3+") == 0
  465               || grub_strcmp ((char *) e + s->sh_offset, 
"LICENSE=GPLv2+") == 0))
  466         return GRUB_ERR_NONE;
  >>>     CID 361072:  Null pointer dereferences  (FORWARD_NULL)
  >>>     Dereferencing null pointer "s".
  467       return grub_error (GRUB_ERR_BAD_MODULE, "incompatible license in 
module %s: %s",
  468                        mod->name, (char *) e + s->sh_offset);
  469     }

I think you should rework the if conditional and split it into two parts.
First you should check if s is not NULL. If it is then print an error, e.g.
"no license section in %s module". Then second if checking for licenses.

Additionally, I think we trust to widely input from the ELF files here.
I would limit all comparisons and messages to first e.g. 64 characters
from the "(char *) e + s->sh_offset". This begs for second patch.

Daniel