[PATCH 0/8] perf/amd/ibs: Fix sample period computations

[PATCH 0/8] perf/amd/ibs: Fix sample period computations

[Ravi Bangoria]

IBS Fetch and IBS Op pmus have constraints on supported sample period
values. The IBS hw behavior could be undefined if they are not followed.
Currently, IBS driver does not honor them correctly and thus a malicious
event could cause issues to the system. Fortunately, the IBS hw is very
resilient so far and IBS pmus are restricted to root only, so the attack
vector is minimal. In any case, these are genuine bugs and must be fixed.

Patches are prepared on v6.11.

Ravi Bangoria (8):
  perf/amd/ibs: Remove IBS_{FETCH|OP}_CONFIG_MASK macros
  perf/amd/ibs: Remove pointless sample period check
  perf/amd/ibs: Fix ->config to sample period calculation for OP pmu
  perf/amd/ibs: Fix perf_ibs_op.cnt_mask for CurCnt
  perf/amd/ibs: Don't allow freq mode event creation through ->config
    interface
  perf/amd/ibs: Add pmu specific minimum period
  perf/amd/ibs: Add ->check_period() callback
  perf/core: Introduce pmu->adjust_period() callback

 arch/x86/events/amd/ibs.c         | 97 +++++++++++++++++++++++--------
 arch/x86/include/asm/perf_event.h |  1 +
 include/linux/perf_event.h        |  5 ++
 kernel/events/core.c              | 12 +++-
 4 files changed, 88 insertions(+), 27 deletions(-)

-- 
2.46.2

[PATCH 1/8] perf/amd/ibs: Remove IBS_{FETCH|OP}_CONFIG_MASK macros

[Ravi Bangoria]

Definition of these macros are very simple and they are used at only one
place. Get rid of unnecessary redirection.

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index e91970b01d62..347353b9eb70 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -28,10 +28,6 @@ static u32 ibs_caps;
 #include <asm/nmi.h>
 #include <asm/amd-ibs.h>
 
-#define IBS_FETCH_CONFIG_MASK	(IBS_FETCH_RAND_EN | IBS_FETCH_MAX_CNT)
-#define IBS_OP_CONFIG_MASK	IBS_OP_MAX_CNT
-
-
 /*
  * IBS states:
  *
@@ -670,7 +666,7 @@ static struct perf_ibs perf_ibs_fetch = {
 		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
 	},
 	.msr			= MSR_AMD64_IBSFETCHCTL,
-	.config_mask		= IBS_FETCH_CONFIG_MASK,
+	.config_mask		= IBS_FETCH_MAX_CNT | IBS_FETCH_RAND_EN,
 	.cnt_mask		= IBS_FETCH_MAX_CNT,
 	.enable_mask		= IBS_FETCH_ENABLE,
 	.valid_mask		= IBS_FETCH_VAL,
@@ -694,7 +690,7 @@ static struct perf_ibs perf_ibs_op = {
 		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
 	},
 	.msr			= MSR_AMD64_IBSOPCTL,
-	.config_mask		= IBS_OP_CONFIG_MASK,
+	.config_mask		= IBS_OP_MAX_CNT,
 	.cnt_mask		= IBS_OP_MAX_CNT | IBS_OP_CUR_CNT |
 				  IBS_OP_CUR_CNT_RAND,
 	.enable_mask		= IBS_OP_ENABLE,
-- 
2.46.2

[PATCH 2/8] perf/amd/ibs: Remove pointless sample period check

[Ravi Bangoria]

Valid perf event sample period value for IBS PMUs (Fetch and Op both)
is limited to multiple of 0x10. perf_ibs_init() has this check:

  if (!event->attr.sample_freq && hwc->sample_period & 0x0f)
          return -EINVAL;

But it's broken since hwc->sample_period will always be 0 when
event->attr.sample_freq is 0 (irrespective of event->attr.freq value.)

One option to fix this is to change the condition:

  - if (!event->attr.sample_freq && hwc->sample_period & 0x0f)
  + if (!event->attr.freq && hwc->sample_period & 0x0f)

However, that will break all userspace tools which have been using IBS
event with sample_period not multiple of 0x10.

Another option is to remove the condition altogether and mask lower
nibble _silently_, same as what current code is inadvertently doing.
I'm preferring this approach as it keeps the existing behavior.

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 347353b9eb70..6b55a8520166 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -294,13 +294,8 @@ static int perf_ibs_init(struct perf_event *event)
 		if (config & perf_ibs->cnt_mask)
 			/* raw max_cnt may not be set */
 			return -EINVAL;
-		if (!event->attr.sample_freq && hwc->sample_period & 0x0f)
-			/*
-			 * lower 4 bits can not be set in ibs max cnt,
-			 * but allowing it in case we adjust the
-			 * sample period to set a frequency.
-			 */
-			return -EINVAL;
+
+		/* Silently mask off lower nibble. IBS hw mandates it. */
 		hwc->sample_period &= ~0x0FULL;
 		if (!hwc->sample_period)
 			hwc->sample_period = 0x10;
