From patchwork Tue Nov 22 18:39:43 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Andrei Borzenkov X-Patchwork-Id: 83473 Delivered-To: patch@linaro.org Received: by 10.140.97.165 with SMTP id m34csp2250755qge; Tue, 22 Nov 2016 10:40:43 -0800 (PST) X-Received: by 10.55.79.17 with SMTP id d17mr26903912qkb.110.1479840043237; Tue, 22 Nov 2016 10:40:43 -0800 (PST) Return-Path: Received: from lists.gnu.org (lists.gnu.org. [2001:4830:134:3::11]) by mx.google.com with ESMTPS id 87si17104851qkq.272.2016.11.22.10.40.43 for (version=TLS1 cipher=AES128-SHA bits=128/128); Tue, 22 Nov 2016 10:40:43 -0800 (PST) Received-SPF: pass (google.com: domain of grub-devel-bounces+patch=linaro.org@gnu.org designates 2001:4830:134:3::11 as permitted sender) client-ip=2001:4830:134:3::11; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@gmail.com; spf=pass (google.com: domain of grub-devel-bounces+patch=linaro.org@gnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom=grub-devel-bounces+patch=linaro.org@gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=gmail.com Received: from localhost ([::1]:57451 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9Fzm-0006jc-Ox for patch@linaro.org; Tue, 22 Nov 2016 13:40:42 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47844) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c9Fyx-0006hY-8X for grub-devel@gnu.org; Tue, 22 Nov 2016 13:39:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c9Fys-00083v-Af for grub-devel@gnu.org; Tue, 22 Nov 2016 13:39:51 -0500 Received: from mail-qt0-x242.google.com ([2607:f8b0:400d:c0d::242]:32946) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c9Fys-00083q-4j for grub-devel@gnu.org; Tue, 22 Nov 2016 13:39:46 -0500 Received: by mail-qt0-x242.google.com with SMTP id n6so3779033qtd.0 for ; Tue, 22 Nov 2016 10:39:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to; bh=+Lde9gfc9hRAsM/2fopqDyF/lTKdhKD9EasqwGh4I7Q=; b=Bq9x5ZJqIPdLhTzCX4lsd/rb/nwynrpDKyY9LCK/l84LyfxrbnPTjTOrDnPmr1tn0l U/J5lryL5CrZYjaYpiBe/wXoiCMJJW//+Sz52b4KWt7JHktN01Qutpku7ii76BqhI8MO xpO89UD0VCo7OS9DuWmL6K/2NqYGNslfl+r2q1HkX66u04gSbgLqqfzgB6EFo3qFqLT0 dZoXR781fJ+n+IHy8Esg8I5/vQR4qcAYIMdRgmkXq0khyXWfzP4AR5zKozXCJYSBcba1 6o2ALw0+WQOPD0VNoQbawCT4qBC3FEogLKTEqYWRg4Mh1TfSQ5cl5zMiOJcJBIur+BWY ZeFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to; bh=+Lde9gfc9hRAsM/2fopqDyF/lTKdhKD9EasqwGh4I7Q=; b=SONnHKoj/1NjBVonZPAbDax9UhUe05VzoZEQuJMMEY4XqK/xx/DozTpxGJGXnV7Ljt rPYt/HTVufI/3Kcl//TLZjWQTYyd/rRSFqhBlCUKDlmUkuThDu47GnIe0caCSF4bEXqD 7W8zlYVFjU6qdfQtCo5dCpv0hiF5OFQiWYBBCL27rtIGsOJ+jI2HjrdZiDh+OtXKgBO8 QABnIkVneK3bIGveEAW7Yw2NGRb6VDr/uAyZ1QtPdtvOpmEKc3DPA4AQTPYCECKHO9VE 1LnS5ga8EnFJf0FckckGNO9lbdYSP5n0ctYGzoLhvlEjRVpjfg915ymenUr34B8Qp/Ks cr6A== X-Gm-Message-State: AKaTC02e63fnBhIbRfl/JmZMBQalC2U09IINM8w9YLSnj2TpaH1SW7ZAbewxSt+ncnPG2w== X-Received: by 10.25.89.137 with SMTP id n131mr4920421lfb.75.1479839985362; Tue, 22 Nov 2016 10:39:45 -0800 (PST) Received: from [192.168.1.44] (ppp109-252-90-55.pppoe.spdop.ru. [109.252.90.55]) by smtp.gmail.com with ESMTPSA id h76sm6563622lji.39.2016.11.22.10.39.43 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 22 Nov 2016 10:39:44 -0800 (PST) Subject: Re: [PATCH v2] grub-file: fix segmentation fault To: Daniel Kiper , =?UTF-8?Q?Vladimir_'=cf=86-coder/phcoder'_Serbinenko?= , The development of GNU GRUB References: <20160412062412.GA16964@linux-9gqx.suse> <20161117190849.GC7245@router-fw-old.local.net-space.pl> <20161122071008.GD17065@linux-9gqx.suse.de> From: Andrei Borzenkov Message-ID: Date: Tue, 22 Nov 2016 21:39:43 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161122071008.GD17065@linux-9gqx.suse.de> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2607:f8b0:400d:c0d::242 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: The development of GNU GRUB Cc: dann frazier Errors-To: grub-devel-bounces+patch=linaro.org@gnu.org Sender: "Grub-devel" 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 > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel From: Andrei Borzenkov 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 --- 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)