[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] Print module name on license check failure
From: |
Robbie Harwood |
Subject: |
Re: [PATCH v3] Print module name on license check failure |
Date: |
Tue, 26 Oct 2021 12:08:23 -0400 |
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.
>> 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