[PATCH net] net: mpls: fix stale pointer if allocation fails during device rename

[PATCH net] net: mpls: fix stale pointer if allocation fails during device rename

[Jakub Kicinski]

lianhui reports that when MPLS fails to register the sysctl table
under new location (during device rename) the old pointers won't
get overwritten and may be freed again (double free).

Handle this gracefully. The best option would be unregistering
the MPLS from the device completely on failure, but unfortunately
mpls_ifdown() can fail. So failing fully is also unreliable.

Another option is to register the new table first then only
remove old one if the new one succeeds. That requires more
code, changes order of notifications and two tables may be
visible at the same time.

sysctl point is not used in the rest of the code - set to NULL
on failures and skip unregister if already NULL.

Reported-by: lianhui tang <bluetlh@gmail.com>
Fixes: 0fae3bf018d9 ("mpls: handle device renames for per-device sysctls")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: kuniyu@amazon.co.jp
CC: gongruiqi1@huawei.com
CC: rshearma@brocade.com
---
 net/mpls/af_mpls.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 35b5f806fdda..dc5165d3eec4 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1428,6 +1428,7 @@ static int mpls_dev_sysctl_register(struct net_device *dev,
 free:
 	kfree(table);
 out:
+	mdev->sysctl = NULL;
 	return -ENOBUFS;
 }
 
