Re: [PATCH v3 0/6] Replace one-element arrays with flexible-array members

From: Kees Cook
Date: Tue Aug 16 2022 - 15:22:21 EST

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 <megasas_aen_polling+0x56>
> - 6312: R_X86_64_PC32 .text.unlikely+0x3ae3
> + 6312: R_X86_64_PC32 .text.unlikely+0x3ae0
> 6316: mov 0x0(%rip),%eax # 631c <megasas_aen_polling+0x5c>
> 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 <megasas_aen_polling+0x6c>
> - 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 <megasas_aen_polling+0x7c>
> - 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 <keescook@xxxxxxxxxxxx>

> 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