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:49, Jan Beulich wrote:
> On 18.07.2022 11:31, Andrew Cooper wrote:
>> On 18/07/2022 10:11, Jan Beulich wrote:
>>> On 15.07.2022 15:26, Andrew Cooper wrote:
>>>> --- 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.
> Hmm, the wording then indeed is ambiguous.

I see your point.  In this case it's meant as "are able to", but this is
still clearer than using "can't" because at least the text matches the
check which triggered it.

>  I read "can" as "are allowed
> to", when we mean "aren't allowed to". Maybe ".text is 256M or more in
> size"? If you mention "offsets", then I think the check should be based
> on actually observing an offset which is too large (which .text size
> alone doesn't guarantee will happen).

It's not just .text on its own because the VMA of offset by 2M, hence
the subtraction of $vma_lo in the main calculation.

There's no point searching for offsets.  There will be one near the end,
so all searching for an offset would do is complicate the critical loop.

How about ".text offsets must not exceed 256M" ?

That should be unambiguous.

~Andrew