[PATCH] grub-file: fix segmentation fault

[PATCH] grub-file: fix segmentation fault

[Michael Chang]

In grub_file_open the file handle returned by file filters has no file->name
set which leads to segmentation fault later referenced by grub_elf_file. We
move the file->name value assignment after file filters to make sure it will be
set and returned.

The stack backtrace for reference.

 gdb --args ./grub-file --is-x86_64-xen-domu /boot/vmlinux-4.1.12-1-default.gz

 (gdb) bt
 #0  0x000000000047597e in grub_strlen (s=0x0) at ../grub-core/kern/misc.c:558
 #1  0x00000000004757e2 in grub_strdup (s=0x0) at ../grub-core/kern/misc.c:463
 #2  0x0000000000406418 in grub_elf_file (file=0x6dfb50, filename=0x0) at ../grub-core/kern/elf.c:89
 #3  0x00000000004043b3 in grub_xen_file (file=0x6dfb50) at ../grub-core/loader/i386/xen_file.c:29
 #4  0x0000000000403930 in grub_cmd_file (ctxt=0x7fffffffe120, argc=1, args=0x6dfa00) at ../grub-core/commands/file.c:425
 #5  0x000000000047e178 in grub_extcmd_dispatcher (cmd=0x6df730, argc=2, args=0x6ddfb0, script=0x0) at ../grub-core/commands/extcmd.c:54
 #6  0x000000000047e1d7 in grub_extcmd_dispatch (cmd=0x6df730, argc=2, args=0x6ddfb0) at ../grub-core/commands/extcmd.c:67
 #7  0x0000000000402945 in main (argc=3, argv=0x7fffffffe2e8) at ../util/grub-file.c:102
 (gdb) frame 3
 #3  0x00000000004043b3 in grub_xen_file (file=0x6dfb50) at ../grub-core/loader/i386/xen_file.c:29
 29        elf = grub_elf_file (file, file->name);
---
 grub-core/kern/file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c
index 668f893..44047d7 100644
--- a/grub-core/kern/file.c
+++ b/grub-core/kern/file.c
@@ -111,9 +111,6 @@ grub_file_open (const char *name)
   if ((file->fs->open) (file, file_name) != GRUB_ERR_NONE)
     goto fail;
 
-  file->name = grub_strdup (name);
-  grub_errno = GRUB_ERR_NONE;
-
   for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters_enabled);
        filter++)
     if (grub_file_filters_enabled[filter])
@@ -123,7 +120,12 @@ grub_file_open (const char *name)
       }
   if (!file)
     grub_file_close (last_file);
-    
+  else
+    {
+      file->name = grub_strdup (name);
+      grub_errno = GRUB_ERR_NONE;
+    }
+
   grub_memcpy (grub_file_filters_enabled, grub_file_filters_all,
 	       sizeof (grub_file_filters_enabled));
 
-- 
2.6.2

Re: [PATCH] grub-file: fix segmentation fault

[Andrei Borzenkov]

08.04.2016 09:43, Michael Chang пишет:
> In grub_file_open the file handle returned by file filters has no file->name
> set which leads to segmentation fault later referenced by grub_elf_file. We
> move the file->name value assignment after file filters to make sure it will be
> set and returned.
> 

This now makes filename unavailable to progress module (which gets the
last grub_file in a chain) and it still does not cover corner case of
failing grub_strdup in grub_file_open.

Fixing the former requires some redesign. But as long as we allow
filename to remain empty in grub_file_open every user must explicitly
check for it being NULL.

> The stack backtrace for reference.
> 
>  gdb --args ./grub-file --is-x86_64-xen-domu /boot/vmlinux-4.1.12-1-default.gz
> 
>  (gdb) bt
>  #0  0x000000000047597e in grub_strlen (s=0x0) at ../grub-core/kern/misc.c:558
>  #1  0x00000000004757e2 in grub_strdup (s=0x0) at ../grub-core/kern/misc.c:463
>  #2  0x0000000000406418 in grub_elf_file (file=0x6dfb50, filename=0x0) at ../grub-core/kern/elf.c:89
>  #3  0x00000000004043b3 in grub_xen_file (file=0x6dfb50) at ../grub-core/loader/i386/xen_file.c:29
>  #4  0x0000000000403930 in grub_cmd_file (ctxt=0x7fffffffe120, argc=1, args=0x6dfa00) at ../grub-core/commands/file.c:425
>  #5  0x000000000047e178 in grub_extcmd_dispatcher (cmd=0x6df730, argc=2, args=0x6ddfb0, script=0x0) at ../grub-core/commands/extcmd.c:54
>  #6  0x000000000047e1d7 in grub_extcmd_dispatch (cmd=0x6df730, argc=2, args=0x6ddfb0) at ../grub-core/commands/extcmd.c:67
>  #7  0x0000000000402945 in main (argc=3, argv=0x7fffffffe2e8) at ../util/grub-file.c:102
>  (gdb) frame 3
>  #3  0x00000000004043b3 in grub_xen_file (file=0x6dfb50) at ../grub-core/loader/i386/xen_file.c:29
>  29        elf = grub_elf_file (file, file->name);
> ---
>  grub-core/kern/file.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c
> index 668f893..44047d7 100644
> --- a/grub-core/kern/file.c
> +++ b/grub-core/kern/file.c
> @@ -111,9 +111,6 @@ grub_file_open (const char *name)
>    if ((file->fs->open) (file, file_name) != GRUB_ERR_NONE)
>      goto fail;
>  
> -  file->name = grub_strdup (name);
> -  grub_errno = GRUB_ERR_NONE;
> -
>    for (filter = 0; file && filter < ARRAY_SIZE (grub_file_filters_enabled);
>         filter++)
>      if (grub_file_filters_enabled[filter])
> @@ -123,7 +120,12 @@ grub_file_open (const char *name)
>        }
>    if (!file)
>      grub_file_close (last_file);
> -    
> +  else
> +    {
> +      file->name = grub_strdup (name);
> +      grub_errno = GRUB_ERR_NONE;
> +    }
> +
>    grub_memcpy (grub_file_filters_enabled, grub_file_filters_all,
>  	       sizeof (grub_file_filters_enabled));
>  
> 

Re: [PATCH] grub-file: fix segmentation fault

[Michael Chang]

On Sat, Apr 09, 2016 at 07:01:50AM +0300, Andrei Borzenkov wrote:
> 08.04.2016 09:43, Michael Chang пишет:
> > In grub_file_open the file handle returned by file filters has no file->name
> > set which leads to segmentation fault later referenced by grub_elf_file. We
> > move the file->name value assignment after file filters to make sure it will be
> > set and returned.
> > 
> 
> This now makes filename unavailable to progress module (which gets the
> last grub_file in a chain) and it still does not cover corner case of
> failing grub_strdup in grub_file_open.

I don't get why the filename would, in the other way round to this patch trying
to fix, become unavailable to progress module? As far as I see the file
progress read hook in grub_file_read would use the file handle returned
from grub_file_open and do not hold another chaining of opened files ..

About covering the grub_strdup failure, the patch didn't do because it's not
the cause for the segfault so leaving it as it is, if you think it necessary we
can handle the error by returning null handle of course.
> 
> Fixing the former requires some redesign. But as long as we allow
> filename to remain empty in grub_file_open every user must explicitly
> check for it being NULL.

For what reason the filename returned by grub_file_open would be empty and how
to know it reasonable from the user ? Adding the check is fine, but still a bug
to me a filename is provided during grub_file_open but get ditched in returned
handle without a reason.

Thanks,
Michael

Re: [PATCH] grub-file: fix segmentation fault

[Andrei Borzenkov]

On Mon, Apr 11, 2016 at 7:00 AM, Michael Chang <mchang@suse.com> wrote:
> On Sat, Apr 09, 2016 at 07:01:50AM +0300, Andrei Borzenkov wrote:
>> 08.04.2016 09:43, Michael Chang пишет:
>> > In grub_file_open the file handle returned by file filters has no file->name
>> > set which leads to segmentation fault later referenced by grub_elf_file. We
>> > move the file->name value assignment after file filters to make sure it will be
>> > set and returned.
>> >
>>
>> This now makes filename unavailable to progress module (which gets the
>> last grub_file in a chain) and it still does not cover corner case of
>> failing grub_strdup in grub_file_open.
>
> I don't get why the filename would, in the other way round to this patch trying
> to fix, become unavailable to progress module? As far as I see the file
> progress read hook in grub_file_read would use the file handle returned
> from grub_file_open and do not hold another chaining of opened files ..
>

porogress module for disk IO relies on file hooks; what we have is

  if (!file->read_hook)
    {
      file->read_hook = grub_file_progress_hook;
      file->read_hook_data = file;
      file->progress_offset = file->offset;
    }

Each filter is implemented as pseudo-fs with own ->read function. This
function calls lower fs or filter as needed. So we basically have

grub_file_read (filter1)
  filter1->read
    grub_file_read (filter2)
      filter2->read
        grub_file_read (real_file)
          real_file->read
            disk_read

