From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B9108C32772 for ; Tue, 16 Aug 2022 19:22:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237101AbiHPTWT (ORCPT ); Tue, 16 Aug 2022 15:22:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41734 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237092AbiHPTWS (ORCPT ); Tue, 16 Aug 2022 15:22:18 -0400 Received: from mail-pg1-x52b.google.com (mail-pg1-x52b.google.com [IPv6:2607:f8b0:4864:20::52b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F1E0D844D4 for ; Tue, 16 Aug 2022 12:22:15 -0700 (PDT) Received: by mail-pg1-x52b.google.com with SMTP id 202so10080053pgc.8 for ; Tue, 16 Aug 2022 12:22:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc; bh=UfXlr/WbEItN9aSdIJX4iaTq/XlXVZMlLqhc7nhIv50=; b=LbBNhh6bXMzkijgu81RzGbZKVTzjiDsuC90Y7Vv43r/5OmxsedMbMtkBNzDnIgXj0n t9Pqj6ZzUoo0cfUx1TtUhRZsYY2okOkn2/FfdlGNySntuZr4gXxY1hJz7HS5327Hs1xN nHUYO+K+b3YcCguUk0nqTPOJ2LUjJc++72OZU= 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:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc; bh=UfXlr/WbEItN9aSdIJX4iaTq/XlXVZMlLqhc7nhIv50=; b=Svk0CKpRlO4s4v63jQzQwzhGvDY3RFhkCVY1FDe43JNdCV7ERJPWSquM81MyTguDdp WaLZt1qagwPfZZtKIkowHfTLmUy/3HOyWePHGYvQylmmottwYWRBIvj73nxfMWyQr7/R 4ZCjd7DEmuFQBVjzn8XkhzAWjX1O+1r3jP1/uXWzbNCCpLwzRJM22GnqyTDqcLc1M/XP +un7/InEXt+lrBFXr7LepyiXfLpLLi/Hbw1Am9w32Lv6fyHrOPHO6fi9DaltNmfSd7Id eUQhOI7ApjMOMJzYdA6VpZj0tdFDwsapGvmoJAK2ZJ3cshSYpKok2W81mBAxxvLt9dFZ KbwA== X-Gm-Message-State: ACgBeo178zxQ3yPAs+lvasxegLWdAyXnzRq4PSuW7kWEUSnzyjBDWswJ MVsRfS9VfuGlZVhrucJobHT8Mw== X-Google-Smtp-Source: AA6agR4ewDb5KpxqmFL9TANETOsh2CoRoqmxiztBenGS5hMalR/nn6+Vm6Yl9PEjQZ4t1VIf1KGtTw== X-Received: by 2002:a63:1a53:0:b0:41f:5298:9b5f with SMTP id a19-20020a631a53000000b0041f52989b5fmr18443389pgm.244.1660677735372; Tue, 16 Aug 2022 12:22:15 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id m13-20020a170902db0d00b0016c46ff9741sm9511951plx.67.2022.08.16.12.22.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Aug 2022 12:22:14 -0700 (PDT) Date: Tue, 16 Aug 2022 12:22:13 -0700 From: Kees Cook To: "Gustavo A. R. Silva" Cc: megaraidlinux.pdl@broadcom.com, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, Kashyap Desai , Sumit Saxena , Shivasharan S , "James E.J. Bottomley" , "Martin K. Petersen" , linux-hardening@vger.kernel.org Subject: Re: [PATCH v3 0/6] Replace one-element arrays with flexible-array members Message-ID: <202208161220.D225B26C9@keescook> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org On Mon, Aug 15, 2022 at 04:35:19PM -0500, Gustavo A. R. Silva wrote: > Hi! > > This series aims to replace one-element arrays with flexible-array > members in drivers/scsi/megaraid/ > > I followed the below steps in order to verify the changes don't > significally impact the code (.text) section generated by the compiler, > for each object file involved: > > 1. Prepare the build with the following settings and configurations: > > linux$ KBF="KBUILD_BUILD_TIMESTAMP=1970-01-01 KBUILD_BUILD_USER=user > KBUILD_BUILD_HOST=host KBUILD_BUILD_VERSION=1" > linux$ make $KBF allyesconfig > linux$ ./scripts/config -d GCOV_KERNEL -d KCOV -d GCC_PLUGINS \ > -d IKHEADERS -d KASAN -d UBSAN \ > -d DEBUG_INFO_NONE \ > -e DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > linux$ make $KBF olddefconfig > > 2. Build drivers/scsi/megaraid/ with the same settings and configurations > as in Step 1, and copy the generated object files in directory before/ > > linux$ make -j128 $KBF drivers/scsi/megaraid/ > linux$ mkdir -p before > linux$ cp drivers/scsi/megaraid/*.o before/ > > 3. Implement all the needed changes and create the patch series. In this > case, six patches. > > linux$ vi code.c > ...do the magic :) > linux$ git format-patch ...all the rest > > 4. Apply a patch at a time (of the previously created series) and, after > applying EACH patch, build (as in Step 2) drivers/scsi/megaraid/ and > copy the generated object files in directory after/ > > 5. Compare the code section (.text) of each before/file.o and > after/file.o. I use the following bash script: > > compare.sh: > ARGS="--disassemble --demangle --reloc --no-show-raw-insn --section=.text" > for i in $(cd before && echo *.o); do > echo $i > diff -u <(objdump $ARGS before/$i | sed "0,/^Disassembly/d") \ > <(objdump $ARGS after/$i | sed "0,/^Disassembly/d") > done > > linux$ ./compare.sh > code_comparison.diff > > 6. Open the code_comparison.diff file from the example above, look for > any differences that might show up and analyze them in order to > determine their impact, and what (if something) should be changed > or study further. > > The above process (code section comparison of object files) is based on > this[0] blog post by Kees Cook. The compiler used to build the code was > GCC-12. > > In this series I only found the following sorts of differences in files > megaraid_sas.o and megaraid_sas_base.o: > > ... > ...@@ -7094,24 +7094,24 @@ > 6302: movq $0x0,0x1e20(%rbx) > 630d: test %r15,%r15 > 6310: je 6316 > - 6312: R_X86_64_PC32 .text.unlikely+0x3ae3 > + 6312: R_X86_64_PC32 .text.unlikely+0x3ae0 > 6316: mov 0x0(%rip),%eax # 631c > 6318: R_X86_64_PC32 event_log_level-0x4 > 631c: mov 0xc(%r15),%r14d > 6320: lea 0x2(%rax),%edx > 6323: cmp $0x6,%edx > 6326: ja 632c > - 6328: R_X86_64_PC32 .text.unlikely+0x3ac3 > + 6328: R_X86_64_PC32 .text.unlikely+0x3ac0 > 632c: mov %r14d,%edx > 632f: sar $0x18,%edx > 6332: mov %edx,%ecx > 6334: cmp %eax,%edx > 6336: jge 633c > - 6338: R_X86_64_PC32 .text.unlikely+0x399c > + 6338: R_X86_64_PC32 .text.unlikely+0x3999 > ... > > All of them have to do with the relocation of symbols in the > .text.unlikely subsection and they don't seem to be of any actual > relevance. So, we can safely ignore them. If there's a revision of this series, it might make sense to explicitly state "no binary code differences" for these changes. (The location of the relocations don't matter, as you say.) Reviewed-by: Kees Cook > Also, notice there is an open issue in bugzilla.kernel.org [1] that's > seems could be fixed by this series. :) > > 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 [2]. > > Link: https://en.wikipedia.org/wiki/Flexible_array_member > Link: https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/109 > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215943 [1] > Link: https://reviews.llvm.org/D126864 [2] > > Thanks > > [0] https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/ > > Changes in v3: > - Split the struct_size() changes into a couple of separate patches. > - Use objdump to compare the code (.text) sections of the object > files before and after the changes. > - Modify MR_FW_RAID_MAP_ALL and MR_DRV_RAID_MAP_ALL structures. Change > suggested by Kees Cook. > > Changes in v2: > - Revert changes in struct MR_FW_RAID_MAP_ALL. > > Gustavo A. R. Silva (6): > scsi: megaraid_sas: Replace one-element array with flexible-array > member in MR_FW_RAID_MAP > scsi: megaraid_sas: Replace one-element array with flexible-array > member in MR_FW_RAID_MAP_DYNAMIC > scsi: megaraid_sas: Replace one-element array with flexible-array > member in MR_DRV_RAID_MAP > scsi: megaraid_sas: Replace one-element array with flexible-array > member in MR_PD_CFG_SEQ_NUM_SYNC > scsi: megaraid_sas: Use struct_size() in code related to struct > MR_FW_RAID_MAP > scsi: megaraid_sas: Use struct_size() in code related to struct > MR_PD_CFG_SEQ_NUM_SYNC > > drivers/scsi/megaraid/megaraid_sas_base.c | 20 ++++++++++---------- > drivers/scsi/megaraid/megaraid_sas_fp.c | 6 +++--- > drivers/scsi/megaraid/megaraid_sas_fusion.c | 2 +- > drivers/scsi/megaraid/megaraid_sas_fusion.h | 12 ++++++------ > 4 files changed, 20 insertions(+), 20 deletions(-) > > -- > 2.34.1 > -- Kees Cook