From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752984AbbCZPV7 (ORCPT ); Thu, 26 Mar 2015 11:21:59 -0400 Received: from cantor2.suse.de ([195.135.220.15]:57229 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751382AbbCZPV5 (ORCPT ); Thu, 26 Mar 2015 11:21:57 -0400 Subject: Re: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow From: Jean Delvare To: Matt Fleming Cc: LKML , Matt Fleming , Ard Biesheuvel , Ivan Khoronzhuk In-Reply-To: <20150326144717.GD6525@codeblueprint.co.uk> References: <20150320095947.644f9c67@endymion.delvare> <20150326130651.GC6525@codeblueprint.co.uk> <1427375705.4261.2.camel@chaos.site> <20150326144717.GD6525@codeblueprint.co.uk> Content-Type: text/plain; charset="UTF-8" Organization: Suse Linux Date: Thu, 26 Mar 2015 16:21:53 +0100 Message-ID: <1427383313.4261.7.camel@chaos.site> Mime-Version: 1.0 X-Mailer: Evolution 2.28.2 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Le Thursday 26 March 2015 à 14:47 +0000, Matt Fleming a écrit : > On Thu, 26 Mar, at 02:15:05PM, Jean Delvare wrote: > > > > I don't actually have a tree, so feel free to pick it. > > OK will do, but this is a regression fix for a potential bug introduced > in commit 6d9ff4733172 ("firmware: dmi_scan: Fix dmi_len type"), right? Not really. I'd rather say it complements commit 6d9ff4733172. The actual bugs were introduced in commit fc43026278b2. But you can't really call it a regression as SMBIOS 3.0 entry points were not supported at all before that commit. Still a reference to commit fc43026278b2 would certainly be appropriate, as anyone backporting that commit will also want to pick the two aforementioned fix-ups (plus ce204e9a4bd8 for that matter.) Thanks, Jean > If so, it would be useful for this patch to make reference to that > commit. > > How about this? > > --- > > From 8aabaf128c85bab02fdbe594481826e137c25da4 Mon Sep 17 00:00:00 2001 > From: Jean Delvare > Date: Fri, 20 Mar 2015 09:59:47 +0100 > Subject: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow > > dmi_num is a u16, dmi_len is a u32, so this construct: > > dmi_num = dmi_len / 4; > > would result in an integer overflow for a DMI table larger than > 256 kB. I've never see such a large table so far, but SMBIOS 3.0 > makes it possible so maybe we'll see such tables in the future. > > So instead of faking a structure count when the entry point does > not provide it, adjust the loop condition in dmi_table() to properly > deal with the case where dmi_num is not set. > > This potential overflow was introduced in commit 6d9ff4733172 > ("firmware: dmi_scan: Fix dmi_len type"). > > Signed-off-by: Jean Delvare > Cc: Matt Fleming > Cc: Ivan Khoronzhuk > Cc: > Acked-by: Ard Biesheuvel > Signed-off-by: Matt Fleming > --- > drivers/firmware/dmi_scan.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c > index 69fac068669f..2eebd28b4c40 100644 > --- a/drivers/firmware/dmi_scan.c > +++ b/drivers/firmware/dmi_scan.c > @@ -86,10 +86,13 @@ static void dmi_table(u8 *buf, u32 len, int num, > int i = 0; > > /* > - * Stop when we see all the items the table claimed to have > - * OR we run off the end of the table (also happens) > + * Stop when we have seen all the items the table claimed to have > + * (SMBIOS < 3.0 only) OR we reach an end-of-table marker OR we run > + * off the end of the table (should never happen but sometimes does > + * on bogus implementations.) > */ > - while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) { > + while ((!num || i < num) && > + (data - buf + sizeof(struct dmi_header)) <= len) { > const struct dmi_header *dm = (const struct dmi_header *)data; > > /* > @@ -529,21 +532,10 @@ static int __init dmi_smbios3_present(const u8 *buf) > if (memcmp(buf, "_SM3_", 5) == 0 && > buf[6] < 32 && dmi_checksum(buf, buf[6])) { > dmi_ver = get_unaligned_be16(buf + 7); > + dmi_num = 0; /* No longer specified */ > dmi_len = get_unaligned_le32(buf + 12); > dmi_base = get_unaligned_le64(buf + 16); > > - /* > - * The 64-bit SMBIOS 3.0 entry point no longer has a field > - * containing the number of structures present in the table. > - * Instead, it defines the table size as a maximum size, and > - * relies on the end-of-table structure type (#127) to be used > - * to signal the end of the table. > - * So let's define dmi_num as an upper bound as well: each > - * structure has a 4 byte header, so dmi_len / 4 is an upper > - * bound for the number of structures in the table. > - */ > - dmi_num = dmi_len / 4; > - > if (dmi_walk_early(dmi_decode) == 0) { > pr_info("SMBIOS %d.%d present.\n", > dmi_ver >> 8, dmi_ver & 0xFF); > -- > 2.1.0