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

[Andrei Borzenkov]

On Mon, Apr 11, 2016 at 7:00 AM, Michael Chang <address@hidden> 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
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel