[PATCH 0/9] EDAC/versalnet: Fix error handling, teardown, and robustness

[PATCH 0/9] EDAC/versalnet: Fix error handling, teardown, and robustness

[Shubhrajyoti Datta]

    This series addresses several correctness and robustness issues in
    the VersalNet EDAC driver

    The main fixes are:

    * Robustness fixes
      - Add NULL checks for mci in handle_error() and remove_one_mc()
        to prevent NULL pointer dereferences when a controller has not
        been fully initialized.
      - Add bounds validation in rpmsg_cb().

    * Teardown improvements
      - Fix teardown ordering in mc_remove() and remove_one_mc() to
        prevent use-after-free conditions.

    * Probe and initialization cleanup
      - Move platform_set_drvdata() from init_one_mc() to mc_probe().
      - Reorder probe flow to initialize MCDI before RPMsg registration.
      - Remove unused pdev parameters from internal initialization
        functions.

    * Resource management fixes
      - Fix error handling in init_one_mc() to avoid a potential
        double-free by reordering device_register() and edac_mc_alloc().
      - Replace sprintf() with init_name by dev_set_name() to avoid
        use-after-free issues caused by deferred kobject cleanup.

    * Code cleanup
      - Use a designated initializer for rpmsg_channel_info.



Prasanna Kumar T S M (1):
  EDAC/versalnet: Fix device_register() error handling in init_one_mc()

Shubhrajyoti Datta (8):
  EDAC/versalnet: Add NULL check for mci in handle_error()
  EDAC/versalnet: Add NULL check for mci in remove_one_mc()
  EDAC/versalnet: Move platform_set_drvdata() to mc_probe()
  EDAC/versalnet: Use dev_set_name() instead of sprintf with init_name
  EDAC/versalnet: Initialize MCDI before RPMsg registration
  EDAC/versalnet: Add bounds validation in rpmsg_cb()
  EDAC/versalnet: Fix use-after-free in remove_one_mc()
  EDAC/versalnet: Use designated initializer for rpmsg_channel_info

 drivers/edac/versalnet_edac.c | 95 +++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 44 deletions(-)

-- 
2.34.1

[PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Shubhrajyoti Datta]

Add a NULL pointer check for mci before use in handle_error() to
prevent a potential NULL dereference when the memory controller
instance is not initialized for a given controller number.

