[PATCH] perf/x86: Fix integer overflow when left shifting an integer more than 32 bits

[PATCH] perf/x86: Fix integer overflow when left shifting an integer more than 32 bits

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

The 64 bit value read from MSR_ARCH_PERFMON_FIXED_CTR_CTRL is being
bit-wise masked with the value (0x03 << i*4). However, the shifted value
is evaluated using 32 bit arithmetic, so will overflow when i > 8.
Fix this by making 0x03 a ULL so that the shift is performed using
64 bit arithmetic.

Addresses-Coverity: ("Unintentional integer overflow")
Fixes: a5ebe0ba3dff ("perf/x86: Check all MSRs before passing hw check")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 arch/x86/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index bafd93c54ffa..59c665c8c2e9 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -261,7 +261,7 @@ static bool check_hw_exists(void)
 		for (i = 0; i < x86_pmu.num_counters_fixed; i++) {
 			if (fixed_counter_disabled(i))
 				continue;
-			if (val & (0x03 << i*4)) {
+			if (val & (0x03ULL << i*4)) {
 				bios_fail = 1;
 				val_fail = val;
 				reg_fail = reg;
-- 
2.30.2

Re: [PATCH] perf/x86: Fix integer overflow when left shifting an integer more than 32 bits

[Peter Zijlstra]

On Tue, Apr 20, 2021 at 03:29:07PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The 64 bit value read from MSR_ARCH_PERFMON_FIXED_CTR_CTRL is being
> bit-wise masked with the value (0x03 << i*4). However, the shifted value
> is evaluated using 32 bit arithmetic, so will overflow when i > 8.
> Fix this by making 0x03 a ULL so that the shift is performed using
> 64 bit arithmetic.
> 
> Addresses-Coverity: ("Unintentional integer overflow")

Strange tag that, also inaccurate, wide shifts are UB and don't behave
consistently.

As is, we've not had hardware with that many fixed counters, but yes,
worth fixing I suppose.

> Fixes: a5ebe0ba3dff ("perf/x86: Check all MSRs before passing hw check")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  arch/x86/events/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index bafd93c54ffa..59c665c8c2e9 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -261,7 +261,7 @@ static bool check_hw_exists(void)
>  		for (i = 0; i < x86_pmu.num_counters_fixed; i++) {
>  			if (fixed_counter_disabled(i))
>  				continue;
> -			if (val & (0x03 << i*4)) {
> +			if (val & (0x03ULL << i*4)) {
>  				bios_fail = 1;
>  				val_fail = val;
>  				reg_fail = reg;
> -- 
> 2.30.2
> 

Re: [PATCH] perf/x86: Fix integer overflow when left shifting an integer more than 32 bits

[Peter Zijlstra]

On Tue, Apr 20, 2021 at 05:03:03PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 20, 2021 at 03:29:07PM +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > The 64 bit value read from MSR_ARCH_PERFMON_FIXED_CTR_CTRL is being
> > bit-wise masked with the value (0x03 << i*4). However, the shifted value
> > is evaluated using 32 bit arithmetic, so will overflow when i > 8.
> > Fix this by making 0x03 a ULL so that the shift is performed using
> > 64 bit arithmetic.
> > 
> > Addresses-Coverity: ("Unintentional integer overflow")
> 
> Strange tag that, also inaccurate, wide shifts are UB and don't behave
> consistently.
> 
> As is, we've not had hardware with that many fixed counters, but yes,
> worth fixing I suppose.

Patch now reads:

---
Subject: perf/x86: Allow for 8<num_fixed_counters<16
From: Colin Ian King <colin.king@canonical.com>
Date: Tue, 20 Apr 2021 15:29:07 +0100

From: Colin Ian King <colin.king@canonical.com>

The 64 bit value read from MSR_ARCH_PERFMON_FIXED_CTR_CTRL is being
bit-wise masked with the value (0x03 << i*4). However, the shifted value
is evaluated using 32 bit arithmetic, so will UB when i > 8. Fix this
by making 0x03 a ULL so that the shift is performed using 64 bit
arithmetic.

This makes the arithmetic internally consistent and preparers for the
day when hardware provides 8<num_fixed_counters<16.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20210420142907.382417-1-colin.king@canonical.com
---
 arch/x86/events/core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -261,7 +261,7 @@ static bool check_hw_exists(void)
 		for (i = 0; i < x86_pmu.num_counters_fixed; i++) {
 			if (fixed_counter_disabled(i))
 				continue;
-			if (val & (0x03 << i*4)) {
+			if (val & (0x03ULL << i*4)) {
 				bios_fail = 1;
 				val_fail = val;
 				reg_fail = reg;

Re: [PATCH] perf/x86: Fix integer overflow when left shifting an integer more than 32 bits

[Colin Ian King]

On 20/04/2021 16:31, Peter Zijlstra wrote:
> On Tue, Apr 20, 2021 at 05:03:03PM +0200, Peter Zijlstra wrote:
>> On Tue, Apr 20, 2021 at 03:29:07PM +0100, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The 64 bit value read from MSR_ARCH_PERFMON_FIXED_CTR_CTRL is being
>>> bit-wise masked with the value (0x03 << i*4). However, the shifted value
>>> is evaluated using 32 bit arithmetic, so will overflow when i > 8.
>>> Fix this by making 0x03 a ULL so that the shift is performed using
>>> 64 bit arithmetic.
>>>
>>> Addresses-Coverity: ("Unintentional integer overflow")
>>
>> Strange tag that, also inaccurate, wide shifts are UB and don't behave
>> consistently.
>>
>> As is, we've not had hardware with that many fixed counters, but yes,
>> worth fixing I suppose.
> 
> Patch now reads:
> 
> ---
> Subject: perf/x86: Allow for 8<num_fixed_counters<16
> From: Colin Ian King <colin.king@canonical.com>
> Date: Tue, 20 Apr 2021 15:29:07 +0100
> 
> From: Colin Ian King <colin.king@canonical.com>
> 
> The 64 bit value read from MSR_ARCH_PERFMON_FIXED_CTR_CTRL is being
> bit-wise masked with the value (0x03 << i*4). However, the shifted value
> is evaluated using 32 bit arithmetic, so will UB when i > 8. Fix this
> by making 0x03 a ULL so that the shift is performed using 64 bit
> arithmetic.
> 
> This makes the arithmetic internally consistent and preparers for the
> day when hardware provides 8<num_fixed_counters<16.

Yep, that's good. Thanks.

> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: https://lkml.kernel.org/r/20210420142907.382417-1-colin.king@canonical.com
> ---
>  arch/x86/events/core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -261,7 +261,7 @@ static bool check_hw_exists(void)
>  		for (i = 0; i < x86_pmu.num_counters_fixed; i++) {
>  			if (fixed_counter_disabled(i))
>  				continue;
> -			if (val & (0x03 << i*4)) {
> +			if (val & (0x03ULL << i*4)) {
>  				bios_fail = 1;
>  				val_fail = val;
>  				reg_fail = reg;
> 

[tip: perf/core] perf/x86: Allow for 8<num_fixed_counters<16

[tip-bot2 for Colin Ian King]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     32d35c4a96ec79446f0d7be308a6eb248b507a0b
Gitweb:        https://git.kernel.org/tip/32d35c4a96ec79446f0d7be308a6eb248b507a0b
Author:        Colin Ian King <colin.king@canonical.com>
AuthorDate:    Tue, 20 Apr 2021 15:29:07 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 23 Apr 2021 09:03:15 +02:00

perf/x86: Allow for 8<num_fixed_counters<16

The 64 bit value read from MSR_ARCH_PERFMON_FIXED_CTR_CTRL is being
bit-wise masked with the value (0x03 << i*4). However, the shifted value
is evaluated using 32 bit arithmetic, so will UB when i > 8. Fix this
by making 0x03 a ULL so that the shift is performed using 64 bit
arithmetic.

This makes the arithmetic internally consistent and preparers for the
day when hardware provides 8<num_fixed_counters<16.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20210420142907.382417-1-colin.king@canonical.com
---
 arch/x86/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 3fe66b7..c7fcc8d 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -278,7 +278,7 @@ bool check_hw_exists(struct pmu *pmu, int num_counters, int num_counters_fixed)
 		for (i = 0; i < num_counters_fixed; i++) {
 			if (fixed_counter_disabled(i, pmu))
 				continue;
-			if (val & (0x03 << i*4)) {
+			if (val & (0x03ULL << i*4)) {
 				bios_fail = 1;
 				val_fail = val;
 				reg_fail = reg;