[PATCH 0/4] RAS/amd/fmpm: Fix OOB, uninitialized data, and error-handling bugs

[PATCH 0/4] RAS/amd/fmpm: Fix OOB, uninitialized data, and error-handling bugs

[Rui Qi]

Hi Yazen, Borislav, Tony,

This series fixes several bugs in the AMD FRU Memory Poison Manager
driver.

Patch 1 fixes an out-of-bounds read in the for_each_fru macro caused by
the comma operator evaluating the array access before the bounds check.
This is technically undefined behavior and would be flagged by UBSan.

Patch 2 fixes an uninitialized stack bitmap in save_new_records() that
could cause the rollback path to clear ERST records that were not created
in the current initialization pass.

Patch 3 makes the max_nr_entries module parameter read-only (0444),
preventing runtime writes that could exceed the allocated flexible array
size.

Patch 4 fixes a spurious BUG when erst_get_record_id_begin() fails,
because the error path unconditionally calls erst_get_record_id_end()
which triggers BUG_ON.

All four bugs have been present since the original introduction of the
AMD FMPM driver.

Rui Qi (4):
  RAS/amd/fmpm: Fix out-of-bounds read in for_each_fru macro
  RAS/amd/fmpm: Clear new records bitmap before rollback
  RAS/amd/fmpm: Make max_nr_entries read-only
  RAS/amd/fmpm: Fix spurious BUG when ERST record enumeration fails

 drivers/ras/amd/fmpm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

--
2.20.1

[PATCH 1/4] RAS/amd/fmpm: Fix out-of-bounds read in for_each_fru macro

[Rui Qi]

The for_each_fru macro evaluates the array access "rec = fru_records[i]"
before the bounds check "i < max_nr_fru" due to the comma operator's
left-to-right evaluation order. When the loop terminates, i equals
max_nr_fru, causing fru_records[max_nr_fru] to be read before the
condition is checked.

While the garbage pointer value assigned to rec is never dereferenced
(the loop exits immediately), this is technically undefined behavior
and would be flagged by UBSan and static analyzers.

Fix by using short-circuit evaluation with && to check the bound first,
only accessing the array when i is within range:

  for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), 1); i++)

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 4ccaaf7b70bf..91c49080873e 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -169,7 +169,7 @@ static unsigned int spa_nr_entries;
 static DEFINE_MUTEX(fmpm_update_mutex);
 
 #define for_each_fru(i, rec) \
-	for (i = 0; rec = fru_records[i], i < max_nr_fru; i++)
+	for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), 1); i++)
 
 static inline u32 get_fmp_len(struct fru_rec *rec)
 {
-- 
2.20.1

Re: [PATCH 1/4] RAS/amd/fmpm: Fix out-of-bounds read in for_each_fru macro

[Yazen Ghannam]

On Fri, Aug 21, 2026 at 05:47:45PM +0800, Rui Qi wrote:

Hi Rui,

Thank you for the patch. I agree with the intent, but I have some minor
feedback.

For the $SUBJECT, please follow the existing prefix format for the file.

Ex. "RAS/AMD/FMPM:"

> The for_each_fru macro evaluates the array access "rec = fru_records[i]"
> before the bounds check "i < max_nr_fru" due to the comma operator's
> left-to-right evaluation order. When the loop terminates, i equals
> max_nr_fru, causing fru_records[max_nr_fru] to be read before the
> condition is checked.
> 
> While the garbage pointer value assigned to rec is never dereferenced
> (the loop exits immediately), this is technically undefined behavior
> and would be flagged by UBSan and static analyzers.

You mention UBSAN as an example. Is that correct for this issue? Would
KASAN be a better example?

> 
> Fix by using short-circuit evaluation with && to check the bound first,
> only accessing the array when i is within range:
> 
>   for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), 1); i++)
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index 4ccaaf7b70bf..91c49080873e 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -169,7 +169,7 @@ static unsigned int spa_nr_entries;
>  static DEFINE_MUTEX(fmpm_update_mutex);
>  
>  #define for_each_fru(i, rec) \
> -	for (i = 0; rec = fru_records[i], i < max_nr_fru; i++)
> +	for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), 1); i++)

I see there's are a couple of similar cases to this elsewhere in the
kernel.

