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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d85f25cf-3c37-dff2-85fd-f8f3a5a57645@huawei.com>
Date:   Tue, 14 Feb 2023 17:33:36 +0800
From:   Gong Ruiqi <gongruiqi1@...wei.com>
To:     Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>
CC:     <netdev@...r.kernel.org>, <edumazet@...gle.com>,
        <pabeni@...hat.com>, lianhui tang <bluetlh@...il.com>,
        <kuniyu@...zon.co.jp>, <rshearma@...cade.com>
Subject: Re: [PATCH net] net: mpls: fix stale pointer if allocation fails
 during device rename

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@...il.com>
> Fixes: 0fae3bf018d9 ("mpls: handle device renames for per-device sysctls")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: kuniyu@...zon.co.jp
> CC: gongruiqi1@...wei.com
> CC: rshearma@...cade.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);