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

[Kees Cook <keescook@chromium.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 <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@chromium.org>

> 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