Each grub_file_read will re-set file hooks to progress on its
argument, but only the very last disk_read will actually interpret
them. And it will get as data pointer to real_file and will fetch file
name from there.

Note that network code explicitly calls progress and does not rely on
above framework at all.

Oh ... and it looks like if user explicitly set file hooks on top
level filter, these hooks are not propagated at all. Which may or may
not be intentional.

This is a mess, and needs cleanup.

> About covering the grub_strdup failure, the patch didn't do because it's not
> the cause for the segfault so leaving it as it is, if you think it necessary we
> can handle the error by returning null handle of course.

I do not know if this is intentional. Vladimir? Personally I'd say
that if we could not allocate several bytes for file name, we unlikely
can continue to do anything useful.

>>
>> Fixing the former requires some redesign. But as long as we allow
>> filename to remain empty in grub_file_open every user must explicitly
>> check for it being NULL.
>
> For what reason the filename returned by grub_file_open would be empty and how
> to know it reasonable from the user ? Adding the check is fine, but still a bug
> to me a filename is provided during grub_file_open but get ditched in returned
> handle without a reason.
>
> Thanks,
> Michael
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] grub-file: fix segmentation fault

[Michael Chang]

On Mon, Apr 11, 2016 at 12:28:35PM +0300, Andrei Borzenkov wrote:
> On Mon, Apr 11, 2016 at 7:00 AM, Michael Chang <mchang@suse.com> wrote:
> > On Sat, Apr 09, 2016 at 07:01:50AM +0300, Andrei Borzenkov wrote:
> >> 08.04.2016 09:43, Michael Chang пишет:
> >> > In grub_file_open the file handle returned by file filters has no file->name
> >> > set which leads to segmentation fault later referenced by grub_elf_file. We
> >> > move the file->name value assignment after file filters to make sure it will be
> >> > set and returned.
> >> >
> >>
> >> This now makes filename unavailable to progress module (which gets the
> >> last grub_file in a chain) and it still does not cover corner case of
> >> failing grub_strdup in grub_file_open.
> >
> > I don't get why the filename would, in the other way round to this patch trying
> > to fix, become unavailable to progress module? As far as I see the file
> > progress read hook in grub_file_read would use the file handle returned
> > from grub_file_open and do not hold another chaining of opened files ..
> >
> 
> porogress module for disk IO relies on file hooks; what we have is
> 
>   if (!file->read_hook)
>     {
>       file->read_hook = grub_file_progress_hook;
>       file->read_hook_data = file;
>       file->progress_offset = file->offset;
>     }
> 
> Each filter is implemented as pseudo-fs with own ->read function. This
> function calls lower fs or filter as needed. So we basically have
> 
> grub_file_read (filter1)
>   filter1->read
>     grub_file_read (filter2)
>       filter2->read
>         grub_file_read (real_file)
>           real_file->read
>             disk_read

Thanks, I modified the send v2 patch to keep the original file->name as io
handle for filters. It will now only assign file->name if it's emtpy in
returned handle from filters to make sure it's available to user.

> 
> Each grub_file_read will re-set file hooks to progress on its
> argument, but only the very last disk_read will actually interpret
> them. And it will get as data pointer to real_file and will fetch file
> name from there.
> 
> Note that network code explicitly calls progress and does not rely on
> above framework at all.
> 
> Oh ... and it looks like if user explicitly set file hooks on top
> level filter, these hooks are not propagated at all. Which may or may
> not be intentional.
> 
> This is a mess, and needs cleanup.

It is really into something that I do not consider or expect to fix, it is too
far from fixing the segmentation fault problem. If we forced into that field
instead of a trivial fix that's really sad to me.

Thanks,
Michael

> 
> > About covering the grub_strdup failure, the patch didn't do because it's not
> > the cause for the segfault so leaving it as it is, if you think it necessary we
> > can handle the error by returning null handle of course.
> 
> I do not know if this is intentional. Vladimir? Personally I'd say
> that if we could not allocate several bytes for file name, we unlikely
> can continue to do anything useful.
> 
> >>
> >> Fixing the former requires some redesign. But as long as we allow
> >> filename to remain empty in grub_file_open every user must explicitly
> >> check for it being NULL.
> >
> > For what reason the filename returned by grub_file_open would be empty and how
> > to know it reasonable from the user ? Adding the check is fine, but still a bug
> > to me a filename is provided during grub_file_open but get ditched in returned
> > handle without a reason.
> >
> > Thanks,
> > Michael
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel