[PATCH v3] Print module name on license check failure

[PATCH v3] Print module name on license check failure

[Robbie Harwood]

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)
+    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)
     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)
       || grub_dl_resolve_dependencies (mod, e)
       || grub_dl_load_segments (mod, e)
       || grub_dl_resolve_symbols (mod, e)
-- 
2.33.0

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

[Daniel Kiper]

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?

>      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

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

[Robbie Harwood]

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

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