[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 */