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

[Daniel Kiper]

On Mon, Oct 25, 2021 at 06:17:03PM -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 | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
> index 48f8a7907..363bacc43 100644
> --- a/grub-core/kern/dl.c
> +++ b/grub-core/kern/dl.c
> @@ -457,14 +457,21 @@ 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))
> +  if (!s)

s == NULL please...

> +    return grub_error (GRUB_ERR_BAD_MODULE,
> +                    "no license section in module %.64s", mod->name);
> +
> +  if (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)

Could use grub_strncmp() instead of grub_strcmp() here?

>      return GRUB_ERR_NONE;
> -  return grub_error (GRUB_ERR_BAD_MODULE, "incompatible license");
> +
> +  return grub_error (GRUB_ERR_BAD_MODULE,
> +                  "incompatible license in module %.64s: %.64s", mod->name,
> +                  (char *) e + s->sh_offset);
>  }
>
>  static grub_err_t
> @@ -641,8 +648,8 @@ grub_dl_load_core_noinit (void *addr, grub_size_t size)
>       constitutes linking) and GRUB core being licensed under GPLv3+.
>       Be sure to understand your license obligations.
>    */
> -  if (grub_dl_check_license (e)
> -      || grub_dl_resolve_name (mod, e)
> +  if (grub_dl_resolve_name (mod, e)
> +      || grub_dl_check_license (mod, e)

I think you should split this patch into two. One which improves error
messages and another which imposes length restrictions on the
grub_strcmp() and grub_error() in the grub_dl_check_license(). And
I think you should use 63 instead of 64 (63 + NUL gives us 64 :-)).

Daniel