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

[Andrei Borzenkov <arvidjaar@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

22.11.2016 10:10, Michael Chang пишет:
> On Fri, Nov 18, 2016 at 11:50:25AM +0300, Andrei Borzenkov wrote:
>> Hmm ... I must admit I am confused how we can get NULL here. Filters
>> are called after primary file->name is set and each filter copies
>> previous struct file, which means returned file will inherit pointer
>> to the same file name.
> 
> No. I don't think so. Looking into gzio or xzio file filters they did not copy
> original handle to new allocated one. And the new handle gets initialized
> without file->name being set from original one. The new handle then returned
> to upper file layer with file->name being null.
> 

Right, I was mistaken.

>> Anyway, exactly because filters themselves do not free file->name this
>> patch means memory leak.
> 
> Same reason above, as long as the filters did not allocate it, they did not
> need to free.
> 

Yes, but your patch allocates copy of name and (some) filters set it to
NULL so it is leaked.

What about attached patch? It does not increase size of kernel.

I am not thrilled as it does require more discipline from filter author.
OTOH we may find some way to consolidate boilerplate later.

@Dann, this also solves your concerns about layering violation in
progress module as side effect. Could you test this patch? Thank you!

>> Michael, could you provide reproducer for it?
> 
> I can still reproduce the segfault on latest git HEAD. Here is kernel image
> attached to reproduce the problem with:
> 
>  grub/build-xen # ./grub-file --is-x86_64-xen-domu /boot/vmlinux-4.8.4-1-default.gz
>  Segmentation fault (core dumped)
> 
> Thanks,
> Michael
> 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: file-filter-filename.patch --]
[-- Type: text/x-patch; name="file-filter-filename.patch", Size: 7502 bytes --]

From: Andrei Borzenkov <arvidjaar@gmail.com>
Subject: [PATCH] file: make sure file name is set when filters are used

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. This
patch tries to fix the problem.

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

Initialize file->name early and make sure it is set in file structure returned
by each filter. Note that each filter must set it to NULL in its ->close method
to avoid double free.

It also makes redundant special case for net in progress module, because now file
name is set also for net files.

Reported by: Michael Chang <mchang@suse.com>

---
 grub-core/commands/verify.c | 1 +
 grub-core/io/bufio.c        | 3 +++
 grub-core/io/gzio.c         | 2 ++
 grub-core/io/lzopio.c       | 2 ++
 grub-core/io/offset.c       | 3 +++
 grub-core/io/xzio.c         | 2 ++
 grub-core/kern/file.c       | 5 ++---
 grub-core/lib/progress.c    | 7 +------
 8 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c
index 67cb1c7..a959929 100644
--- a/grub-core/commands/verify.c
+++ b/grub-core/commands/verify.c
@@ -845,6 +845,7 @@ verified_close (struct grub_file *file)
 
   /* device and name are freed by parent */
   file->device = 0;
+  /* Must set to NULL to prevent double free in grub_file_close */
   file->name = 0;
 
   return grub_errno;
diff --git a/grub-core/io/bufio.c b/grub-core/io/bufio.c
index 2243827..3a8a53c 100644
--- a/grub-core/io/bufio.c
+++ b/grub-core/io/bufio.c
@@ -72,6 +72,7 @@ grub_bufio_open (grub_file_t io, int size)
   bufio->block_size = size;
 
   file->device = io->device;
+  file->name = io->name;
   file->size = io->size;
   file->data = bufio;
   file->fs = &grub_bufio_fs;
@@ -191,6 +192,8 @@ grub_bufio_close (grub_file_t file)
   grub_free (bufio);
 
   file->device = 0;
+  /* Must set to NULL to prevent double free in grub_file_close */
+  file->name = 0;
 
   return grub_errno;
 }
diff --git a/grub-core/io/gzio.c b/grub-core/io/gzio.c
index 0f2ea6b..17dcbe3 100644
--- a/grub-core/io/gzio.c
+++ b/grub-core/io/gzio.c
@@ -1144,6 +1144,7 @@ grub_gzio_open (grub_file_t io, const char *name __attribute__ ((unused)))
   gzio->file = io;
 
   file->device = io->device;
+  file->name = io->name;
   file->data = gzio;
   file->fs = &grub_gzio_fs;
   file->not_easily_seekable = 1;
@@ -1291,6 +1292,7 @@ grub_gzio_close (grub_file_t file)
 
   /* No need to close the same device twice.  */
   file->device = 0;
+  /* Must set to NULL to prevent double free in grub_file_close */
   file->name = 0;
 
   return grub_errno;
diff --git a/grub-core/io/lzopio.c b/grub-core/io/lzopio.c
index 7559c6c..52acde4 100644
--- a/grub-core/io/lzopio.c
+++ b/grub-core/io/lzopio.c
@@ -427,6 +427,7 @@ grub_lzopio_open (grub_file_t io,
   lzopio->file = io;
 
   file->device = io->device;
+  file->name = io->name;
   file->data = lzopio;
   file->fs = &grub_lzopio_fs;
   file->size = GRUB_FILE_SIZE_UNKNOWN;
@@ -523,6 +524,7 @@ grub_lzopio_close (grub_file_t file)
 
   /* Device must not be closed twice.  */
   file->device = 0;
+  /* Must set to NULL to prevent double free in grub_file_close */
   file->name = 0;
   return grub_errno;
 }
diff --git a/grub-core/io/offset.c b/grub-core/io/offset.c
index ebed0eb..1de192e 100644
--- a/grub-core/io/offset.c
+++ b/grub-core/io/offset.c
@@ -46,6 +46,8 @@ grub_offset_close (grub_file_t file)
 
   /* No need to close the same device twice.  */
   file->device = 0;
+  /* Must set to NULL to prevent double free in grub_file_close */
+  file->name = 0;
 
   return 0;
 }
@@ -88,6 +90,7 @@ grub_file_offset_open (grub_file_t parent, grub_off_t start, grub_off_t size)
   off_data->parent = parent;
 
   off_file->device = parent->device;
+  off_file->name = parent->name;
   off_file->data = off_data;
   off_file->fs = &grub_offset_fs;
   off_file->size = size;
diff --git a/grub-core/io/xzio.c b/grub-core/io/xzio.c
index a3536ad..cdf6cef 100644
--- a/grub-core/io/xzio.c
+++ b/grub-core/io/xzio.c
@@ -189,6 +189,7 @@ grub_xzio_open (grub_file_t io,
   xzio->file = io;
 
   file->device = io->device;
+  file->name = io->name;
   file->data = xzio;
   file->fs = &grub_xzio_fs;
   file->size = GRUB_FILE_SIZE_UNKNOWN;
@@ -319,6 +320,7 @@ grub_xzio_close (grub_file_t file)
 
   /* Device must not be closed twice.  */
   file->device = 0;
+  /* Must set to NULL to prevent double free in grub_file_close */
   file->name = 0;
   return grub_errno;
 }
diff --git a/grub-core/kern/file.c b/grub-core/kern/file.c
index 668f893..dd6c28a 100644
--- a/grub-core/kern/file.c
+++ b/grub-core/kern/file.c
@@ -88,6 +88,8 @@ grub_file_open (const char *name)
     goto fail;
 
   file->device = device;
+  file->name = grub_strdup (name);
+  grub_errno = GRUB_ERR_NONE;
 
   /* In case of relative pathnames and non-Unix systems (like Windows)
    * name of host files may not start with `/'. Blocklists for host files
@@ -111,9 +113,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])
diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
index 4b7cbbc..c8efdb1 100644
--- a/grub-core/lib/progress.c
+++ b/grub-core/lib/progress.c
@@ -77,12 +77,7 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
 	percent = grub_divmod64 (100 * file->progress_offset,
 				 file->size, 0);
 
-      /* grub_net_fs_open() saves off partial file structure before name is initialized.
-         It already saves passed file name in net structure so just use it in this case.
-       */
-      if (file->device->net)
-	partial_file_name = grub_strrchr (file->device->net->name, '/');
-      else if (file->name) /* grub_file_open() may leave it as NULL */
+      if (file->name) /* grub_file_open() may leave it as NULL */
 	partial_file_name = grub_strrchr (file->name, '/');
       else
 	partial_file_name = NULL;
-- 
tg: (c9a8d03..) u/file-filter-filename (depends on: master)