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

[Daniel Kiper]

On Tue, Oct 26, 2021 at 12:08:23PM -0400, Robbie Harwood wrote:
> Daniel Kiper <dkiper@net-space.pl> writes:
>
> > 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?
>
> I don't object to doing so for clarity, but strictly speaking I don't
> think it's needed as the LICENSE= strings are terminated.

Huh! Yeah, you are right. So, please ignore my stupid comment.

> >>      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().
>
> I can do that, but the NULL-check and strcmp are both pre-existing code
> from when the license check was added.  Are you sure?

Ditto.

> > And I think you should use 63 instead of 64 (63 + NUL gives us 64
> > :-)).

I will fix it myself before pushing v3.

Daniel