[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] grub-file: fix segmentation fault
From: |
Michael Chang |
Subject: |
Re: [PATCH v2] grub-file: fix segmentation fault |
Date: |
Wed, 23 Nov 2016 14:44:29 +0800 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Tue, Nov 22, 2016 at 09:39:43PM +0300, Andrei Borzenkov wrote:
> 22.11.2016 10:10, Michael Chang пишет:
>
> What about attached patch? It does not increase size of kernel.
Just some nitpicks in comments and a potential leak in fail path (See below),
otherwise LGTM.
>
> I am not thrilled as it does require more discipline from filter author.
> OTOH we may find some way to consolidate boilerplate later.
Yes, that's also why I tend to not touch the filter but trying to paper over it
on the parent's side, to be less frustrated to the filter authors.
>
> @Dann, this also solves your concerns about layering violation in
> progress module as side effect. Could you test this patch? Thank you!
I think this may deserve a separate patch (in file.c and progress.c) as it did
not fixed by the segfault problem here. But that's all up to you, as
consolidating all file->name related fix in one patch is nothing wrong.
> From: Andrei Borzenkov <address@hidden>
> 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 <address@hidden>
>
> ---
> 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 */
As long as the comment is intended to make things more obvious, I'd suggest to
address again it's for "parent's" grub_file_close and be more in line with
previous comment has claimed.
Same applies for all similar comments below.
> 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;
> -
Well.. I think file->name now has to be free also in the "goto fail" path.
Thanks,
Michael
> 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)
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel