On Tue, Oct 26, 2021 at 12:08:23PM -0400, Robbie Harwood wrote: > Daniel Kiper <dki...@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 <rharw...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel