Re: [PATCH] Avoid NULL pointer dereference in progress module.

[Andrei Borzenkov]

21.09.2015 18:11, Dann Frazier пишет:
> On Fri, Aug 21, 2015 at 8:24 AM, Dann Frazier
> <address@hidden> wrote:
> > On Thu, Aug 20, 2015 at 11:55 AM, Andrei Borzenkov <address@hidden> wrote:
> > > 18.08.2015 00:57, dann frazier пишет:
> > > > grub_net_fs_open() saves off a copy of the file structure it gets passed
> > > > and
> > > > uses it to create a bufio structure. It then overwrites the passed in file
> > > > structure with this new bufio structure. Since file->name doesn't get set
> > > > until we return back to grub_file_open(), it means that only the bufio
> > > > structure gets a valid file->name. The "real" file's name is left
> > > > uninitialized. This leads to a crash when the progress module hook is
> > > > called
> > > > on it. Fix this by adding a copy of the filename during the switcheroo.
> > >
> > >
> > > Actually name was already saved off as device->net-name. What about attached
> > > patch? I'll commit leak fix separately.
> >
> > It does seem like it pokes a hole in the fs abstraction, but it works for me.
>
> hey Andrei,
>   Did you need anything more from me wrt this fix?

Sorry for delay. We already need to differentiate between disk and net 
files in other places anyway. So I think it is OK as a simple fix. I 
committed it.

May be bufio needs to be converted to filter but this is too intrusive 
for this.

>   -dann
>
> > > > grub_file_close() will free that string in the success case, we only need
> > > > to handle the free in an error. While we're at it, also fix a leaked file
> > > > structure in the case that grub_strdup() fails when setting
> > > > file->device->net->name.
> > > >
> > > > Finally, make progress more robust by checking for NULL before reading the
> > > > name. Andrei noticed that this could occur if grub_strdup() fails in
> > > > grub_file_open().
> > > >
> > > > Signed-off-by: dann frazier <address@hidden>
> > > > ---
> > > >    grub-core/lib/progress.c |  3 +--
> > > >    grub-core/net/net.c      | 14 +++++++++++++-
> > > >    2 files changed, 14 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
> > > > index 63a0767..2775554 100644
> > > > --- a/grub-core/lib/progress.c
> > > > +++ b/grub-core/lib/progress.c
> > > > @@ -70,8 +70,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector
> > > > __attribute__ ((unused)),
> > > >          percent = grub_divmod64 (100 * file->progress_offset,
> > > >                                   file->size, 0);
> > > >
> > > > -      partial_file_name = grub_strrchr (file->name, '/');
> > > > -      if (partial_file_name)
> > > > +      if (file->name && (partial_file_name = grub_strrchr (file->name,
> > > > '/')))
> > > >          partial_file_name++;
> > > >          else
> > > >          partial_file_name = "";
> > > > diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> > > > index 21a4e94..9f83765 100644
> > > > --- a/grub-core/net/net.c
> > > > +++ b/grub-core/net/net.c
> > > > @@ -1391,7 +1391,17 @@ grub_net_fs_open (struct grub_file *file_out, const
> > > > char *name)
> > > >      file->device->net->packs.last = NULL;
> > > >      file->device->net->name = grub_strdup (name);
> > > >      if (!file->device->net->name)
> > > > -    return grub_errno;
> > > > +    {
> > > > +      grub_free (file);
> > > > +      return grub_errno;
> > > > +    }
> > > > +  file->name = grub_strdup (name);
> > > > +  if (!file->name)
> > > > +    {
> > > > +      grub_free (file->device->net->name);
> > > > +      grub_free (file);
> > > > +      return grub_errno;
> > > > +    }
> > > >
> > > >      err = file->device->net->protocol->open (file, name);
> > > >      if (err)
> > > > @@ -1401,6 +1411,7 @@ grub_net_fs_open (struct grub_file *file_out, const
> > > > char *name)
> > > >            grub_netbuff_free (file->device->net->packs.first->nb);
> > > >            grub_net_remove_packet (file->device->net->packs.first);
> > > >          }
> > > > +      grub_free (file->name);
> > > >          grub_free (file->device->net->name);
> > > >          grub_free (file);
> > > >          return err;
> > > > @@ -1414,6 +1425,7 @@ grub_net_fs_open (struct grub_file *file_out, const
> > > > char *name)
> > > >            grub_net_remove_packet (file->device->net->packs.first);
> > > >          }
> > > >          file->device->net->protocol->close (file);
> > > > +      grub_free (file->name);
> > > >          grub_free (file->device->net->name);
> > > >          grub_free (file);
> > > >          return grub_errno;