@@ -1437,6 +1438,9 @@ static void mpls_dev_sysctl_unregister(struct net_device *dev,
 	struct net *net = dev_net(dev);
 	struct ctl_table *table;
 
+	if (!mdev->sysctl)
+		return;
+
 	table = mdev->sysctl->ctl_table_arg;
 	unregister_net_sysctl_table(mdev->sysctl);
 	kfree(table);
-- 
2.39.1

Re: [PATCH net] net: mpls: fix stale pointer if allocation fails during device rename

[Gong Ruiqi]

Just be curious: would this be a simpler solution?

@@ -1439,6 +1439,7 @@ static void mpls_dev_sysctl_unregister(struct
net_device *dev,

        table = mdev->sysctl->ctl_table_arg;
        unregister_net_sysctl_table(mdev->sysctl);
+       mdev->sysctl = NULL;
        kfree(table);

        mpls_netconf_notify_devconf(net, RTM_DELNETCONF, 0, mdev);

However I'm not sure if we need to preserve the old value of
mdev->sysctl after we unregister it.


On 2023/02/14 14:53, Jakub Kicinski wrote:
> lianhui reports that when MPLS fails to register the sysctl table
> under new location (during device rename) the old pointers won't
> get overwritten and may be freed again (double free).
> 
> Handle this gracefully. The best option would be unregistering
> the MPLS from the device completely on failure, but unfortunately
> mpls_ifdown() can fail. So failing fully is also unreliable.
> 
> Another option is to register the new table first then only
> remove old one if the new one succeeds. That requires more
> code, changes order of notifications and two tables may be
> visible at the same time.
> 
> sysctl point is not used in the rest of the code - set to NULL
> on failures and skip unregister if already NULL.
> 
> Reported-by: lianhui tang <bluetlh@gmail.com>
> Fixes: 0fae3bf018d9 ("mpls: handle device renames for per-device sysctls")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: kuniyu@amazon.co.jp
> CC: gongruiqi1@huawei.com
> CC: rshearma@brocade.com
> ---
>  net/mpls/af_mpls.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index 35b5f806fdda..dc5165d3eec4 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -1428,6 +1428,7 @@ static int mpls_dev_sysctl_register(struct net_device *dev,
>  free:
>  	kfree(table);
>  out:
> +	mdev->sysctl = NULL;
>  	return -ENOBUFS;
>  }
>  
> @@ -1437,6 +1438,9 @@ static void mpls_dev_sysctl_unregister(struct net_device *dev,
>  	struct net *net = dev_net(dev);
>  	struct ctl_table *table;
>  
> +	if (!mdev->sysctl)
> +		return;
> +
>  	table = mdev->sysctl->ctl_table_arg;
>  	unregister_net_sysctl_table(mdev->sysctl);
>  	kfree(table);

Re: [PATCH net] net: mpls: fix stale pointer if allocation fails during device rename

[Jakub Kicinski]

On Tue, 14 Feb 2023 17:33:36 +0800 Gong Ruiqi wrote:
> Just be curious: would this be a simpler solution?
> 
> @@ -1439,6 +1439,7 @@ static void mpls_dev_sysctl_unregister(struct
> net_device *dev,
> 
>         table = mdev->sysctl->ctl_table_arg;
>         unregister_net_sysctl_table(mdev->sysctl);
> +       mdev->sysctl = NULL;
>         kfree(table);
> 
>         mpls_netconf_notify_devconf(net, RTM_DELNETCONF, 0, mdev);
> 
> However I'm not sure if we need to preserve the old value of
> mdev->sysctl after we unregister it.

It'd work too, I decided to limit the zeroing to the exception case
because of recent discussions on the list. The argument there was that
zeroing in cases were we don't expect it to be necessary may hide bugs.
We generally try to avoid defensive programming in the kernel.

Re: [PATCH net] net: mpls: fix stale pointer if allocation fails during device rename

[Gong Ruiqi]

On 2023/02/15 5:23, Jakub Kicinski wrote:
> On Tue, 14 Feb 2023 17:33:36 +0800 Gong Ruiqi wrote:
>> Just be curious: would this be a simpler solution?
>>
>> @@ -1439,6 +1439,7 @@ static void mpls_dev_sysctl_unregister(struct
>> net_device *dev,
>>
>>         table = mdev->sysctl->ctl_table_arg;
>>         unregister_net_sysctl_table(mdev->sysctl);
>> +       mdev->sysctl = NULL;
>>         kfree(table);
>>
>>         mpls_netconf_notify_devconf(net, RTM_DELNETCONF, 0, mdev);
>>
>> However I'm not sure if we need to preserve the old value of
>> mdev->sysctl after we unregister it.
> 
> It'd work too, I decided to limit the zeroing to the exception case
> because of recent discussions on the list. The argument there was that
> zeroing in cases were we don't expect it to be necessary may hide bugs.
> We generally try to avoid defensive programming in the kernel.

Actually my original thought was not to do defensive programming, but to
clearly mark it as invalid after its de-registration. Nevertheless "to
avoid defensive programming in the kernel" is a good point :)

And oops, for my proposal the complete solution should be:

@@ -1437,8 +1437,12 @@ static void mpls_dev_sysctl_unregister(struct
net_device *dev,
        struct net *net = dev_net(dev);
        struct ctl_table *table;

+       if (!mdev->sysctl)
+               return;
+
        table = mdev->sysctl->ctl_table_arg;
        unregister_net_sysctl_table(mdev->sysctl);
+       mdev->sysctl = NULL;
        kfree(table);

        mpls_netconf_notify_devconf(net, RTM_DELNETCONF, 0, mdev);

to avoid NULL dereference at `table = mdev->sysctl->...` if we try to
unregister the device after a failed renaming. Then it looks really
similar with your patch xD. So yeah I'm ok with both of them.

Re: [PATCH net] net: mpls: fix stale pointer if allocation fails during device rename

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Mon, 13 Feb 2023 22:53:55 -0800 you wrote:
> lianhui reports that when MPLS fails to register the sysctl table
> under new location (during device rename) the old pointers won't
> get overwritten and may be freed again (double free).
> 
> Handle this gracefully. The best option would be unregistering
> the MPLS from the device completely on failure, but unfortunately
> mpls_ifdown() can fail. So failing fully is also unreliable.
> 
> [...]

Here is the summary with links:
  - [net] net: mpls: fix stale pointer if allocation fails during device rename
    https://git.kernel.org/netdev/net/c/fda6c89fe3d9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html