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 <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

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
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel