Message ID | 20230107031406.gonna.761-kees@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp1143797wrt; Fri, 6 Jan 2023 19:37:08 -0800 (PST) X-Google-Smtp-Source: AMrXdXtYdrRAWPjeGHWLEuWfnNOCCyyIhpoLc5C2XdDOgGoluP3277C9Cg9+js2Q298B6IkUVfE2 X-Received: by 2002:a05:6a21:78a6:b0:ad:c4e5:5a41 with SMTP id bf38-20020a056a2178a600b000adc4e55a41mr119484730pzc.21.1673062627785; Fri, 06 Jan 2023 19:37:07 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1673062627; cv=none; d=google.com; s=arc-20160816; b=klzVRc7JoMb8LC0CNtkP90ntPbTybTXcU//NQlYIMjuiZ7Ux7oaYN6vb6FvBTdUWoN usqjAR8AcfSW/f3nGY/UgdoMsrpc8JMeA0+6urPB6lrWTb1C+k1GCG3FMvlJDqL0tMnq cvXHlIvyKbmQWMW/qmCH+ZPxp2WnFeC2GhDAub6e7jnlLEoqA52CNInnkMV/HqpL3Jgo owKy/+vyLi1CKY/ETbjgziWEVfSGKQs+tWFMPeuXT3x/9lPPMPhMWQALmXmbt1FaHqBX refijFzjyzt47fTjXC2SZv42lr+9Cq2ezdd+pr907QoDwNjULTbuMRSOjqa5IbXMDtCl hpKA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=95kJcOt/ikThNKTdqewmJ/CgBxR51Y5xOIZwLQSCcrE=; b=XFngf9GB483jwqAxSAKvF1x4kgKRvPo4RUMWxcloZfe/pm2FsDR8DLmeW6RVI53Czn hAV262tmJnmF37XbcPon3uQHXCe09j/YkCF5shjkngd3nR/xRXhZQsxkWWo3rWHDBVWT Zk5O3jAjFQ0PoGbnxwHJUhVV9e1O66YWi4qTXl07T21AepqozFn7JNu2bwKEFOTahz14 mZOdn4YmbdPN+tCo5q7T8gquOlz1BSKeqkKJgHuni1O2+atmHZjdvsKEhSPxv8UanPTw FE3frXelXjOAhvT0tx40CvFWnNTmQjP/xXCRcx/eREVwoDmBrkbIKl1CPesgSaT89SfB 1xgw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=kYwsB08+; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i10-20020a63a84a000000b00477cfa54894si3039194pgp.300.2023.01.06.19.36.53; Fri, 06 Jan 2023 19:37:07 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=kYwsB08+; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236293AbjAGDOQ (ORCPT <rfc822;dolce.eric@gmail.com> + 99 others); Fri, 6 Jan 2023 22:14:16 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35488 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229542AbjAGDOP (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 6 Jan 2023 22:14:15 -0500 Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F35448B53F for <linux-kernel@vger.kernel.org>; Fri, 6 Jan 2023 19:14:13 -0800 (PST) Received: by mail-pj1-x1036.google.com with SMTP id o7-20020a17090a0a0700b00226c9b82c3aso3753088pjo.3 for <linux-kernel@vger.kernel.org>; Fri, 06 Jan 2023 19:14:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=95kJcOt/ikThNKTdqewmJ/CgBxR51Y5xOIZwLQSCcrE=; b=kYwsB08+CWBm2ntQJcxzd406oSmeUX7X62aURGdszpiOSJ5peUzvreisyRJmZqlttW aLML4KJoMVRA7+8gCiobfD2jb0s6uhIzyUBNxH5UHMRQxwY3cdjln+s+4d1TzeFwIXI6 2fFyXSEw+mI6nWmc0G3f89SmYpdFS7VAkfgbw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=95kJcOt/ikThNKTdqewmJ/CgBxR51Y5xOIZwLQSCcrE=; b=oRUCtoRCf+AxeTPXSc/HgbOSUptdFM3TzLPW+AouSBLXwrqPZGq8OHLaDDSCrYTDN6 GEWiw8vUO2zdTbOFNC1Q0IzLXQNYhfH1jH2qGQFKtil8BZjvZ/3M09JmrQAS7QyXL5nI fxaLlfjk/Ch5QnrtekGwDdXGoSCsJ8Whv/UqEgkvcWiBdAj6Hgv/oamPWDmCvA7fOwMb plY06or/3PtqGcvSPgIWL4/bhFZaWBZ8S2FtGIFlR8ShGEMGtJ9Z0flyLU9yuuaMkkwp aX/cAeHppjQK6WcHEO4HxIffRHegIRbe5ZSlkrBOg2TkdUOcYXMjXHtUB8w6x94YlcNh ZqJA== X-Gm-Message-State: AFqh2koKGd8VihLl//FkiC66R7HHOPStjwtasWo5NFyld5vAYjo9xINP qtsgHvzlT4Cz/vluLSmeFp9HZQ== X-Received: by 2002:a05:6a20:65a9:b0:a5:df86:f0e1 with SMTP id p41-20020a056a2065a900b000a5df86f0e1mr77542877pzh.16.1673061253478; Fri, 06 Jan 2023 19:14:13 -0800 (PST) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id bb9-20020a170902bc8900b00192c4055e72sm1598011plb.173.2023.01.06.19.14.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 06 Jan 2023 19:14:12 -0800 (PST) From: Kees Cook <keescook@chromium.org> To: Jack Rosenthal <jrosenth@chromium.org> Cc: Kees Cook <keescook@chromium.org>, Paul Menzel <pmenzel@molgen.mpg.de>, Guenter Roeck <groeck@chromium.org>, Julius Werner <jwerner@chromium.org>, Brian Norris <briannorris@chromium.org>, Stephen Boyd <swboyd@chromium.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org Subject: [PATCH v2] firmware: coreboot: Check size of table entry and split memcpy Date: Fri, 6 Jan 2023 19:14:10 -0800 Message-Id: <20230107031406.gonna.761-kees@kernel.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=2707; h=from:subject:message-id; bh=Hmpc+o4lwHPnt4nPVCo2lthNb1BNnwcQ159clj/k64U=; b=owEBbQKS/ZANAwAKAYly9N/cbcAmAcsmYgBjuOOBQ0hsMATs0ww/lc0c22TSgpuQUnwEptZZRMZS mHoWZV2JAjMEAAEKAB0WIQSlw/aPIp3WD3I+bhOJcvTf3G3AJgUCY7jjgQAKCRCJcvTf3G3AJmNRD/ 9mC1D7F+9noWBc7+He3LuDcSkJtwNHumb6K17bFminY+yYvSz0OgYUIPwrgryLRNmECI1uDxqdc/1X 5iQGFdWTJX5vbZuAbQJpelg7ld3fmwFygs2w1D6NBFcO9V7czUPWwi6wE0b/+IfCOThlcQl21XaXgu tUAp7S2diCDjAf87FHArjHnizy+aDEF6E58IRQIFNR4mddB7z+0HAjumzGLsGhBXFVnb+hLnphbAB2 h0kkD0KwFpn8cnv6HxcZnT3+bxWnLO3StAHg6y69kb+j/IiODn4x7aPGiu3mjPJ9TCfu76TEkV4NVq nKNEFytEWzQoC/2HZ0cksrEwAWnM8CE1GiVvsGCXSKqHmkqTMqg1X7T12F76KHK37BEbm7uxm6YHFc q0oblhyQy8VmQ73HLXcjfqIDh+TkuakSEa7tvg7fkXvXhyAY821on/lXXvGGsHLDRmGtU2meFV9qSO tBMrtCoFn+R70d2JcBxOgA0OtOhOo33HnnFSMDaDuHbGn9hTYOYYRcm0GjDes+zXhcGaHHvf/CQabi 19D+9hHoZatdZ09FRh/DrhFMu4KEwhac+xgSU1ejDpCGDp1oIT7gdtmiQ03e5aNYUFgN3Ds59Qh+5p 0FMfJnm0QqsKYHGs/ph93J/svkxphKbhkg1g61WoaJetwltufydGJOyP5hRg== X-Developer-Key: i=keescook@chromium.org; a=openpgp; fpr=A5C3F68F229DD60F723E6E138972F4DFDC6DC026 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1754333317610626061?= X-GMAIL-MSGID: =?utf-8?q?1754333317610626061?= |
Series |
[v2] firmware: coreboot: Check size of table entry and split memcpy
|
|
Commit Message
Kees Cook
Jan. 7, 2023, 3:14 a.m. UTC
The memcpy() of the data following a coreboot_table_entry couldn't
be evaluated by the compiler under CONFIG_FORTIFY_SOURCE. To make it
easier to reason about, add an explicit flexible array member to struct
coreboot_device so the entire entry can be copied at once. Additionally,
validate the sizes before copying. Avoids this run-time false positive
warning:
memcpy: detected field-spanning write (size 168) of single field "&device->entry" at drivers/firmware/google/coreboot_table.c:103 (size 8)
Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Link: https://lore.kernel.org/all/03ae2704-8c30-f9f0-215b-7cdf4ad35a9a@molgen.mpg.de/
Cc: Jack Rosenthal <jrosenth@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2: move flex array to struct coreboot_device (julius)
v1: https://lore.kernel.org/lkml/20230106045327.never.413-kees@kernel.org
---
drivers/firmware/google/coreboot_table.c | 9 +++++++--
drivers/firmware/google/coreboot_table.h | 1 +
2 files changed, 8 insertions(+), 2 deletions(-)
Comments
On Fri, Jan 6, 2023 at 7:14 PM Kees Cook <keescook@chromium.org> wrote: > > The memcpy() of the data following a coreboot_table_entry couldn't > be evaluated by the compiler under CONFIG_FORTIFY_SOURCE. To make it > easier to reason about, add an explicit flexible array member to struct > coreboot_device so the entire entry can be copied at once. Additionally, > validate the sizes before copying. Avoids this run-time false positive > warning: > > memcpy: detected field-spanning write (size 168) of single field "&device->entry" at drivers/firmware/google/coreboot_table.c:103 (size 8) > > Reported-by: Paul Menzel <pmenzel@molgen.mpg.de> > Link: https://lore.kernel.org/all/03ae2704-8c30-f9f0-215b-7cdf4ad35a9a@molgen.mpg.de/ > Cc: Jack Rosenthal <jrosenth@chromium.org> > Cc: Guenter Roeck <groeck@chromium.org> > Cc: Julius Werner <jwerner@chromium.org> > Cc: Brian Norris <briannorris@chromium.org> > Cc: Stephen Boyd <swboyd@chromium.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Guenter Roeck <groeck@chromium.org> > --- > v2: move flex array to struct coreboot_device (julius) > v1: https://lore.kernel.org/lkml/20230106045327.never.413-kees@kernel.org > --- > drivers/firmware/google/coreboot_table.c | 9 +++++++-- > drivers/firmware/google/coreboot_table.h | 1 + > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c > index 2652c396c423..564a3c908838 100644 > --- a/drivers/firmware/google/coreboot_table.c > +++ b/drivers/firmware/google/coreboot_table.c > @@ -93,14 +93,19 @@ static int coreboot_table_populate(struct device *dev, void *ptr) > for (i = 0; i < header->table_entries; i++) { > entry = ptr_entry; > > - device = kzalloc(sizeof(struct device) + entry->size, GFP_KERNEL); > + if (entry->size < sizeof(*entry)) { > + dev_warn(dev, "coreboot table entry too small!\n"); > + return -EINVAL; > + } > + > + device = kzalloc(sizeof(device->dev) + entry->size, GFP_KERNEL); > if (!device) > return -ENOMEM; > > device->dev.parent = dev; > device->dev.bus = &coreboot_bus_type; > device->dev.release = coreboot_device_release; > - memcpy(&device->entry, ptr_entry, entry->size); > + memcpy(device->raw, entry, entry->size); > > switch (device->entry.tag) { > case LB_TAG_CBMEM_ENTRY: > diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h > index 37f4d335a606..d814dca33a08 100644 > --- a/drivers/firmware/google/coreboot_table.h > +++ b/drivers/firmware/google/coreboot_table.h > @@ -79,6 +79,7 @@ struct coreboot_device { > struct lb_cbmem_ref cbmem_ref; > struct lb_cbmem_entry cbmem_entry; > struct lb_framebuffer framebuffer; > + DECLARE_FLEX_ARRAY(u8, raw); > }; > }; > > -- > 2.34.1 >
Reviewed-by: Julius Werner <jwerner@chromium.org> > - memcpy(&device->entry, ptr_entry, entry->size); > + memcpy(device->raw, entry, entry->size); nit: It's a bit odd to change the source pointer from ptr_entry to entry here. Technically the static analyzer would be within its rights to give you a warning for that as well, because you're now "overrunning" the source struct instead of the destination one.
On Mon, Jan 09, 2023 at 04:02:26PM +0100, Julius Werner wrote: > Reviewed-by: Julius Werner <jwerner@chromium.org> > > > - memcpy(&device->entry, ptr_entry, entry->size); > > + memcpy(device->raw, entry, entry->size); > > nit: It's a bit odd to change the source pointer from ptr_entry to > entry here. Technically the static analyzer would be within its rights > to give you a warning for that as well, because you're now > "overrunning" the source struct instead of the destination one. True. We've been focused on write overflows, but yeah, since the location of the flex array changed, I'll switch this back to ptr_entry.
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c index 2652c396c423..564a3c908838 100644 --- a/drivers/firmware/google/coreboot_table.c +++ b/drivers/firmware/google/coreboot_table.c @@ -93,14 +93,19 @@ static int coreboot_table_populate(struct device *dev, void *ptr) for (i = 0; i < header->table_entries; i++) { entry = ptr_entry; - device = kzalloc(sizeof(struct device) + entry->size, GFP_KERNEL); + if (entry->size < sizeof(*entry)) { + dev_warn(dev, "coreboot table entry too small!\n"); + return -EINVAL; + } + + device = kzalloc(sizeof(device->dev) + entry->size, GFP_KERNEL); if (!device) return -ENOMEM; device->dev.parent = dev; device->dev.bus = &coreboot_bus_type; device->dev.release = coreboot_device_release; - memcpy(&device->entry, ptr_entry, entry->size); + memcpy(device->raw, entry, entry->size); switch (device->entry.tag) { case LB_TAG_CBMEM_ENTRY: diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h index 37f4d335a606..d814dca33a08 100644 --- a/drivers/firmware/google/coreboot_table.h +++ b/drivers/firmware/google/coreboot_table.h @@ -79,6 +79,7 @@ struct coreboot_device { struct lb_cbmem_ref cbmem_ref; struct lb_cbmem_entry cbmem_entry; struct lb_framebuffer framebuffer; + DECLARE_FLEX_ARRAY(u8, raw); }; };