From patchwork Tue Mar 31 16:46:15 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin O'Connor X-Patchwork-Id: 456696 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id C173014018C for ; Wed, 1 Apr 2015 03:47:03 +1100 (AEDT) Received: from localhost ([::1]:39651 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YczJd-0002Xw-PF for incoming@patchwork.ozlabs.org; Tue, 31 Mar 2015 12:47:01 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57351) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YczJ0-0001mt-Qm for qemu-devel@nongnu.org; Tue, 31 Mar 2015 12:46:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YczIx-00016w-Eu for qemu-devel@nongnu.org; Tue, 31 Mar 2015 12:46:22 -0400 Received: from mail-qg0-f43.google.com ([209.85.192.43]:34191) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YczIx-00016j-Ax for qemu-devel@nongnu.org; Tue, 31 Mar 2015 12:46:19 -0400 Received: by qgep97 with SMTP id p97so19729335qge.1 for ; Tue, 31 Mar 2015 09:46:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=aSV2Ko5VhuPH4fDmxBIgOFOviQi8HkiM3UtHzwIwXfI=; b=TTbplrgtAB/zJdmbMiRtZ84zsGogM0u8UIwFjYCuLfJLAvo6mgNJcdS0cMxNlPBpsP S1ocPIZqgZk4B+hp12NhjYxtDHjtJhwSIPPbRSxmVm+DGrm0rB+ficzBK6oQiNA4IRHF S0Us4T7TTzkZ4YtaJGdeoN6sX95yzz87DYpmnwlXuqNonF7PVHCnBz498mpvQsu4LWlP zWmOgK3ciydp89mazjd9jZc7wWFMhGLH8YPTxgqZtMud/P9yqH9JQj/ifc9e0z50napq FfhVJrabbcPB2A/kKCSw3AU++6amDD4Z9cEJcwn5Dq61V8S9smnIzrxDpNQZygjZFNPL OXIA== X-Gm-Message-State: ALoCoQlA5R6NweHaaVm0KrodmcqqR+SJd/J3uw3id7zeqzhJDbvBCFoXly32P3Y1r0layZZGcyEF X-Received: by 10.55.22.168 with SMTP id 40mr79963668qkw.101.1427820377766; Tue, 31 Mar 2015 09:46:17 -0700 (PDT) Received: from localhost (207-172-170-53.c3-0.avec-ubr1.nyr-avec.ny.cable.rcn.com. [207.172.170.53]) by mx.google.com with ESMTPSA id 144sm10174837qhx.45.2015.03.31.09.46.16 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 31 Mar 2015 09:46:16 -0700 (PDT) Date: Tue, 31 Mar 2015 12:46:15 -0400 From: Kevin O'Connor To: "Gabriel L. Somlo" Message-ID: <20150331164615.GA29388@morn.localdomain> References: <1427756790-7922-1-git-send-email-brogers@suse.com> <20150331040250.GA23911@morn.localdomain> <551A5FB6020000480010C836@prv-mh.provo.novell.com> <20150331163729.GB4523@HEDWIG.INI.CMU.EDU> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150331163729.GB4523@HEDWIG.INI.CMU.EDU> User-Agent: Mutt/1.5.23 (2014-03-12) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.85.192.43 Cc: seabios@seabios.org, kraxel@redhat.com, qemu-devel@nongnu.org, Bruce Rogers Subject: Re: [Qemu-devel] [SeaBIOS] [PATCH] smbios: ensure comparison SMBIOS string can't be paragraph aligned X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Tue, Mar 31, 2015 at 12:37:30PM -0400, Gabriel L. Somlo wrote: > On Tue, Mar 31, 2015 at 08:49:58AM -0600, Bruce Rogers wrote: > > >>> On 3/30/2015 at 10:02 PM, Kevin O'Connor wrote: > > > On Mon, Mar 30, 2015 at 05:06:30PM -0600, Bruce Rogers wrote: > > >> The SMBIOS anchor string _SM_ is stored within SeaBIOS to validate > > >> an SMBIOS entry point structure. There is the possibility (observed) > > >> that this comparison string ends up paragraph aligned and mistakenly > > >> found during a search for the real SMBIOS entry point. Ensure it will > > >> never end up on a paragraph boundary by storing it at odd alignment. > > > > > > Thanks. > > > > > > What OS was this on? It's really an OS bug as the OS needs to check > > > both the signature and the checksum. > > > > > > My preferred approach to addressing this would be to turn > > > p->anchor_string into a u32 and do an integer compare instead of a > > > string compare. Although technically this can lead to the same > > > potential issue, in practice it should not happen because SeaBIOS' > > > init code is relocated out of the f-segment during startup (while > > > static strings are generally not). > > > > > > -Kevin > > > > This was actually flagged by the QEMU make check test in > > tests/bios-tables-test.c, where the code has asserts based on the > > first find of the _SM_ string found on a paragraph boundary in the > > f segment. It assumes that the string found is sufficient to identify > > the smbios entry point structure. > > > > I read in http://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.0.0.pdf > > where there are number of steps needed to verify the entry-point, beyond > > finding the anchor string. So I assume that the make check test needs to > > be fixed. > > > > But I wonder if bios creators are also supposed to ensure that there is no > > such string findable on a paragraph boundary so as to avoid any potential > > confusion? I don't have expereince writing bios's so I am only guessing > > at that. > > I guess on physical hardware that would be mitigated by the BIOS's > .rodata (or whatever the equivalent thing is called in BIOS-speak) > NOT falling within 0xf0000..0xfffff :) > > If there's no way to guarantee that with SeaBIOS, I suppose the > qemu test could be rewritten to account for the possibility of > "false positive" signatures... I would do both - change SeaBIOS to (in practice) not put "_SM_" in the f-segment and change the QEMU test to not stop on the first "_SM_" it finds. SeaBIOS patch below (untested). -Kevin diff --git a/src/fw/biostables.c b/src/fw/biostables.c index 50a891b..450aca2 100644 --- a/src/fw/biostables.c +++ b/src/fw/biostables.c @@ -271,7 +271,7 @@ copy_smbios(void *pos) if (SMBiosAddr) return; struct smbios_entry_point *p = pos; - if (memcmp(p->anchor_string, "_SM_", 4)) + if (p->signature != SMBIOS_SIGNATURE) return; if (checksum(pos, 0x10) != 0) return; diff --git a/src/fw/smbios.c b/src/fw/smbios.c index dba0541..f3b5ad9 100644 --- a/src/fw/smbios.c +++ b/src/fw/smbios.c @@ -37,7 +37,7 @@ smbios_entry_point_setup(u16 max_structure_size, struct smbios_entry_point ep; memset(&ep, 0, sizeof(ep)); - memcpy(ep.anchor_string, "_SM_", 4); + ep.signature = SMBIOS_SIGNATURE; ep.length = 0x1f; ep.smbios_major_version = 2; ep.smbios_minor_version = 4; diff --git a/src/std/smbios.h b/src/std/smbios.h index 0513716..4ccf2ea 100644 --- a/src/std/smbios.h +++ b/src/std/smbios.h @@ -3,11 +3,13 @@ #include "types.h" // u32 +#define SMBIOS_SIGNATURE 0x5f4d535f // "_SM_" + /* SMBIOS entry point -- must be written to a 16-bit aligned address between 0xf0000 and 0xfffff. */ struct smbios_entry_point { - char anchor_string[4]; + u32 signature; u8 checksum; u8 length; u8 smbios_major_version;