diff mbox

efi: check *path non-null before grub_strrchr

Message ID 20141105125331.GB22224@bivouac.eciton.net
State Accepted
Commit 004a2b1efdd782cf946387d2060ad9250d61c435
Headers show

Commit Message

Leif Lindholm Nov. 5, 2014, 12:53 p.m. UTC
The EFI version of grub_machine_get_bootlocation crops the boot image
name back to the last / in order to get a directory path. However, it
does not check that *name is actually set before calling grub_strrchr
to do this, and neither does grub_strrchr before dereferencing a NULL
pointer.

Parent function, grub_set_prefix_and_root, does check the pointer
before using.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 grub-core/kern/efi/init.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

 void
--
1.7.10.4

Comments

Andrei Borzenkov Nov. 5, 2014, 2:33 p.m. UTC | #1
On Wed, Nov 5, 2014 at 3:53 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> The EFI version of grub_machine_get_bootlocation crops the boot image
> name back to the last / in order to get a directory path. However, it
> does not check that *name is actually set before calling grub_strrchr
> to do this, and neither does grub_strrchr before dereferencing a NULL
> pointer.
>

I wonder - do you actually have firmware that returns empty path?

> Parent function, grub_set_prefix_and_root, does check the pointer
> before using.
>
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  grub-core/kern/efi/init.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index 942ab02..e9c85de 100644
> --- a/grub-core/kern/efi/init.c
> +++ b/grub-core/kern/efi/init.c
> @@ -63,10 +63,13 @@ grub_machine_get_bootlocation (char **device, char
> **path)
>    if (!*device && grub_efi_net_config)
>      grub_efi_net_config (image->device_handle, device, path);
>
> -  /* Get the directory.  */
> -  p = grub_strrchr (*path, '/');
> -  if (p)
> -    *p = '\0';
> +  if (*path)
> +    {
> +      /* Get the directory.  */
> +      p = grub_strrchr (*path, '/');
> +      if (p)
> +        *p = '\0';
> +    }
>  }
>
>  void
> --
> 1.7.10.4
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm Nov. 5, 2014, 2:52 p.m. UTC | #2
On Wed, Nov 05, 2014 at 05:33:40PM +0300, Andrei Borzenkov wrote:
> On Wed, Nov 5, 2014 at 3:53 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > The EFI version of grub_machine_get_bootlocation crops the boot image
> > name back to the last / in order to get a directory path. However, it
> > does not check that *name is actually set before calling grub_strrchr
> > to do this, and neither does grub_strrchr before dereferencing a NULL
> > pointer.
> >
> 
> I wonder - do you actually have firmware that returns empty path?

I did (internal development version), and that's being fixed that end
too, but more graceful error handling in GRUB would still be nice.

/
    Leif
Andrei Borzenkov Nov. 5, 2014, 3:19 p.m. UTC | #3
On Wed, Nov 5, 2014 at 5:52 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Nov 05, 2014 at 05:33:40PM +0300, Andrei Borzenkov wrote:
>> On Wed, Nov 5, 2014 at 3:53 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > The EFI version of grub_machine_get_bootlocation crops the boot image
>> > name back to the last / in order to get a directory path. However, it
>> > does not check that *name is actually set before calling grub_strrchr
>> > to do this, and neither does grub_strrchr before dereferencing a NULL
>> > pointer.
>> >
>>
>> I wonder - do you actually have firmware that returns empty path?
>
> I did (internal development version), and that's being fixed that end
> too, but more graceful error handling in GRUB would still be nice.
>

Sure. I wish we could display some meaningful warning here, but it is
too early at this stage. If get_loaded_image fails to return proper
image path, booting is probably screwed anyway.

You have commit access, right?
Leif Lindholm Nov. 7, 2014, 2:19 p.m. UTC | #4
On Wed, Nov 05, 2014 at 06:19:16PM +0300, Andrei Borzenkov wrote:
> On Wed, Nov 5, 2014 at 5:52 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Wed, Nov 05, 2014 at 05:33:40PM +0300, Andrei Borzenkov wrote:
> >> On Wed, Nov 5, 2014 at 3:53 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> > The EFI version of grub_machine_get_bootlocation crops the boot image
> >> > name back to the last / in order to get a directory path. However, it
> >> > does not check that *name is actually set before calling grub_strrchr
> >> > to do this, and neither does grub_strrchr before dereferencing a NULL
> >> > pointer.
> >> >
> >>
> >> I wonder - do you actually have firmware that returns empty path?
> >
> > I did (internal development version), and that's being fixed that end
> > too, but more graceful error handling in GRUB would still be nice.
> >
> 
> Sure. I wish we could display some meaningful warning here, but it is
> too early at this stage. If get_loaded_image fails to return proper
> image path, booting is probably screwed anyway.

Indeed, but with this patch you would at least get a confused error
message later on.
 
> You have commit access, right?

Yes - clear to push?

/
    Leif
Andrei Borzenkov Nov. 15, 2014, 5 p.m. UTC | #5
В Fri, 7 Nov 2014 14:19:58 +0000
Leif Lindholm <leif.lindholm@linaro.org> пишет:

> On Wed, Nov 05, 2014 at 06:19:16PM +0300, Andrei Borzenkov wrote:
> > On Wed, Nov 5, 2014 at 5:52 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > > On Wed, Nov 05, 2014 at 05:33:40PM +0300, Andrei Borzenkov wrote:
> > >> On Wed, Nov 5, 2014 at 3:53 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >> > The EFI version of grub_machine_get_bootlocation crops the boot image
> > >> > name back to the last / in order to get a directory path. However, it
> > >> > does not check that *name is actually set before calling grub_strrchr
> > >> > to do this, and neither does grub_strrchr before dereferencing a NULL
> > >> > pointer.
> > >> >
> > >>
> > >> I wonder - do you actually have firmware that returns empty path?
> > >
> > > I did (internal development version), and that's being fixed that end
> > > too, but more graceful error handling in GRUB would still be nice.
> > >
> > 
> > Sure. I wish we could display some meaningful warning here, but it is
> > too early at this stage. If get_loaded_image fails to return proper
> > image path, booting is probably screwed anyway.
> 
> Indeed, but with this patch you would at least get a confused error
> message later on.
>  
> > You have commit access, right?
> 
> Yes - clear to push?
> 

Yes. It is clear bug fix.

> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
diff mbox

Patch

diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
index 942ab02..e9c85de 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -63,10 +63,13 @@  grub_machine_get_bootlocation (char **device, char
**path)
   if (!*device && grub_efi_net_config)
     grub_efi_net_config (image->device_handle, device, path);

-  /* Get the directory.  */
-  p = grub_strrchr (*path, '/');
-  if (p)
-    *p = '\0';
+  if (*path)
+    {
+      /* Get the directory.  */
+      p = grub_strrchr (*path, '/');
+      if (p)
+        *p = '\0';
+    }
 }