I'd prefer using "( , true)" to clearly indicate a boolean for the
conditional. Using "( , 1)" looks too much like an index/value at first
glance. At least, it does to me.

Thanks,
Yazen

[PATCH 2/4] RAS/amd/fmpm: Clear new records bitmap before rollback

[Rui Qi]

save_new_records() uses a stack bitmap to track which ERST records were
created during the current initialization pass. If a later write fails,
the rollback path tests this bitmap to decide which records should be
removed again.

DECLARE_BITMAP() does not initialize stack storage, so the rollback path
can observe stale bits and attempt to clear records that were not created
by this function. Clear the bitmap before it is used so that only records
successfully written in the current pass are rolled back.

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 91c49080873e..0231163e2634 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -533,6 +533,8 @@ static int save_new_records(void)
 	unsigned int i;
 	int ret = 0;
 
+	bitmap_zero(new_records, FMPM_MAX_NR_FRU);
+
 	for_each_fru(i, rec) {
 		/* No need to update saved records that match the current record size. */
 		if (rec->hdr.record_length == max_rec_len)
-- 
2.20.1

Re: [PATCH 2/4] RAS/amd/fmpm: Clear new records bitmap before rollback

[Yazen Ghannam]

On Fri, Aug 21, 2026 at 05:47:46PM +0800, Rui Qi wrote:
> save_new_records() uses a stack bitmap to track which ERST records were
> created during the current initialization pass. If a later write fails,
> the rollback path tests this bitmap to decide which records should be
> removed again.
> 
> DECLARE_BITMAP() does not initialize stack storage, so the rollback path
> can observe stale bits and attempt to clear records that were not created
> by this function. Clear the bitmap before it is used so that only records
> successfully written in the current pass are rolled back.
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index 91c49080873e..0231163e2634 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -533,6 +533,8 @@ static int save_new_records(void)
>  	unsigned int i;
>  	int ret = 0;
>  
> +	bitmap_zero(new_records, FMPM_MAX_NR_FRU);
> +

Rather than clear the bitmap, it can be initialized to zero.

  DECLARE_BITMAP(new_records, FMPM_MAX_NR_FRU) = { 0 }

Thanks,
Yazen

[PATCH 3/4] RAS/amd/fmpm: Make max_nr_entries read-only

[Rui Qi]

max_nr_entries is used during module init to calculate max_rec_len.
That length determines the size of each allocated FRU record and is not
resized after init.

Leaving the parameter writable lets a later sysfs write raise the runtime
limit used by update_fru_record(), allowing entries beyond the allocated
flexible array to be written.

Expose the parameter as read-only so it can still be set at module load
time, but cannot diverge from the allocation size afterwards.

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 0231163e2634..14a103de9d62 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -138,7 +138,7 @@ static struct dentry *fmpm_dfs_entries;
  * No input or '0' will default to FMPM_DEFAULT_MAX_NR_ENTRIES.
  */
 static u8 max_nr_entries;
-module_param(max_nr_entries, byte, 0644);
+module_param(max_nr_entries, byte, 0444);
 MODULE_PARM_DESC(max_nr_entries,
 		 "Maximum number of memory poison descriptor entries per FRU");
 
-- 
2.20.1

Re: [PATCH 3/4] RAS/amd/fmpm: Make max_nr_entries read-only

[Yazen Ghannam]

On Fri, Aug 21, 2026 at 05:47:47PM +0800, Rui Qi wrote:
> max_nr_entries is used during module init to calculate max_rec_len.
> That length determines the size of each allocated FRU record and is not
> resized after init.
> 
> Leaving the parameter writable lets a later sysfs write raise the runtime
> limit used by update_fru_record(), allowing entries beyond the allocated
> flexible array to be written.
> 
> Expose the parameter as read-only so it can still be set at module load
> time, but cannot diverge from the allocation size afterwards.
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index 0231163e2634..14a103de9d62 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -138,7 +138,7 @@ static struct dentry *fmpm_dfs_entries;
>   * No input or '0' will default to FMPM_DEFAULT_MAX_NR_ENTRIES.
>   */
>  static u8 max_nr_entries;
> -module_param(max_nr_entries, byte, 0644);
> +module_param(max_nr_entries, byte, 0444);
>  MODULE_PARM_DESC(max_nr_entries,
>  		 "Maximum number of memory poison descriptor entries per FRU");
>  

Same minor feedback for the $SUBJECT prefix. Otherwise, this looks good
to me.

Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Thanks,
Yazen

[PATCH 4/4] RAS/amd/fmpm: Fix spurious BUG when ERST record enumeration fails

[Rui Qi]

When erst_get_record_id_begin() returns an error, get_saved_records()
jumps to the out_end label which unconditionally calls
erst_get_record_id_end(). This is wrong because:

- If erst_disable is true, begin() returns -ENODEV without
  incrementing the refcount. Then end() hits BUG_ON(erst_disable)
  and panics.

- If mutex_lock_interruptible() is interrupted, begin() returns
  -EINTR without incrementing the refcount. Then end() decrements
  refcount below zero, hitting BUG_ON(refcount < 0).

The comment in erst_get_record_id_end() explicitly states that it
should not be called when erst_get_record_id_begin() failed.

Fix by adding a separate out_free label that only does kfree(),
skipping the erst_get_record_id_end() call when begin() failed.

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 14a103de9d62..22627f6278c0 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -675,7 +675,7 @@ static int get_saved_records(void)
 
 	ret = erst_get_record_id_begin(&pos);
 	if (ret < 0)
-		goto out_end;
+		goto out_free;
 
 	while (!erst_get_record_id_next(&pos, &record_id)) {
 		if (record_id == APEI_ERST_INVALID_RECORD_ID)
@@ -716,6 +716,7 @@ static int get_saved_records(void)
 
 out_end:
 	erst_get_record_id_end();
+out_free:
 	kfree(old);
 out:
 	return ret;
-- 
2.20.1

Re: [PATCH 4/4] RAS/amd/fmpm: Fix spurious BUG when ERST record enumeration fails

[Yazen Ghannam]

On Fri, Aug 21, 2026 at 05:47:48PM +0800, Rui Qi wrote:
> When erst_get_record_id_begin() returns an error, get_saved_records()
> jumps to the out_end label which unconditionally calls
> erst_get_record_id_end(). This is wrong because:
> 
> - If erst_disable is true, begin() returns -ENODEV without
>   incrementing the refcount. Then end() hits BUG_ON(erst_disable)
>   and panics.
> 
> - If mutex_lock_interruptible() is interrupted, begin() returns
>   -EINTR without incrementing the refcount. Then end() decrements
>   refcount below zero, hitting BUG_ON(refcount < 0).
> 
> The comment in erst_get_record_id_end() explicitly states that it
> should not be called when erst_get_record_id_begin() failed.

The comment doesn't say that exactly. It does say that *id_end() should
not be called if "erst_disable is true".

> 
> Fix by adding a separate out_free label that only does kfree(),
> skipping the erst_get_record_id_end() call when begin() failed.
> 

A new label isn't necessarily needed. Please see below.

> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index 14a103de9d62..22627f6278c0 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -675,7 +675,7 @@ static int get_saved_records(void)
>  
>  	ret = erst_get_record_id_begin(&pos);
>  	if (ret < 0)
> -		goto out_end;
> +		goto out_free;

Change this to "goto out;" ... 

>  
>  	while (!erst_get_record_id_next(&pos, &record_id)) {
>  		if (record_id == APEI_ERST_INVALID_RECORD_ID)
> @@ -716,6 +716,7 @@ static int get_saved_records(void)
>  
>  out_end:
>  	erst_get_record_id_end();
> +out_free:
>  	kfree(old);
>  out:

... and move "out:" above kfree(old);

>  	return ret;


It's safe to call kfree() on a NULL pointer. So the memory allocation
failure path is not affected.

Thanks,
Yazen

[PATCH v2 0/4] RAS/AMD/FMPM: Fix OOB, uninitialized data, and error-handling bugs

[Rui Qi]

Hi Yazen, Borislav, Tony,

This series fixes several bugs in the AMD FRU Memory Poison Manager
driver.

Patch 1 fixes an out-of-bounds read in the for_each_fru macro caused by
the comma operator evaluating the array access before the bounds check.

Patch 2 fixes an uninitialized stack bitmap in save_new_records() that
could cause the rollback path to clear ERST records that were not created
in the current initialization pass.

Patch 3 makes the max_nr_entries module parameter read-only (0444),
preventing runtime writes that could exceed the allocated flexible array
size.

Patch 4 fixes a spurious BUG when erst_get_record_id_begin() fails,
because the error path unconditionally calls erst_get_record_id_end()
which triggers BUG_ON.

All four bugs have been present since the original introduction of the
AMD FMPM driver.

Changes since v1 [1]:
- All patches: Use RAS/AMD/FMPM: subject prefix to match existing
  convention (Yazen Ghannam)
- Patch 1: Replace UBSan with KASAN in commit message, as KASAN is the
  appropriate sanitizer for out-of-bounds memory accesses (Yazen Ghannam)
- Patch 1: Use ", true" instead of ", 1" in the for_each_fru macro to
  clearly indicate a boolean value (Yazen Ghannam)
- Patch 2: Initialize DECLARE_BITMAP at declaration with = { 0 } instead
  of calling bitmap_zero() separately (Yazen Ghannam)
- Patch 4: Fix commit message to accurately describe the comment in
  erst_get_record_id_end() (Yazen Ghannam)
- Patch 4: Simplify error path by using goto out and moving the out:
  label above kfree(old), removing the out_free label (Yazen Ghannam)

[1] https://lore.kernel.org/r/20260821094748.145394-1-qirui.001@bytedance.com

Rui Qi (4):
  RAS/AMD/FMPM: Fix out-of-bounds read in for_each_fru macro
  RAS/AMD/FMPM: Clear new records bitmap before rollback
  RAS/AMD/FMPM: Make max_nr_entries read-only
  RAS/AMD/FMPM: Fix spurious BUG when ERST record enumeration fails

 drivers/ras/amd/fmpm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--
2.20.1

[PATCH v2 1/4] RAS/AMD/FMPM: Fix out-of-bounds read in for_each_fru macro

[Rui Qi]

The for_each_fru macro evaluates the array access "rec = fru_records[i]"
before the bounds check "i < max_nr_fru" due to the comma operator's
left-to-right evaluation order. When the loop terminates, i equals
max_nr_fru, causing fru_records[max_nr_fru] to be read before the
condition is checked.

While the garbage pointer value assigned to rec is never dereferenced
(the loop exits immediately), this is technically undefined behavior
and would be flagged by KASAN and static analyzers.

Fix by using short-circuit evaluation with && to check the bound first,
only accessing the array when i is within range:

  for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), true); i++)

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 4ccaaf7b70bf..81d7f02c053d 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -169,7 +169,7 @@ static unsigned int spa_nr_entries;
 static DEFINE_MUTEX(fmpm_update_mutex);
 
 #define for_each_fru(i, rec) \
