| Message ID | ZGQn63U4IeRUiJWb@work |
|---|---|
| State | New |
| Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org>
Delivered-To: ouuuleilei@gmail.com
Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp805837vqo;
Tue, 16 May 2023 18:20:03 -0700 (PDT)
X-Google-Smtp-Source:
ACHHUZ5T6C5nWbc61ucnYFDIaYbQ1IQ/OuCBndygqgDvipaZSrjuTu4Z2hJimIJQWRO3IaxHDmvi
X-Received: by 2002:a17:90b:3910:b0:250:2ad4:b459 with SMTP id
ob16-20020a17090b391000b002502ad4b459mr38969921pjb.39.1684286403534;
Tue, 16 May 2023 18:20:03 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; t=1684286403; cv=none;
d=google.com; s=arc-20160816;
b=ayfI5SL2CFZyGPi8Xdqq2OxzY3dN3EZDTcJR856DG8YZzR8eYxHLGxZuoPGNJuaXjt
WVgZF8NWaat9kyheR35B3Hotc2t+vuy7VwoZOsbKqCsF+MFtunA/3mVs1cRKdqCG/LAA
2Lx39NcGEZCIw282fhajt+c6/hDwH02GROXyUBaurMMT43OUtGM4TWxqOKjvTyPFo8Na
jcaMFxR5FxvS2sV6dMFSLakr4zXmbVQhdsoo0Lv+grXeG+4LcE85F/jsCLQdqXAFnhlz
O5LvcazMQ2cucgK4ZyMelqMt7ZNPdtzBaefSlU/zvrkmtgRFvMEw9FzPWoDjrUMeC1lI
DpxQ==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
s=arc-20160816;
h=list-id:precedence:content-disposition:mime-version:message-id
:subject:cc:to:from:date:dkim-signature;
bh=ZT2m+RF+EoOabei0GWfmGOphfjLHKNHheTVmtKx0k4c=;
b=Qu6zIlvQbAegmzesYQrwmVRrG6koKhQMO77dxpfzPdOjyQ09kNI400mUFHios+V6z1
oUBDQaWb7heu4wSUxmDxSJ7p17yaVDGn0Pigmh4diBCJ7XRaJnvObZLf3fVvCDbr+aMs
Q4CRHD3F0LLjq46FqkFKXXO6HOyuP25+LMcUpeE1UWoTui1lDW35koqFzYACP/zIsECr
7Kq1dWc6vdKlVCedbZ14BrGkKhpoF4o3nWM5YRWH+VdtRp7bJKQEcGuPcjNS/skQ5hLx
9jPbyKVCr0Hg2Wlv46ZAb7Bee2cA4kVH2azE4h1Mt6RK3YDLi6m7s8pGD6dcXYtZCgYf
FiNg==
ARC-Authentication-Results: i=1; mx.google.com;
dkim=pass header.i=@kernel.org header.s=k20201202 header.b=AMP3rdH1;
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=kernel.org
Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20])
by mx.google.com with ESMTP id
y17-20020a17090aa41100b0023fb875a929si495909pjp.106.2023.05.16.18.19.51;
Tue, 16 May 2023 18:20:03 -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=@kernel.org header.s=k20201202 header.b=AMP3rdH1;
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=kernel.org
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S230494AbjEQBC4 (ORCPT <rfc822;peekingduck44@gmail.com>
+ 99 others); Tue, 16 May 2023 21:02:56 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60090 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S229635AbjEQBCx (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Tue, 16 May 2023 21:02:53 -0400
Received: from dfw.source.kernel.org (dfw.source.kernel.org
[IPv6:2604:1380:4641:c500::1])
by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4557E1BFD;
Tue, 16 May 2023 18:02:52 -0700 (PDT)
Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by dfw.source.kernel.org (Postfix) with ESMTPS id DB20264073;
Wed, 17 May 2023 01:02:51 +0000 (UTC)
Received: by smtp.kernel.org (Postfix) with ESMTPSA id EF893C433EF;
Wed, 17 May 2023 01:02:49 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;
s=k20201202; t=1684285371;
bh=nVMhlmmUXfL1NdxdVCdqZzrzgKQ9uLWQHIfd1j3SZzA=;
h=Date:From:To:Cc:Subject:From;
b=AMP3rdH1zjSserQL0c9yAZPalpvwfhgz/4bMBrVqJE2dS6Ln6f/w24UpmFrinkwaG
MOW9rbW8RAsq4t6F+zAId0LrfuI/Fc3EcDNvhMYPjiu+RcI0rbEmkdy7fqZiSRsx6I
TQp1QqVq6YAKat7pbiwEp0AvhX2B6xZUB5+9pe2jiQYe+NCJKjK2XhGGpakPwJ2qbr
GJQnJRk5u78PDtRIVhh4P8Z0wXTNe37uc5cn8rZsoytD27gs7/ijuQz21G530mkx1u
Pd5HjfJJVLAdUxX8a1guhBCKCoeMRuxccDt5LheXXoWdLYXVvR64bnUGEK8QVO2VQQ
5ydG6NC7cR25g==
Date: Tue, 16 May 2023 19:03:39 -0600
From: "Gustavo A. R. Silva" <gustavoars@kernel.org>
To: Stanimir Varbanov <stanimir.k.varbanov@gmail.com>,
Vikash Garodia <quic_vgarodia@quicinc.com>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <andersson@kernel.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-kernel@vger.kernel.org,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
linux-hardening@vger.kernel.org
Subject: [PATCH][next] media: venus: hfi_cmds: Replace fake flex-array with
flexible-array member
Message-ID: <ZGQn63U4IeRUiJWb@work>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
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?1766102299698739567?=
X-GMAIL-MSGID: =?utf-8?q?1766102299698739567?=
|
| Series |
[next] media: venus: hfi_cmds: Replace fake flex-array with flexible-array member
|
|
Commit Message
Gustavo A. R. Silva
May 17, 2023, 1:03 a.m. UTC
One-element arrays are deprecated, and we are replacing them with flexible
array members instead. So, replace one-element arrays with flexible-array
members in struct hfi_sys_set_resource_pkt, and refactor the rest of
the code, accordingly.
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].
The only binary differences seen before/after changes are the
following:
17ba: mov %rbx,%rdi
17bd: call 17c2 <pkt_sys_set_resource+0x42>
17be: R_X86_64_PLT32 __tsan_write4-0x4
- 17c2: movl $0x14,(%rbx)
+ 17c2: movl $0x10,(%rbx)
17c8: lea 0x4(%rbx),%rdi
17cc: call 17d1 <pkt_sys_set_resource+0x51>
17cd: R_X86_64_PLT32 __tsan_write4-0x4
which is expected once this accounts for the following line of code
at drivers/media/platform/qcom/venus/hfi_cmds.c:73
73 pkt->hdr.size = sizeof(*pkt);
and as *pkt is of type struct hfi_sys_set_resource_pkt, sizeof(*pkt) is
reduced by 4 bytes, due to the flex-array transformation.
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/293
Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
drivers/media/platform/qcom/venus/hfi_cmds.c | 2 +-
drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
Comments
On 17.05.2023 03:03, Gustavo A. R. Silva wrote: > One-element arrays are deprecated, and we are replacing them with flexible > array members instead. So, replace one-element arrays with flexible-array > members in struct hfi_sys_set_resource_pkt, and refactor the rest of > the code, accordingly. > > 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]. > > The only binary differences seen before/after changes are the > following: > > 17ba: mov %rbx,%rdi > 17bd: call 17c2 <pkt_sys_set_resource+0x42> > 17be: R_X86_64_PLT32 __tsan_write4-0x4 > - 17c2: movl $0x14,(%rbx) > + 17c2: movl $0x10,(%rbx) > 17c8: lea 0x4(%rbx),%rdi > 17cc: call 17d1 <pkt_sys_set_resource+0x51> > 17cd: R_X86_64_PLT32 __tsan_write4-0x4 > > which is expected once this accounts for the following line of code > at drivers/media/platform/qcom/venus/hfi_cmds.c:73 > > 73 pkt->hdr.size = sizeof(*pkt); > > and as *pkt is of type struct hfi_sys_set_resource_pkt, sizeof(*pkt) is > reduced by 4 bytes, due to the flex-array transformation. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/293 > Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1] > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > drivers/media/platform/qcom/venus/hfi_cmds.c | 2 +- > drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c > index 3f74d518ad08..7c82e212434e 100644 > --- a/drivers/media/platform/qcom/venus/hfi_cmds.c > +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c > @@ -83,7 +83,7 @@ int pkt_sys_set_resource(struct hfi_sys_set_resource_pkt *pkt, u32 id, u32 size, > res->size = size; > res->mem = addr; > pkt->resource_type = HFI_RESOURCE_OCMEM; > - pkt->hdr.size += sizeof(*res) - sizeof(u32); > + pkt->hdr.size += sizeof(*res); > break; > } > case VIDC_RESOURCE_NONE: > diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h > index ba74d03eb9cd..dd9c5066442d 100644 > --- a/drivers/media/platform/qcom/venus/hfi_cmds.h > +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h > @@ -56,7 +56,7 @@ struct hfi_sys_set_resource_pkt { > struct hfi_pkt_hdr hdr; > u32 resource_handle; > u32 resource_type; > - u32 resource_data[1]; > + u32 resource_data[]; Would making this an u32* be a better resolution? Konrad > }; > > struct hfi_sys_release_resource_pkt {
On 17.05.2023 04:11, Konrad Dybcio wrote: > > > On 17.05.2023 03:03, Gustavo A. R. Silva wrote: >> One-element arrays are deprecated, and we are replacing them with flexible >> array members instead. So, replace one-element arrays with flexible-array >> members in struct hfi_sys_set_resource_pkt, and refactor the rest of >> the code, accordingly. >> >> 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]. >> >> The only binary differences seen before/after changes are the >> following: >> >> 17ba: mov %rbx,%rdi >> 17bd: call 17c2 <pkt_sys_set_resource+0x42> >> 17be: R_X86_64_PLT32 __tsan_write4-0x4 >> - 17c2: movl $0x14,(%rbx) >> + 17c2: movl $0x10,(%rbx) >> 17c8: lea 0x4(%rbx),%rdi >> 17cc: call 17d1 <pkt_sys_set_resource+0x51> >> 17cd: R_X86_64_PLT32 __tsan_write4-0x4 >> >> which is expected once this accounts for the following line of code >> at drivers/media/platform/qcom/venus/hfi_cmds.c:73 >> >> 73 pkt->hdr.size = sizeof(*pkt); >> >> and as *pkt is of type struct hfi_sys_set_resource_pkt, sizeof(*pkt) is >> reduced by 4 bytes, due to the flex-array transformation. >> >> Link: https://github.com/KSPP/linux/issues/79 >> Link: https://github.com/KSPP/linux/issues/293 >> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1] >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> --- >> drivers/media/platform/qcom/venus/hfi_cmds.c | 2 +- >> drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c >> index 3f74d518ad08..7c82e212434e 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c >> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c >> @@ -83,7 +83,7 @@ int pkt_sys_set_resource(struct hfi_sys_set_resource_pkt *pkt, u32 id, u32 size, >> res->size = size; >> res->mem = addr; >> pkt->resource_type = HFI_RESOURCE_OCMEM; >> - pkt->hdr.size += sizeof(*res) - sizeof(u32); >> + pkt->hdr.size += sizeof(*res); >> break; >> } >> case VIDC_RESOURCE_NONE: >> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h >> index ba74d03eb9cd..dd9c5066442d 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_cmds.h >> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h >> @@ -56,7 +56,7 @@ struct hfi_sys_set_resource_pkt { >> struct hfi_pkt_hdr hdr; >> u32 resource_handle; >> u32 resource_type; >> - u32 resource_data[1]; >> + u32 resource_data[]; > Would making this an u32* be a better resolution? Nevermind, I overthought this by thinking in the terms of its size and not the data within the struct... Maybe struct_size could be used instead of subtracting sizeof(u32) though? Konrad > > Konrad >> }; >> >> struct hfi_sys_release_resource_pkt {
On Tue, May 16, 2023 at 07:03:39PM -0600, Gustavo A. R. Silva wrote: > One-element arrays are deprecated, and we are replacing them with flexible > array members instead. So, replace one-element arrays with flexible-array > members in struct hfi_sys_set_resource_pkt, and refactor the rest of > the code, accordingly. > > 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]. > > The only binary differences seen before/after changes are the > following: > > 17ba: mov %rbx,%rdi > 17bd: call 17c2 <pkt_sys_set_resource+0x42> > 17be: R_X86_64_PLT32 __tsan_write4-0x4 > - 17c2: movl $0x14,(%rbx) > + 17c2: movl $0x10,(%rbx) > 17c8: lea 0x4(%rbx),%rdi > 17cc: call 17d1 <pkt_sys_set_resource+0x51> > 17cd: R_X86_64_PLT32 __tsan_write4-0x4 > > which is expected once this accounts for the following line of code > at drivers/media/platform/qcom/venus/hfi_cmds.c:73 > > 73 pkt->hdr.size = sizeof(*pkt); > > and as *pkt is of type struct hfi_sys_set_resource_pkt, sizeof(*pkt) is > reduced by 4 bytes, due to the flex-array transformation. Based on the other place that was subtracting the 1 element, this looks like hfi_cmds.c:73 is an existing sizing bug that is now fixed with this patch, yes? Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/293 > Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1] > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > drivers/media/platform/qcom/venus/hfi_cmds.c | 2 +- > drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c > index 3f74d518ad08..7c82e212434e 100644 > --- a/drivers/media/platform/qcom/venus/hfi_cmds.c > +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c > @@ -83,7 +83,7 @@ int pkt_sys_set_resource(struct hfi_sys_set_resource_pkt *pkt, u32 id, u32 size, > res->size = size; > res->mem = addr; > pkt->resource_type = HFI_RESOURCE_OCMEM; > - pkt->hdr.size += sizeof(*res) - sizeof(u32); > + pkt->hdr.size += sizeof(*res); > break; > } > case VIDC_RESOURCE_NONE: > diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h > index ba74d03eb9cd..dd9c5066442d 100644 > --- a/drivers/media/platform/qcom/venus/hfi_cmds.h > +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h > @@ -56,7 +56,7 @@ struct hfi_sys_set_resource_pkt { > struct hfi_pkt_hdr hdr; > u32 resource_handle; > u32 resource_type; > - u32 resource_data[1]; > + u32 resource_data[]; > }; > > struct hfi_sys_release_resource_pkt { > -- > 2.34.1 >
On Wed, May 17, 2023 at 10:50:53AM -0700, Kees Cook wrote: > On Tue, May 16, 2023 at 07:03:39PM -0600, Gustavo A. R. Silva wrote: > > One-element arrays are deprecated, and we are replacing them with flexible > > array members instead. So, replace one-element arrays with flexible-array > > members in struct hfi_sys_set_resource_pkt, and refactor the rest of > > the code, accordingly. > > > > 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]. > > > > The only binary differences seen before/after changes are the > > following: > > > > 17ba: mov %rbx,%rdi > > 17bd: call 17c2 <pkt_sys_set_resource+0x42> > > 17be: R_X86_64_PLT32 __tsan_write4-0x4 > > - 17c2: movl $0x14,(%rbx) > > + 17c2: movl $0x10,(%rbx) > > 17c8: lea 0x4(%rbx),%rdi > > 17cc: call 17d1 <pkt_sys_set_resource+0x51> > > 17cd: R_X86_64_PLT32 __tsan_write4-0x4 > > > > which is expected once this accounts for the following line of code > > at drivers/media/platform/qcom/venus/hfi_cmds.c:73 > > > > 73 pkt->hdr.size = sizeof(*pkt); > > > > and as *pkt is of type struct hfi_sys_set_resource_pkt, sizeof(*pkt) is > > reduced by 4 bytes, due to the flex-array transformation. > > Based on the other place that was subtracting the 1 element, this looks Do you mean the one you commented on yesterday? https://lore.kernel.org/linux-hardening/ZGPk3PpvYzjD1+0%2F@work/ this? -- Gustavo > like hfi_cmds.c:73 is an existing sizing bug that is now fixed with this > patch, yes? > > Reviewed-by: Kees Cook <keescook@chromium.org> > > -Kees > > > > > Link: https://github.com/KSPP/linux/issues/79 > > Link: https://github.com/KSPP/linux/issues/293 > > Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1] > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > > --- > > drivers/media/platform/qcom/venus/hfi_cmds.c | 2 +- > > drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c > > index 3f74d518ad08..7c82e212434e 100644 > > --- a/drivers/media/platform/qcom/venus/hfi_cmds.c > > +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c > > @@ -83,7 +83,7 @@ int pkt_sys_set_resource(struct hfi_sys_set_resource_pkt *pkt, u32 id, u32 size, > > res->size = size; > > res->mem = addr; > > pkt->resource_type = HFI_RESOURCE_OCMEM; > > - pkt->hdr.size += sizeof(*res) - sizeof(u32); > > + pkt->hdr.size += sizeof(*res); > > break; > > } > > case VIDC_RESOURCE_NONE: > > diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h > > index ba74d03eb9cd..dd9c5066442d 100644 > > --- a/drivers/media/platform/qcom/venus/hfi_cmds.h > > +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h > > @@ -56,7 +56,7 @@ struct hfi_sys_set_resource_pkt { > > struct hfi_pkt_hdr hdr; > > u32 resource_handle; > > u32 resource_type; > > - u32 resource_data[1]; > > + u32 resource_data[]; > > }; > > > > struct hfi_sys_release_resource_pkt { > > -- > > 2.34.1 > > > > -- > Kees Cook
On 5/17/2023 11:20 PM, Kees Cook wrote: > On Tue, May 16, 2023 at 07:03:39PM -0600, Gustavo A. R. Silva wrote: >> One-element arrays are deprecated, and we are replacing them with flexible >> array members instead. So, replace one-element arrays with flexible-array >> members in struct hfi_sys_set_resource_pkt, and refactor the rest of >> the code, accordingly. >> >> 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]. >> >> The only binary differences seen before/after changes are the >> following: >> >> 17ba: mov %rbx,%rdi >> 17bd: call 17c2 <pkt_sys_set_resource+0x42> >> 17be: R_X86_64_PLT32 __tsan_write4-0x4 >> - 17c2: movl $0x14,(%rbx) >> + 17c2: movl $0x10,(%rbx) >> 17c8: lea 0x4(%rbx),%rdi >> 17cc: call 17d1 <pkt_sys_set_resource+0x51> >> 17cd: R_X86_64_PLT32 __tsan_write4-0x4 >> >> which is expected once this accounts for the following line of code >> at drivers/media/platform/qcom/venus/hfi_cmds.c:73 >> >> 73 pkt->hdr.size = sizeof(*pkt); >> >> and as *pkt is of type struct hfi_sys_set_resource_pkt, sizeof(*pkt) is >> reduced by 4 bytes, due to the flex-array transformation. > > Based on the other place that was subtracting the 1 element, this looks > like hfi_cmds.c:73 is an existing sizing bug that is now fixed with this > patch, yes? It is not a bug. The way it is structured is like [packet header + payload]. The packet header size should be (size of header + payload). Line 73 calculates the size of packet header, while line 86 calculates size of payload. Since payload starts from the last u32, the total size needs to be reduced by size of u32. > Reviewed-by: Kees Cook <keescook@chromium.org> > > -Kees > >> >> Link: https://github.com/KSPP/linux/issues/79 >> Link: https://github.com/KSPP/linux/issues/293 >> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1] >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> The patch looks good. As stated in previous patch, lets combine this into a single series. >> --- >> drivers/media/platform/qcom/venus/hfi_cmds.c | 2 +- >> drivers/media/platform/qcom/venus/hfi_cmds.h | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c >> index 3f74d518ad08..7c82e212434e 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c >> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c >> @@ -83,7 +83,7 @@ int pkt_sys_set_resource(struct hfi_sys_set_resource_pkt *pkt, u32 id, u32 size, >> res->size = size; >> res->mem = addr; >> pkt->resource_type = HFI_RESOURCE_OCMEM; >> - pkt->hdr.size += sizeof(*res) - sizeof(u32); >> + pkt->hdr.size += sizeof(*res); >> break; >> } >> case VIDC_RESOURCE_NONE: >> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h >> index ba74d03eb9cd..dd9c5066442d 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_cmds.h >> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h >> @@ -56,7 +56,7 @@ struct hfi_sys_set_resource_pkt { >> struct hfi_pkt_hdr hdr; >> u32 resource_handle; >> u32 resource_type; >> - u32 resource_data[1]; >> + u32 resource_data[]; >> }; >> >> struct hfi_sys_release_resource_pkt { >> -- >> 2.34.1 >> >
diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c index 3f74d518ad08..7c82e212434e 100644 --- a/drivers/media/platform/qcom/venus/hfi_cmds.c +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c @@ -83,7 +83,7 @@ int pkt_sys_set_resource(struct hfi_sys_set_resource_pkt *pkt, u32 id, u32 size, res->size = size; res->mem = addr; pkt->resource_type = HFI_RESOURCE_OCMEM; - pkt->hdr.size += sizeof(*res) - sizeof(u32); + pkt->hdr.size += sizeof(*res); break; } case VIDC_RESOURCE_NONE: diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.h b/drivers/media/platform/qcom/venus/hfi_cmds.h index ba74d03eb9cd..dd9c5066442d 100644 --- a/drivers/media/platform/qcom/venus/hfi_cmds.h +++ b/drivers/media/platform/qcom/venus/hfi_cmds.h @@ -56,7 +56,7 @@ struct hfi_sys_set_resource_pkt { struct hfi_pkt_hdr hdr; u32 resource_handle; u32 resource_type; - u32 resource_data[1]; + u32 resource_data[]; }; struct hfi_sys_release_resource_pkt {