Re: [PATCH V4 20/24] xen/arm: io: Harden sign extension check

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Oleksandr,

Oleksandr Tyshchenko writes:

> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> In the ideal world we would never get an undefined behavior when
> propagating the sign bit since that bit can only be set for access
> size smaller than the register size (i.e byte/half-word for aarch32,
> byte/half-word/word for aarch64).
>
> In the real world we need to care for *possible* hardware bug such as
> advertising a sign extension for either 64-bit (or 32-bit) on Arm64
> (resp. Arm32).
>
> So harden a bit more the code to prevent undefined behavior when
> propagating the sign bit in case of buggy hardware.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>

>
> ---
> Please note, this is a split/cleanup/hardening of Julien's PoC:
> "Add support for Guest IO forwarding to a device emulator"
>
> Changes V3 -> V4:
>    - new patch
> ---
>  xen/include/asm-arm/traps.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/include/asm-arm/traps.h b/xen/include/asm-arm/traps.h
> index e301c44..992d537 100644
> --- a/xen/include/asm-arm/traps.h
> +++ b/xen/include/asm-arm/traps.h
> @@ -93,7 +93,8 @@ static inline register_t sign_extend(const struct hsr_dabt 
> dabt, register_t r)
>       * Note that we expect the read handler to have zeroed the bits
>       * outside the requested access size.
>       */
> -    if ( dabt.sign && (r & (1UL << (size - 1))) )
> +    if ( dabt.sign && (size < sizeof(register_t) * 8) &&
> +         (r & (1UL << (size - 1))) )
>      {
>          /*
>           * We are relying on register_t using the same as


-- 
Volodymyr Babchuk at EPAM