[PATCH] trivial fixes for windows-efi branch

[PATCH] trivial fixes for windows-efi branch

[Andrey Borzenkov]

Background - I have pending rewrite for Linux to directly use sysfs
instead of relying on efibootmgr. This should fix long standing bug
where grub-install may delete wrong boot entry due to fuzzy matching.
So I just tried to rebase it on top of this branch. I think we could
reuse part of code that builds EFI device path; everything else is probably
too different.

Below fixes for problems I hit when testing it.

==

1. Check for errors when reading Boot* variables
2. Fix core dump when variable name was not found (it tried to compare
   null pointer)

Both are likely related to signed vs. unsigned comaprison mismatch where
negative values are treated as large positive.

3. Prefer minimal free boot number found, not highest one.
4. Fix length of file path component in EFI device path

---
 grub-core/osdep/windows/platform.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/grub-core/osdep/windows/platform.c b/grub-core/osdep/windows/platform.c
index e017796..4da0ed0 100644
--- a/grub-core/osdep/windows/platform.c
+++ b/grub-core/osdep/windows/platform.c
@@ -246,6 +246,8 @@ grub_install_register_efi (grub_device_t efidir_grub_dev,
 	  void *current = NULL;
 	  ssize_t current_len;
 	  current = get_efi_variable_bootn (i, &current_len);
+	  if (current_len < 0)
+	    continue; /* Should we abort on error? */
 	  if (current_len < (distrib16_len + 1) * sizeof (grub_uint16_t)
 	      + 6)
 	    {
@@ -273,10 +275,16 @@ grub_install_register_efi (grub_device_t efidir_grub_dev,
 	  void *current = NULL;
 	  ssize_t current_len;
 	  current = get_efi_variable_bootn (i, &current_len);
+	  if (current_len < -1)
+	    continue; /* Should we abort here? */
 	  if (current_len == -1)
 	    {
-	      order_num = i;
-	      have_order_num = 1;
+	      if (!have_order_num)
+	      {
+		order_num = i;
+		have_order_num = 1;
+	      }
+	      continue;
 	    }
 	  if (current_len < (distrib16_len + 1) * sizeof (grub_uint16_t)
 	      + 6)
@@ -369,9 +377,9 @@ grub_install_register_efi (grub_device_t efidir_grub_dev,
 				   path8_len * GRUB_MAX_UTF16_PER_UTF8,
 				   (const grub_uint8_t *) efifile_path,
 				   path8_len, 0);
-  filep->path_name[path16_len + 1] = 0;
+  filep->path_name[path16_len] = 0;
   filep->header.length = sizeof (*filep) + (path16_len + 1) * sizeof (grub_uint16_t);
-  pathptr = &filep->path_name[path16_len + 2];
+  pathptr = &filep->path_name[path16_len + 1];
   endp = pathptr;
   endp->type = GRUB_EFI_END_DEVICE_PATH_TYPE;
   endp->subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
-- 
tg: (f3b8170..) u/windows-efi-fixes (depends on: origin/windows-efi)

Re: [PATCH] trivial fixes for windows-efi branch

[Vladimir 'phcoder' Serbinenko]

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

Sounds like you're looking at old version. I've fixed those problems and
it's already merged in master. Could you have a look and say if any of
those still a problem? As for merging with Linux part we could just have
primitives set variable and get variable and have everything else shared
On Dec 15, 2013 9:04 AM, "Andrey Borzenkov" <arvidjaar@gmail.com> wrote:

> Background - I have pending rewrite for Linux to directly use sysfs
> instead of relying on efibootmgr. This should fix long standing bug
> where grub-install may delete wrong boot entry due to fuzzy matching.
> So I just tried to rebase it on top of this branch. I think we could
> reuse part of code that builds EFI device path; everything else is probably
> too different.
>
> Below fixes for problems I hit when testing it.
>
> ==
>
> 1. Check for errors when reading Boot* variables
> 2. Fix core dump when variable name was not found (it tried to compare
>    null pointer)
>
> Both are likely related to signed vs. unsigned comaprison mismatch where
> negative values are treated as large positive.
>
> 3. Prefer minimal free boot number found, not highest one.
> 4. Fix length of file path component in EFI device path
>
> ---
>  grub-core/osdep/windows/platform.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/osdep/windows/platform.c
> b/grub-core/osdep/windows/platform.c
> index e017796..4da0ed0 100644
> --- a/grub-core/osdep/windows/platform.c
> +++ b/grub-core/osdep/windows/platform.c
> @@ -246,6 +246,8 @@ grub_install_register_efi (grub_device_t
> efidir_grub_dev,
>           void *current = NULL;
>           ssize_t current_len;
>           current = get_efi_variable_bootn (i, &current_len);
> +         if (current_len < 0)
> +           continue; /* Should we abort on error? */
>           if (current_len < (distrib16_len + 1) * sizeof (grub_uint16_t)
>               + 6)
>             {
> @@ -273,10 +275,16 @@ grub_install_register_efi (grub_device_t
> efidir_grub_dev,
>           void *current = NULL;
>           ssize_t current_len;
>           current = get_efi_variable_bootn (i, &current_len);
> +         if (current_len < -1)
> +           continue; /* Should we abort here? */
>           if (current_len == -1)
>             {
> -             order_num = i;
> -             have_order_num = 1;
> +             if (!have_order_num)
> +             {
> +               order_num = i;
> +               have_order_num = 1;
> +             }
> +             continue;
>             }
>           if (current_len < (distrib16_len + 1) * sizeof (grub_uint16_t)
>               + 6)
> @@ -369,9 +377,9 @@ grub_install_register_efi (grub_device_t
> efidir_grub_dev,
>                                    path8_len * GRUB_MAX_UTF16_PER_UTF8,
>                                    (const grub_uint8_t *) efifile_path,
>                                    path8_len, 0);
> -  filep->path_name[path16_len + 1] = 0;
> +  filep->path_name[path16_len] = 0;
>    filep->header.length = sizeof (*filep) + (path16_len + 1) * sizeof
> (grub_uint16_t);
> -  pathptr = &filep->path_name[path16_len + 2];
> +  pathptr = &filep->path_name[path16_len + 1];
>    endp = pathptr;
>    endp->type = GRUB_EFI_END_DEVICE_PATH_TYPE;
>    endp->subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
> --
> tg: (f3b8170..) u/windows-efi-fixes (depends on: origin/windows-efi)
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 4196 bytes --]

Re: [PATCH] trivial fixes for windows-efi branch

[Andrey Borzenkov]

В Sun, 15 Dec 2013 12:43:49 +0100
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:

> Sounds like you're looking at old version. I've fixed those problems and
> it's already merged in master. Could you have a look and say if any of
> those still a problem?

Yes, it fixed file path and variable not existed in exhaustive search.
Still see comments below.

diff --git a/grub-core/osdep/windows/platform.c
b/grub-core/osdep/windows/platform.c index b123256..3f4ad5e 100644
--- a/grub-core/osdep/windows/platform.c
+++ b/grub-core/osdep/windows/platform.c
@@ -246,6 +246,8 @@ grub_install_register_efi (grub_device_t
efidir_grub_dev, void *current = NULL;
 	  ssize_t current_len;
 	  current = get_efi_variable_bootn (i, &current_len);
+	  if (current_len < 0)
+	    continue; /* FIXME Should we abort on error? */
 	  if (current_len < (distrib16_len + 1) * sizeof
(grub_uint16_t)
 	      + 6)
 	    {

Same potential problem with signed vs. unsigned comparison. Variable is
not guaranteed to exist here and -1 is treated as very large unsigned
value.

@@ -275,13 +277,18 @@ grub_install_register_efi (grub_device_t
efidir_grub_dev, void *current = NULL;
 	  ssize_t current_len;
 	  current = get_efi_variable_bootn (i, &current_len);
+	  if (current_len < -1)
+	    continue; /* FIXME Should we abort on error? */

For completeness we should handle failures to avoid crash.

 	  if (current_len == -1)
 	    {
-	      order_num = i;
-	      have_order_num = 1;
-	      grub_util_info ("Creating new entry at Boot%04x",
-			      order_num);
-	      break;
+	      if (!have_order_num)
+		{
+		  order_num = i;
+		  have_order_num = 1;
+		  grub_util_info ("Creating new entry at Boot%04x",
+				  order_num);
+		}
+	      continue;
 	    }

You changed it to stop on first non-existing variable; but this can
miss another one later with the same description. This will result in
two different boot entries with the same name.

I cannot test it on Windows; on Linux code appears to be fast enough.

>                       As for merging with Linux part we could just have
> primitives set variable and get variable and have everything else shared

Yes, I'll get a look.

Re: [PATCH] trivial fixes for windows-efi branch

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 15.12.2013 13:41, Andrey Borzenkov wrote:
> В Sun, 15 Dec 2013 12:43:49 +0100
> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
> 
>> Sounds like you're looking at old version. I've fixed those problems and
>> it's already merged in master. Could you have a look and say if any of
>> those still a problem?
> 
> Yes, it fixed file path and variable not existed in exhaustive search.
> Still see comments below.
> 
> diff --git a/grub-core/osdep/windows/platform.c
> b/grub-core/osdep/windows/platform.c index b123256..3f4ad5e 100644
> --- a/grub-core/osdep/windows/platform.c
> +++ b/grub-core/osdep/windows/platform.c
> @@ -246,6 +246,8 @@ grub_install_register_efi (grub_device_t
> efidir_grub_dev, void *current = NULL;
>  	  ssize_t current_len;
>  	  current = get_efi_variable_bootn (i, &current_len);
> +	  if (current_len < 0)
> +	    continue; /* FIXME Should we abort on error? */
>  	  if (current_len < (distrib16_len + 1) * sizeof
> (grub_uint16_t)
>  	      + 6)
>  	    {
> 
> Same potential problem with signed vs. unsigned comparison. Variable is
> not guaranteed to exist here and -1 is treated as very large unsigned
> value.
> 
Yes, apparently mingw treats here as siged comparison and no warning is
emitted. As for the comment, the reasons for possible failure are
somewhat unclear to me, other than permission denied one. On Windows you
wouldn't get here if permission is denied (you'd stop at token
manipulation). Some kind of internal EFI failure may leave some
variables unusable while otherwise allowing algorithm to work.
> @@ -275,13 +277,18 @@ grub_install_register_efi (grub_device_t
> efidir_grub_dev, void *current = NULL;
>  	  ssize_t current_len;
>  	  current = get_efi_variable_bootn (i, &current_len);
> +	  if (current_len < -1)
> +	    continue; /* FIXME Should we abort on error? */
> 
> For completeness we should handle failures to avoid crash.
> 
Same.
Go ahead for those 2 hunks.
>  	  if (current_len == -1)
>  	    {
> -	      order_num = i;
> -	      have_order_num = 1;
> -	      grub_util_info ("Creating new entry at Boot%04x",
> -			      order_num);
> -	      break;
> +	      if (!have_order_num)
> +		{
> +		  order_num = i;
> +		  have_order_num = 1;
> +		  grub_util_info ("Creating new entry at Boot%04x",
> +				  order_num);
> +		}
> +	      continue;
>  	    }
> 
> You changed it to stop on first non-existing variable; but this can
> miss another one later with the same description. This will result in
> two different boot entries with the same name.
> 
> I cannot test it on Windows; on Linux code appears to be fast enough.
> 
I'll test think hunk on my next reboot.
>>                       As for merging with Linux part we could just have
>> primitives set variable and get variable and have everything else shared
> 
> Yes, I'll get a look.
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

Re: [PATCH] trivial fixes for windows-efi branch

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 15.12.2013 13:41, Andrey Borzenkov wrote:
> В Sun, 15 Dec 2013 12:43:49 +0100
> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
> 
>> Sounds like you're looking at old version. I've fixed those problems and
>> it's already merged in master. Could you have a look and say if any of
>> those still a problem?
> 
> Yes, it fixed file path and variable not existed in exhaustive search.
> Still see comments below.
> 
> diff --git a/grub-core/osdep/windows/platform.c
> b/grub-core/osdep/windows/platform.c index b123256..3f4ad5e 100644
> --- a/grub-core/osdep/windows/platform.c
> +++ b/grub-core/osdep/windows/platform.c
> @@ -246,6 +246,8 @@ grub_install_register_efi (grub_device_t
> efidir_grub_dev, void *current = NULL;
>  	  ssize_t current_len;
>  	  current = get_efi_variable_bootn (i, &current_len);
> +	  if (current_len < 0)
> +	    continue; /* FIXME Should we abort on error? */
>  	  if (current_len < (distrib16_len + 1) * sizeof
> (grub_uint16_t)
>  	      + 6)
>  	    {
> 
> Same potential problem with signed vs. unsigned comparison. Variable is
> not guaranteed to exist here and -1 is treated as very large unsigned
> value.
> 
> @@ -275,13 +277,18 @@ grub_install_register_efi (grub_device_t
> efidir_grub_dev, void *current = NULL;
>  	  ssize_t current_len;
>  	  current = get_efi_variable_bootn (i, &current_len);
> +	  if (current_len < -1)
> +	    continue; /* FIXME Should we abort on error? */
> 
> For completeness we should handle failures to avoid crash.
> 
>  	  if (current_len == -1)
>  	    {
> -	      order_num = i;
> -	      have_order_num = 1;
> -	      grub_util_info ("Creating new entry at Boot%04x",
> -			      order_num);
> -	      break;
> +	      if (!have_order_num)
> +		{
> +		  order_num = i;
> +		  have_order_num = 1;
> +		  grub_util_info ("Creating new entry at Boot%04x",
> +				  order_num);
> +		}
> +	      continue;
>  	    }
> 
> You changed it to stop on first non-existing variable; but this can
> miss another one later with the same description. This will result in
> two different boot entries with the same name.
> 
> I cannot test it on Windows; on Linux code appears to be fast enough.
> 
I've tested on windows (as applied in phcoder/windows64 branch). It's
fast enough as well. So go ahead for all 3 hunks.
>>                       As for merging with Linux part we could just have
>> primitives set variable and get variable and have everything else shared
> 
> Yes, I'll get a look.
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]