netdev - Re: [PATCH net 1/2] net: ethernet: ftgmac100: prevent use after free on unregister when using NCSI

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <0123d308bb8577e7ccb5d99c504cec389ba8fe15.camel@codeconstruct.com.au>
Date: Tue, 29 Oct 2024 12:32:53 +0800
From: Jeremy Kerr <jk@...econstruct.com.au>
To: Andrew Lunn <andrew@...n.ch>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>,  Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,  Joel Stanley
 <joel@....id.au>, Jacky Chou <jacky_chou@...eedtech.com>, Jacob Keller
 <jacob.e.keller@...el.com>,  netdev@...r.kernel.org
Subject: Re: [PATCH net 1/2] net: ethernet: ftgmac100: prevent use after
 free on unregister when using NCSI

Hi Andrew,

> ftgmac100_remove() should be a mirror of ftgmac100_probe() which does
> not register the ncsi device....

Sure it does:

    static int ftgmac100_probe(struct platform_device *pdev)
    {

        /* ... */

        if (np && of_get_property(np, "use-ncsi", NULL)) {
                if (!IS_ENABLED(CONFIG_NET_NCSI)) {
                        dev_err(&pdev->dev, "NCSI stack not enabled\n");
                        err = -EINVAL;
                        goto err_phy_connect;
                }

                dev_info(&pdev->dev, "Using NCSI interface\n");
                priv->use_ncsi = true;
 =>             priv->ndev = ncsi_register_dev(netdev, ftgmac100_ncsi_handler);
                if (!priv->ndev) {
                        err = -EINVAL;
                        goto err_phy_connect;
                }
- so we're symmetrical in that regard.

On unbind, ->remove is called before ->ndo_stop, as the latter is
invoked through the unregister_netdev():

    [   62.869014] Call trace: 
    [   62.869079]  unwind_backtrace from show_stack+0x18/0x1c
    [   62.869386]  show_stack from dump_stack_lvl+0x68/0x74
    [   62.869575]  dump_stack_lvl from print_report+0x130/0x4d8
    [   62.869771]  print_report from kasan_report+0xa8/0xe8
    [   62.869956]  kasan_report from detach_if_pending+0x49c/0x518
    [   62.870156]  detach_if_pending from timer_delete+0xc4/0x124
    [   62.870350]  timer_delete from work_grab_pending+0x8c/0x8e4
    [   62.870543]  work_grab_pending from __cancel_work+0x84/0x25c
    [   62.870744]  __cancel_work from __cancel_work_sync+0x1c/0x130
    [   62.870930]  __cancel_work_sync from phy_stop+0x118/0x268
    [   62.871114]  phy_stop from ftgmac100_stop+0x160/0x2dc
    [   62.871289]  ftgmac100_stop from __dev_close_many+0x1c8/0x300
    [   62.871481]  __dev_close_many from dev_close_many+0x238/0x578
    [   62.871674]  dev_close_many from unregister_netdevice_many_notify+0x460/0x2368
    [   62.871900]  unregister_netdevice_many_notify from unregister_netdevice_queue+0x27c/0x32c
    [   62.872144]  unregister_netdevice_queue from unregister_netdev+0x20/0x28
    [   62.872348]  unregister_netdev from ftgmac100_remove+0x8c/0x24c
    [   62.872542]  ftgmac100_remove from platform_remove+0x84/0xa4
    [   62.872730]  platform_remove from device_release_driver_internal+0x428/0x5e4
    [   62.872952]  device_release_driver_internal from unbind_store+0xb8/0x108
    [   62.873163]  unbind_store from kernfs_fop_write_iter+0x3a4/0x590
    [   62.873364]  kernfs_fop_write_iter from vfs_write+0x65c/0xec8
    [   62.873567]  vfs_write from ksys_write+0xec/0x1d4
    [   62.873735]  ksys_write from ret_fast_syscall+0x0/0x54

As the ordering in ftgmac100_remove() is:


        if (priv->ndev)
                ncsi_unregister_dev(priv->ndev);
        unregister_netdev(netdev);

which, is (I assume intentionally) symmetric with the _probe, which
does:

                priv->ndev = ncsi_register_dev(netdev, ftgmac100_ncsi_handler);

        /* ... */

        register_netdev(netdev)

So we would either re-order _remove() to do the ncsi_unregister() after
the unregister_netdev(), breaking the symmetry there, or we check for a
valid ncsi device in ->ndo_stop. I have chosen the latter for this
change.

Cheers,


Jeremy