From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1bMUJf-0001uA-0n for mharc-grub-devel@gnu.org; Mon, 11 Jul 2016 02:03:39 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44837) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMUJc-0001to-3a for grub-devel@gnu.org; Mon, 11 Jul 2016 02:03:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bMUJY-0005Mt-TT for grub-devel@gnu.org; Mon, 11 Jul 2016 02:03:36 -0400 Received: from smtp.nue.novell.com ([195.135.221.5]:44936) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMUJY-0005MK-KG for grub-devel@gnu.org; Mon, 11 Jul 2016 02:03:32 -0400 Received: from nwb-ext-pat.microfocus.com ([10.120.13.103]) by smtp.nue.novell.com with ESMTP (TLS encrypted); Mon, 11 Jul 2016 08:03:29 +0200 Received: from linux-9gqx.suse.de (nwb-a10-snat.microfocus.com [10.120.13.201]) by nwb-ext-pat.microfocus.com with ESMTP (TLS encrypted); Mon, 11 Jul 2016 07:03:08 +0100 Date: Mon, 11 Jul 2016 14:02:59 +0800 From: Michael Chang To: grub-devel@gnu.org Subject: Re: [PATCH] dns: fix heap corruption Message-ID: <20160711060259.GA26485@linux-9gqx.suse.de> Mail-Followup-To: grub-devel@gnu.org References: <20160707091817.GA16571@linux-9gqx.suse.de> <578004FD.5020601@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <578004FD.5020601@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 195.135.221.5 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: , X-List-Received-Date: Mon, 11 Jul 2016 06:03:37 -0000 On Fri, Jul 08, 2016 at 10:54:37PM +0300, Andrei Borzenkov wrote: > 07.07.2016 12:18, Michael Chang пишет: > > Since commit f9d1b4422efb2c06e5472fb2c304712e2029796b I occasionally bumped > > into heap corruption problem during dns lookup. > > > > After tracing the issue, it looks the *data->addresses array is not correctly > > allocated. It need to hold accumulated dns look up result but not only the new > > result in new message. The heap corruption occured when appending new result to > > it. > > > > This patch fixed the issue for me by reallocating the array if it found too > > small to hold all the result. > > > > I'm not sure. I think we discussed this with Josef back then. The code > apparently was assuming single response; and if we are going to collect > multiple answers, we need to filter out duplicates at least and also not > depend on packet order to select between A and AAAA. OK. > > Does attached patch fix corruption for you? I think that is the least > intrusive as bug fix, and we need to revisit code to properly handle > multiple responses later. Yes, it does. I have tested several times to make sure it doesn't happen. Thanks for review. Michael > > > Thanks, > > > > --- > > grub-core/net/dns.c | 23 +++++++++++++++++------ > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c > > index 89741dd..b8d8873 100644 > > --- a/grub-core/net/dns.c > > +++ b/grub-core/net/dns.c > > @@ -276,14 +276,25 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)), > > ptr++; > > ptr += 4; > > } > > - *data->addresses = grub_malloc (sizeof ((*data->addresses)[0]) > > - * grub_be_to_cpu16 (head->ancount)); > > - if (!*data->addresses) > > + > > + if (ALIGN_UP (grub_be_to_cpu16 (head->ancount) + *data->naddresses, 4) > ALIGN_UP (*data->naddresses, 4)) > > { > > - grub_errno = GRUB_ERR_NONE; > > - grub_netbuff_free (nb); > > - return GRUB_ERR_NONE; > > + grub_net_network_level_address_t *old_addresses = *data->addresses; > > + *data->addresses = grub_malloc (sizeof ((*data->addresses)[0]) > > + * ALIGN_UP (grub_be_to_cpu16 (head->ancount) + *data->naddresses, 4)); > > + if (!*data->addresses) > > + { > > + grub_errno = GRUB_ERR_NONE; > > + grub_netbuff_free (nb); > > + return GRUB_ERR_NONE; > > + } > > + if (*data->naddresses) > > + { > > + grub_memcpy (*data->addresses, old_addresses, sizeof ((*data->addresses)[0]) * (*data->naddresses)); > > + grub_free (old_addresses); > > + } > > } > > + > > reparse_ptr = ptr; > > reparse: > > for (i = 0, ptr = reparse_ptr; i < grub_be_to_cpu16 (head->ancount); i++) > > > > From: Andrei Borzenkov > Subject: [PATCH] dns: out of bounds for data->addresses in recv_hook > > We may get more than one response before exiting out of loop in > grub_net_dns_lookup. We never really use more than the very first > addresses during lookup so there is little point in collecting all > of them. Just quit early if we already have some reply. > > Code needs serious redesign to actually collect multiple answers > and select the best fit according to requested type (IPv4 nr IPv6). > > --- > grub-core/net/dns.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c > index 89741dd..5d9afe0 100644 > --- a/grub-core/net/dns.c > +++ b/grub-core/net/dns.c > @@ -238,6 +238,15 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)), > char *redirect_save = NULL; > grub_uint32_t ttl_all = ~0U; > > + /* Code apparently assumed that only one packet is received as response. > + We may get multiple responses due to network condition, so check here > + and quit early. */ > + if (*data->addresses) > + { > + grub_netbuff_free (nb); > + return GRUB_ERR_NONE; > + } > + > head = (struct dns_header *) nb->data; > ptr = (grub_uint8_t *) (head + 1); > if (ptr >= nb->tail) > -- > tg: (b524fa2..) u/dns-fix-naddresses-out-of-bounds (depends on: master) > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel