From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1ZkuiX-0000Fc-U7 for mharc-grub-devel@gnu.org; Sat, 10 Oct 2015 10:01:45 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36502) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkplH-00021G-Hu for grub-devel@gnu.org; Sat, 10 Oct 2015 04:44:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZkplC-0003i2-H3 for grub-devel@gnu.org; Sat, 10 Oct 2015 04:44:15 -0400 Received: from mail-lb0-x232.google.com ([2a00:1450:4010:c04::232]:33310) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkplC-0003hv-5d for grub-devel@gnu.org; Sat, 10 Oct 2015 04:44:10 -0400 Received: by lbos8 with SMTP id s8so101357122lbo.0 for ; Sat, 10 Oct 2015 01:44:09 -0700 (PDT) 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:content-type:content-transfer-encoding; bh=j8GQVd97u/MfnBC3war0YP0EERAAvHmuekMm5H/pe3g=; b=CZjonSwsjAfyQnbPg3q+B6qSTInDasD9HdqlFv9olrkyMK2wJGywMzCq5SlTZxxqsS yPGUjRT24GllajnUjGQSA89tOHHRnk2rXGIoXR60ex1a0mkaAMLQu4HUwiAerCizDFeX PHKhRTIhxfDK3MkDMsJ+k5DXpcrMzCyd5aLZmIJM9o4IuWf1jSUOMdN1zKReKqIq1XNZ M4bYznTZRaz1nmj6zfxq4uhBZSIomWEructuU3TAE19DHRhyFgE6zFa+PgzKoCXXjjdd bOQNf4JkHwcYGGkUNu6SLduElMM3e6E1OP1wrBXwstmCPnCyrrb8U6OZOPavoHEvGlir PjWA== X-Received: by 10.112.53.131 with SMTP id b3mr8642292lbp.55.1444466649523; Sat, 10 Oct 2015 01:44:09 -0700 (PDT) Received: from [192.168.1.43] (ppp91-76-142-206.pppoe.mtu-net.ru. [91.76.142.206]) by smtp.gmail.com with ESMTPSA id as6sm983530lbc.16.2015.10.10.01.44.08 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 10 Oct 2015 01:44:08 -0700 (PDT) Subject: Re: [PATCH] Avoid NULL pointer dereference in progress module. To: Dann Frazier References: <1439394805-8573-1-git-send-email-dann.frazier@canonical.com> <20150813210409.GA26195@fluid.dannf> <55D22CAA.3040200@gmail.com> <20150817215733.GA10858@fluid.dannf> <55D614A3.8070805@gmail.com> From: Andrei Borzenkov Message-ID: <5618CFD7.7050608@gmail.com> Date: Sat, 10 Oct 2015 11:44:07 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c04::232 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: Sat, 10 Oct 2015 08:44:16 -0000 21.09.2015 18:11, Dann Frazier пишет: > On Fri, Aug 21, 2015 at 8:24 AM, Dann Frazier > wrote: >> On Thu, Aug 20, 2015 at 11:55 AM, Andrei Borzenkov 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 >>>> --- >>>> 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; >>>> >>>