From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mfL13-0004zb-OZ for mharc-grub-devel@gnu.org; Tue, 26 Oct 2021 07:53:17 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:42108) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mfL10-0004xb-55 for grub-devel@gnu.org; Tue, 26 Oct 2021 07:53:14 -0400 Received: from dibed.net-space.pl ([84.10.22.86]:52177) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1mfL0x-00087H-5W for grub-devel@gnu.org; Tue, 26 Oct 2021 07:53:13 -0400 Received: from router-fw.i.net-space.pl ([192.168.52.1]:40962 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S1939018AbhJZLxH (ORCPT ); Tue, 26 Oct 2021 13:53:07 +0200 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Tue, 26 Oct 2021 13:52:51 +0200 From: Daniel Kiper To: Robbie Harwood Cc: grub-devel@gnu.org Subject: Re: [PATCH v3] Print module name on license check failure Message-ID: <20211026115251.v6wkvcuuk3kuwrpd@tomti.i.net-space.pl> References: <20211025221703.168221-1-rharwood@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211025221703.168221-1-rharwood@redhat.com> User-Agent: NeoMutt/20170113 (1.7.2) Received-SPF: pass client-ip=84.10.22.86; envelope-from=dkiper@net-space.pl; helo=dibed.net-space.pl X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Oct 2021 11:53:14 -0000 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 > --- > 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