| Message ID | 20230704154332.3014896-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Avoid over-length shift in arm_cpu_sve_finalize() error case | expand |
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>
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~
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.
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",
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(-)