-	for (i = 0; rec = fru_records[i], i < max_nr_fru; i++)
+	for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), true); i++)
 
 static inline u32 get_fmp_len(struct fru_rec *rec)
 {
-- 
2.20.1

Re: [PATCH v2 1/4] RAS/AMD/FMPM: Fix out-of-bounds read in for_each_fru macro

[Yazen Ghannam]

On Wed, Aug 26, 2026 at 11:53:11AM +0800, Rui Qi wrote:
> The for_each_fru macro evaluates the array access "rec = fru_records[i]"
> before the bounds check "i < max_nr_fru" due to the comma operator's
> left-to-right evaluation order. When the loop terminates, i equals
> max_nr_fru, causing fru_records[max_nr_fru] to be read before the
> condition is checked.
> 
> While the garbage pointer value assigned to rec is never dereferenced
> (the loop exits immediately), this is technically undefined behavior
> and would be flagged by KASAN and static analyzers.
> 
> Fix by using short-circuit evaluation with && to check the bound first,
> only accessing the array when i is within range:
> 
>   for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), true); i++)
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index 4ccaaf7b70bf..81d7f02c053d 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -169,7 +169,7 @@ static unsigned int spa_nr_entries;
>  static DEFINE_MUTEX(fmpm_update_mutex);
>  
>  #define for_each_fru(i, rec) \
> -	for (i = 0; rec = fru_records[i], i < max_nr_fru; i++)
> +	for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), true); i++)
>  
>  static inline u32 get_fmp_len(struct fru_rec *rec)
>  {
> -- 

Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Thanks,
Yazen

[PATCH v2 2/4] RAS/AMD/FMPM: Clear new records bitmap before rollback

[Rui Qi]

save_new_records() uses a stack bitmap to track which ERST records were
created during the current initialization pass. If a later write fails,
the rollback path tests this bitmap to decide which records should be
removed again.

DECLARE_BITMAP() does not initialize stack storage, so the rollback path
can observe stale bits and attempt to clear records that were not created
by this function. Initialize the bitmap to zero at declaration so that
only records successfully written in the current pass are rolled back.

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 81d7f02c053d..e3f7bd053479 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -528,7 +528,7 @@ static void set_rec_fields(struct fru_rec *rec)
 
 static int save_new_records(void)
 {
-	DECLARE_BITMAP(new_records, FMPM_MAX_NR_FRU);
+	DECLARE_BITMAP(new_records, FMPM_MAX_NR_FRU) = { 0 };
 	struct fru_rec *rec;
 	unsigned int i;
 	int ret = 0;
-- 
2.20.1

Re: [PATCH v2 2/4] RAS/AMD/FMPM: Clear new records bitmap before rollback

[Yazen Ghannam]

On Wed, Aug 26, 2026 at 11:53:12AM +0800, Rui Qi wrote:
> save_new_records() uses a stack bitmap to track which ERST records were
> created during the current initialization pass. If a later write fails,
> the rollback path tests this bitmap to decide which records should be
> removed again.
> 
> DECLARE_BITMAP() does not initialize stack storage, so the rollback path
> can observe stale bits and attempt to clear records that were not created
> by this function. Initialize the bitmap to zero at declaration so that
> only records successfully written in the current pass are rolled back.
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index 81d7f02c053d..e3f7bd053479 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -528,7 +528,7 @@ static void set_rec_fields(struct fru_rec *rec)
>  
>  static int save_new_records(void)
>  {
> -	DECLARE_BITMAP(new_records, FMPM_MAX_NR_FRU);
> +	DECLARE_BITMAP(new_records, FMPM_MAX_NR_FRU) = { 0 };
>  	struct fru_rec *rec;
>  	unsigned int i;
>  	int ret = 0;
> -- 

Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Thanks,
Yazen

[PATCH v2 3/4] RAS/AMD/FMPM: Make max_nr_entries read-only

[Rui Qi]

max_nr_entries is used during module init to calculate max_rec_len.
That length determines the size of each allocated FRU record and is not
resized after init.

Leaving the parameter writable lets a later sysfs write raise the runtime
limit used by update_fru_record(), allowing entries beyond the allocated
flexible array to be written.

Expose the parameter as read-only so it can still be set at module load
time, but cannot diverge from the allocation size afterwards.

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index e3f7bd053479..c13db1f743e5 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -138,7 +138,7 @@ static struct dentry *fmpm_dfs_entries;
  * No input or '0' will default to FMPM_DEFAULT_MAX_NR_ENTRIES.
  */
 static u8 max_nr_entries;
-module_param(max_nr_entries, byte, 0644);
+module_param(max_nr_entries, byte, 0444);
 MODULE_PARM_DESC(max_nr_entries,
 		 "Maximum number of memory poison descriptor entries per FRU");
 
-- 
2.20.1

Re: [PATCH v2 3/4] RAS/AMD/FMPM: Make max_nr_entries read-only

[Yazen Ghannam]

On Wed, Aug 26, 2026 at 11:53:13AM +0800, Rui Qi wrote:
> max_nr_entries is used during module init to calculate max_rec_len.
> That length determines the size of each allocated FRU record and is not
> resized after init.
> 
> Leaving the parameter writable lets a later sysfs write raise the runtime
> limit used by update_fru_record(), allowing entries beyond the allocated
> flexible array to be written.
> 
> Expose the parameter as read-only so it can still be set at module load
> time, but cannot diverge from the allocation size afterwards.
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index e3f7bd053479..c13db1f743e5 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -138,7 +138,7 @@ static struct dentry *fmpm_dfs_entries;
>   * No input or '0' will default to FMPM_DEFAULT_MAX_NR_ENTRIES.
>   */
>  static u8 max_nr_entries;
> -module_param(max_nr_entries, byte, 0644);
> +module_param(max_nr_entries, byte, 0444);
>  MODULE_PARM_DESC(max_nr_entries,
>  		 "Maximum number of memory poison descriptor entries per FRU");
>  
> -- 

Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Thanks,
Yazen

[PATCH v2 4/4] RAS/AMD/FMPM: Fix spurious BUG when ERST record enumeration fails

[Rui Qi]

When erst_get_record_id_begin() returns an error, get_saved_records()
jumps to the out_end label which unconditionally calls
erst_get_record_id_end(). This is wrong because:

- If erst_disable is true, begin() returns -ENODEV without
  incrementing the refcount. Then end() hits BUG_ON(erst_disable)
  and panics.

- If mutex_lock_interruptible() is interrupted, begin() returns
  -EINTR without incrementing the refcount. Then end() decrements
  refcount below zero, hitting BUG_ON(refcount < 0).

The comment in erst_get_record_id_end() warns that it should not be
called when erst_disable is true, so callers must not invoke it after
begin() fails.

Fix by jumping to the out label when begin() fails, skipping the
erst_get_record_id_end() call. This is safe because kfree() handles
NULL pointers.

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index c13db1f743e5..48a437042953 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -673,7 +673,7 @@ static int get_saved_records(void)
 
 	ret = erst_get_record_id_begin(&pos);
 	if (ret < 0)
-		goto out_end;
+		goto out;
 
 	while (!erst_get_record_id_next(&pos, &record_id)) {
 		if (record_id == APEI_ERST_INVALID_RECORD_ID)
@@ -714,8 +714,8 @@ static int get_saved_records(void)
 
 out_end:
 	erst_get_record_id_end();
-	kfree(old);
 out:
+	kfree(old);
 	return ret;
 }
 
-- 
2.20.1

Re: [PATCH v2 4/4] RAS/AMD/FMPM: Fix spurious BUG when ERST record enumeration fails

[Yazen Ghannam]

On Wed, Aug 26, 2026 at 11:53:14AM +0800, Rui Qi wrote:
> When erst_get_record_id_begin() returns an error, get_saved_records()
> jumps to the out_end label which unconditionally calls
> erst_get_record_id_end(). This is wrong because:
> 
> - If erst_disable is true, begin() returns -ENODEV without
>   incrementing the refcount. Then end() hits BUG_ON(erst_disable)
>   and panics.
> 
> - If mutex_lock_interruptible() is interrupted, begin() returns
>   -EINTR without incrementing the refcount. Then end() decrements
>   refcount below zero, hitting BUG_ON(refcount < 0).
> 
> The comment in erst_get_record_id_end() warns that it should not be
> called when erst_disable is true, so callers must not invoke it after
> begin() fails.
> 
> Fix by jumping to the out label when begin() fails, skipping the
> erst_get_record_id_end() call. This is safe because kfree() handles
> NULL pointers.
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index c13db1f743e5..48a437042953 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -673,7 +673,7 @@ static int get_saved_records(void)
>  
>  	ret = erst_get_record_id_begin(&pos);
>  	if (ret < 0)
> -		goto out_end;
> +		goto out;
>  
>  	while (!erst_get_record_id_next(&pos, &record_id)) {
>  		if (record_id == APEI_ERST_INVALID_RECORD_ID)
> @@ -714,8 +714,8 @@ static int get_saved_records(void)
>  
>  out_end:
>  	erst_get_record_id_end();
> -	kfree(old);
>  out:
> +	kfree(old);
>  	return ret;
>  }
>  
> -- 

The patch is okay, but the 'erst_disable' part didn't make sense to me.
So I went over it with an AI assistant. Response is below.

Basically, the commit message needs to be reworded to cover the actual
issue.

Thanks,
Yazen

=========================

`erst_get_record_id_begin()` has two ways to fail, and the commit
message describes both. Only one of them can happen when fmpm calls it.

```c
int erst_get_record_id_begin(int *pos)
{
	if (erst_disable)
		return -ENODEV;			/* case 1 */

	rc = mutex_lock_interruptible(&erst_record_id_cache.lock);
	if (rc)
		return rc;			/* case 2: -EINTR */
	erst_record_id_cache.refcount++;
	...
```

**Case 1 (`-ENODEV`) can't happen from `get_saved_records()`:**
- `fru_mem_poison_init()` already returns `-ENODEV` when `erst_disable`
  is set, before it calls `get_saved_records()`.
- `erst_disable` has only two writers: the `erst_disable` boot parameter
  (`__setup`) and the error path of `erst_init()`.
- `erst_init()` is a `device_initcall` in `drivers/acpi/`, which links
  ahead of `drivers/ras/`. When fmpm is built in, `erst_init()` has
  already run by the time fmpm's initcall runs. When fmpm is a module,
  it loads later still.
- So `erst_disable` can't change between fmpm's check and the `begin()`
  call, and the `BUG_ON(erst_disable)` in `end()` can't fire here.

**Case 2 (`-EINTR`) can happen, but only under narrow conditions:**
- The lock has to be contended. `mutex_lock_interruptible()` takes an
  uncontended lock without checking for signals. Other code that takes
  `erst_record_id_cache.lock` includes the other `begin()` callers:
  `erst_open_pstore()`, `erst_dbg_open()` and `apei_read_mce()`.
- A signal has to be pending while the task waits. That's realistic when
  fmpm is a module, for example Ctrl-C or SIGKILL sent to `modprobe`
  while pstore or erst-dbg holds the lock. When fmpm is built in, its
  init runs in the `kernel_init` thread before userspace starts, so no
  signal can reach it.
- Before the patch, this path calls `end()`. The refcount drops to -1
  and hits `BUG_ON(refcount < 0)` while `erst_record_id_cache.lock` is
  held. The oops kills the task with the mutex still locked, so every
  later ERST user blocks on it forever.

In short, "reachable" was loose wording. The `-ENODEV` case can't happen
from fmpm at all. The `-EINTR` case can happen, but only when fmpm is a
module, the lock is contended, and the load is interrupted by a signal.
The patch fixes that case, which is real. The commit message just leads
with the case that can't happen from fmpm and understates the
consequence of the one that can.

Re: [PATCH v2 0/4] RAS/AMD/FMPM: Fix OOB, uninitialized data, and error-handling bugs

[Rui Qi]

On 8/26/26 11:53 AM, Rui Qi wrote:
> Hi Yazen, Borislav, Tony,
> 
> This series fixes several bugs in the AMD FRU Memory Poison Manager
> driver.
> 
> Patch 1 fixes an out-of-bounds read in the for_each_fru macro caused by
> the comma operator evaluating the array access before the bounds check.
> 
> Patch 2 fixes an uninitialized stack bitmap in save_new_records() that
> could cause the rollback path to clear ERST records that were not created
> in the current initialization pass.
> 
> Patch 3 makes the max_nr_entries module parameter read-only (0444),
> preventing runtime writes that could exceed the allocated flexible array
> size.
> 
> Patch 4 fixes a spurious BUG when erst_get_record_id_begin() fails,
> because the error path unconditionally calls erst_get_record_id_end()
> which triggers BUG_ON.
> 
> All four bugs have been present since the original introduction of the
> AMD FMPM driver.
> 
> Changes since v1 [1]:
> - All patches: Use RAS/AMD/FMPM: subject prefix to match existing
>   convention (Yazen Ghannam)
> - Patch 1: Replace UBSan with KASAN in commit message, as KASAN is the
>   appropriate sanitizer for out-of-bounds memory accesses (Yazen Ghannam)
> - Patch 1: Use ", true" instead of ", 1" in the for_each_fru macro to
>   clearly indicate a boolean value (Yazen Ghannam)
> - Patch 2: Initialize DECLARE_BITMAP at declaration with = { 0 } instead
>   of calling bitmap_zero() separately (Yazen Ghannam)
> - Patch 4: Fix commit message to accurately describe the comment in
>   erst_get_record_id_end() (Yazen Ghannam)
> - Patch 4: Simplify error path by using goto out and moving the out:
>   label above kfree(old), removing the out_free label (Yazen Ghannam)
> 
> [1] https://lore.kernel.org/r/20260821094748.145394-1-qirui.001@bytedance.com
> 
> Rui Qi (4):
>   RAS/AMD/FMPM: Fix out-of-bounds read in for_each_fru macro
>   RAS/AMD/FMPM: Clear new records bitmap before rollback
>   RAS/AMD/FMPM: Make max_nr_entries read-only
>   RAS/AMD/FMPM: Fix spurious BUG when ERST record enumeration fails
> 
>  drivers/ras/amd/fmpm.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> --
> 2.20.1

Hi Yazen,

Gentle ping on this series.

This v2 incorporates all your feedback on v1, including the subject
prefix updates and the suggested changes to patches 1, 2, and 4.
Could you please take another look when you have a chance?

Thanks,
Rui