Re: [PATCH v2 2/2] xen: Fix latent check-endbr.sh bug with 32bit build environments

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 18/07/2022 10:11, Jan Beulich wrote:
> On 15.07.2022 15:26, Andrew Cooper wrote:
>> While Xen's current VMA means it works, the mawk fix (i.e. using $((0xN)) in
>> the shell) isn't portable in 32bit shells.  See the code comment for the fix.
>>
>> The fix found a second latent bug.  Recombining $vma_hi/lo should have used
>> printf "%s%08x" and only worked previously because $vma_lo had bits set in
>> it's top nibble.  Combining with the main fix, %08x becomes %07x.
>>
>> Fixes: $XXX patch 1
>> Reported-by: Jan Beulich <JBeulich@xxxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks, but...

> with, I guess, ...
>
>> --- a/xen/tools/check-endbr.sh
>> +++ b/xen/tools/check-endbr.sh
>> @@ -61,19 +61,36 @@ ${OBJDUMP} -j .text $1 -d -w | grep '    endbr64 *$' | 
>> cut -f 1 -d ':' > $VALID &
>>  #    the lower bits, rounding integers to the nearest 4k.
>>  #
>>  #    Instead, use the fact that Xen's .text is within a 1G aligned region, 
>> and
>> -#    split the VMA in half so AWK's numeric addition is only working on 32 
>> bit
>> -#    numbers, which don't lose precision.
>> +#    split the VMA so AWK's numeric addition is only working on <32 bit
>> +#    numbers, which don't lose precision.  (See point 5)
>>  #
>>  # 4) MAWK doesn't support plain hex constants (an optional part of the POSIX
>>  #    spec), and GAWK and MAWK can't agree on how to work with hex constants 
>> in
>>  #    a string.  Use the shell to convert $vma_lo to decimal before passing 
>> to
>>  #    AWK.
>>  #
>> +# 5) Point 4 isn't fully portable.  POSIX only requires that $((0xN)) be
>> +#    evaluated as long, which in 32bit shells turns negative if bit 31 of 
>> the
>> +#    VMA is set.  AWK then interprets this negative number as a double 
>> before
>> +#    adding the offsets from the binary grep.
>> +#
>> +#    Instead of doing an 8/8 split with vma_hi/lo, do a 9/7 split.
>> +#
>> +#    The consequence of this is that for all offsets, $vma_lo + offset needs
>> +#    to be less that 256M (i.e. 7 nibbles) so as to be successfully 
>> recombined
>> +#    with the 9 nibbles of $vma_hi.  This is fine; .text is at the start of 
>> a
>> +#    1G aligned region, and Xen is far far smaller than 256M, but leave 
>> safety
>> +#    check nevertheless.
>> +#
>>  eval $(${OBJDUMP} -j .text $1 -h |
>> -    $AWK '$2 == ".text" {printf "vma_hi=%s\nvma_lo=%s\n", substr($4, 1, 8), 
>> substr($4, 9, 16)}')
>> +    $AWK '$2 == ".text" {printf "vma_hi=%s\nvma_lo=%s\n", substr($4, 1, 9), 
>> substr($4, 10, 16)}')
>>  
>>  ${OBJCOPY} -j .text $1 -O binary $TEXT_BIN
>>  
>> +bin_sz=$(stat -c '%s' $TEXT_BIN)
>> +[ "$bin_sz" -ge $(((1 << 28) - $vma_lo)) ] &&
>> +    { echo "$MSG_PFX Error: .text offsets can exceed 256M" >&2; exit 1; }
> ... s/can/cannot/ ?

Why?  "Can" is correct here.  If the offsets can't exceed 256M, then
everything is good.

~Andrew