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

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

[Peter Maydell]

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.

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",
-- 
2.34.1

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

[Philippe Mathieu-Daudé]

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>

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

[Peter Maydell]

On Tue, 4 Jul 2023 at 16:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 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)
>
> ---

Only by introducing a conditional in the case where the length
isn't a compile time constant.
Like the extract and deposit functions, the assumption is that
you're operating on a field that actually exists and isn't
zero-width.

thanks
-- PMM

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

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> 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.

Hmm that does make me worry we could have more land mines waiting to be
found. Would converting MAKE_64BIT_MASK into an inline function and
asserting be a better solution?

>
> 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>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Richard Henderson]

On 7/4/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.
> 
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Richard Henderson]

On 7/4/23 18:00, Alex Bennée wrote:
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> 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.
> 
> Hmm that does make me worry we could have more land mines waiting to be
> found. Would converting MAKE_64BIT_MASK into an inline function and
> asserting be a better solution?

I'd be tempted to keep a macro, and use __builtin_constant_p to make sure this expands to 
a constant if possible.  Ideally constants would be diagnosed at compile-time and runtime 
values get runtime asserts.


r~

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

[Philippe Mathieu-Daudé]

On 5/7/23 16:45, Richard Henderson wrote:
> On 7/4/23 18:00, Alex Bennée wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> 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.
>>
>> Hmm that does make me worry we could have more land mines waiting to be
>> found. Would converting MAKE_64BIT_MASK into an inline function and
>> asserting be a better solution?
> 
> I'd be tempted to keep a macro, and use __builtin_constant_p to make 
> sure this expands to a constant if possible.  Ideally constants would be 
> diagnosed at compile-time and runtime values get runtime asserts.

Indeed inlined function doesn't work because MAKE_64BIT_MASK() is
used in static const value definitions:

include/hw/cxl/cxl_component.h:52:1: error: expression is not an integer 
constant expression
CXLx_CAPABILITY_HEADER(LINK, 0x8)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/hw/cxl/cxl_component.h:50:9: note: expanded from macro 
'CXLx_CAPABILITY_HEADER'
         FIELD(CXL_##type##_CAPABILITY_HEADER, PTR, 20, 12)
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/hw/registerfields.h:46:41: note: expanded from macro 'FIELD'
                                         MAKE_64BIT_MASK(shift, length)};
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This builds however:

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

---

But then UB is still present at runtime.