[PATCH net] mctp i2c: fix double free of midev on netdev register failure

[PATCH net] mctp i2c: fix double free of midev on netdev register failure

[Kenny Hsieh]

mctp_i2c_add_netdev() initialises midev before registering the netdev,
and frees it on its own error path when mctp_register_netdev() fails.
The netdev's ndo_uninit, mctp_i2c_ndo_uninit(), frees it too.

register_netdevice() calls ndo_uninit on its later failures: the
err_uninit path and a NETDEV_REGISTER notifier error. On those, midev is
freed twice. The second mctp_i2c_midev_free() runs list_del() on an entry
that is already poisoned, and drops a second reference on both the
adapter device and the client device, which can free either early.

Earlier failures do not reach ndo_uninit. mctp_add_dev() failing,
ethtool_check_ops() failing and a name that is already taken all return
before it, so the error path's own free is still needed for those.

Make mctp_i2c_midev_free() safe to call twice, by clearing midev->client
once it has run and returning early when it is already clear. Both
callers keep working unchanged.

Tested on an AST2700 BMC by taking the netdev name before creating it,
so the register fails early with -EEXIST. midev is freed once, and a
later create on the same adapter succeeds.

The double free was pointed out by the Sashiko AI review of a pending
mctp-i2c change, as a problem that predates it.

Fixes: f5b8abf9fc3d ("mctp i2c: MCTP I2C binding driver")
Closes: https://lore.kernel.org/netdev/179239912623.3135.11352652156626817288@kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Kenny Hsieh <kenny_hsieh@jabil.com>
---
 drivers/net/mctp/mctp-i2c.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
index b02476d33..bca379af3 100644
--- a/drivers/net/mctp/mctp-i2c.c
+++ b/drivers/net/mctp/mctp-i2c.c
@@ -788,6 +788,13 @@ static void mctp_i2c_midev_free(struct mctp_i2c_dev *midev)
 	struct mctp_i2c_client *mcli = midev->client;
 	unsigned long flags;
 
+	/* Runs from ndo_uninit and from the mctp_i2c_add_netdev() error
+	 * path, and register_netdevice() calls ndo_uninit on some failures
+	 * but not others. A NULL client means midev is already freed.
+	 */
+	if (!mcli)
+		return;
+
 	if (midev->tx_thread) {
 		kthread_stop(midev->tx_thread);
 		midev->tx_thread = NULL;
@@ -810,6 +817,7 @@ static void mctp_i2c_midev_free(struct mctp_i2c_dev *midev)
 	skb_queue_purge(&midev->tx_queue);
 	put_device(&midev->adapter->dev);
 	put_device(&mcli->client->dev);
+	midev->client = NULL;
 }
 
 /* Stops, unregisters, and frees midev */
-- 
2.50.1

Re: [PATCH net] mctp i2c: fix double free of midev on netdev register failure

[Matt Johnston]

Hi Kenny,

Thanks for looking into this.

On Thu, 2026-10-22 at 11:17 +0800, Kenny Hsieh wrote:
> mctp_i2c_add_netdev() initialises midev before registering the netdev,
> and frees it on its own error path when mctp_register_netdev() fails.
> The netdev's ndo_uninit, mctp_i2c_ndo_uninit(), frees it too.
> 
> register_netdevice() calls ndo_uninit on its later failures: the
> err_uninit path and a NETDEV_REGISTER notifier error. On those, midev is
> freed twice. The second mctp_i2c_midev_free() runs list_del() on an entry
> that is already poisoned, and drops a second reference on both the
> adapter device and the client device, which can free either early.

I wonder if it would be better to delete mctp_i2c_ndo_uninit() and move the
the mctp_i2c_midev_free() to mctp_i2c_unregister(), before free_netdev().
Then the free sequence would be symmetrical with the init sequence in
mctp_i2c_add_netdev.

