From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1ZSZ4j-0005uQ-Gi for mharc-grub-devel@gnu.org; Thu, 20 Aug 2015 19:16:49 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33419) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZSU4G-0006k3-Qp for grub-devel@gnu.org; Thu, 20 Aug 2015 13:56:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZSU4B-00072z-F6 for grub-devel@gnu.org; Thu, 20 Aug 2015 13:56:00 -0400 Received: from mail-lb0-x234.google.com ([2a00:1450:4010:c04::234]:33068) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZSU4B-00071d-24 for grub-devel@gnu.org; Thu, 20 Aug 2015 13:55:55 -0400 Received: by lbbsx3 with SMTP id sx3so28856762lbb.0 for ; Thu, 20 Aug 2015 10:55:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type; bh=Lb8F8obg/zRVODPBzTYfpyPDCSunI1rok28xFZIXWeU=; b=c/mOGJKFYcPhQNCPgHtZ/CNhrKQOGX+iLDUbu2Qhhw1hRAkGhf5Lq7Y4mc5pczoMMe 1LTSG8k1YHrPaZFqG47EF70WwfrUEnRZ/FJQIH70pFCoDLzm+++9oB1yxExWFoO3lbF5 gl3wWVkXFDkPc8NJfqCcyXsYZJHAMTVnXMTCCZ6wr1WS6qn1TRkc2/GhkwPcNru+Cd8C KYuk8FaA/4Rd65n89czQ/WWzSVflS77BQiH2GepscBcg2akNNc0CRQAfFvKtrdDwVb5U mnV5JzB6HS5g6pwmiVoO0LKChs/0iMEY23quX+z93PcCTZmxumwfVqrHcgUXOR5FlKXB 6QlQ== X-Received: by 10.152.25.196 with SMTP id e4mr3939860lag.15.1440093353717; Thu, 20 Aug 2015 10:55:53 -0700 (PDT) Received: from [192.168.1.43] (ppp91-76-5-127.pppoe.mtu-net.ru. [91.76.5.127]) by smtp.gmail.com with ESMTPSA id bx5sm1431034lbc.36.2015.08.20.10.55.52 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 20 Aug 2015 10:55:53 -0700 (PDT) Message-ID: <55D614A3.8070805@gmail.com> Date: Thu, 20 Aug 2015 20:55:47 +0300 From: Andrei Borzenkov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: dann frazier Subject: Re: [PATCH] Avoid NULL pointer dereference in progress module. References: <1439394805-8573-1-git-send-email-dann.frazier@canonical.com> <20150813210409.GA26195@fluid.dannf> <55D22CAA.3040200@gmail.com> <20150817215733.GA10858@fluid.dannf> In-Reply-To: <20150817215733.GA10858@fluid.dannf> Content-Type: multipart/mixed; boundary="------------060205020602010700000109" X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c04::234 Cc: The development of GNU GRUB X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Aug 2015 23:16:48 -0000 This is a multi-part message in MIME format. --------------060205020602010700000109 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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. > 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 > --- > 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; > --------------060205020602010700000109 Content-Type: text/x-patch; name="proress-net.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="proress-net.patch" From: Andrei Borzenkov Subject: [PATCH] progress: avoid NULL dereference for net files >From original patch by 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. grub_net_fs_open() already saved copy of file name as ->net->name, so change progress module to use it. Also, grub_file_open may leave file->name as NULL if grub_strdup fails. Check for it. Also-By: dann frazier --- grub-core/lib/progress.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c index 63a0767..7c723eb 100644 --- a/grub-core/lib/progress.c +++ b/grub-core/lib/progress.c @@ -23,6 +23,7 @@ #include #include #include +#include GRUB_MOD_LICENSE ("GPLv3+"); @@ -70,7 +71,12 @@ 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 (file->device->net) + partial_file_name = grub_strrchr (file->device->net->name, '/'); + else if (file->name) /* grub_file_open() may leave it as NULL */ + partial_file_name = grub_strrchr (file->name, '/'); + else + partial_file_name = NULL; if (partial_file_name) partial_file_name++; else -- tg: (ba218c1..) u/progress-net-file (depends on: master) --------------060205020602010700000109--