Re: [PATCH] target/arm: Avoid over-length shift in arm_cpu_sve_finalize() error case

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 4/7/23 17:43, Peter Maydell wrote:
> If you build QEMU with the clang sanitizer enabled, you can see it
> fire when running the arm-cpu-features test:
> 
> $ QTEST_QEMU_BINARY=./build/arm-clang/qemu-system-aarch64 ./build/arm-clang/tests/qtest/arm-cpu-features
> [...]
> ../../target/arm/cpu64.c:125:19: runtime error: shift exponent 64 is too large for 64-bit type 'unsigned long long'
> [...]
> 
> This happens because the user can specify some incorrect SVE
> properties that result in our calculating a max_vq of 0.  We catch
> this and error out, but before we do that we calculate
> 
>   vq_mask = MAKE_64BIT_MASK(0, max_vq);$
> 
> and the MAKE_64BIT_MASK() call is only valid for lengths that are
> greater than zero, so we hit the undefined behaviour.

Can we fix it generically?

-- >8 --
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -28,3 +28,3 @@
  #define MAKE_64BIT_MASK(shift, length) \
-    (((~0ULL) >> (64 - (length))) << (shift))
+    ((length) ? (((~0ULL) >> (64 - (length))) << (shift)) : 0)

---

> 
> Change the logic so that if max_vq is 0 we specifically set vq_mask
> to 0 without going via MAKE_64BIT_MASK().  This lets us drop the
> max_vq check from the error-exit logic, because if max_vq is 0 then
> vq_map must now be 0.
> 
> The UB only happens in the case where the user passed us an incorrect
> set of SVE properties, so it's not a big problem in practice.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/cpu64.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 6eaf8e32cfa..6012e4ef549 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -122,10 +122,10 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>           vq = ctz32(tmp) + 1;
>   
>           max_vq = vq <= ARM_MAX_VQ ? vq - 1 : ARM_MAX_VQ;
> -        vq_mask = MAKE_64BIT_MASK(0, max_vq);
> +        vq_mask = max_vq > 0 ? MAKE_64BIT_MASK(0, max_vq) : 0;
>           vq_map = vq_supported & ~vq_init & vq_mask;
>   
> -        if (max_vq == 0 || vq_map == 0) {
> +        if (vq_map == 0) {
>               error_setg(errp, "cannot disable sve%d", vq * 128);
>               error_append_hint(errp, "Disabling sve%d results in all "
>                                 "vector lengths being disabled.\n",

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>