Prior to mctp_i2c_midev_free() calling mctp_unregister_netdev() it would need
to deselect mcli->sel if matching (the reason mctp_i2c_ndo_uninit() existed):

	/* Perform cleanup here to ensure that mcli->sel isn't holding
	 * a reference that would prevent unregister_netdevice()
	 * from completing.

Cheers,
Matt


> Earlier failures do not reach ndo_uninit. mctp_add_dev() failing,
> ethtool_check_ops() failing and a name that is already taken all return
> before it, so the error path's own free is still needed for those.
> 
> Make mctp_i2c_midev_free() safe to call twice, by clearing midev->client
> once it has run and returning early when it is already clear. Both
> callers keep working unchanged.
> 
> Tested on an AST2700 BMC by taking the netdev name before creating it,
> so the register fails early with -EEXIST. midev is freed once, and a
> later create on the same adapter succeeds.
> 
> The double free was pointed out by the Sashiko AI review of a pending
> mctp-i2c change, as a problem that predates it.
> 
> Fixes: f5b8abf9fc3d ("mctp i2c: MCTP I2C binding driver")
> Closes: https://lore.kernel.org/netdev/179239912623.3135.11352652156626817288@kernel.org/
> Cc: stable@vger.kernel.org
> Signed-off-by: Kenny Hsieh <kenny_hsieh@jabil.com>
> ---
>  drivers/net/mctp/mctp-i2c.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
> index b02476d33..bca379af3 100644
> --- a/drivers/net/mctp/mctp-i2c.c
> +++ b/drivers/net/mctp/mctp-i2c.c
> @@ -788,6 +788,13 @@ static void mctp_i2c_midev_free(struct mctp_i2c_dev *midev)
>  	struct mctp_i2c_client *mcli = midev->client;
>  	unsigned long flags;
>  
> +	/* Runs from ndo_uninit and from the mctp_i2c_add_netdev() error
> +	 * path, and register_netdevice() calls ndo_uninit on some failures
> +	 * but not others. A NULL client means midev is already freed.
> +	 */
> +	if (!mcli)
> +		return;
> +
>  	if (midev->tx_thread) {
>  		kthread_stop(midev->tx_thread);
>  		midev->tx_thread = NULL;
> @@ -810,6 +817,7 @@ static void mctp_i2c_midev_free(struct mctp_i2c_dev *midev)
>  	skb_queue_purge(&midev->tx_queue);
>  	put_device(&midev->adapter->dev);
>  	put_device(&mcli->client->dev);
> +	midev->client = NULL;
>  }
>  
>  /* Stops, unregisters, and frees midev */

Re: [PATCH net] mctp i2c: fix double free of midev on netdev register failure

[Kenny Hsieh]

Hi Matt,

On Thu, 2026-10-22 at 12:39 +0800, Matt Johnston wrote:
> I wonder if it would be better to delete mctp_i2c_ndo_uninit() and move the
> the mctp_i2c_midev_free() to mctp_i2c_unregister(), before free_netdev().
> Then the free sequence would be symmetrical with the init sequence in
> mctp_i2c_add_netdev.

Agreed, that is cleaner than making the free idempotent. Here's what I'm
planning, let me know if it looks right and I'll go ahead:

  - in mctp_i2c_unregister(), deselect mcli->sel if it points at this
    midev, before mctp_unregister_netdev(), so its dev_hold() doesn't
    block the unregister.
  - call mctp_i2c_midev_free() after mctp_unregister_netdev() and before
    free_netdev(), and take the sel handling out of it since that now
    happens earlier.
  - leave the mctp_i2c_add_netdev() error path calling
    mctp_i2c_midev_free() as it is, which then becomes the only free on
    a failed register.
  - delete mctp_i2c_ndo_uninit() and .ndo_uninit.

Re: [PATCH net] mctp i2c: fix double free of midev on netdev register failure

[Matt Johnston]

On Thu, 2026-10-22 at 07:22 +0000, Kenny Hsieh wrote:
> 
> Agreed, that is cleaner than making the free idempotent. Here's what I'm
> planning, let me know if it looks right and I'll go ahead:
> 
>   - in mctp_i2c_unregister(), deselect mcli->sel if it points at this
>     midev, before mctp_unregister_netdev(), so its dev_hold() doesn't
>     block the unregister.
>   - call mctp_i2c_midev_free() after mctp_unregister_netdev() and before
>     free_netdev(), and take the sel handling out of it since that now
>     happens earlier.
>   - leave the mctp_i2c_add_netdev() error path calling
>     mctp_i2c_midev_free() as it is, which then becomes the only free on
>     a failed register.
>   - delete mctp_i2c_ndo_uninit() and .ndo_uninit.

Yes, that looks good.

Cheers,
Matt