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. >> 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? > And I think you should use 63 instead of 64 (63 + NUL gives us 64 > :-)). Fine. Be well, --Robbie
signature.asc
Description: PGP signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel