| Message ID | 20141105125331.GB22224@bivouac.eciton.net |
|---|---|
| State | Accepted |
| Commit | 004a2b1efdd782cf946387d2060ad9250d61c435 |
| Headers | show |
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
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
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?
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
В 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 --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'; + } }
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