-- 
2.46.2

[PATCH 3/8] perf/amd/ibs: Fix ->config to sample period calculation for OP pmu

[Ravi Bangoria]

Instead of using standard perf_event_attr->freq=0 and ->sample_period
fields, IBS event in 'sample period mode' can also be opened by setting
period value directly in perf_event_attr->config in a MaxCnt bit-field
format.

IBS OP MaxCnt bits are defined as:

  (high bits) IbsOpCtl[26:20] = IbsOpMaxCnt[26:20]
  (low bits)  IbsOpCtl[15:0]  = IbsOpMaxCnt[19:4]

Perf event sample period can be derived from MaxCnt bits as:

  sample_period = (high bits) | ((low_bits) << 4);

However, current code just masks MaxCnt bits and shifts all of them,
including high bits, which is incorrect. Fix it.

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 6b55a8520166..79422d2e301b 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -268,7 +268,7 @@ static int perf_ibs_init(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
 	struct perf_ibs *perf_ibs;
-	u64 max_cnt, config;
+	u64 config;
 	int ret;
 
 	perf_ibs = get_ibs_pmu(event->attr.type);
@@ -300,10 +300,19 @@ static int perf_ibs_init(struct perf_event *event)
 		if (!hwc->sample_period)
 			hwc->sample_period = 0x10;
 	} else {
-		max_cnt = config & perf_ibs->cnt_mask;
+		u64 period = 0;
+
+		if (perf_ibs == &perf_ibs_op) {
+			period = (config & IBS_OP_MAX_CNT) << 4;
+			if (ibs_caps & IBS_CAPS_OPCNTEXT)
+				period |= config & IBS_OP_MAX_CNT_EXT_MASK;
+		} else {
+			period = (config & IBS_FETCH_MAX_CNT) << 4;
+		}
+
 		config &= ~perf_ibs->cnt_mask;
-		event->attr.sample_period = max_cnt << 4;
-		hwc->sample_period = event->attr.sample_period;
+		event->attr.sample_period = period;
+		hwc->sample_period = period;
 	}
 
 	if (!hwc->sample_period)
-- 
2.46.2

[PATCH 4/8] perf/amd/ibs: Fix perf_ibs_op.cnt_mask for CurCnt

[Ravi Bangoria]

IBS Op uses two counters: MaxCnt and CurCnt. MaxCnt is programmed with
the desired sample period. IBS hw generates sample when CurCnt reaches
to MaxCnt. The size of these counter used to be 20 bits but later they
were extended to 27 bits. The 7 bit extension is indicated by CPUID
Fn8000_001B_EAX[6 / OpCntExt].

perf_ibs->cnt_mask variable contains bit masks for MaxCnt and CurCnt.
But IBS driver does not set upper 7 bits of CurCnt in cnt_mask even
when OpCntExt CPUID bit is set. Fix this.

