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.1On 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~
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>
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
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
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~
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.
© 2016 - 2024 Red Hat, Inc.