This is added as a defensive check. In our current firmware
the event will not be generated.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index d1af5e175f7e..316f8f79c4d8 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -439,6 +439,8 @@ static void handle_error(struct mc_priv  *priv, struct ecc_status *stat,
 		return;
 
 	mci = priv->mci[ctl_num];
+	if (!mci)
+		return;
 
 	if (stat->error_type == MC5_ERR_TYPE_CE) {
 		pinf = stat->ceinfo[stat->channel];
-- 
2.34.1

Re: [PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Borislav Petkov]

On Fri, Jul 24, 2026 at 10:49:37PM +0530, Shubhrajyoti Datta wrote:
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index d1af5e175f7e..316f8f79c4d8 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -439,6 +439,8 @@ static void handle_error(struct mc_priv  *priv, struct ecc_status *stat,
>  		return;
>  
>  	mci = priv->mci[ctl_num];
> +	if (!mci)
> +		return;

You have a WARN_ON_ONCE right before that line which checks against
NUM_CONTROLLERS and init_versalnet() unwinds all the setup the moment
init_one_mc() fails for one of the MCs.

So why are we adding dead code?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Pandey, Radhey Shyam]

On 7/27/2026 5:22 AM, Borislav Petkov wrote:
> On Fri, Jul 24, 2026 at 10:49:37PM +0530, Shubhrajyoti Datta wrote:
>> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
>> index d1af5e175f7e..316f8f79c4d8 100644
>> --- a/drivers/edac/versalnet_edac.c
>> +++ b/drivers/edac/versalnet_edac.c
>> @@ -439,6 +439,8 @@ static void handle_error(struct mc_priv  *priv, struct ecc_status *stat,
>>   		return;
>>   
>>   	mci = priv->mci[ctl_num];
>> +	if (!mci)
>> +		return;
> 
> You have a WARN_ON_ONCE right before that line which checks against
> NUM_CONTROLLERS and init_versalnet() unwinds all the setup the moment
> init_one_mc() fails for one of the MCs.

The gap in the driver is that init_one_mc() returns success without
setting priv->mci[i] when the bus width decodes to DEV_UNKNOWN, while
buggy firmware / future firmware rpmsg error path can still calls
handle_error().

Happy to drop the check if you prefer otherwise we can keep it as a
cheap guard for a firmware/driver mismatch.

Shubrajyoti: Please feel free to add/correct. I'm still ramping up on
this driver and may have overlooked details.

> 
> So why are we adding dead code?
> 

[PATCH 2/9] EDAC/versalnet: Add NULL check for mci in remove_one_mc()

[Shubhrajyoti Datta]

If a controller's configuration specifies an unrecognized bus width,
init_one_mc() returns 0 but skips allocating priv->mci[ctl_num], leaving
it NULL. On module unload or probe failure, remove_one_mc() is called for
all controllers and unconditionally dereferences priv->mci[i], causing a
kernel panic.

Add a NULL pointer check for mci before dereferencing it.

 Unable to handle kernel NULL pointer dereference at virtual address 0000000000000390
 Internal error: Oops: 0000000096000004 [#1] SMP
 Hardware name: Xilinx Versal NET VNX (DT)
 pc : mc_remove+0x34/0x88
 lr : mc_remove+0x4c/0x88
 Call trace:
  mc_remove+0x34/0x88
  platform_remove+0x2c/0x70
  device_remove+0x48/0x7c
  device_release_driver_internal+0x1c8/0x224
  device_driver_detach+0x18/0x28
  unbind_store+0xb4/0xb8

Fixes: 62a9fc50e8d9 ("EDAC/versalnet: Refactor memory controller initialization and cleanup")
Cc: stable@vger.kernel.org
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 316f8f79c4d8..05dc34504cc2 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -769,6 +769,9 @@ static void remove_one_mc(struct mc_priv *priv, int i)
 	struct mem_ctl_info *mci;
 
 	mci = priv->mci[i];
+	if (!mci)
+		return;
+
 	device_unregister(mci->pdev);
 	edac_mc_del_mc(mci->pdev);
 	edac_mc_free(mci);
-- 
2.34.1

Re: [PATCH 2/9] EDAC/versalnet: Add NULL check for mci in remove_one_mc()

[Pandey, Radhey Shyam]

On 7/24/2026 10:49 PM, Shubhrajyoti Datta wrote:
> If a controller's configuration specifies an unrecognized bus width,
> init_one_mc() returns 0 but skips allocating priv->mci[ctl_num], leaving
> it NULL. On module unload or probe failure, remove_one_mc() is called for
> all controllers and unconditionally dereferences priv->mci[i], causing a
> kernel panic.
> 
> Add a NULL pointer check for mci before dereferencing it.
> 
>   Unable to handle kernel NULL pointer dereference at virtual address 0000000000000390
>   Internal error: Oops: 0000000096000004 [#1] SMP
>   Hardware name: Xilinx Versal NET VNX (DT)
>   pc : mc_remove+0x34/0x88
>   lr : mc_remove+0x4c/0x88
>   Call trace:
>    mc_remove+0x34/0x88
>    platform_remove+0x2c/0x70
>    device_remove+0x48/0x7c
>    device_release_driver_internal+0x1c8/0x224
>    device_driver_detach+0x18/0x28
>    unbind_store+0xb4/0xb8
> 
> Fixes: 62a9fc50e8d9 ("EDAC/versalnet: Refactor memory controller initialization and cleanup")

Confirm on fixes tag. The bug likely goes back to the original driver.
> Cc: stable@vger.kernel.org

There is no CC to stable kernel in email.

Below changes looks fine to me. With above tag fixed.

Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Thanks!

> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> 
>   drivers/edac/versalnet_edac.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 316f8f79c4d8..05dc34504cc2 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -769,6 +769,9 @@ static void remove_one_mc(struct mc_priv *priv, int i)
>   	struct mem_ctl_info *mci;
>   
>   	mci = priv->mci[i];
> +	if (!mci)
> +		return;
> +
>   	device_unregister(mci->pdev);
>   	edac_mc_del_mc(mci->pdev);
>   	edac_mc_free(mci);

[PATCH 3/9] EDAC/versalnet: Move platform_set_drvdata() to mc_probe()

[Shubhrajyoti Datta]

Move platform_set_drvdata() out of init_one_mc() and into mc_probe()
so that the driver data is set once during probe rather than being
redundantly set on each memory controller initialization.

The pdev parameter in init_one_mc() and init_versalnet() is no longer
referenced. Remove it from both function signatures.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 05dc34504cc2..03b6e0958f17 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -777,7 +777,7 @@ static void remove_one_mc(struct mc_priv *priv, int i)
 	edac_mc_free(mci);
 }
 
-static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i)
+static int init_one_mc(struct mc_priv *priv, int i)
 {
 	u32 num_chans, rank, dwidth, config;
 	struct edac_mc_layer layers[2];
@@ -849,8 +849,6 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
 	priv->mci[i] = mci;
 	priv->dwidth = dt;
 
-	platform_set_drvdata(pdev, priv);
-
 	return 0;
 
 err_unreg:
@@ -863,12 +861,12 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
 	return rc;
 }
 
-static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
+static int init_versalnet(struct mc_priv *priv)
 {
 	int rc, i;
 
 	for (i = 0; i < NUM_CONTROLLERS; i++) {
-		rc = init_one_mc(priv, pdev, i);
+		rc = init_one_mc(priv, i);
 		if (rc) {
 			while (i--)
 				remove_one_mc(priv, i);
@@ -914,6 +912,7 @@ static int mc_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
+	platform_set_drvdata(pdev, priv);
 	amd_rpmsg_id_table[0].driver_data = (kernel_ulong_t)priv;
 
 	rc = register_rpmsg_driver(&amd_rpmsg_driver);
@@ -928,7 +927,7 @@ static int mc_probe(struct platform_device *pdev)
 
 	priv->mcdi->r5_rproc = rp;
 
-	rc = init_versalnet(priv, pdev);
+	rc = init_versalnet(priv);
 	if (rc)
 		goto err_init;
 
-- 
2.34.1

Re: [PATCH 3/9] EDAC/versalnet: Move platform_set_drvdata() to mc_probe()

[Pandey, Radhey Shyam]

On 7/24/2026 10:49 PM, Shubhrajyoti Datta wrote:
> Move platform_set_drvdata() out of init_one_mc() and into mc_probe()
> so that the driver data is set once during probe rather than being
> redundantly set on each memory controller initialization.
> 
> The pdev parameter in init_one_mc() and init_versalnet() is no longer
> referenced. Remove it from both function signatures.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Thanks!
> ---
> 
>   drivers/edac/versalnet_edac.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 05dc34504cc2..03b6e0958f17 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -777,7 +777,7 @@ static void remove_one_mc(struct mc_priv *priv, int i)
>   	edac_mc_free(mci);
>   }
>   
> -static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i)
> +static int init_one_mc(struct mc_priv *priv, int i)
>   {
>   	u32 num_chans, rank, dwidth, config;
>   	struct edac_mc_layer layers[2];
> @@ -849,8 +849,6 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
>   	priv->mci[i] = mci;
>   	priv->dwidth = dt;
>   
> -	platform_set_drvdata(pdev, priv);
> -
>   	return 0;
>   
>   err_unreg:
> @@ -863,12 +861,12 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
>   	return rc;
>   }
>   
> -static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
> +static int init_versalnet(struct mc_priv *priv)
>   {
>   	int rc, i;
>   
>   	for (i = 0; i < NUM_CONTROLLERS; i++) {
> -		rc = init_one_mc(priv, pdev, i);
> +		rc = init_one_mc(priv, i);
>   		if (rc) {
>   			while (i--)
>   				remove_one_mc(priv, i);
> @@ -914,6 +912,7 @@ static int mc_probe(struct platform_device *pdev)
>   		goto err_alloc;
>   	}
>   
> +	platform_set_drvdata(pdev, priv);
>   	amd_rpmsg_id_table[0].driver_data = (kernel_ulong_t)priv;
>   
>   	rc = register_rpmsg_driver(&amd_rpmsg_driver);
> @@ -928,7 +927,7 @@ static int mc_probe(struct platform_device *pdev)
>   
>   	priv->mcdi->r5_rproc = rp;
>   
> -	rc = init_versalnet(priv, pdev);
> +	rc = init_versalnet(priv);
>   	if (rc)
>   		goto err_init;
>   

[PATCH 4/9] EDAC/versalnet: Fix device_register() error handling in init_one_mc()

[Shubhrajyoti Datta]

From: Prasanna Kumar T S M <ptsm@linux.microsoft.com>

When device_register() fails, it must be followed by put_device()
rather than kfree(), because device_register() calls
device_initialize() which sets up the device refcount. The matching
release function versal_edac_release() handles the actual kfree().

To simplify error handling and avoid complex unwinding, split
device_register() into device_initialize() and device_add().
Initialize the device early so put_device() can be used in all
error paths.

Fixes: d5fe2fec6c40 ("EDAC: Add a driver for the AMD Versal NET DDR controller")
Cc: stable@vger.kernel.org
Signed-off-by: Prasanna Kumar T S M <ptsm@linux.microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 03b6e0958f17..3c9eaea5a106 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -785,7 +785,7 @@ static int init_one_mc(struct mc_priv *priv, int i)
 	char name[MC_NAME_LEN];
 	struct device *dev;
 	enum dev_type dt;
-	int rc;
+	int rc = -ENOMEM;
 
 	config = priv->adec[CONF + i * ADEC_NUM];
 	num_chans = FIELD_GET(MC5_NUM_CHANS_MASK, config);
@@ -817,23 +817,23 @@ static int init_one_mc(struct mc_priv *priv, int i)
 	layers[1].size = num_chans;
 	layers[1].is_virt_csrow = false;
 
-	rc = -ENOMEM;
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return rc;
 
-	mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers, sizeof(struct mc_priv));
-	if (!mci) {
-		edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for MC%d\n", i);
-		goto err_dev_free;
-	}
-
 	sprintf(name, "versal-net-ddrmc5-edac-%d", i);
 
 	dev->init_name = name;
 	dev->release = versal_edac_release;
+	device_initialize(dev);
 
-	rc = device_register(dev);
+	mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers, sizeof(struct mc_priv));
+	if (!mci) {
+		edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for MC%d\n", i);
+		goto err_put_dev;
+	}
+
+	rc = device_add(dev);
 	if (rc)
 		goto err_mc_free;
 
@@ -843,7 +843,7 @@ static int init_one_mc(struct mc_priv *priv, int i)
 	rc = edac_mc_add_mc(mci);
 	if (rc) {
 		edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with EDAC core\n", i);
-		goto err_unreg;
+		goto err_dev_del;
 	}
 
 	priv->mci[i] = mci;
@@ -851,12 +851,12 @@ static int init_one_mc(struct mc_priv *priv, int i)
 
 	return 0;
 
-err_unreg:
-	device_unregister(mci->pdev);
+err_dev_del:
+	device_del(dev);
 err_mc_free:
 	edac_mc_free(mci);
-err_dev_free:
-	kfree(dev);
+err_put_dev:
+	put_device(dev);
 
 	return rc;
 }
-- 
2.34.1

[PATCH 5/9] EDAC/versalnet: Use dev_set_name() instead of sprintf with init_name

[Shubhrajyoti Datta]

The previous code used sprintf() to format a device name into a local
stack buffer and then assigned it to dev->init_name. Since kobject
cleanup can be deferred asynchronously (e.g. when
CONFIG_DEBUG_KOBJECT_RELEASE is enabled), dev_name(dev) could be
accessed after init_one_mc() returns and the stack frame containing the
name buffer is gone, resulting in a use-after-free.

This is fixed by switching to dev_set_name(), which dynamically
allocates and manages the name string internally, but the now-unused
local char name[MC_NAME_LEN] buffer and the MC_NAME_LEN macro are
not needed so remove them.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 3c9eaea5a106..1caaba653fc0 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -70,8 +70,6 @@
 #define XDDR5_BUS_WIDTH_32		1
 #define XDDR5_BUS_WIDTH_16		2
 
-#define MC_NAME_LEN			32
-
 /**
  * struct ecc_error_info - ECC error log information.
  * @burstpos:		Burst position.
@@ -782,7 +780,6 @@ static int init_one_mc(struct mc_priv *priv, int i)
 	u32 num_chans, rank, dwidth, config;
 	struct edac_mc_layer layers[2];
 	struct mem_ctl_info *mci;
-	char name[MC_NAME_LEN];
 	struct device *dev;
 	enum dev_type dt;
 	int rc = -ENOMEM;
@@ -821,9 +818,7 @@ static int init_one_mc(struct mc_priv *priv, int i)
 	if (!dev)
 		return rc;
 
-	sprintf(name, "versal-net-ddrmc5-edac-%d", i);
 
-	dev->init_name = name;
 	dev->release = versal_edac_release;
 	device_initialize(dev);
 
@@ -833,6 +828,10 @@ static int init_one_mc(struct mc_priv *priv, int i)
 		goto err_put_dev;
 	}
 
+	rc = dev_set_name(dev, "versal-net-ddrmc5-edac-%d", i);
+	if (rc)
+		goto err_mc_free;
+
 	rc = device_add(dev);
 	if (rc)
 		goto err_mc_free;
-- 
2.34.1

[PATCH 6/9] EDAC/versalnet: Initialize MCDI before RPMsg registration

[Shubhrajyoti Datta]

setup_mcdi() currently allocates the MCDI instance, assigns the RPMsg
endpoint, and retrieves DDR configuration data. The DDR configuration
path invokes cdx_mcdi_rpc(), which requires a functional RPMsg endpoint.

Split setup_mcdi() so that only MCDI allocation and initialization are
performed before RPMsg registration. Move endpoint assignment and DDR
configuration retrieval until after register_rpmsg_driver() succeeds.
Update the error paths to match the new initialization order, ensuring
resources are released in reverse order of acquisition.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 1caaba653fc0..e9561242f292 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -559,7 +559,7 @@ static void get_ddr_config(u32 index, u32 *buffer, struct cdx_mcdi *amd_mcdi)
 static int setup_mcdi(struct mc_priv *mc_priv)
 {
 	struct cdx_mcdi *amd_mcdi;
-	int ret, i;
+	int ret;
 
 	amd_mcdi = kzalloc_obj(*amd_mcdi);
 	if (!amd_mcdi)
@@ -572,12 +572,7 @@ static int setup_mcdi(struct mc_priv *mc_priv)
 		return ret;
 	}
 
-	amd_mcdi->ept = mc_priv->ept;
 	mc_priv->mcdi = amd_mcdi;
-
-	for (i = 0; i < NUM_CONTROLLERS; i++)
-		get_ddr_config(i, &mc_priv->adec[ADEC_NUM * i], amd_mcdi);
-
 	return 0;
 }
 
@@ -886,7 +881,7 @@ static int mc_probe(struct platform_device *pdev)
 {
 	struct mc_priv *priv;
 	struct rproc *rp;
-	int rc;
+	int rc, i;
 
 	struct device_node *r5_core_node __free(device_node) =
 		of_parse_phandle(pdev->dev.of_node, "amd,rproc", 0);
@@ -914,18 +909,22 @@ static int mc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, priv);
 	amd_rpmsg_id_table[0].driver_data = (kernel_ulong_t)priv;
 
+	rc = setup_mcdi(priv);
+	if (rc)
+		goto err_alloc;
+
 	rc = register_rpmsg_driver(&amd_rpmsg_driver);
 	if (rc) {
 		edac_printk(KERN_ERR, EDAC_MC, "Failed to register RPMsg driver: %d\n", rc);
-		goto err_alloc;
-	}
-
-	rc = setup_mcdi(priv);
-	if (rc)
 		goto err_unreg;
+	}
 
+	priv->mcdi->ept = priv->ept;
 	priv->mcdi->r5_rproc = rp;
 
+	for (i = 0; i < NUM_CONTROLLERS; i++)
+		get_ddr_config(i, &priv->adec[ADEC_NUM * i], priv->mcdi);
+
 	rc = init_versalnet(priv);
 	if (rc)
 		goto err_init;
@@ -933,11 +932,11 @@ static int mc_probe(struct platform_device *pdev)
 	return 0;
 
 err_init:
-	cdx_mcdi_finish(priv->mcdi);
-	kfree(priv->mcdi);
+	unregister_rpmsg_driver(&amd_rpmsg_driver);
 
 err_unreg:
-	unregister_rpmsg_driver(&amd_rpmsg_driver);
+	cdx_mcdi_finish(priv->mcdi);
+	kfree(priv->mcdi);
 
 err_alloc:
 	rproc_shutdown(rp);
-- 
2.34.1

[PATCH 7/9] EDAC/versalnet: Add bounds validation in rpmsg_cb()

[Shubhrajyoti Datta]

The firmware-supplied offset and length values from the RPMsg payload
are used without validation to index into mc_priv->regs[] (REG_MAX=152
entries). A malformed or buggy firmware message could write past the end
of the array, corrupting adjacent structure members and the kernel heap.
Add check for the same.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index e9561242f292..baca90f44c58 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -602,6 +602,9 @@ static int rpmsg_cb(struct rpmsg_device *rpdev, void *data,
 	length = result[MSG_ERR_LENGTH];
 	offset = result[MSG_ERR_OFFSET];
 
+	if (offset + length > REG_MAX)
+		return -EINVAL;
+
 	/*
 	 * The data can come in two stretches. Construct the regs from two
 	 * messages. The offset indicates the offset from which the data is to
-- 
2.34.1

[PATCH 8/9] EDAC/versalnet: Fix use-after-free in remove_one_mc()

[Shubhrajyoti Datta]

device_unregister() drops the last reference on the device and invokes
versal_edac_release() which calls kfree(dev). The subsequent call to
edac_mc_del_mc(mci->pdev) then dereferences the freed pointer.

Fix by saving the device pointer, calling edac_mc_del_mc() and
edac_mc_free() first, then device_unregister() last so the device
is freed only after all users are done with it.

Fixes: 62a9fc50e8d9 ("EDAC/versalnet: Refactor memory controller initialization and cleanup")
Cc: stable@vger.kernel.org
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index baca90f44c58..ba295714d972 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -763,14 +763,16 @@ static void versal_edac_release(struct device *dev)
 static void remove_one_mc(struct mc_priv *priv, int i)
 {
 	struct mem_ctl_info *mci;
+	struct device *dev;
 
 	mci = priv->mci[i];
 	if (!mci)
 		return;
 
-	device_unregister(mci->pdev);
-	edac_mc_del_mc(mci->pdev);
+	dev = mci->pdev;
+	edac_mc_del_mc(dev);
 	edac_mc_free(mci);
+	device_unregister(dev);
 }
 
 static int init_one_mc(struct mc_priv *priv, int i)
-- 
2.34.1

[PATCH 9/9] EDAC/versalnet: Use designated initializer for rpmsg_channel_info

[Shubhrajyoti Datta]

Replace field-by-field assignment of struct rpmsg_channel_info with a
designated initializer. This also fixes the incorrect use of strscpy
with strlen which could lead to silent truncation of the channel name.

Fixes: d5fe2fec6c40 ("EDAC: Add a driver for the AMD Versal NET DDR controller")
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index ba295714d972..88b24eec4206 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -720,14 +720,14 @@ MODULE_DEVICE_TABLE(rpmsg, amd_rpmsg_id_table);
 
 static int rpmsg_probe(struct rpmsg_device *rpdev)
 {
-	struct rpmsg_channel_info chinfo;
 	struct mc_priv *pg;
+	struct rpmsg_channel_info chinfo = {
+		.src = RPMSG_ADDR_ANY,
+		.dst = rpdev->dst,
+		.name = "error_ipc",
+	};
 
 	pg = (struct mc_priv *)amd_rpmsg_id_table[0].driver_data;
-	chinfo.src = RPMSG_ADDR_ANY;
-	chinfo.dst = rpdev->dst;
-	strscpy(chinfo.name, amd_rpmsg_id_table[0].name,
-		strlen(amd_rpmsg_id_table[0].name));
 
 	pg->ept = rpmsg_create_ept(rpdev, rpmsg_cb, NULL, chinfo);
 	if (!pg->ept)
-- 
2.34.1