Message ID | Y1yetX1CHsr+fibp@mail.google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp1164256wru; Fri, 28 Oct 2022 20:35:34 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6oXN05Kilo6DlBMpqAw/rNF/NLtN8n64w3pLGUqz3x9YEH5BltZPS9vgN3kVExKWOjFGPc X-Received: by 2002:a63:6bc5:0:b0:460:bd9a:64b8 with SMTP id g188-20020a636bc5000000b00460bd9a64b8mr2464932pgc.257.1667014534641; Fri, 28 Oct 2022 20:35:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667014534; cv=none; d=google.com; s=arc-20160816; b=poCSjZ1fQmPA1/WNOgNhGhLMvGhG6IgytNPL+riwC5KBW8t0a6Diu+DMC8jo4DXudT kTNS9IiZc5DRsRed502KFwz9dyHtFNp0a8j0rfKW0o3C0WOj+FX69wE2I3CCLgHq3Nzp t4yRh9x71A0PcfItYp65OvGyqGiYuy2ZiqyOo98dQLe3fZDSehFUI7Cs6fqIEM3UGmnU GbKcI8GqEirxl5vFq2sM/5pjGft72kLD0zH2la9DAyL7g1xTQncX0F1FiZTsONEMT9gU 6TX/SoHWxeLXq3V5TZHgF+13SkBcIQPHlSBqAzMKBl7q3AnRVyLT3PaK6Rg5QbWIOCgi o3JQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :message-id:subject:cc:to:from:date:dkim-signature; bh=VHF9euNOOmuwTSh72fVwWU/hhjv7B36F6rq6GHbm0Ws=; b=Kn/tDVyBk4B5tIrbbmp52UQullyE+B5aOOBoH51vXZqekH/bVJHs6IQz+neGtK5NNc AOUQ0flNSqT7DbuyMvFgajU2z05SYRvjacd+ZXnUWvcoKNGxeol9/vnz1tLXBIRyJ8pJ sOt1o6EgPE91eVDHfoCBswpG5xwYEk8/IpnH+IFPgNdUiXK6hGt9IXw1bxDQwYCzFoNC bv1Y9uUgFNgHOt9Zt9F7LGpoQq41WOguqQ/aCtBoCGL35u1auRA0JFXi4i3isvWPyepd sNEHA9FDR6ylTR7KRwyL9OnO6ND7mz7pK4Lhg3dKGxV4lnMoC0fMRTFfC3D0Ao26xjKz iHsg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=jK5E1boC; 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=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id rj9-20020a17090b3e8900b001fab0d18bcasi714980pjb.66.2022.10.28.20.35.21; Fri, 28 Oct 2022 20:35:34 -0700 (PDT) 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=@gmail.com header.s=20210112 header.b=jK5E1boC; 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=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229647AbiJ2DcS (ORCPT <rfc822;pusanteemu@gmail.com> + 99 others); Fri, 28 Oct 2022 23:32:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49854 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229489AbiJ2DcQ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 28 Oct 2022 23:32:16 -0400 Received: from mail-pf1-x435.google.com (mail-pf1-x435.google.com [IPv6:2607:f8b0:4864:20::435]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 358FA109D40; Fri, 28 Oct 2022 20:32:15 -0700 (PDT) Received: by mail-pf1-x435.google.com with SMTP id m6so6369922pfb.0; Fri, 28 Oct 2022 20:32:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=in-reply-to:content-disposition:mime-version:message-id:subject:cc :to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=VHF9euNOOmuwTSh72fVwWU/hhjv7B36F6rq6GHbm0Ws=; b=jK5E1boC3hu8E7IKKZ6CHa/8w55eH6o4SWLIwK/DleiUhoRlRDg0nhbUtjDD76L7q5 93OjujLRcE31fBxMQ2A0Gq0z7tfH3bVlJrRbhHYggtjqQOD3sXuqsv9rLVy3tUHpT+Yc VdPaxC13yA9VPuPQa1jUO32XmDCgV05Qw6tk86KFBLTLTEi1+16B2KRw/EOFxOQhUxRd 23NpFy8Er4yVa2BlmTYeUNvzKWgQPO81xAkxWaP0kL+zrWMnGOgCmpkjnHdOb3S3ECSE 9rmz+S8TZ8vjt7qSFnvAXDJHEa/g8fsHS8FkZdPyqZsJl1Sk4yoAL+/UVz9wjHiUBH4h TpeQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:message-id:subject:cc :to:from:date:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=VHF9euNOOmuwTSh72fVwWU/hhjv7B36F6rq6GHbm0Ws=; b=kA7DITuHr4/ncGOpM2w2vhY9R60L9IQQnw0z56tJkBsHt3eQ+RNBrw8yUqYDpxKmI1 Frg6JzkYpOfiv9qwNGW2tsSlFXGR3CX0IL+1p4spxGCclnsnMPTn1eXKRyHdVXAW/v7c tHL3V3SRTtaVxY9/LGQHGBBfuytBePj3y2S+UqCpzda/r1ofXJ7Vhb+4UJ6qsoRheajr Dg2vr7PZkwCvILkL8EGNnz2e4fw3W6W8MusCN/MAGrUTc1RnprqNXIBtyoFy5rAgq3n5 QKGFwl8QMbcP//rvP7NMOyWcE0QFqduNmu/J1g+rNfPzZFc3NdwAaFLFyd3z60w2+vQa MPNw== X-Gm-Message-State: ACrzQf1bDMdiAz9lbAE1JMEp2xVOxBxDUp5MYjNfdn+XPI9vylpldjdI A3oMmfAFIytiDbwwpRE6EzU= X-Received: by 2002:aa7:8b46:0:b0:56c:349f:191e with SMTP id i6-20020aa78b46000000b0056c349f191emr2663102pfd.29.1667014334674; Fri, 28 Oct 2022 20:32:14 -0700 (PDT) Received: from mail.google.com (122-58-209-93-fibre.sparkbb.co.nz. [122.58.209.93]) by smtp.gmail.com with ESMTPSA id r2-20020a17090a2e8200b002086ac07041sm166491pjd.44.2022.10.28.20.32.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Oct 2022 20:32:13 -0700 (PDT) Date: Sat, 29 Oct 2022 16:32:05 +1300 From: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> To: Alex Deucher <alexander.deucher@amd.com>, Christian =?utf-8?b?S8O2bmln?= <christian.koenig@amd.com>, "Pan, Xinhui" <Xinhui.Pan@amd.com>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org, paulo.miguel.almeida.rodenas@gmail.com Subject: [PATCH v2] [next] drm/radeon: Replace one-element array with flexible-array member Message-ID: <Y1yetX1CHsr+fibp@mail.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <Y1trhRE3nK5iAY6q@mail.google.com> X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham 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?1747909878097083132?= X-GMAIL-MSGID: =?utf-8?q?1747991432664036598?= |
Series |
[v2,next] drm/radeon: Replace one-element array with flexible-array member
|
|
Commit Message
Paulo Miguel Almeida
Oct. 29, 2022, 3:32 a.m. UTC
One-element arrays are deprecated, and we are replacing them with
flexible array members instead. So, replace one-element array with
flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and
refactor the rest of the code accordingly.
It's worth mentioning that doing a build before/after this patch results
in no binary output differences.
This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
routines on memcpy() and help us make progress towards globally
enabling -fstrict-flex-arrays=3 [1].
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/239
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1]
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Changelog:
v2: no binary output differences patch; report binary changes findings
on commit log. Res: Kees Cook.
This request was made in an identical, yet different, patch but the
same feedback applies.
https://lore.kernel.org/lkml/Y1x3MtRJ8ckXxlJn@mail.google.com/
v1: https://lore.kernel.org/lkml/Y1trhRE3nK5iAY6q@mail.google.com/
---
drivers/gpu/drm/radeon/atombios.h | 2 +-
drivers/gpu/drm/radeon/radeon_atombios.c | 7 +++++--
2 files changed, 6 insertions(+), 3 deletions(-)
Comments
On Sat, Oct 29, 2022 at 04:32:05PM +1300, Paulo Miguel Almeida wrote: > One-element arrays are deprecated, and we are replacing them with > flexible array members instead. So, replace one-element array with > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and > refactor the rest of the code accordingly. > > It's worth mentioning that doing a build before/after this patch results > in no binary output differences. Thanks for checking it! > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [1]. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/239 > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> Reviewed-by: Kees Cook <keescook@chromium.org>
On Fri, Oct 28, 2022 at 11:32 PM Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> wrote: > > One-element arrays are deprecated, and we are replacing them with > flexible array members instead. So, replace one-element array with > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and > refactor the rest of the code accordingly. > > It's worth mentioning that doing a build before/after this patch results > in no binary output differences. > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [1]. This seems like a worthy goal, and I'm not opposed to the patch per se, but it seems a bit at odds with what this header represents. atombios.h represents the memory layout of the data stored in the ROM on the GPU. This changes the memory layout of that ROM. We can work around that in the driver code, but if someone were to take this header to try and write some standalone tool or use it for something else in the kernel, it would not reflect reality. Alex > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/239 > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > --- > Changelog: > > v2: no binary output differences patch; report binary changes findings > on commit log. Res: Kees Cook. > > This request was made in an identical, yet different, patch but the > same feedback applies. > https://lore.kernel.org/lkml/Y1x3MtRJ8ckXxlJn@mail.google.com/ > > v1: https://lore.kernel.org/lkml/Y1trhRE3nK5iAY6q@mail.google.com/ > --- > drivers/gpu/drm/radeon/atombios.h | 2 +- > drivers/gpu/drm/radeon/radeon_atombios.c | 7 +++++-- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/atombios.h b/drivers/gpu/drm/radeon/atombios.h > index da35a970fcc0..235e59b547a1 100644 > --- a/drivers/gpu/drm/radeon/atombios.h > +++ b/drivers/gpu/drm/radeon/atombios.h > @@ -3615,7 +3615,7 @@ typedef struct _ATOM_FAKE_EDID_PATCH_RECORD > { > UCHAR ucRecordType; > UCHAR ucFakeEDIDLength; > - UCHAR ucFakeEDIDString[1]; // This actually has ucFakeEdidLength elements. > + UCHAR ucFakeEDIDString[]; // This actually has ucFakeEdidLength elements. > } ATOM_FAKE_EDID_PATCH_RECORD; > > typedef struct _ATOM_PANEL_RESOLUTION_PATCH_RECORD > diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c > index 204127bad89c..4ad5a328d920 100644 > --- a/drivers/gpu/drm/radeon/radeon_atombios.c > +++ b/drivers/gpu/drm/radeon/radeon_atombios.c > @@ -1727,8 +1727,11 @@ struct radeon_encoder_atom_dig *radeon_atombios_get_lvds_info(struct > } > } > record += fake_edid_record->ucFakeEDIDLength ? > - fake_edid_record->ucFakeEDIDLength + 2 : > - sizeof(ATOM_FAKE_EDID_PATCH_RECORD); > + struct_size(fake_edid_record, > + ucFakeEDIDString, > + fake_edid_record->ucFakeEDIDLength) : > + /* empty fake edid record must be 3 bytes long */ > + sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1; > break; > case LCD_PANEL_RESOLUTION_RECORD_TYPE: > panel_res_record = (ATOM_PANEL_RESOLUTION_PATCH_RECORD *)record; > -- > 2.37.3 >
On Tue, Nov 01, 2022 at 10:42:14AM -0400, Alex Deucher wrote: > On Fri, Oct 28, 2022 at 11:32 PM Paulo Miguel Almeida > <paulo.miguel.almeida.rodenas@gmail.com> wrote: > > > > One-element arrays are deprecated, and we are replacing them with > > flexible array members instead. So, replace one-element array with > > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and > > refactor the rest of the code accordingly. > > > > It's worth mentioning that doing a build before/after this patch results > > in no binary output differences. > > > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > > routines on memcpy() and help us make progress towards globally > > enabling -fstrict-flex-arrays=3 [1]. > > This seems like a worthy goal, and I'm not opposed to the patch per > se, but it seems a bit at odds with what this header represents. > atombios.h represents the memory layout of the data stored in the ROM > on the GPU. This changes the memory layout of that ROM. We can work > around that in the driver code, but if someone were to take this > header to try and write some standalone tool or use it for something > else in the kernel, it would not reflect reality. > > Alex > Hi Alex, thanks for taking the time to review this patch. I see where you are coming from and why you may be apprehensive with the approach. From my humble point of view, I think that the artificial line that separates "what we can change at will" and "what we should be extra careful not to break/confuse others" is whether the header file is part of the UAPI. Given that atombios.h isn't publicly accessible, I tend to gravitate towards the first one. > > + /* empty fake edid record must be 3 bytes long */ > + sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1; I am assuming that this is the part of the patch that makes you feel concerned about how devs will get it (should they copy this header), is that right? If so, would a comment on the ATOM_FAKE_EDID_PATCH_RECORD struct specifying the size of the struct when empty do the trick? - Paulo A.
On Tue, Nov 1, 2022 at 5:14 PM Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> wrote: > > On Tue, Nov 01, 2022 at 10:42:14AM -0400, Alex Deucher wrote: > > On Fri, Oct 28, 2022 at 11:32 PM Paulo Miguel Almeida > > <paulo.miguel.almeida.rodenas@gmail.com> wrote: > > > > > > One-element arrays are deprecated, and we are replacing them with > > > flexible array members instead. So, replace one-element array with > > > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and > > > refactor the rest of the code accordingly. > > > > > > It's worth mentioning that doing a build before/after this patch results > > > in no binary output differences. > > > > > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > > > routines on memcpy() and help us make progress towards globally > > > enabling -fstrict-flex-arrays=3 [1]. > > > > This seems like a worthy goal, and I'm not opposed to the patch per > > se, but it seems a bit at odds with what this header represents. > > atombios.h represents the memory layout of the data stored in the ROM > > on the GPU. This changes the memory layout of that ROM. We can work > > around that in the driver code, but if someone were to take this > > header to try and write some standalone tool or use it for something > > else in the kernel, it would not reflect reality. > > > > Alex > > > Hi Alex, thanks for taking the time to review this patch. > > I see where you are coming from and why you may be apprehensive with the > approach. From my humble point of view, I think that the artificial line > that separates "what we can change at will" and "what we should be extra > careful not to break/confuse others" is whether the header file is part > of the UAPI. Given that atombios.h isn't publicly accessible, I tend to > gravitate towards the first one. It may not be publicly accessible, but it describes a physical thing that is burned into millions of GPU boards out in the wild. There are tons of open source tools out there that take these headers from the kernel to be able to parse the date in the ROM on the GPU. If those applications sync up with the latest version of the header from the kernel, it will break their tools. The next time someone from AMD syncs up the header with the version maintained by the vbios team, the change could accidently sneak back in and break the code. It seems to me in this particular case that the header should reflect the physical layout of the ROM since that is what it describes. Alex > > > > + /* empty fake edid record must be 3 bytes long */ > > + sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1; > > I am assuming that this is the part of the patch that makes you feel > concerned about how devs will get it (should they copy this header), > is that right? > > If so, would a comment on the ATOM_FAKE_EDID_PATCH_RECORD struct > specifying the size of the struct when empty do the trick? > > - Paulo A. >
On Tue, Nov 01, 2022 at 10:42:14AM -0400, Alex Deucher wrote: > On Fri, Oct 28, 2022 at 11:32 PM Paulo Miguel Almeida > <paulo.miguel.almeida.rodenas@gmail.com> wrote: > > > > One-element arrays are deprecated, and we are replacing them with > > flexible array members instead. So, replace one-element array with > > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and > > refactor the rest of the code accordingly. > > > > It's worth mentioning that doing a build before/after this patch results > > in no binary output differences. > > > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > > routines on memcpy() and help us make progress towards globally > > enabling -fstrict-flex-arrays=3 [1]. > > This seems like a worthy goal, and I'm not opposed to the patch per > se, but it seems a bit at odds with what this header represents. > atombios.h represents the memory layout of the data stored in the ROM > on the GPU. This changes the memory layout of that ROM. We can work It doesn't though. Or maybe I'm misunderstanding what you mean. > around that in the driver code, but if someone were to take this > header to try and write some standalone tool or use it for something > else in the kernel, it would not reflect reality. Does the ROM always only have a single byte there? This seems unlikely given the member "ucFakeEDIDLength" (and the code below). The problem on the kernel side is that the code just before the patch context in drivers/gpu/drm/amd/amdgpu/atombios_encoders.c will become a problem soon: if (fake_edid_record->ucFakeEDIDLength) { struct edid *edid; int edid_size = max((int)EDID_LENGTH, (int)fake_edid_record->ucFakeEDIDLength); edid = kmalloc(edid_size, GFP_KERNEL); if (edid) { memcpy((u8 *)edid, (u8 *)&fake_edid_record->ucFakeEDIDString[0], fake_edid_record->ucFakeEDIDLength); if (drm_edid_is_valid(edid)) { ... the memcpy() from "fake_edid_record->ucFakeEDIDString" will eventually start to WARN, since the size of that field will be seen as a single byte under -fstrict-flex-arrays. At this moment, no, there's neither source bounds checking nor -fstrict-flex-arrays, but we're trying to clean up everything we can find now so that we don't have to do this all again later. :) -Kees P.S. Also this could be shorter with fewer casts: struct edid *edid; u8 edid_size = max_t(u8, EDID_LENGTH, fake_edid_record->ucFakeEDIDLength); edid = kmemdup(fake_edid_record->ucFakeEDIDString, edid_size, GFP_KERNEL); if (edid) { if (drm_edid_is_valid(edid)) { adev->mode_info.bios_hardcoded_edid = edid; ...
On Tue, Nov 1, 2022 at 5:54 PM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Nov 01, 2022 at 10:42:14AM -0400, Alex Deucher wrote: > > On Fri, Oct 28, 2022 at 11:32 PM Paulo Miguel Almeida > > <paulo.miguel.almeida.rodenas@gmail.com> wrote: > > > > > > One-element arrays are deprecated, and we are replacing them with > > > flexible array members instead. So, replace one-element array with > > > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and > > > refactor the rest of the code accordingly. > > > > > > It's worth mentioning that doing a build before/after this patch results > > > in no binary output differences. > > > > > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > > > routines on memcpy() and help us make progress towards globally > > > enabling -fstrict-flex-arrays=3 [1]. > > > > This seems like a worthy goal, and I'm not opposed to the patch per > > se, but it seems a bit at odds with what this header represents. > > atombios.h represents the memory layout of the data stored in the ROM > > on the GPU. This changes the memory layout of that ROM. We can work > > It doesn't though. Or maybe I'm misunderstanding what you mean. > > > around that in the driver code, but if someone were to take this > > header to try and write some standalone tool or use it for something > > else in the kernel, it would not reflect reality. > > Does the ROM always only have a single byte there? This seems unlikely > given the member "ucFakeEDIDLength" (and the code below). I'm not sure. I'm mostly concerned about this: record += fake_edid_record->ucFakeEDIDLength ? fake_edid_record->ucFakeEDIDLength + 2 : sizeof(ATOM_FAKE_EDID_PATCH_RECORD); Presumably the record should only exist if ucFakeEDIDLength is non 0, but I don't know if there are some OEMs out there that just included an empty record for some reason. Maybe the code is wrong today and there are some OEMs that include it and the array is already size 0. In that case, Paulo's original patches are probably more correct. Alex > > The problem on the kernel side is that the code just before the patch > context in drivers/gpu/drm/amd/amdgpu/atombios_encoders.c will become > a problem soon: > > if (fake_edid_record->ucFakeEDIDLength) { > struct edid *edid; > int edid_size = > max((int)EDID_LENGTH, (int)fake_edid_record->ucFakeEDIDLength); > edid = kmalloc(edid_size, GFP_KERNEL); > if (edid) { > memcpy((u8 *)edid, (u8 *)&fake_edid_record->ucFakeEDIDString[0], > fake_edid_record->ucFakeEDIDLength); > > if (drm_edid_is_valid(edid)) { > ... > > the memcpy() from "fake_edid_record->ucFakeEDIDString" will eventually > start to WARN, since the size of that field will be seen as a single byte > under -fstrict-flex-arrays. At this moment, no, there's neither source > bounds checking nor -fstrict-flex-arrays, but we're trying to clean up > everything we can find now so that we don't have to do this all again > later. :) > > -Kees > > P.S. Also this could be shorter with fewer casts: > > struct edid *edid; > u8 edid_size = > max_t(u8, EDID_LENGTH, fake_edid_record->ucFakeEDIDLength); > edid = kmemdup(fake_edid_record->ucFakeEDIDString, edid_size, GFP_KERNEL); > if (edid) { > if (drm_edid_is_valid(edid)) { > adev->mode_info.bios_hardcoded_edid = edid; > ... > > -- > Kees Cook
On Tue, Nov 01, 2022 at 06:09:16PM -0400, Alex Deucher wrote: > On Tue, Nov 1, 2022 at 5:54 PM Kees Cook <keescook@chromium.org> wrote: > > Does the ROM always only have a single byte there? This seems unlikely > > given the member "ucFakeEDIDLength" (and the code below). > > I'm not sure. I'm mostly concerned about this: > > record += fake_edid_record->ucFakeEDIDLength ? > fake_edid_record->ucFakeEDIDLength + 2 : > sizeof(ATOM_FAKE_EDID_PATCH_RECORD); But this is exactly what the code currently does, as noted in the commit log: "It's worth mentioning that doing a build before/after this patch results in no binary output differences. > Presumably the record should only exist if ucFakeEDIDLength is non 0, > but I don't know if there are some OEMs out there that just included > an empty record for some reason. Maybe the code is wrong today and > there are some OEMs that include it and the array is already size 0. > In that case, Paulo's original patches are probably more correct. Right, but if true, that seems to be a distinctly separate bug fix?
On Tue, Nov 1, 2022 at 6:41 PM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Nov 01, 2022 at 06:09:16PM -0400, Alex Deucher wrote: > > On Tue, Nov 1, 2022 at 5:54 PM Kees Cook <keescook@chromium.org> wrote: > > > Does the ROM always only have a single byte there? This seems unlikely > > > given the member "ucFakeEDIDLength" (and the code below). > > > > I'm not sure. I'm mostly concerned about this: > > > > record += fake_edid_record->ucFakeEDIDLength ? > > fake_edid_record->ucFakeEDIDLength + 2 : > > sizeof(ATOM_FAKE_EDID_PATCH_RECORD); > > But this is exactly what the code currently does, as noted in the commit > log: "It's worth mentioning that doing a build before/after this patch > results in no binary output differences. > > > Presumably the record should only exist if ucFakeEDIDLength is non 0, > > but I don't know if there are some OEMs out there that just included > > an empty record for some reason. Maybe the code is wrong today and > > there are some OEMs that include it and the array is already size 0. > > In that case, Paulo's original patches are probably more correct. > > Right, but if true, that seems to be a distinctly separate bug fix? You've convinced me. Applied. Thanks, Alex > > -- > Kees Cook
diff --git a/drivers/gpu/drm/radeon/atombios.h b/drivers/gpu/drm/radeon/atombios.h index da35a970fcc0..235e59b547a1 100644 --- a/drivers/gpu/drm/radeon/atombios.h +++ b/drivers/gpu/drm/radeon/atombios.h @@ -3615,7 +3615,7 @@ typedef struct _ATOM_FAKE_EDID_PATCH_RECORD { UCHAR ucRecordType; UCHAR ucFakeEDIDLength; - UCHAR ucFakeEDIDString[1]; // This actually has ucFakeEdidLength elements. + UCHAR ucFakeEDIDString[]; // This actually has ucFakeEdidLength elements. } ATOM_FAKE_EDID_PATCH_RECORD; typedef struct _ATOM_PANEL_RESOLUTION_PATCH_RECORD diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c index 204127bad89c..4ad5a328d920 100644 --- a/drivers/gpu/drm/radeon/radeon_atombios.c +++ b/drivers/gpu/drm/radeon/radeon_atombios.c @@ -1727,8 +1727,11 @@ struct radeon_encoder_atom_dig *radeon_atombios_get_lvds_info(struct } } record += fake_edid_record->ucFakeEDIDLength ? - fake_edid_record->ucFakeEDIDLength + 2 : - sizeof(ATOM_FAKE_EDID_PATCH_RECORD); + struct_size(fake_edid_record, + ucFakeEDIDString, + fake_edid_record->ucFakeEDIDLength) : + /* empty fake edid record must be 3 bytes long */ + sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1; break; case LCD_PANEL_RESOLUTION_RECORD_TYPE: panel_res_record = (ATOM_PANEL_RESOLUTION_PATCH_RECORD *)record;