IBS driver uses cnt_mask[CurCnt] bits only while disabling an event.
Fortunately, CurCnt bits are not read from MSR while re-enabling the
event, instead MaxCnt is programmed with desired period and CurCnt is
set to 0. Hence, we did not see any issues so far.

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c         | 3 ++-
 arch/x86/include/asm/perf_event.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 79422d2e301b..152f9116af1e 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -1222,7 +1222,8 @@ static __init int perf_ibs_op_init(void)
 	if (ibs_caps & IBS_CAPS_OPCNTEXT) {
 		perf_ibs_op.max_period  |= IBS_OP_MAX_CNT_EXT_MASK;
 		perf_ibs_op.config_mask	|= IBS_OP_MAX_CNT_EXT_MASK;
-		perf_ibs_op.cnt_mask    |= IBS_OP_MAX_CNT_EXT_MASK;
+		perf_ibs_op.cnt_mask    |= (IBS_OP_MAX_CNT_EXT_MASK |
+					    IBS_OP_CUR_CNT_EXT_MASK);
 	}
 
 	if (ibs_caps & IBS_CAPS_ZEN4)
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index 91b73571412f..72f1bcb0fa31 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -498,6 +498,7 @@ struct pebs_xmm {
  */
 #define IBS_OP_CUR_CNT		(0xFFF80ULL<<32)
 #define IBS_OP_CUR_CNT_RAND	(0x0007FULL<<32)
+#define IBS_OP_CUR_CNT_EXT_MASK	(0x7FULL<<52)
 #define IBS_OP_CNT_CTL		(1ULL<<19)
 #define IBS_OP_VAL		(1ULL<<18)
 #define IBS_OP_ENABLE		(1ULL<<17)
-- 
2.46.2

[PATCH 5/8] perf/amd/ibs: Don't allow freq mode event creation through ->config interface

[Ravi Bangoria]

Most perf_event_attr->config bits directly maps to IBS_{FETCH|OP}_CTL
MSR. Since the sample period is programmed in these control registers,
IBS PMU driver allows opening an IBS event by setting sample period
value directly in perf_event_attr->config instead of using explicit
perf_event_attr->sample_period interface.

However, this logic is not applicable for freq mode events since the
semantics of control register fields are applicable only to fixed
sample period whereas the freq mode event adjusts sample period after
each and every sample. Currently, IBS driver (unintentionally) allows
creating freq mode event via ->config interface, which is semantically
wrong as well as detrimental because it can be misused to bypass
perf_event_max_sample_rate checks.

Don't allow freq mode event creation through perf_event_attr->config
interface.

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 152f9116af1e..368ed839b612 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -302,6 +302,9 @@ static int perf_ibs_init(struct perf_event *event)
 	} else {
 		u64 period = 0;
 
+		if (event->attr.freq)
+			return -EINVAL;
+
 		if (perf_ibs == &perf_ibs_op) {
 			period = (config & IBS_OP_MAX_CNT) << 4;
 			if (ibs_caps & IBS_CAPS_OPCNTEXT)
-- 
2.46.2

[PATCH 6/8] perf/amd/ibs: Add pmu specific minimum period

[Ravi Bangoria]

0x10 is the minimum sample period for IBS Fetch and 0x90 for IBS Op.
Current IBS pmu driver uses 0x10 for both the pmus, which is incorrect.
Fix it by adding pmu specific minimum period values in struct perf_ibs.

Also, bail out opening a 'sample period mode' event if the user requested
sample period is less than pmu supported minimum value. For a 'freq mode'
event, start calibrating sample period from pmu specific minimum period.

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 368ed839b612..e7522ba45a7e 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -83,6 +83,7 @@ struct perf_ibs {
 	u64				cnt_mask;
 	u64				enable_mask;
 	u64				valid_mask;
+	u16				min_period;
 	u64				max_period;
 	unsigned long			offset_mask[1];
 	int				offset_max;
@@ -295,10 +296,14 @@ static int perf_ibs_init(struct perf_event *event)
 			/* raw max_cnt may not be set */
 			return -EINVAL;
 
-		/* Silently mask off lower nibble. IBS hw mandates it. */
-		hwc->sample_period &= ~0x0FULL;
-		if (!hwc->sample_period)
-			hwc->sample_period = 0x10;
+		if (event->attr.freq) {
+			hwc->sample_period = perf_ibs->min_period;
+		} else {
+			/* Silently mask off lower nibble. IBS hw mandates it. */
+			hwc->sample_period &= ~0x0FULL;
+			if (hwc->sample_period < perf_ibs->min_period)
+				return -EINVAL;
+		}
 	} else {
 		u64 period = 0;
 
@@ -316,10 +321,10 @@ static int perf_ibs_init(struct perf_event *event)
 		config &= ~perf_ibs->cnt_mask;
 		event->attr.sample_period = period;
 		hwc->sample_period = period;
-	}
 
-	if (!hwc->sample_period)
-		return -EINVAL;
+		if (hwc->sample_period < perf_ibs->min_period)
+			return -EINVAL;
+	}
 
 	/*
 	 * If we modify hwc->sample_period, we also need to update
@@ -340,7 +345,8 @@ static int perf_ibs_set_period(struct perf_ibs *perf_ibs,
 	int overflow;
 
 	/* ignore lower 4 bits in min count: */
-	overflow = perf_event_set_period(hwc, 1<<4, perf_ibs->max_period, period);
+	overflow = perf_event_set_period(hwc, perf_ibs->min_period,
+					 perf_ibs->max_period, period);
 	local64_set(&hwc->prev_count, 0);
 
 	return overflow;
@@ -677,6 +683,7 @@ static struct perf_ibs perf_ibs_fetch = {
 	.cnt_mask		= IBS_FETCH_MAX_CNT,
 	.enable_mask		= IBS_FETCH_ENABLE,
 	.valid_mask		= IBS_FETCH_VAL,
+	.min_period		= 0x10,
 	.max_period		= IBS_FETCH_MAX_CNT << 4,
 	.offset_mask		= { MSR_AMD64_IBSFETCH_REG_MASK },
 	.offset_max		= MSR_AMD64_IBSFETCH_REG_COUNT,
@@ -702,6 +709,7 @@ static struct perf_ibs perf_ibs_op = {
 				  IBS_OP_CUR_CNT_RAND,
 	.enable_mask		= IBS_OP_ENABLE,
 	.valid_mask		= IBS_OP_VAL,
+	.min_period		= 0x90,
 	.max_period		= IBS_OP_MAX_CNT << 4,
 	.offset_mask		= { MSR_AMD64_IBSOP_REG_MASK },
 	.offset_max		= MSR_AMD64_IBSOP_REG_COUNT,
-- 
2.46.2

[PATCH 7/8] perf/amd/ibs: Add ->check_period() callback

[Ravi Bangoria]

IBS Fetch and IBS Op pmus have constraints on sample period. The sample
period is verified at the time of opening an event but not at the ioctl()
interface. Hence, a user can open an event with valid period but change
it later with ioctl(). Add a ->check_period() callback to verify the
period provided at ioctl() is also valid.

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index e7522ba45a7e..33728ed6d7a6 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -551,6 +551,28 @@ static void perf_ibs_del(struct perf_event *event, int flags)
 
 static void perf_ibs_read(struct perf_event *event) { }
 
+static int perf_ibs_check_period(struct perf_event *event, u64 value)
+{
+	struct perf_ibs *perf_ibs;
+	u64 low_nibble;
+
+	if (event->attr.freq)
+		return 0;
+
+	perf_ibs = container_of(event->pmu, struct perf_ibs, pmu);
+	low_nibble = value & 0xFULL;
+
+	/*
+	 * This contradicts with perf_ibs_init() which allows sample period
+	 * with lower nibble bits set but silently masks them off. Whereas
+	 * this returns error.
+	 */
+	if (low_nibble || value < perf_ibs->min_period)
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * We need to initialize with empty group if all attributes in the
  * group are dynamic.
@@ -676,6 +698,7 @@ static struct perf_ibs perf_ibs_fetch = {
 		.start		= perf_ibs_start,
 		.stop		= perf_ibs_stop,
 		.read		= perf_ibs_read,
+		.check_period	= perf_ibs_check_period,
 		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
 	},
 	.msr			= MSR_AMD64_IBSFETCHCTL,
@@ -701,6 +724,7 @@ static struct perf_ibs perf_ibs_op = {
 		.start		= perf_ibs_start,
 		.stop		= perf_ibs_stop,
 		.read		= perf_ibs_read,
+		.check_period	= perf_ibs_check_period,
 		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
 	},
 	.msr			= MSR_AMD64_IBSOPCTL,
-- 
2.46.2

[PATCH 8/8] perf/core: Introduce pmu->adjust_period() callback

[Ravi Bangoria]

Many hardware pmus have constraints about sample period. For ex, minimum
supported sample period for IBS Op pmu is 0x90, the sample period must
be multiple of 0x10 for IBS Fetch and IBS Op.

Add an optional callback adjust_period() to struct pmu to allow pmu
specific drivers to adjust sample period calculated by generic code.
This will ensure the sample_period value will always be valid and no
additional code is required in PMU specific drivers to re-adjust the
period.

Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
---
 arch/x86/events/amd/ibs.c  | 11 +++++++++++
 include/linux/perf_event.h |  5 +++++
 kernel/events/core.c       | 12 ++++++++++--
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 33728ed6d7a6..0d1db2fffc5b 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -573,6 +573,15 @@ static int perf_ibs_check_period(struct perf_event *event, u64 value)
 	return 0;
 }
 
+static u64 perf_ibs_adjust_period(struct perf_event *event, u64 period)
+{
+	struct perf_ibs *perf_ibs = container_of(event->pmu, struct perf_ibs, pmu);
+
+	period &= ~0xFULL;
+
+	return period < perf_ibs->min_period ? perf_ibs->min_period : period;
+}
+
 /*
  * We need to initialize with empty group if all attributes in the
  * group are dynamic.
@@ -699,6 +708,7 @@ static struct perf_ibs perf_ibs_fetch = {
 		.stop		= perf_ibs_stop,
 		.read		= perf_ibs_read,
 		.check_period	= perf_ibs_check_period,
+		.adjust_period	= perf_ibs_adjust_period,
 		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
 	},
 	.msr			= MSR_AMD64_IBSFETCHCTL,
@@ -725,6 +735,7 @@ static struct perf_ibs perf_ibs_op = {
 		.stop		= perf_ibs_stop,
 		.read		= perf_ibs_read,
 		.check_period	= perf_ibs_check_period,
+		.adjust_period	= perf_ibs_adjust_period,
 		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
 	},
 	.msr			= MSR_AMD64_IBSOPCTL,
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 1a8942277dda..eca8581d8e5d 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -540,6 +540,11 @@ struct pmu {
 	 * Check period value for PERF_EVENT_IOC_PERIOD ioctl.
 	 */
 	int (*check_period)		(struct perf_event *event, u64 value); /* optional */
+
+	/*
+	 * Adjust period value according to pmu constraints.
+	 */
+	u64 (*adjust_period)		(struct perf_event *event, u64 period); /* optional */
 };
 
 enum perf_addr_filter_action_t {
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 8a6c6bbcd658..f3de683ec716 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4100,9 +4100,9 @@ static void perf_adjust_period(struct perf_event *event, u64 nsec, u64 count, bo
 	if (!sample_period)
 		sample_period = 1;
 
-	hwc->sample_period = sample_period;
+	hwc->sample_period = event->pmu->adjust_period(event, sample_period);
 
-	if (local64_read(&hwc->period_left) > 8*sample_period) {
+	if (local64_read(&hwc->period_left) > 8*hwc->sample_period) {
 		if (disable)
 			event->pmu->stop(event, PERF_EF_UPDATE);
 
@@ -11363,6 +11363,11 @@ static int perf_event_nop_int(struct perf_event *event, u64 value)
 	return 0;
 }
 
+static u64 perf_pmu_nop_adjust_period(struct perf_event *event, u64 period)
+{
+	return period;
+}
+
 static DEFINE_PER_CPU(unsigned int, nop_txn_flags);
 
 static void perf_pmu_start_txn(struct pmu *pmu, unsigned int flags)
@@ -11641,6 +11646,9 @@ int perf_pmu_register(struct pmu *pmu, const char *name, int type)
 	if (!pmu->check_period)
 		pmu->check_period = perf_event_nop_int;
 
+	if (!pmu->adjust_period)
+		pmu->adjust_period = perf_pmu_nop_adjust_period;
+
 	if (!pmu->event_idx)
 		pmu->event_idx = perf_event_idx_default;
 
-- 
2.46.2

Re: [PATCH 2/8] perf/amd/ibs: Remove pointless sample period check

[Namhyung Kim]

Hello,

On Mon, Oct 07, 2024 at 03:48:04AM +0000, Ravi Bangoria wrote:
> Valid perf event sample period value for IBS PMUs (Fetch and Op both)
> is limited to multiple of 0x10. perf_ibs_init() has this check:
> 
>   if (!event->attr.sample_freq && hwc->sample_period & 0x0f)
>           return -EINVAL;
> 
> But it's broken since hwc->sample_period will always be 0 when
> event->attr.sample_freq is 0 (irrespective of event->attr.freq value.)

Right, hwc->sample_period is set to event->attr.sample_period and it's
the same as sample_freq since there are in a union.

struct perf_event_attr {
	__u32			type;
	__u32			size;
	__u64			config;
	union {
		__u64		sample_period;
		__u64		sample_freq;
	};
	...

perf_event_alloc() sets the hwc->sample_period and changes it only when
both attr->freq and attr->sample_freq are not zero.

> 
> One option to fix this is to change the condition:
> 
>   - if (!event->attr.sample_freq && hwc->sample_period & 0x0f)
>   + if (!event->attr.freq && hwc->sample_period & 0x0f)

Right, I believe this is the intention.

> 
> However, that will break all userspace tools which have been using IBS
> event with sample_period not multiple of 0x10.

Correct.

> 
> Another option is to remove the condition altogether and mask lower
> nibble _silently_, same as what current code is inadvertently doing.
> I'm preferring this approach as it keeps the existing behavior.

Agreed.  The condition never worked and should be safe to remove.

Thanks,
Namhyung

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>  arch/x86/events/amd/ibs.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> index 347353b9eb70..6b55a8520166 100644
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c
> @@ -294,13 +294,8 @@ static int perf_ibs_init(struct perf_event *event)
>  		if (config & perf_ibs->cnt_mask)
>  			/* raw max_cnt may not be set */
>  			return -EINVAL;
> -		if (!event->attr.sample_freq && hwc->sample_period & 0x0f)
> -			/*
> -			 * lower 4 bits can not be set in ibs max cnt,
> -			 * but allowing it in case we adjust the
> -			 * sample period to set a frequency.
> -			 */
> -			return -EINVAL;
> +
> +		/* Silently mask off lower nibble. IBS hw mandates it. */
>  		hwc->sample_period &= ~0x0FULL;
>  		if (!hwc->sample_period)
>  			hwc->sample_period = 0x10;
> -- 
> 2.46.2
> 

Re: [PATCH 5/8] perf/amd/ibs: Don't allow freq mode event creation through ->config interface

[Namhyung Kim]

On Mon, Oct 07, 2024 at 03:48:07AM +0000, Ravi Bangoria wrote:
> Most perf_event_attr->config bits directly maps to IBS_{FETCH|OP}_CTL
> MSR. Since the sample period is programmed in these control registers,
> IBS PMU driver allows opening an IBS event by setting sample period
> value directly in perf_event_attr->config instead of using explicit
> perf_event_attr->sample_period interface.
> 
> However, this logic is not applicable for freq mode events since the
> semantics of control register fields are applicable only to fixed
> sample period whereas the freq mode event adjusts sample period after
> each and every sample. Currently, IBS driver (unintentionally) allows
> creating freq mode event via ->config interface, which is semantically
> wrong as well as detrimental because it can be misused to bypass
> perf_event_max_sample_rate checks.
> 
> Don't allow freq mode event creation through perf_event_attr->config
> interface.

Sounds reasonable.  I agree the freq mode should use the standard
interface using attr->sample_freq.  But I'm not sure if the behaivor is
defined when attr->freq is set and attr->sample_freq is 0.  Maybe this
should be handled in the generic code.

Thanks,
Namhyung

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>  arch/x86/events/amd/ibs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> index 152f9116af1e..368ed839b612 100644
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c
> @@ -302,6 +302,9 @@ static int perf_ibs_init(struct perf_event *event)
>  	} else {
>  		u64 period = 0;
>  
> +		if (event->attr.freq)
> +			return -EINVAL;
> +
>  		if (perf_ibs == &perf_ibs_op) {
>  			period = (config & IBS_OP_MAX_CNT) << 4;
>  			if (ibs_caps & IBS_CAPS_OPCNTEXT)
> -- 
> 2.46.2
> 

Re: [PATCH 6/8] perf/amd/ibs: Add pmu specific minimum period

[Namhyung Kim]

On Mon, Oct 07, 2024 at 03:48:08AM +0000, Ravi Bangoria wrote:
> 0x10 is the minimum sample period for IBS Fetch and 0x90 for IBS Op.
> Current IBS pmu driver uses 0x10 for both the pmus, which is incorrect.
> Fix it by adding pmu specific minimum period values in struct perf_ibs.
> 
> Also, bail out opening a 'sample period mode' event if the user requested
> sample period is less than pmu supported minimum value. For a 'freq mode'
> event, start calibrating sample period from pmu specific minimum period.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>  arch/x86/events/amd/ibs.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> index 368ed839b612..e7522ba45a7e 100644
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c
> @@ -83,6 +83,7 @@ struct perf_ibs {
>  	u64				cnt_mask;
>  	u64				enable_mask;
>  	u64				valid_mask;
> +	u16				min_period;
>  	u64				max_period;
>  	unsigned long			offset_mask[1];
>  	int				offset_max;
> @@ -295,10 +296,14 @@ static int perf_ibs_init(struct perf_event *event)
>  			/* raw max_cnt may not be set */
>  			return -EINVAL;
>  
> -		/* Silently mask off lower nibble. IBS hw mandates it. */
> -		hwc->sample_period &= ~0x0FULL;
> -		if (!hwc->sample_period)
> -			hwc->sample_period = 0x10;
> +		if (event->attr.freq) {
> +			hwc->sample_period = perf_ibs->min_period;
> +		} else {
> +			/* Silently mask off lower nibble. IBS hw mandates it. */
> +			hwc->sample_period &= ~0x0FULL;
> +			if (hwc->sample_period < perf_ibs->min_period)
> +				return -EINVAL;

Maybe it needs to check perf_ibs->max_period as well.

Thanks,
Namhyung

> +		}
>  	} else {
>  		u64 period = 0;
>  
> @@ -316,10 +321,10 @@ static int perf_ibs_init(struct perf_event *event)
>  		config &= ~perf_ibs->cnt_mask;
>  		event->attr.sample_period = period;
>  		hwc->sample_period = period;
> -	}
>  
> -	if (!hwc->sample_period)
> -		return -EINVAL;
> +		if (hwc->sample_period < perf_ibs->min_period)
> +			return -EINVAL;
> +	}
>  
>  	/*
>  	 * If we modify hwc->sample_period, we also need to update
> @@ -340,7 +345,8 @@ static int perf_ibs_set_period(struct perf_ibs *perf_ibs,
>  	int overflow;
>  
>  	/* ignore lower 4 bits in min count: */
> -	overflow = perf_event_set_period(hwc, 1<<4, perf_ibs->max_period, period);
> +	overflow = perf_event_set_period(hwc, perf_ibs->min_period,
> +					 perf_ibs->max_period, period);
>  	local64_set(&hwc->prev_count, 0);
>  
>  	return overflow;
> @@ -677,6 +683,7 @@ static struct perf_ibs perf_ibs_fetch = {
>  	.cnt_mask		= IBS_FETCH_MAX_CNT,
>  	.enable_mask		= IBS_FETCH_ENABLE,
>  	.valid_mask		= IBS_FETCH_VAL,
> +	.min_period		= 0x10,
>  	.max_period		= IBS_FETCH_MAX_CNT << 4,
>  	.offset_mask		= { MSR_AMD64_IBSFETCH_REG_MASK },
>  	.offset_max		= MSR_AMD64_IBSFETCH_REG_COUNT,
> @@ -702,6 +709,7 @@ static struct perf_ibs perf_ibs_op = {
>  				  IBS_OP_CUR_CNT_RAND,
>  	.enable_mask		= IBS_OP_ENABLE,
>  	.valid_mask		= IBS_OP_VAL,
> +	.min_period		= 0x90,
>  	.max_period		= IBS_OP_MAX_CNT << 4,
>  	.offset_mask		= { MSR_AMD64_IBSOP_REG_MASK },
>  	.offset_max		= MSR_AMD64_IBSOP_REG_COUNT,
> -- 
> 2.46.2
> 

Re: [PATCH 7/8] perf/amd/ibs: Add ->check_period() callback

[Namhyung Kim]

On Mon, Oct 07, 2024 at 03:48:09AM +0000, Ravi Bangoria wrote:
> IBS Fetch and IBS Op pmus have constraints on sample period. The sample
> period is verified at the time of opening an event but not at the ioctl()
> interface. Hence, a user can open an event with valid period but change
> it later with ioctl(). Add a ->check_period() callback to verify the
> period provided at ioctl() is also valid.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>  arch/x86/events/amd/ibs.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
> index e7522ba45a7e..33728ed6d7a6 100644
> --- a/arch/x86/events/amd/ibs.c
> +++ b/arch/x86/events/amd/ibs.c
> @@ -551,6 +551,28 @@ static void perf_ibs_del(struct perf_event *event, int flags)
>  
>  static void perf_ibs_read(struct perf_event *event) { }
>  
> +static int perf_ibs_check_period(struct perf_event *event, u64 value)
> +{
> +	struct perf_ibs *perf_ibs;
> +	u64 low_nibble;
> +
> +	if (event->attr.freq)
> +		return 0;
> +
> +	perf_ibs = container_of(event->pmu, struct perf_ibs, pmu);
> +	low_nibble = value & 0xFULL;
> +
> +	/*
> +	 * This contradicts with perf_ibs_init() which allows sample period
> +	 * with lower nibble bits set but silently masks them off. Whereas
> +	 * this returns error.
> +	 */
> +	if (low_nibble || value < perf_ibs->min_period)
> +		return -EINVAL;

You may want to check max_period too.

Thanks,
Namhyung

> +
> +	return 0;
> +}
> +
>  /*
>   * We need to initialize with empty group if all attributes in the
>   * group are dynamic.
> @@ -676,6 +698,7 @@ static struct perf_ibs perf_ibs_fetch = {
>  		.start		= perf_ibs_start,
>  		.stop		= perf_ibs_stop,
>  		.read		= perf_ibs_read,
> +		.check_period	= perf_ibs_check_period,
>  		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
>  	},
>  	.msr			= MSR_AMD64_IBSFETCHCTL,
> @@ -701,6 +724,7 @@ static struct perf_ibs perf_ibs_op = {
>  		.start		= perf_ibs_start,
>  		.stop		= perf_ibs_stop,
>  		.read		= perf_ibs_read,
> +		.check_period	= perf_ibs_check_period,
>  		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
>  	},
>  	.msr			= MSR_AMD64_IBSOPCTL,
> -- 
> 2.46.2
> 

Re: [PATCH 5/8] perf/amd/ibs: Don't allow freq mode event creation through ->config interface

[Ravi Bangoria]

>> Don't allow freq mode event creation through perf_event_attr->config
>> interface.
> 
> Sounds reasonable.  I agree the freq mode should use the standard
> interface using attr->sample_freq.  But I'm not sure if the behaivor is
> defined when attr->freq is set and attr->sample_freq is 0.  Maybe this
> should be handled in the generic code.

I also could not find any reason to allow {freq=1, sample_freq=0}, but:

1) perf_event_open() allows it.
2) ioctl(PERF_EVENT_IOC_PERIOD) allows it.
3) all generic code explicitly checks for ->sample_freq != 0 wherever
   ->freq == 1.

I will prepare and post a patch to reject such event and see if there
are any objections.

Thanks,
Ravi

Re: [PATCH 6/8] perf/amd/ibs: Add pmu specific minimum period

[Ravi Bangoria]

>> @@ -295,10 +296,14 @@ static int perf_ibs_init(struct perf_event *event)
>>  			/* raw max_cnt may not be set */
>>  			return -EINVAL;
>>  
>> -		/* Silently mask off lower nibble. IBS hw mandates it. */
>> -		hwc->sample_period &= ~0x0FULL;
>> -		if (!hwc->sample_period)
>> -			hwc->sample_period = 0x10;
>> +		if (event->attr.freq) {
>> +			hwc->sample_period = perf_ibs->min_period;
>> +		} else {
>> +			/* Silently mask off lower nibble. IBS hw mandates it. */
>> +			hwc->sample_period &= ~0x0FULL;
>> +			if (hwc->sample_period < perf_ibs->min_period)
>> +				return -EINVAL;
> 
> Maybe it needs to check perf_ibs->max_period as well.

We do allow event with sample_period > pmu->max_period and handle it with
"period_left" logic.

Thanks,
Ravi

Re: [PATCH 5/8] perf/amd/ibs: Don't allow freq mode event creation through ->config interface

[Namhyung Kim]

On Tue, Oct 08, 2024 at 11:00:37AM +0530, Ravi Bangoria wrote:
> >> Don't allow freq mode event creation through perf_event_attr->config
> >> interface.
> > 
> > Sounds reasonable.  I agree the freq mode should use the standard
> > interface using attr->sample_freq.  But I'm not sure if the behaivor is
> > defined when attr->freq is set and attr->sample_freq is 0.  Maybe this
> > should be handled in the generic code.
> 
> I also could not find any reason to allow {freq=1, sample_freq=0}, but:
> 
> 1) perf_event_open() allows it.
> 2) ioctl(PERF_EVENT_IOC_PERIOD) allows it.
> 3) all generic code explicitly checks for ->sample_freq != 0 wherever
>    ->freq == 1.
> 
> I will prepare and post a patch to reject such event and see if there
> are any objections.

Hmm.. now I think that the kernel won't treat it as a sampling event and
would ignore the attr.freq value.  Setting IOC_PERIOD to 0 would disable
sampling then.  Sorry for the noise.

Thanks,
Namhyung

Re: [PATCH 6/8] perf/amd/ibs: Add pmu specific minimum period

[Namhyung Kim]

On Tue, Oct 08, 2024 at 11:16:43AM +0530, Ravi Bangoria wrote:
> >> @@ -295,10 +296,14 @@ static int perf_ibs_init(struct perf_event *event)
> >>  			/* raw max_cnt may not be set */
> >>  			return -EINVAL;
> >>  
> >> -		/* Silently mask off lower nibble. IBS hw mandates it. */
> >> -		hwc->sample_period &= ~0x0FULL;
> >> -		if (!hwc->sample_period)
> >> -			hwc->sample_period = 0x10;
> >> +		if (event->attr.freq) {
> >> +			hwc->sample_period = perf_ibs->min_period;
> >> +		} else {
> >> +			/* Silently mask off lower nibble. IBS hw mandates it. */
> >> +			hwc->sample_period &= ~0x0FULL;
> >> +			if (hwc->sample_period < perf_ibs->min_period)
> >> +				return -EINVAL;
> > 
> > Maybe it needs to check perf_ibs->max_period as well.
> 
> We do allow event with sample_period > pmu->max_period and handle it with
> "period_left" logic.

Oh, ok then.

Thanks,
Namhyung

Re: [PATCH 0/8] perf/amd/ibs: Fix sample period computations

[Namhyung Kim]

On Mon, Oct 07, 2024 at 03:48:02AM +0000, Ravi Bangoria wrote:
> IBS Fetch and IBS Op pmus have constraints on supported sample period
> values. The IBS hw behavior could be undefined if they are not followed.
> Currently, IBS driver does not honor them correctly and thus a malicious
> event could cause issues to the system. Fortunately, the IBS hw is very
> resilient so far and IBS pmus are restricted to root only, so the attack
> vector is minimal. In any case, these are genuine bugs and must be fixed.
> 
> Patches are prepared on v6.11.
> 
> Ravi Bangoria (8):
>   perf/amd/ibs: Remove IBS_{FETCH|OP}_CONFIG_MASK macros
>   perf/amd/ibs: Remove pointless sample period check
>   perf/amd/ibs: Fix ->config to sample period calculation for OP pmu
>   perf/amd/ibs: Fix perf_ibs_op.cnt_mask for CurCnt
>   perf/amd/ibs: Don't allow freq mode event creation through ->config
>     interface
>   perf/amd/ibs: Add pmu specific minimum period
>   perf/amd/ibs: Add ->check_period() callback
>   perf/core: Introduce pmu->adjust_period() callback

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> 
>  arch/x86/events/amd/ibs.c         | 97 +++++++++++++++++++++++--------
>  arch/x86/include/asm/perf_event.h |  1 +
>  include/linux/perf_event.h        |  5 ++
>  kernel/events/core.c              | 12 +++-
>  4 files changed, 88 insertions(+), 27 deletions(-)
> 
> -- 
> 2.46.2
> 

Re: [PATCH 0/8] perf/amd/ibs: Fix sample period computations

[Ravi Bangoria]

On 09-Oct-24 12:03 PM, Namhyung Kim wrote:
> On Mon, Oct 07, 2024 at 03:48:02AM +0000, Ravi Bangoria wrote:
>> IBS Fetch and IBS Op pmus have constraints on supported sample period
>> values. The IBS hw behavior could be undefined if they are not followed.
>> Currently, IBS driver does not honor them correctly and thus a malicious
>> event could cause issues to the system. Fortunately, the IBS hw is very
>> resilient so far and IBS pmus are restricted to root only, so the attack
>> vector is minimal. In any case, these are genuine bugs and must be fixed.
>>
>> Patches are prepared on v6.11.
>>
>> Ravi Bangoria (8):
>>   perf/amd/ibs: Remove IBS_{FETCH|OP}_CONFIG_MASK macros
>>   perf/amd/ibs: Remove pointless sample period check
>>   perf/amd/ibs: Fix ->config to sample period calculation for OP pmu
>>   perf/amd/ibs: Fix perf_ibs_op.cnt_mask for CurCnt
>>   perf/amd/ibs: Don't allow freq mode event creation through ->config
>>     interface
>>   perf/amd/ibs: Add pmu specific minimum period
>>   perf/amd/ibs: Add ->check_period() callback
>>   perf/core: Introduce pmu->adjust_period() callback
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks Namhyung.

Peter/Ingo, gentle reminder :)

Thanks,
Ravi